Fix JS object conversion in unions #20265

Xanewok · 2018-03-10T14:00:06Z

Requires safe Heap::boxed constructor from servo/rust-mozjs#395 (more info on it is in the PR).

Since unions currently assume that their respective members root themselves and can be stored on heap, I modified the union member object conversion branch to convert to a RootedTraceableBox<Heap<*mut JSObject>> (which is the currently generated type for objects in said unions).

I did it only for Unions and not dictionaries, since some dictionaries had bare *mut JSObject members - is this a mistake and something that needs further fixing?

Does this need a test, e.g. passing a union with object to a function that returns said object, and comparing the results for equality?

r? @jdm

Generated code with this patch (for StringOrObject):

impl FromJSValConvertible for StringOrObject {
    type Config = ();
    unsafe fn from_jsval(cx: *mut JSContext,
                         value: HandleValue,
                         _option: ())
                         -> Result<ConversionResult<StringOrObject>, ()> {
        if value.get().is_object() {
            match StringOrObject::TryConvertToObject(cx, value) {
                Err(_) => return Err(()),
                Ok(Some(value)) => return Ok(ConversionResult::Success(StringOrObject::Object(value))),
                Ok(None) => (),
            }

        }
        // (...)
    }
}

impl StringOrObject {
    // (...)
    unsafe fn TryConvertToObject(cx: *mut JSContext, value: HandleValue) -> Result<Option<RootedTraceableBox<Heap<*mut JSObject>>>, ()> {
        Ok(Some(RootedTraceableBox::from_box(Heap::boxed(value.get().to_object()))))
    }
}

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

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

This change is

highfive · 2018-03-10T14:00:14Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/trace.rs
@fitzgen: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/trace.rs
@KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/trace.rs

highfive · 2018-03-10T14:00:16Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

Xanewok · 2018-03-10T17:14:29Z

It seems that Taskcluster failed on linux-tests with...

It looks like rustup is not installed. See instructions at https://github.com/servo/servo/#setting-up-your-environment
Build FAILED in 0:00:00

Should I disregard that?

bors-servo · 2018-03-12T14:33:24Z

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

jdm · 2018-03-12T19:36:07Z

Yes, you can always disregard taskcluster.

jdm · 2018-03-12T23:12:05Z

Oh, this is the same commit I reviewed in the typed array PR, isn't it? I think adding a test here would be a useful improvement.

Xanewok · 2018-03-13T02:06:55Z

Added a WPT test where check if returned value is the same as the argument passed to an identity function. (On an unrelated note I think it might be a good idea to separate a 'codegen' or similar directory that'd house custom_auto_rooter.html and codegen_union.html and possibly others related in the future?)

On master the test fails with assert_equals: expected (object) object "[object Object]" but got (string) "[object Object]", where it tries to convert the argument to string, but the test passes with this branch.

Is this test good enough?

Now what's left is to use mozjs 0.1.14 when it's published and fix *mut JSObject in dictionaries, in addition to unions.

bors-servo · 2018-03-13T12:37:34Z

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

jdm · 2018-03-13T20:44:19Z

1.14 has finally been published.


        Use mozjs 0.1.14


        Support objects in WebIDL unions

Fixes #17011


        Add WPT test for WebIDL union conversion containing object values

bors-servo · 2018-03-14T13:35:09Z

📌 Commit 712812d has been approved by jdm

bors-servo · 2018-03-14T13:44:19Z

⌛ Testing commit 712812d with merge e68d4f4...


        Auto merge of #20265 - Xanewok:fix-js-objects-in-unions, r=jdm

@jdm

Fix JS object conversion in unions  Requires safe `Heap::boxed` constructor from servo/rust-mozjs#395 (more info on it is in the PR). Since unions currently assume that their respective members root themselves and can be stored on heap, I modified the union member object conversion branch to convert to a `RootedTraceableBox<Heap<*mut JSObject>>` (which is the currently generated type for objects in said unions). I did it only for Unions and not dictionaries, since some dictionaries had bare `*mut JSObject` members - is this a mistake and something that needs further fixing? Does this need a test, e.g. passing a union with object to a function that returns said object, and comparing the results for equality? r? @jdm Generated code with this patch (for `StringOrObject`): ```rust impl FromJSValConvertible for StringOrObject { type Config = (); unsafe fn from_jsval(cx: *mut JSContext, value: HandleValue, _option: ()) -> Result<ConversionResult<StringOrObject>, ()> { if value.get().is_object() { match StringOrObject::TryConvertToObject(cx, value) { Err(_) => return Err(()), Ok(Some(value)) => return Ok(ConversionResult::Success(StringOrObject::Object(value))), Ok(None) => (), } } // (...) } } impl StringOrObject { // (...) unsafe fn TryConvertToObject(cx: *mut JSContext, value: HandleValue) -> Result<Option<RootedTraceableBox<Heap<*mut JSObject>>>, ()> { Ok(Some(RootedTraceableBox::from_box(Heap::boxed(value.get().to_object())))) } } ``` ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #17011 (github issue number if applicable).  - [ ] 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/20265)

bors-servo · 2018-03-14T13:52:31Z

💔 Test failed - mac-rel-wpt3

Xanewok · 2018-03-14T14:12:37Z

It seems like this is related to the mozangle build failure?
One of the errors:

cargo:warning=gfx/angle/checkout/src/common/angleutils.h:33:5: error: unknown type name 'constexpr'

And warnings:

cargo:warning=gfx/angle/checkout/include/GLSLANG/ShaderVars.h:19:26: warning: alias declarations are a C++11 extension [-Wc++11-extensions]
cargo:warning=gfx/angle/checkout/include/GLSLANG/ShaderVars.h:48:6: warning: scoped enumerations are a C++11 extension [-Wc++11-extensions]
cargo:warning=gfx/angle/checkout/include/GLSLANG/ShaderVars.h:289:23: warning: defaulted function definitions are a C++11 extension [-Wc++11-extensions]
cargo:warning=gfx/angle/checkout/include/GLSLANG/ShaderLang.h:276:26: warning: alias declarations are a C++11 extension [-Wc++11-extensions]
cargo:warning=gfx/angle/checkout/include/GLSLANG/ShaderLang.h:459:18: warning: alias declarations are a C++11 extension [-Wc++11-extensions]
cargo:warning=gfx/angle/checkout/src/common/angleutils.h:34:22: warning: defaulted function definitions are a C++11 extension [-Wc++11-extensions]
cargo:warning=gfx/angle/checkout/src/common/angleutils.h:37:39: warning: deleted function definitions are a C++11 extension [-Wc++11-extensions]
cargo:warning=gfx/angle/checkout/src/common/angleutils.h:38:42: warning: deleted function definitions are a C++11 extension [-Wc++11-extensions]
cargo:warning=gfx/angle/checkout/src/common/angleutils.h:108:10: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]
cargo:warning=gfx/angle/checkout/src/common/angleutils.h:108:24: warning: range-based for loop is a C++11 extension [-Wc++11-extensions]
cargo:warning=gfx/angle/checkout/src/common/debug.h:37:21: warning: alias declarations are a C++11 extension [-Wc++11-extensions]
warning: build failed, waiting for other jobs to finish...

Xanewok · 2018-03-14T14:42:14Z

@bors-servo retry

bors-servo · 2018-03-14T14:42:14Z

@Xanewok: 🔑 Insufficient privileges: and not in try users

jdm · 2018-03-14T14:55:49Z

@bors-servo retry

#20281

Xanewok · 2018-03-14T14:58:51Z

Ah, I was confused by the star, my bad!

bors-servo · 2018-03-14T16:04:26Z

⌛ Testing commit 712812d with merge e597cd9...


        Auto merge of #20265 - Xanewok:fix-js-objects-in-unions, r=jdm

@jdm

Fix JS object conversion in unions  Requires safe `Heap::boxed` constructor from servo/rust-mozjs#395 (more info on it is in the PR). Since unions currently assume that their respective members root themselves and can be stored on heap, I modified the union member object conversion branch to convert to a `RootedTraceableBox<Heap<*mut JSObject>>` (which is the currently generated type for objects in said unions). I did it only for Unions and not dictionaries, since some dictionaries had bare `*mut JSObject` members - is this a mistake and something that needs further fixing? Does this need a test, e.g. passing a union with object to a function that returns said object, and comparing the results for equality? r? @jdm Generated code with this patch (for `StringOrObject`): ```rust impl FromJSValConvertible for StringOrObject { type Config = (); unsafe fn from_jsval(cx: *mut JSContext, value: HandleValue, _option: ()) -> Result<ConversionResult<StringOrObject>, ()> { if value.get().is_object() { match StringOrObject::TryConvertToObject(cx, value) { Err(_) => return Err(()), Ok(Some(value)) => return Ok(ConversionResult::Success(StringOrObject::Object(value))), Ok(None) => (), } } // (...) } } impl StringOrObject { // (...) unsafe fn TryConvertToObject(cx: *mut JSContext, value: HandleValue) -> Result<Option<RootedTraceableBox<Heap<*mut JSObject>>>, ()> { Ok(Some(RootedTraceableBox::from_box(Heap::boxed(value.get().to_object())))) } } ``` ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #17011 (github issue number if applicable).  - [ ] 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/20265)

bors-servo · 2018-03-14T17:25:55Z

☀️ 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 e597cd9 to master...

Xanewok · 2018-03-14T17:27:15Z

🎉 Now onto the typed arrays!


        Auto merge of #20267 - Xanewok:typed-arrays-stack-heap-variants, r=Xa…

@jdm

…newok Support JS typed arrays as arguments and in WebIDL unions  Supersedes #20205. This brings support to receiving typed arrays as function arguments (those are stack-rooted with CustomAutoRooter) and also being a member of a union (which is basically heap-rooted? similarly to other webidl unions). This is based on my other PR #20265 (which means it has to pull an external rust-mozjs branch and contains some also slightly unrelated changes here) since it shares `RootedTraceableBox::from_box` here and some other additional changes at rust-mozjs, but it can be easily separated if needed. I tried adding support to nullable typed arrays but couldn't work around an issue of codegen always sticking a "OrNull" at the end of my type (presumably because of [this](https://github.com/servo/servo/blob/master/components/script/dom/bindings/codegen/parser/WebIDL.py#L2241)?). How would I go about avoiding the suffix with nullable arguments? If we were to add also support for nullable typed arrays then I think we wouldn't be blocked anymore on this in WebGL 1.0. r? @jdm ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] 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/20267)


        Auto merge of #20314 - Xanewok:remove-heap-constructor, r=jdm

@jdm

Don't use unsafe Heap::new constructor  Pulls servo/rust-mozjs#398 and aims to close servo/rust-mozjs#343. We can't convert from `JSVal` to `Heap<JSVal>` safely (due to GC barriers we can't move Heap value after changing its underlying value to something meaningful, e.g. non-null or non-undefined), so I decided to also wrap the Heap values in a Box (and in dictionaries in RootedTraceableBox, see #20265 (comment) and the issue for more details) in dictionaries. Since we allocate more to be safe, I think it'd be good to also do some sort of a JS perf run, if there is any to see if there's any significant overhead. r? @jdm ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because checking for not moving Heap after setting a value would require encoding a lot more info in type system (Heap) and I'm not sure how to do that and end up with an ergonomic and consistent API    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20314)


        servo: Merge #20314 - Don't use unsafe Heap::new constructor (from Xa…

@jdm

…newok:remove-heap-constructor); r=jdm  Pulls servo/rust-mozjs#398 and aims to close servo/rust-mozjs#343. We can't convert from `JSVal` to `Heap<JSVal>` safely (due to GC barriers we can't move Heap value after changing its underlying value to something meaningful, e.g. non-null or non-undefined), so I decided to also wrap the Heap values in a Box (and in dictionaries in RootedTraceableBox, see servo/servo#20265 (comment) and the issue for more details) in dictionaries. Since we allocate more to be safe, I think it'd be good to also do some sort of a JS perf run, if there is any to see if there's any significant overhead. r? @jdm ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because checking for not moving Heap after setting a value would require encoding a lot more info in type system (Heap) and I'm not sure how to do that and end up with an ergonomic and consistent API   Source-Repo: https://github.com/servo/servo Source-Revision: 23b6f569d09f9b10a70ca1b5c8a48f7da45853e2 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : c609cd593bbb75cbbe584687a7caf814de3059fc


        servo: Merge #20314 - Don't use unsafe Heap::new constructor (from Xa…

…newok:remove-heap-constructor); r=jdm  Pulls servo/rust-mozjs#398 and aims to close servo/rust-mozjs#343. We can't convert from `JSVal` to `Heap<JSVal>` safely (due to GC barriers we can't move Heap value after changing its underlying value to something meaningful, e.g. non-null or non-undefined), so I decided to also wrap the Heap values in a Box (and in dictionaries in RootedTraceableBox, see servo/servo#20265 (comment) and the issue for more details) in dictionaries. Since we allocate more to be safe, I think it'd be good to also do some sort of a JS perf run, if there is any to see if there's any significant overhead. r? jdm ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because checking for not moving Heap after setting a value would require encoding a lot more info in type system (Heap) and I'm not sure how to do that and end up with an ergonomic and consistent API   Source-Repo: https://github.com/servo/servo Source-Revision: 23b6f569d09f9b10a70ca1b5c8a48f7da45853e2 UltraBlame original commit: e520f3e643b75c05fa5f4b31a584e69920e06d52


        servo: Merge #20314 - Don't use unsafe Heap::new constructor (from Xa…

…newok:remove-heap-constructor); r=jdm  Pulls servo/rust-mozjs#398 and aims to close servo/rust-mozjs#343. We can't convert from `JSVal` to `Heap<JSVal>` safely (due to GC barriers we can't move Heap value after changing its underlying value to something meaningful, e.g. non-null or non-undefined), so I decided to also wrap the Heap values in a Box (and in dictionaries in RootedTraceableBox, see servo/servo#20265 (comment) and the issue for more details) in dictionaries. Since we allocate more to be safe, I think it'd be good to also do some sort of a JS perf run, if there is any to see if there's any significant overhead. r? jdm ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because checking for not moving Heap after setting a value would require encoding a lot more info in type system (Heap) and I'm not sure how to do that and end up with an ergonomic and consistent API   Source-Repo: https://github.com/servo/servo Source-Revision: 23b6f569d09f9b10a70ca1b5c8a48f7da45853e2 UltraBlame original commit: e520f3e643b75c05fa5f4b31a584e69920e06d52


        servo: Merge #20314 - Don't use unsafe Heap::new constructor (from Xa…

…newok:remove-heap-constructor); r=jdm  Pulls servo/rust-mozjs#398 and aims to close servo/rust-mozjs#343. We can't convert from `JSVal` to `Heap<JSVal>` safely (due to GC barriers we can't move Heap value after changing its underlying value to something meaningful, e.g. non-null or non-undefined), so I decided to also wrap the Heap values in a Box (and in dictionaries in RootedTraceableBox, see servo/servo#20265 (comment) and the issue for more details) in dictionaries. Since we allocate more to be safe, I think it'd be good to also do some sort of a JS perf run, if there is any to see if there's any significant overhead. r? jdm ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because checking for not moving Heap after setting a value would require encoding a lot more info in type system (Heap) and I'm not sure how to do that and end up with an ergonomic and consistent API   Source-Repo: https://github.com/servo/servo Source-Revision: 23b6f569d09f9b10a70ca1b5c8a48f7da45853e2 UltraBlame original commit: e520f3e643b75c05fa5f4b31a584e69920e06d52

highfive assigned jdm Mar 10, 2018

highfive added the S-awaiting-review label Mar 10, 2018

Xanewok mentioned this pull request Mar 10, 2018

Support JS typed arrays as arguments and in WebIDL unions #20267

Merged

2 of 5 tasks complete

highfive added the S-needs-rebase label Mar 12, 2018

Xanewok force-pushed the Xanewok:fix-js-objects-in-unions branch from 1eba6c0 to 9e2e92b Mar 12, 2018

jdm added S-blocked-on-external and removed S-needs-rebase labels Mar 12, 2018

jdm added S-needs-code-changes and removed S-awaiting-review labels Mar 12, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 13, 2018

Xanewok force-pushed the Xanewok:fix-js-objects-in-unions branch from 96c6fc4 to 3067902 Mar 13, 2018

highfive added the S-needs-rebase label Mar 13, 2018

Xanewok force-pushed the Xanewok:fix-js-objects-in-unions branch 2 times, most recently from 10176a7 to 51fdc34 Mar 13, 2018

jdm removed S-awaiting-review S-needs-rebase labels Mar 13, 2018

jdm approved these changes Mar 13, 2018

View changes

Xanewok added 3 commits Mar 10, 2018

Use mozjs 0.1.14

725f9cb

Support objects in WebIDL unions

17ecbaf

Fixes #17011

Add WPT test for WebIDL union conversion containing object values

Loading status checks…

712812d

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 14, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 14, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Mar 14, 2018

bors-servo merged commit 712812d into servo:master Mar 14, 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 Mar 14, 2018

This was referenced Mar 14, 2018

Conversion to (DOMString or object) is broken #14019

Closed

Heap constructor is unsafe servo/rust-mozjs#343

Closed

Don't use unsafe Heap::new constructor #20314

Merged

servo / servo

Fix JS object conversion in unions #20265

Fix JS object conversion in unions #20265

Xanewok commented Mar 10, 2018 •

edited by SimonSapin

highfive commented Mar 10, 2018

highfive commented Mar 10, 2018

Xanewok commented Mar 10, 2018

bors-servo commented Mar 12, 2018

jdm commented Mar 12, 2018

jdm commented Mar 12, 2018

Xanewok commented Mar 13, 2018

bors-servo commented Mar 13, 2018

jdm commented Mar 13, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

Xanewok commented Mar 14, 2018

Xanewok commented Mar 14, 2018

bors-servo commented Mar 14, 2018

jdm commented Mar 14, 2018

Xanewok commented Mar 14, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

Xanewok commented Mar 14, 2018

servo / servo

Join GitHub today

Fix JS object conversion in unions #20265

Fix JS object conversion in unions #20265

Conversation

Xanewok commented Mar 10, 2018 • edited by SimonSapin

highfive commented Mar 10, 2018

highfive commented Mar 10, 2018

Xanewok commented Mar 10, 2018

bors-servo commented Mar 12, 2018

jdm commented Mar 12, 2018

jdm commented Mar 12, 2018

Xanewok commented Mar 13, 2018

bors-servo commented Mar 13, 2018

jdm commented Mar 13, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

Xanewok commented Mar 14, 2018

Xanewok commented Mar 14, 2018

bors-servo commented Mar 14, 2018

jdm commented Mar 14, 2018

Xanewok commented Mar 14, 2018

bors-servo commented Mar 14, 2018

bors-servo commented Mar 14, 2018

Xanewok commented Mar 14, 2018

Xanewok commented Mar 10, 2018 •

edited by SimonSapin