search

servo / servo

Servo, the embeddable, independent, memory-safe, modular, parallel web rendering engine
https://servo.org
Mozilla Public License 2.0
28.23k stars 3.01k forks source link

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

Open gterzian opened 10 months ago

gterzian commented 10 months ago

Part of https://github.com/servo/servo/issues/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>:

https://github.com/servo/servo/blob/8bbcf0abafc22cd840cd03f36b5b3b7d2d815493/components/script/dom/audiobuffer.rs#L265

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): https://github.com/servo/servo/blob/8bbcf0abafc22cd840cd03f36b5b3b7d2d815493/components/script/dom/bindings/codegen/CodegenRust.py#L1463

  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 commented 10 months ago

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

gterzian commented 10 months ago

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 https://github.com/servo/servo/issues/30891
  3. Should return an enum, see https://github.com/servo/servo/issues/30890
  4. Should return a WebGL extension, which is actually an object? in WebIDL. See https://github.com/servo/servo/issues/30892
gterzian commented 10 months ago

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 commented 10 months ago

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