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 impl: Replace use of NonNull<JSObject> #30889

Open
gterzian opened this issue Dec 19, 2023 · 4 comments
Open

WebIDL impl: Replace use of NonNull<JSObject> #30889

gterzian opened this issue Dec 19, 2023 · 4 comments
Labels
A-content/script Related to the script thread I-safety Some piece of code violates memory safety guarantees. L-javascript Javascript is required

Comments

@gterzian
Copy link
Member

gterzian commented Dec 19, 2023

Part of #30862

Where appropriate, we should remove the direct use of unsafe NonNull<JSObject>, and replace it with use of the appropriate safe wrapper found in /mozjs/src/typedarray.rs.

Example: AudioBuffer's getChannelData

WedIDL says to return a Float32Array, but implementation returns a NonNull<JSObject>:

fn GetChannelData(&self, cx: JSContext, channel: u32) -> Fallible<NonNull<JSObject>> {

The return value of the WebIDL method is backed by a Heap<*mut JSObject> on the dom struct.

Solution: use js::typedarray::Float32Array instead of js::jsapi::JSObject.

Bonus: wrap the use of js::typedarray inside something at dom/bindings/. (maybe see bindings/iterable.rs for an example)


How to do it?

  1. Update codegen(look also at third_party/webidl)

codegen starts at(probably needs other updates):

def getRetvalDeclarationForType(returnType, descriptorProvider):

  1. Make sure that the right _Methods are generated.
  2. Update the Rust code, where the struct would the typed array directly, and implement the WebIDL methods returning the typed array.
  3. Bonus: put the typed array behind something in dom/bindings so that we don't use js:: directly elsewhere. For example, this kind of code should be hidden from view. In some way this is probably not a bonus, but a prerequisite for 3.

More investigation is probably necessary.

@gterzian gterzian added A-content/script Related to the script thread L-javascript Javascript is required labels Dec 19, 2023
@gterzian
Copy link
Member Author

@jdm Does this make sense? Do you know why this wasn't done to begin with?

@gterzian
Copy link
Member Author

gterzian commented Dec 19, 2023

Had a quick look everywhere, it seems there are really four cases:

  1. Should return some variant of js::typedarray (the majority). Dealt with in this issue
  2. Should return a ReadableStream, see WebIDL impl: remove unsafe JSObject when returning a ReadableStream #30891
  3. Should return an enum, see WebIDL impl: remove unsafe JSObject from return value of Document::NamedGetter #30890
  4. Should return a WebGL extension, which is actually an object? in WebIDL. See WebIDL impl: remove unsafe JSObject from WebGLExtensionWrapper #30892

@gterzian
Copy link
Member Author

See also https://github.com/servo/mozjs/blob/de842d84a07eabac4e1bfe72a954b74ee0244b82/mozjs/src/conversions.rs#L722

It may be that what is required is simply implementing ToJSValConvertible for the various types in js::rust::typedarray

In some ways it would in that case still be nicer to wrap the use of js:: inside dom/bindings...

@jdm
Copy link
Member

jdm commented Dec 19, 2023

This seems reasonable to me! I'm not sure why we didn't integrate the concrete types originally.

@gterzian gterzian mentioned this issue Jan 3, 2024
5 tasks
@gterzian gterzian changed the title WebIDL impl: Replace use of NonNull<JSObject> with TypedArray WebIDL impl: Replace use of NonNull<JSObject> Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/script Related to the script thread I-safety Some piece of code violates memory safety guarantees. L-javascript Javascript is required
Projects
None yet
Development

No branches or pull requests

2 participants