WebIDL: Use Uint8ClampedArray instead of raw JSObject in bindings #31317

Taym95 · 2024-02-11T18:42:10Z

Part #31064, we would like to remove the direct use of unsafe NonNull<JSObject>, and replace it with use of the appropriate safe wrapper found in typedarray.rs, in this PR we are replacing unsafe NonNull<JSObject> with Uint8ClampedArray , according to spec

./mach build -d does not report any errors
./mach test-tidy does not report any errors

These changes do not require tests because there is not behavior change

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

components/script/dom/bindings/codegen/CodegenRust.py

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

gterzian

Thanks for the contrib again! Let's change the new_initialized method a bit.

gterzian · 2024-02-12T05:55:43Z

components/script/dom/bindings/typedarrays.rs

@@ -31,6 +31,16 @@ where
    T: TypedArrayElement + TypedArrayElementCreator,
    T::Element: Clone + Copy,
 {
+    pub fn new_initialized(js_object: Option<*mut JSObject>) -> HeapTypedArray<T> {


So let's add an pub enum like so:

HeapTypedArrayInit { Object(*mut JSObject), Info { len: usize } }

And let's make it the init argument to this method.

The Info variant should be creating the array using a rooted object and call create_typed_array_with_length(which should be private).

Re the Object variant, I hope it can be typed later as a result of doing #31320

This is much better, I updated the code, thanks

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

gterzian

Thanks for the contrib! Couple of changes suggested...

gterzian · 2024-02-13T08:08:33Z

components/script/dom/imagedata.rs

+    fn Data(&self, _: JSContext) -> Uint8ClampedArray {
+        self.data
+            .get_internal()
+            .expect("Failed to get Data from ImageData.")


Let's return JsError as we did in the other PR.

should we do it for other changes we merged, we have lot of .get_internal().expect ?

I think so, but it depends on the spec. It would be good to do this in a separate PR so each cases can be investigated separately(it may require updating test expectations also).

I think we only return error when function returns Fallible but not in this case?

yes! Although we can check the spec to see if the function should be fallible...

From the spec, I Don't see Data fallible, what do you think?

The spec doesn't say it direclty, but wording like "initialize the data attribute of imageData to a new Uint8ClampedArray object" implies using the Web IDL and JS operations which are in fact fallible.

Let's return JsError, by adding the Throws attribute to

servo/components/script/dom/webidls/CanvasRenderingContext2D.webidl

Line 262 in c94d584

readonly attribute Uint8ClampedArray data;

gterzian · 2024-02-13T08:15:48Z

components/script/dom/bindings/typedarrays.rs

+                    .expect("Creating TypedArray from length should never fail");
+            let heap_typed_array = HeapTypedArray::<T>::default();
+            heap_typed_array
+                .set_data(cx, unsafe { typed_array.as_slice() })


unsafe is unnecessary, because the entire module allows it.

I still get this operation is unsafe and requires an unsafe function or block when I remove it

I thought it's better to keep unsafe here instead of using it in new_with_jsobject

ah yes sorry, you need unsafe blocks for unsafe calls(but there is no need to mark the method as allowing unsafe, which is what I had in mind somehow).

gterzian · 2024-02-13T08:18:17Z

components/script/dom/bindings/typedarrays.rs

+                create_typed_array_with_length::<T>(cx, len as usize, array.handle_mut())
+                    .expect("Creating TypedArray from length should never fail");
+            let heap_typed_array = HeapTypedArray::<T>::default();
+            heap_typed_array


I think you can simply do heap_typed_array.internal.set(*typed_array)

Yes heap_typed_array.internal.set(*array) should be okey

gterzian · 2024-02-13T08:28:28Z

components/script/dom/imagedata.rs

-        let array = array_res.map_err(|_| {
+        let heap_typed_array =
+            new_initialized_heap_typed_array(HeapTypedArrayInit::Object(jsobject));
+        let array = heap_typed_array.acquire_data(cx).map_err(|_| {


acquire_data detaches the buffer, so instead we want to use get_internal.

Indeed thanks, fixed

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

…JS_Object-in-bindings

gterzian

Just a few last changes!

gterzian · 2024-02-15T09:20:19Z

components/script/dom/imagedata.rs

+    fn Data(&self, _: JSContext) -> Uint8ClampedArray {
+        self.data
+            .get_internal()
+            .expect("Failed to get Data from ImageData.")


The spec doesn't say it direclty, but wording like "initialize the data attribute of imageData to a new Uint8ClampedArray object" implies using the Web IDL and JS operations which are in fact fallible.

Let's return JsError, by adding the Throws attribute to

servo/components/script/dom/webidls/CanvasRenderingContext2D.webidl

Line 262 in c94d584

readonly attribute Uint8ClampedArray data;

gterzian · 2024-02-15T09:22:27Z

components/script/dom/bindings/typedarrays.rs

@@ -31,6 +31,32 @@ unsafe impl<T> crate::dom::bindings::trace::JSTraceable for HeapTypedArray<T> {
    }
 }

+pub fn new_initialized_heap_typed_array<T>(init: HeapTypedArrayInit) -> HeapTypedArray<T>


Let's return a result, and return the error from create_typed_array_with_length. (creating the buffer is falling, as we saw in the other PR).

…array Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

gterzian

Awesome! LGTM with just one nit...

components/script/dom/bindings/typedarrays.rs

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

gterzian

Great work, thanks!

gterzian · 2024-02-16T11:25:17Z

I'll add it to the queue, although it may require a rebase.

…JS_Object-in-bindings

WebIDL: Use Uint8ClampedArray instead of raw JSObject in bindings

5501d40

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

Taym95 commented Feb 11, 2024

View reviewed changes

components/script/dom/bindings/codegen/CodegenRust.py Show resolved Hide resolved

fmt

4b90a71

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

gterzian requested changes Feb 12, 2024

View reviewed changes

gterzian mentioned this pull request Feb 12, 2024

WebIDL: Fix ImageData constructor to take a Uint8ClampedArray instead of object #31320

Open

Taym95 added 3 commits February 12, 2024 14:18

introduce new_initialized_heap_typed_array function

3c138a5

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

Remove unsed unsafe_code

e206e70

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

Use doc comments for ImageData

c395253

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

gterzian requested changes Feb 13, 2024

View reviewed changes

Taym95 added 2 commits February 14, 2024 16:45

Use get_internal instead of acquire_data

4962214

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

Merge branch 'main' into WebIDL-Use-Uint8ClampedArray-instead-of-raw-…

edfc2f2

…JS_Object-in-bindings

gterzian requested changes Feb 15, 2024

View reviewed changes

Taym95 added 2 commits February 15, 2024 12:00

Handle JS errors in ImageData GetData and new_initialized_heap_typed_…

3442c88

…array Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

Fix wrong assert that causes CRASH in test

48a1220

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

gterzian approved these changes Feb 15, 2024

View reviewed changes

components/script/dom/bindings/typedarrays.rs Outdated Show resolved Hide resolved

Taym95 added 2 commits February 15, 2024 16:05

Early return for error

b5b756f

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

Address review comments

c56240c

Signed-off-by: Bentaimia Haddadi <haddadi.taym@gmail.com>

gterzian approved these changes Feb 16, 2024

View reviewed changes

gterzian added this pull request to the merge queue Feb 16, 2024

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Feb 16, 2024

Merge branch 'main' into WebIDL-Use-Uint8ClampedArray-instead-of-raw-…

779d109

…JS_Object-in-bindings

mrobinson added this pull request to the merge queue Feb 16, 2024

Merged via the queue into servo:main with commit 328c376 Feb 16, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WebIDL: Use Uint8ClampedArray instead of raw JSObject in bindings #31317

WebIDL: Use Uint8ClampedArray instead of raw JSObject in bindings #31317

Taym95 commented Feb 11, 2024 •

edited

gterzian left a comment

gterzian Feb 12, 2024

gterzian Feb 12, 2024

Taym95 Feb 12, 2024

gterzian left a comment

gterzian Feb 13, 2024

Taym95 Feb 13, 2024

gterzian Feb 13, 2024

Taym95 Feb 13, 2024 •

edited

gterzian Feb 14, 2024

Taym95 Feb 14, 2024 •

edited

gterzian Feb 15, 2024 •

edited

Taym95 Feb 15, 2024

gterzian Feb 13, 2024

Taym95 Feb 13, 2024

Taym95 Feb 13, 2024

gterzian Feb 14, 2024 •

edited

gterzian Feb 13, 2024

Taym95 Feb 14, 2024

gterzian Feb 13, 2024

Taym95 Feb 14, 2024

gterzian left a comment

gterzian Feb 15, 2024 •

edited

gterzian Feb 15, 2024

Taym95 Feb 15, 2024

gterzian left a comment

gterzian left a comment

gterzian commented Feb 16, 2024

WebIDL: Use Uint8ClampedArray instead of raw JSObject in bindings #31317

WebIDL: Use Uint8ClampedArray instead of raw JSObject in bindings #31317

Conversation

Taym95 commented Feb 11, 2024 • edited

gterzian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gterzian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Taym95 Feb 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Taym95 Feb 14, 2024 • edited

Choose a reason for hiding this comment

gterzian Feb 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gterzian Feb 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gterzian left a comment

Choose a reason for hiding this comment

gterzian Feb 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gterzian left a comment

Choose a reason for hiding this comment

gterzian left a comment

Choose a reason for hiding this comment

gterzian commented Feb 16, 2024

Taym95 commented Feb 11, 2024 •

edited

Taym95 Feb 13, 2024 •

edited

Taym95 Feb 14, 2024 •

edited

gterzian Feb 15, 2024 •

edited

gterzian Feb 14, 2024 •

edited

gterzian Feb 15, 2024 •

edited