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.

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

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

2 changes: 2 additions & 0 deletions components/script/dom/bindings/codegen/CodegenRust.py
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
46 changes: 46 additions & 0 deletions components/script/dom/bindings/typedarrays.rs
Expand Up @@ -26,6 +26,35 @@ unsafe impl<T> crate::dom::bindings::trace::JSTraceable for HeapTypedArray<T> {
}
}

pub fn new_initialized_heap_typed_array<T>(init: HeapTypedArrayInit) -> 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.

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

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

where
T: TypedArrayElement + TypedArrayElementCreator,
T::Element: Clone + Copy,
{
match init {
HeapTypedArrayInit::Object(js_object) => HeapTypedArray {
internal: Heap::boxed(js_object),
phantom: PhantomData::default(),
},
HeapTypedArrayInit::Info { len, cx } => {
rooted!(in (*cx) let mut array = ptr::null_mut::<JSObject>());
let typed_array =
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
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 you can simply do heap_typed_array.internal.set(*typed_array)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

.set_data(cx, unsafe { typed_array.as_slice() })
Copy link
Member

Choose a reason for hiding this comment

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

unsafe is unnecessary, because the entire module allows it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@gterzian gterzian Feb 14, 2024

Choose a reason for hiding this comment

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

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

.expect("Failed to set internal data.");
heap_typed_array
},
}
}

pub enum HeapTypedArrayInit {
Object(*mut JSObject),
Info { len: u32, cx: JSContext },
}

impl<T> HeapTypedArray<T>
where
T: TypedArrayElement + TypedArrayElementCreator,
Expand Down Expand Up @@ -142,3 +171,20 @@ where
TypedArray::from(dest.get())
}
}

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())
}
}
78 changes: 38 additions & 40 deletions components/script/dom/imagedata.rs
Expand Up @@ -3,18 +3,19 @@
* 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::{
new_initialized_heap_typed_array, HeapTypedArray, HeapTypedArrayInit,
};
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};
Expand All @@ -28,7 +29,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 +55,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 +64,9 @@ 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 =
new_initialized_heap_typed_array(HeapTypedArrayInit::Object(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,16 +89,13 @@ 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))
}

#[allow(unsafe_code)]
unsafe fn new_without_jsobject(
fn new_without_jsobject(
global: &GlobalScope,
proto: Option<HandleObject>,
width: u32,
Expand All @@ -106,37 +104,35 @@ impl ImageData {
if width == 0 || height == 0 {
return Err(Error::IndexSize);
}
let cx = GlobalScope::get_cx();
let len = width * height * 4;

let imagedata = Box::new(ImageData {
reflector_: Reflector::new(),
width: width,
height: height,
data: Heap::default(),
data: new_initialized_heap_typed_array::<ClampedU8>(HeapTypedArrayInit::Info {
len,
cx,
}),
});

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());

Ok(reflect_dom_object_with_proto(imagedata, global, proto))
}
// https://html.spec.whatwg.org/multipage/#pixel-manipulation:dom-imagedata-3
#[allow(unsafe_code, non_snake_case)]
/// <https://html.spec.whatwg.org/multipage/#pixel-manipulation:dom-imagedata-3>
#[allow(non_snake_case)]
pub fn Constructor(
global: &GlobalScope,
proto: Option<HandleObject>,
width: u32,
height: u32,
) -> Fallible<DomRoot<Self>> {
unsafe { Self::new_without_jsobject(global, proto, width, height) }
Self::new_without_jsobject(global, proto, width, height)
}

// https://html.spec.whatwg.org/multipage/#pixel-manipulation:dom-imagedata-4
#[allow(unsafe_code, unused_variables, non_snake_case)]
pub unsafe fn Constructor_(
/// <https://html.spec.whatwg.org/multipage/#pixel-manipulation:dom-imagedata-4>
#[allow(unused_variables, non_snake_case)]
pub fn Constructor_(
cx: JSContext,
global: &GlobalScope,
proto: Option<HandleObject>,
Expand All @@ -150,17 +146,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 All @@ -180,18 +176,20 @@ impl ImageData {
}

impl ImageDataMethods for ImageData {
// https://html.spec.whatwg.org/multipage/#dom-imagedata-width
/// <https://html.spec.whatwg.org/multipage/#dom-imagedata-width>
fn Width(&self) -> u32 {
self.width
}

// https://html.spec.whatwg.org/multipage/#dom-imagedata-height
/// <https://html.spec.whatwg.org/multipage/#dom-imagedata-height>
fn Height(&self) -> u32 {
self.height
}

// 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")
/// <https://html.spec.whatwg.org/multipage/#dom-imagedata-data>
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

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

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

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

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