Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 12 commits into from Feb 16, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5501d40
WebIDL: Use Uint8ClampedArray instead of raw JSObject in bindings
Taym95 Feb 11, 2024
4b90a71
fmt
Taym95 Feb 11, 2024
3c138a5
introduce new_initialized_heap_typed_array function
Taym95 Feb 12, 2024
e206e70
Remove unsed unsafe_code
Taym95 Feb 12, 2024
c395253
Use doc comments for ImageData
Taym95 Feb 12, 2024
4962214
Use get_internal instead of acquire_data
Taym95 Feb 14, 2024
edfc2f2
Merge branch 'main' into WebIDL-Use-Uint8ClampedArray-instead-of-raw-…
Taym95 Feb 14, 2024
3442c88
Handle JS errors in ImageData GetData and new_initialized_heap_typed_…
Taym95 Feb 15, 2024
48a1220
Fix wrong assert that causes CRASH in test
Taym95 Feb 15, 2024
b5b756f
Early return for error
Taym95 Feb 15, 2024
c56240c
Address review comments
Taym95 Feb 16, 2024
779d109
Merge branch 'main' into WebIDL-Use-Uint8ClampedArray-instead-of-raw-…
Taym95 Feb 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions components/script/dom/bindings/codegen/CodegenRust.py
View file
Open in desktop
Expand Up @@ -128,6 +128,7 @@ def wrapInNativeContainerType(type, inner):
IDLType.Tags.uint32array: 'Uint32Array',
IDLType.Tags.float32array: 'Float32Array',
IDLType.Tags.float64array: 'Float64Array',
IDLType.Tags.uint8clampedarray: 'Uint8ClampedArray',
Taym95 marked this conversation as resolved.
Show resolved Hide resolved
}

numericTags = [
Expand Down Expand Up @@ -6516,6 +6517,7 @@ def generate_imports(config, cgthings, descriptors, callbacks=None, dictionaries
'js::typedarray::Uint32Array',
'js::typedarray::Float32Array',
'js::typedarray::Float64Array',
'js::typedarray::Uint8ClampedArray',
'crate::dom',
'crate::dom::bindings',
'crate::dom::bindings::codegen::InterfaceObjectMap',
Expand Down
27 changes: 27 additions & 0 deletions components/script/dom/bindings/typedarrays.rs
View file
Open in desktop
Expand Up @@ -31,6 +31,16 @@ where
T: TypedArrayElement + TypedArrayElementCreator,
T::Element: Clone + Copy,
{
pub fn new_initialized(js_object: Option<*mut JSObject>) -> HeapTypedArray<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better, I updated the code, thanks

match js_object {
Some(inner_js_object) => HeapTypedArray {
internal: Heap::boxed(inner_js_object),
phantom: PhantomData::default(),
},
None => Self::default(),
}
}

pub fn default() -> HeapTypedArray<T> {
HeapTypedArray {
internal: Box::new(Heap::default()),
Expand Down Expand Up @@ -142,3 +152,20 @@ where
TypedArray::from(dest.get())
}
}

pub fn create_typed_array_with_length<T>(
cx: JSContext,
len: usize,
dest: MutableHandleObject,
) -> Result<TypedArray<T, *mut JSObject>, ()>
where
T: TypedArrayElement + TypedArrayElementCreator,
{
let res = unsafe { TypedArray::<T, *mut JSObject>::create(*cx, CreateWith::Length(len), dest) };

if res.is_err() {
Err(())
} else {
TypedArray::from(dest.get())
}
}
52 changes: 27 additions & 25 deletions components/script/dom/imagedata.rs
View file
Open in desktop
Expand Up @@ -3,22 +3,22 @@
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */

use std::borrow::Cow;
use std::default::Default;
use std::ptr;
use std::ptr::NonNull;
use std::vec::Vec;

use dom_struct::dom_struct;
use euclid::default::{Rect, Size2D};
use ipc_channel::ipc::IpcSharedMemory;
use js::jsapi::{Heap, JSObject};
use js::rust::{HandleObject, Runtime};
use js::typedarray::{CreateWith, Uint8ClampedArray};
use js::jsapi::JSObject;
use js::rust::HandleObject;
use js::typedarray::{ClampedU8, CreateWith, Uint8ClampedArray};

use super::bindings::typedarrays::HeapTypedArray;
use crate::dom::bindings::codegen::Bindings::CanvasRenderingContext2DBinding::ImageDataMethods;
use crate::dom::bindings::error::{Error, Fallible};
use crate::dom::bindings::reflector::{reflect_dom_object_with_proto, Reflector};
use crate::dom::bindings::root::DomRoot;
use crate::dom::bindings::typedarrays::create_typed_array_with_length;
use crate::dom::globalscope::GlobalScope;
use crate::script_runtime::JSContext;

Expand All @@ -28,7 +28,7 @@ pub struct ImageData {
width: u32,
height: u32,
#[ignore_malloc_size_of = "mozjs"]
data: Heap<*mut JSObject>,
data: HeapTypedArray<ClampedU8>,
}

impl ImageData {
Expand All @@ -54,8 +54,7 @@ impl ImageData {
}
}

#[allow(unsafe_code)]
unsafe fn new_with_jsobject(
fn new_with_jsobject(
global: &GlobalScope,
proto: Option<HandleObject>,
width: u32,
Expand All @@ -64,8 +63,8 @@ impl ImageData {
) -> Fallible<DomRoot<ImageData>> {
// checking jsobject type
let cx = GlobalScope::get_cx();
typedarray!(in(*cx) let array_res: Uint8ClampedArray = jsobject);
let array = array_res.map_err(|_| {
let heap_typed_array = HeapTypedArray::new_initialized(Some(jsobject));
let array = heap_typed_array.acquire_data(cx).map_err(|_| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed thanks, fixed

Error::Type("Argument to Image data is not an Uint8ClampedArray".to_owned())
})?;

Expand All @@ -88,11 +87,9 @@ impl ImageData {
reflector_: Reflector::new(),
width: width,
height: height,
data: Heap::default(),
data: heap_typed_array,
});

(*imagedata).data.set(jsobject);

Ok(reflect_dom_object_with_proto(imagedata, global, proto))
}

Expand All @@ -111,15 +108,18 @@ impl ImageData {
reflector_: Reflector::new(),
width: width,
height: height,
data: Heap::default(),
data: HeapTypedArray::new_initialized(None),
});

let len = width * height * 4;
let cx = GlobalScope::get_cx();

rooted!(in (*cx) let mut array = ptr::null_mut::<JSObject>());
Uint8ClampedArray::create(*cx, CreateWith::Length(len as usize), array.handle_mut())
.unwrap();
(*imagedata).data.set(array.get());
let typed_array =
create_typed_array_with_length::<ClampedU8>(cx, len as usize, array.handle_mut())
.expect("Creating ClampedU8 array from length should never fail");

let _ = (*imagedata).data.set_data(cx, typed_array.as_slice());

Ok(reflect_dom_object_with_proto(imagedata, global, proto))
}
Expand Down Expand Up @@ -150,17 +150,17 @@ impl ImageData {
/// Nothing must change the array on the JS side while the slice is live.
#[allow(unsafe_code)]
pub unsafe fn as_slice(&self) -> &[u8] {
assert!(!self.data.get().is_null());
let cx = Runtime::get();
assert!(!cx.is_null());
typedarray!(in(cx) let array: Uint8ClampedArray = self.data.get());
let array = array.as_ref().unwrap();
assert!(!self.data.is_initialized());
let internal_data = self
.data
.get_internal()
.expect("Failed to get Data from ImageData.");
// NOTE(nox): This is just as unsafe as `as_slice` itself even though we
// are extending the lifetime of the slice, because the data in
// this ImageData instance will never change. The method is thus unsafe
// because the array may be manipulated from JS while the reference
// is live.
let ptr = array.as_slice() as *const _;
let ptr: *const [u8] = internal_data.as_slice() as *const _;
&*ptr
}

Expand Down Expand Up @@ -191,7 +191,9 @@ impl ImageDataMethods for ImageData {
}

// https://html.spec.whatwg.org/multipage/#dom-imagedata-data
fn Data(&self, _: JSContext) -> NonNull<JSObject> {
NonNull::new(self.data.get()).expect("got a null pointer")
fn Data(&self, _: JSContext) -> Uint8ClampedArray {
self.data
.get_internal()
.expect("Failed to get Data from ImageData.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@Taym95 Taym95 Feb 13, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Taym95 Taym95 Feb 14, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@gterzian gterzian Feb 15, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
}
19 changes: 10 additions & 9 deletions components/script/dom/testbinding.rs
View file
Open in desktop
Expand Up @@ -5,14 +5,14 @@
// check-tidy: no specs after this line

use std::borrow::ToOwned;
use std::ptr::NonNull;
use std::ptr::{self, NonNull};
use std::rc::Rc;

use dom_struct::dom_struct;
use js::jsapi::{Heap, JSObject, JS_NewPlainObject, JS_NewUint8ClampedArray};
use js::jsapi::{Heap, JSObject, JS_NewPlainObject};
use js::jsval::{JSVal, NullValue};
use js::rust::{CustomAutoRooterGuard, HandleObject, HandleValue};
use js::typedarray;
use js::typedarray::{self, Uint8ClampedArray};
use script_traits::serializable::BlobImpl;
use script_traits::MsDuration;
use servo_config::prefs;
Expand Down Expand Up @@ -41,6 +41,7 @@ use crate::dom::bindings::reflector::{reflect_dom_object_with_proto, DomObject,
use crate::dom::bindings::root::DomRoot;
use crate::dom::bindings::str::{ByteString, DOMString, USVString};
use crate::dom::bindings::trace::RootedTraceableBox;
use crate::dom::bindings::typedarrays::create_typed_array;
use crate::dom::bindings::weakref::MutableWeakRef;
use crate::dom::blob::Blob;
use crate::dom::globalscope::GlobalScope;
Expand Down Expand Up @@ -209,12 +210,12 @@ impl TestBindingMethods for TestBinding {
ByteStringOrLong::ByteString(ByteString::new(vec![]))
}
fn SetUnion9Attribute(&self, _: ByteStringOrLong) {}
#[allow(unsafe_code)]
fn ArrayAttribute(&self, cx: SafeJSContext) -> NonNull<JSObject> {
unsafe {
rooted!(in(*cx) let array = JS_NewUint8ClampedArray(*cx, 16));
NonNull::new(array.get()).expect("got a null pointer")
}
fn ArrayAttribute(&self, cx: SafeJSContext) -> Uint8ClampedArray {
let data: [u8; 16] = [0; 16];

rooted!(in (*cx) let mut array = ptr::null_mut::<JSObject>());
create_typed_array(cx, &data, array.handle_mut())
.expect("Creating ClampedU8 array should never fail")
}
fn AnyAttribute(&self, _: SafeJSContext) -> JSVal {
NullValue()
Expand Down
3 changes: 2 additions & 1 deletion third_party/WebIDL/WebIDL.py
View file
Open in desktop
Expand Up @@ -2409,6 +2409,7 @@ class IDLType(IDLObject):
"uint32array",
"float32array",
"float64array",
"uint8clampedarray",
"dictionary",
"enum",
"callback",
Expand Down Expand Up @@ -3643,7 +3644,7 @@ class IDLBuiltinType(IDLType):
Types.ArrayBufferView: IDLType.Tags.interface,
Types.Int8Array: IDLType.Tags.int8array,
Types.Uint8Array: IDLType.Tags.uint8array,
Types.Uint8ClampedArray: IDLType.Tags.interface,
Types.Uint8ClampedArray: IDLType.Tags.uint8clampedarray,
Types.Int16Array: IDLType.Tags.int16array,
Types.Uint16Array: IDLType.Tags.uint16array,
Types.Int32Array: IDLType.Tags.int32array,
Expand Down