Root sequence<any> params using CustomAutoRooter #19644

Xanewok · 2017-12-25T22:01:29Z

Attempt at #16678. At the moment this pulls an external remove-handle-conv branch for rust-mozjs (removing FromJSValConvertible implementation for HandleValue and adding one for *mut JSObject)

In short, this roots the sequence<any> and sequence<object> function arguments using CustomAutoRooter (introduced in servo/rust-mozjs#382).

As per #16678 (comment) the underlying vector contains raw gc thing pointers instead of handles. To do that, this introduces new possible isMember = "AutoRoot" value in CodegenRust.py.
The rooted argument is passed to a function wrapped in a CustomAutoRooterGuard RAII root guard (e.g. as CustomAutoRooterGuard<Vec<JSVal>>)

I felt quite out of my depth when trying to adapt CodegenRust.py code, so I'd appreciate any help with how can be done better/in a more clean manner.
Also the name CustomAutoRooterGuard is quite lengthy, so I'm also open to changing it (AutoRoot? AutoRootGuard?)

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #16678 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-12-25T22:01:35Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon.

highfive · 2017-12-25T22:01:36Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/dom/webidls/TestBinding.webidl
@fitzgen: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/dom/webidls/TestBinding.webidl
@KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/dom/webidls/TestBinding.webidl

highfive · 2017-12-25T22:01:37Z

Warning

These commits modify unsafe code. Please review it carefully!

jdm

This is a good start. I'd like to see what it looks like if we don't overload isMember, as well as having a test that verifies that it works correctly.

jdm · 2017-12-29T00:02:59Z

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

@@ -700,6 +700,8 @@ def wrapObjectTemplate(templateBody, nullValue, isDefinitelyObject, type,
    assert not (isEnforceRange and isClamp)  # These are mutually exclusive

    if type.isSequence() or type.isRecord():
+        if isArgument and type_needs_auto_root(type):
+            isMember = "AutoRoot"


Rather than overloading isMember for this, I think we should add a new isAutoRooted argument to getJSToNativeConversionInfo. I think we could remove this logic and just pass isAutoRooted=isAutoRooted to the getJSToNativeConversionInfo on the next line.

jdm · 2017-12-29T00:02:59Z

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

@@ -709,6 +711,9 @@ def wrapObjectTemplate(templateBody, nullValue, isDefinitelyObject, type,
        if type.nullable():
            declType = CGWrapper(declType, pre="Option<", post=" >")

+        if (isArgument and type_needs_auto_root(type)):


Similarly, I think this could be replaced by if isAutoRooted:.

jdm · 2017-12-29T00:02:59Z

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

+                default = "UndefinedValue()"
+            else:
+                raise TypeError("Can't handle non-null, non-undefined default value here")
+            return handleOptional("${val}.get()", declType, default)


Rather than duplicate this code, I would prefer to have a special check around declType.

jdm · 2017-12-29T00:02:59Z

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

-                template, replacementVariables, declType, "arg%d" % index)
+                template, replacementVariables, declType, arg)
+
+            # TODO: Only for sequence<{any,object}> now


Why is this TODO necessary? Isn't this check encapsulated in type_needs_auto_root?

jdm · 2017-12-29T00:02:59Z

components/script/dom/testbinding.rs

@@ -449,6 +450,10 @@ impl TestBindingMethods for TestBinding {
    fn PassCallbackFunction(&self, _: Rc<Function>) {}
    fn PassCallbackInterface(&self, _: Rc<EventListener>) {}
    fn PassSequence(&self, _: Vec<i32>) {}
+    #[allow(unsafe_code)]
+    unsafe fn PassAnySequence(&self, _: *mut JSContext, _: CustomAutoRooterGuard<Vec<JSVal>>) {}


It would be useful to have an automated test that verifies that the values passed in actually make sense, since that was a problem with the old sequence<any> code. Perhaps have a method that accepts a sequence and returns it, and compare the array values in JS?

Xanewok · 2017-12-29T20:54:02Z

I addressed the review comments in the new commit. I still have to include a test for that.

@jdm I have a question regarding adding the test.
From what I saw, the automated tests for this should be here? Should I test the gc using window.gc() like in transitionend_safety.html?

I figured it'd be good to check if the values inside passed sequence in some test function receiving sequence are not all undefined? Or maybe use specific ones? In the latter case, given how far away the actual tests in html are from testbinding.rs file, I'm not sure the specified value usage will be clear.

By the was, is there a way to run only a specific .html test using ./mach test wpt <test>? Pointing to the tests/wpt/mozilla/tests/mozilla executes the full test suite with 1335 tests, but when I point to a specific .html inside test run starts but doesn't execute a single test.
Found guide at https://github.com/servo/servo/blob/master/tests/wpt/README.md. Running a single test using ./mach test-wpt <path-to-html> now.

Xanewok · 2017-12-29T23:10:16Z

Added an additional WPT test. Hope this is what you had in mind, didn't touch Servo tests before 🤔

emilio

Thanks a lot for finally fixing this! :)

emilio · 2018-01-03T02:41:53Z

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

 def argument_type(descriptorProvider, ty, optional=False, defaultValue=None, variadic=False):
    info = getJSToNativeConversionInfo(
-        ty, descriptorProvider, isArgument=True)
+        ty, descriptorProvider, isArgument=True,
+        isAutoRooted=True if type_needs_auto_root(ty) else False)


nit: just isAutoRooted=type_needs_auto_root(ty)?

Right, thanks!

jdm

The codegen changes look much nicer. Thanks!

jdm · 2018-01-03T20:10:05Z

tests/wpt/mozilla/tests/mozilla/custom_auto_rooter.html

+</head>
+<script>
+test(function() {
+  function assert_array_equals(lhs, rhs) {


This should already exist: http://web-platform-tests.org/writing-tests/testharness-api.html

Fixed, thanks!

jdm · 2018-01-03T20:10:05Z

tests/wpt/mozilla/tests/mozilla/custom_auto_rooter.html

+  assert_array_equals(seq, t.anySequencePassthrough(seq));
+
+  var seq = new Array(null, {'inner': 1, 'a': 'as', 'toString': function() { seq = null; return Object.prototype.toString(this); }}, {'inner': 2}, null, undefined);
+  assert_array_equals(seq, t.anySequencePassthrough(seq));


Just to verify, have you checked if any of these tests fail without your codegen changes?

Yes. Previously, with Xanewok@01d1039:

var seq = [5, "str", { key: "val" }, undefined]; console.log("seq: " + seq); var lvalue = t.anySequencePassthrough(seq); console.log("lvalue: " + lvalue); console.log("rvalue: " + t.anySequencePassthrough(seq)); assert_array_equals(seq, t.anySequencePassthrough(seq), "Returned simple array is the same");

errors with:

0:13.20 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:27314) "seq: 5,str,[object Object]," 0:13.20 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:27314) "lvalue: 0,5e-324,5e-324,5e-324" 0:13.20 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:27314) "rvalue: 0,5e-324,5e-324,5e-324" ... Expected PASS, got FAIL assert_array_equals: Returned simple array is the same property 0, expected 0 but got 5


        Auto merge of #388 - Xanewok:remove-handle-conv, r=jdm

Remove FromJSValConvertible implementation for HandleValue Needed for servo/servo#19644. Removes FromJSValConvertible implementations for `HandleValue`, so `Vec<HandleValue>` conversion is impossible. Also adds the implementation for `*mut JSObject` so the `sequence<object>` -> `CustomAutoRooterGuard<Vec<*mut JSObject>>` conversion works.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/388)

jdm · 2018-01-03T22:16:10Z

Once servo/rust-mozjs#388 merges then we can update this to not use the branch.


        Auto merge of #388 - Xanewok:remove-handle-conv, r=jdm

Remove FromJSValConvertible implementation for HandleValue Needed for servo/servo#19644. Removes FromJSValConvertible implementations for `HandleValue`, so `Vec<HandleValue>` conversion is impossible. Also adds the implementation for `*mut JSObject` so the `sequence<object>` -> `CustomAutoRooterGuard<Vec<*mut JSObject>>` conversion works.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/388)

bors-servo · 2018-01-05T09:36:03Z

☔ The latest upstream changes (presumably #19614) made this pull request unmergeable. Please resolve the merge conflicts.


        Auto merge of #388 - Xanewok:remove-handle-conv, r=jdm

Remove FromJSValConvertible implementation for HandleValue Needed for servo/servo#19644. Removes FromJSValConvertible implementations for `HandleValue`, so `Vec<HandleValue>` conversion is impossible. Also adds the implementation for `*mut JSObject` so the `sequence<object>` -> `CustomAutoRooterGuard<Vec<*mut JSObject>>` conversion works.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/388)

jdm · 2018-01-05T17:08:12Z

Upstream PR merged.


        Root sequence<{any,object}> IDL arguments using CustomAutoRooter

Also pulls in mozjs 0.1.10 to support the change.

Xanewok · 2018-01-05T19:02:58Z

r? @jdm

jdm · 2018-01-05T19:55:43Z

@bors-servo r+

bors-servo · 2018-01-05T19:55:43Z

📌 Commit a01d1ea has been approved by jdm

bors-servo · 2018-01-05T20:02:54Z

⌛ Testing commit a01d1ea with merge 989d2fd...


        Auto merge of #19644 - Xanewok:root-seq-any, r=jdm

Root sequence<any> params using CustomAutoRooter  Attempt at #16678. At the moment this pulls an external [remove-handle-conv](https://github.com/Xanewok/rust-mozjs/tree/remove-handle-conv) branch for rust-mozjs (removing `FromJSValConvertible` implementation for `HandleValue` and adding one for `*mut JSObject`) In short, this roots the `sequence<any>` and `sequence<object>` function arguments using `CustomAutoRooter` (introduced in servo/rust-mozjs#382). As per #16678 (comment) the underlying vector contains raw gc thing pointers instead of handles. To do that, this introduces new possible `isMember = "AutoRoot"` value in CodegenRust.py. The rooted argument is passed to a function wrapped in a `CustomAutoRooterGuard` [RAII root guard](https://github.com/servo/rust-mozjs/blob/ed5e37a288b5738d9b571b8100b4a22a2c00f075/src/rust.rs#L586-L588) (e.g. as `CustomAutoRooterGuard<Vec<JSVal>>`) I felt quite out of my depth when trying to adapt CodegenRust.py code, so I'd appreciate any help with how can be done better/in a more clean manner. Also the name `CustomAutoRooterGuard` is quite lengthy, so I'm also open to changing it (`AutoRoot`? `AutoRootGuard`?) ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #16678 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19644)

bors-servo · 2018-01-05T21:08:05Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 989d2fd to master...

highfive assigned asajeffrey Dec 25, 2017

highfive added the S-awaiting-review label Dec 25, 2017

Xanewok changed the title ~~Root seq any~~ Root sequence<any> params using CustomAutoRooter Dec 25, 2017

Xanewok mentioned this pull request Dec 25, 2017

Remove FromJSValConvertible implementation for HandleValue servo/rust-mozjs#388

Merged

jdm mentioned this pull request Dec 28, 2017

Implement bindings for WebIDL typed arrays #5014

Closed

jdm requested changes Dec 29, 2017

View changes

jdm added S-needs-code-changes S-needs-tests and removed S-awaiting-review labels Dec 29, 2017

jdm assigned jdm and unassigned asajeffrey Dec 29, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 29, 2017

Xanewok force-pushed the Xanewok:root-seq-any branch 2 times, most recently from a08c5db to 7b66289 Dec 30, 2017

emilio reviewed Jan 3, 2018

View changes

Xanewok force-pushed the Xanewok:root-seq-any branch from 312eb37 to eaebf21 Jan 3, 2018

Xanewok mentioned this pull request Jan 3, 2018

Unions containing objects and primitive types are broken when converting from a JS value #17011

Closed

jdm requested changes Jan 3, 2018

View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-tests labels Jan 3, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 3, 2018

jdm approved these changes Jan 3, 2018

View changes

jdm added S-blocked-on-external and removed S-awaiting-review labels Jan 3, 2018

highfive added the S-needs-rebase label Jan 5, 2018

jdm added S-needs-code-changes and removed S-blocked-on-external labels Jan 5, 2018

jdm added the S-needs-squash label Jan 5, 2018

Root sequence<{any,object}> IDL arguments using CustomAutoRooter

Loading status checks…

a01d1ea

Also pulls in mozjs 0.1.10 to support the change.

Xanewok force-pushed the Xanewok:root-seq-any branch from cc568cd to a01d1ea Jan 5, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 5, 2018

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase S-needs-squash labels Jan 5, 2018

bors-servo merged commit a01d1ea into servo:master Jan 5, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jan 5, 2018

servo / servo

Root sequence<any> params using CustomAutoRooter #19644

Root sequence<any> params using CustomAutoRooter #19644

Xanewok commented Dec 25, 2017 •

edited

highfive commented Dec 25, 2017

highfive commented Dec 25, 2017

highfive commented Dec 25, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Xanewok commented Dec 29, 2017 •

edited

Xanewok commented Dec 29, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Jan 3, 2018

bors-servo commented Jan 5, 2018

jdm commented Jan 5, 2018

Xanewok commented Jan 5, 2018

jdm commented Jan 5, 2018

bors-servo commented Jan 5, 2018

bors-servo commented Jan 5, 2018

bors-servo commented Jan 5, 2018

servo / servo

Join GitHub today

Root sequence<any> params using CustomAutoRooter #19644

Root sequence<any> params using CustomAutoRooter #19644

Conversation

Xanewok commented Dec 25, 2017 • edited

highfive commented Dec 25, 2017

highfive commented Dec 25, 2017

highfive commented Dec 25, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Xanewok commented Dec 29, 2017 • edited

Xanewok commented Dec 29, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Jan 3, 2018

bors-servo commented Jan 5, 2018

jdm commented Jan 5, 2018

Xanewok commented Jan 5, 2018

jdm commented Jan 5, 2018

bors-servo commented Jan 5, 2018

bors-servo commented Jan 5, 2018

bors-servo commented Jan 5, 2018

Xanewok commented Dec 25, 2017 •

edited

Xanewok commented Dec 29, 2017 •

edited