Use higher level handles from JS bindings #20246

marmistrz · 2018-03-08T18:23:00Z

…reak servo

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Make Handle objects take lifetimes rust-mozjs#153

There are tests for these changes OR
These changes do not require tests because this is just migration to a lifetimed API

This change is

jdm · 2018-03-08T19:20:10Z

The build appears unbroken, which honestly makes me extremely skeptical!

jdm · 2018-03-08T19:20:54Z

Cargo.lock contains this modification:

+[[patch.unused]]
+name = "mozjs"
+version = "0.1.13"
+source = "git+https://github.com/marmistrz/rust-mozjs#43316429560aecc3abdf807d56eb4ac1a0912e60"

which means the updated rust-mozjs wasn't used.

marmistrz · 2018-03-08T19:49:31Z

Thanks! Why was explicit cargo update needed? Where did you find this modification to Cargo.lock?

jdm · 2018-03-08T20:11:35Z

The modification showed up in TravisCI because we have a job that checks if Cargo.lock was modified during the build. The explicit update was necessary because the version in the Cargo.toml that was used by the patch did not match the version that was included in Cargo.lock already.

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

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

jdm · 2018-03-27T18:51:08Z

components/script/dom/bindings/proxyhandler.rs

@@ -166,7 +178,7 @@ pub unsafe fn get_expando_object(obj: HandleObject, expando: MutableHandleObject

 /// Get the expando object, or create it if it doesn't exist yet.
 /// Fails on JSAPI failure.
-pub unsafe fn ensure_expando_object(cx: *mut JSContext, obj: HandleObject, expando: MutableHandleObject) {
+pub unsafe fn ensure_expando_object(cx: *mut JSContext, obj: RawHandleObject, mut expando: MutableHandleObject) {


Why is this a raw handle?

Because using lifetimed handles would require 2 Handle::from_raw's (get_expando_object and some extern "C" function) instead of one into() in the codegen.

Is it ok then or should I switch to Handle?

This is ok.

jdm · 2018-03-27T18:51:27Z

components/script/dom/bindings/proxyhandler.rs

                                                   -> bool {
    *is_ordinary = true;
    proto.set(GetStaticPrototype(proxy.get()));
    true
 }

 /// Get the expando object, or null if there is none.
-pub unsafe fn get_expando_object(obj: HandleObject, expando: MutableHandleObject) {
+pub unsafe fn get_expando_object(obj: RawHandleObject, mut expando: MutableHandleObject) {


Let's take a non-raw handle here.

get_expando_object is always called from extern C functions, which need to have raw handles. With one exception of ensure_expando_handle.

I see no benefit here.

That is a fair argument.

jdm · 2018-03-27T18:53:15Z

components/script/dom/bindings/proxyhandler.rs

                                               -> DOMProxyShadowsResult {
    // TODO: support OverrideBuiltins when #12978 is fixed.

    rooted!(in(cx) let mut expando = ptr::null_mut::<JSObject>());
    get_expando_object(object, expando.handle_mut());
    if !expando.get().is_null() {
        let mut has_own = false;
-        if !JS_AlreadyHasOwnPropertyById(cx, expando.handle(), id, &mut has_own) {
+        if !JS_AlreadyHasOwnPropertyById(


Let's use a separate local variable for this rather than spread the if condition over multiple lines.

jdm · 2018-03-27T18:54:09Z

components/script/dom/bindings/mozmap.rs

@@ -96,10 +96,10 @@ impl<T: ToJSValConvertible> ToJSValConvertible for MozMap<T> {
            value.to_jsval(cx, js_value.handle_mut());

            assert!(JS_DefineUCProperty2(cx,


What if we call the wrapper function here instead?

Good catch! :)

jdm · 2018-03-27T18:54:46Z

components/script/dom/bindings/interface.rs

@@ -443,7 +455,7 @@ unsafe fn define_name(cx: *mut JSContext, obj: HandleObject, name: &[u8]) {
    assert!(JS_DefineProperty2(cx,


What if we call the wrapper function here instead?

but we're using the wrapper:

use js::rust::wrappers::{JS_DefineProperty, JS_DefineProperty1, JS_DefineProperty2};

Then how are we passing a raw handle as an argument?

The fourth argument is a HandleString, which is defined in js::jsapi but was never a part of js::rust, so I didn't add it either.

Should I introduce a Rusty equivalent of

typedef Handle<JSString*> HandleString

into rust-mozjs/js::rust?

Yes please. We can do it in a followup PR though.

jdm · 2018-03-27T18:57:27Z

components/script/dom/bindings/interface.rs

    rval.set(JS_NewObjectWithUniqueType(cx, class, proto));
-    assert!(!rval.ptr.is_null());


!rval.get().is_null()

jdm · 2018-03-27T18:57:40Z

components/script/dom/bindings/interface.rs

    assert!(!constants.is_empty());
    rval.set(JS_NewObject(cx, ptr::null()));
-    assert!(!rval.ptr.is_null());


!rval.get().is_null()

jdm · 2018-03-27T19:01:03Z

components/script/dom/bindings/utils.rs

@@ -144,8 +153,8 @@ pub unsafe fn get_property_on_prototype(cx: *mut JSContext,
        return false;
    }
    *found = has_property;
-    let no_output = vp.ptr.is_null();
-    if !has_property || no_output {
+    // let no_output = vp.ptr.is_null(); // TODO can we remove?


Yes, we can remove this one.

I think we can still check !rval.get().is_null().

Why do we remove checks in one place but leave them in the other?

Because this check was wrong to begin with (we should have been checking vp.get.is_null()), and it was removed from the equivalent upstream code two years ago.

jdm · 2018-03-27T19:10:49Z

components/script/dom/eventtarget.rs

@@ -453,7 +454,7 @@ impl EventTarget {
                            args.as_ptr(),
                            body.as_ptr(),
                            body.len() as size_t,
-                            handler.handle_mut())
+                            handler.handle_mut().into())


What about calling the wrapper function?

handler is a MutableHandleFunction, a type which has never been a part of js::rust.

This can be improved along with the HandleString in a followup PR.

jdm · 2018-03-27T19:16:34Z

components/script/timers.rs

@@ -520,7 +521,7 @@ impl JsTimerTask {
    // Returning Handles directly from Heap values is inherently unsafe, but here it's
    // always done via rooted JsTimers, which is safe.
    #[allow(unsafe_code)]
-    fn collect_heap_args(&self, args: &[Heap<JSVal>]) -> Vec<HandleValue> {
+    fn collect_heap_args<'b>(&self, args: &'b[Heap<JSVal>]) -> Vec<HandleValue<'b>> {


nit: space before [

jdm · 2018-03-27T19:17:16Z

@bors-servo try

bors-servo · 2018-03-27T19:17:21Z

⌛ Trying commit 5dc2d25 with merge ce6a8b5...

Use higher level handles from JS bindings …reak servo  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix servo/rust-mozjs#153  - [ ] There are tests for these changes OR - [x] These changes do not require tests because this is just migration to a lifetimed API    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20246)

bors-servo · 2018-03-27T19:23:50Z

💔 Test failed - linux-rel-wpt

bors-servo · 2018-03-28T02:44:37Z

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

bors-servo · 2018-03-28T17:29:47Z

💔 Test failed - linux-rel-css

…zjs#393

marmistrz · 2018-03-28T19:29:57Z

@jdm squashed and ready to merge. ./mach test-tidy --all returns no issues on my local machine.

jdm · 2018-03-28T19:41:40Z

@bors-servo r+

bors-servo · 2018-03-28T19:41:40Z

📌 Commit 356c57e has been approved by jdm

bors-servo · 2018-03-28T20:22:05Z

⌛ Testing commit 356c57e with merge 9149524...

Use higher level handles from JS bindings …reak servo  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix servo/rust-mozjs#153  - [ ] There are tests for these changes OR - [x] These changes do not require tests because this is just migration to a lifetimed API    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20246)

bors-servo · 2018-03-28T21:18:26Z

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

jdm · 2018-03-28T23:03:29Z

Exciting! Thanks for pushing through all the problems @marmistrz!

Lifetime all the type aliases for Handle and MutableHandle That's a follow-up to servo/servo#20246 Especially: * servo/servo#20246 (comment) * servo/servo#20246 (comment)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/420)

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 8, 2018

highfive added the S-needs-rebase There are merge conflict errors. label Mar 12, 2018

marmistrz force-pushed the test-lifetime-handles branch 3 times, most recently from 1b872a1 to 4946b19 Compare March 14, 2018 20:38

marmistrz closed this Mar 23, 2018

marmistrz force-pushed the test-lifetime-handles branch from 4946b19 to 91398cf Compare March 23, 2018 14:36

marmistrz reopened this Mar 27, 2018

marmistrz force-pushed the test-lifetime-handles branch from 525e924 to 9f5ecd5 Compare March 27, 2018 17:22

marmistrz changed the title ~~[Do not merge] A dummy PR to track if servo/rust-mozjs#393 doesn't break servo~~ Adapt servo for servo/rust-mozjs#393 changes Mar 27, 2018

jdm changed the title ~~Adapt servo for servo/rust-mozjs#393 changes~~ Use higher level handles from JS bindings Mar 27, 2018

jdm removed the S-needs-rebase There are merge conflict errors. label Mar 27, 2018

jdm requested changes Mar 27, 2018

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 27, 2018

highfive added the S-tests-failed The changes caused existing tests to fail. label Mar 27, 2018

highfive added S-needs-rebase There are merge conflict errors. and removed S-tests-failed The changes caused existing tests to fail. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 28, 2018

jdm removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. S-tests-failed The changes caused existing tests to fail. labels Mar 28, 2018

highfive added S-tests-failed The changes caused existing tests to fail. S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Mar 28, 2018

jdm added S-fails-tidy `./mach test-tidy` reported errors. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 28, 2018

Adapt Servo for mozjs 0.6 and the changes introduced in servo/rust-mo…

356c57e

…zjs#393

marmistrz force-pushed the test-lifetime-handles branch from 3bd097f to 356c57e Compare March 28, 2018 19:28

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 28, 2018

highfive assigned jdm Mar 28, 2018

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Mar 28, 2018

bors-servo merged commit 356c57e into servo:master Mar 28, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 28, 2018

This was referenced Mar 28, 2018

[WIP] Make DOM bindings methods not take raw JSContext pointers #20424

Closed

Implement WebGL getUniform API #20330

Closed

jdm added the This Week In Servo (TWiS) label Mar 28, 2018

marmistrz mentioned this pull request Apr 4, 2018

Lifetime all the type aliases for Handle and MutableHandle servo/rust-mozjs#420

Merged

		@@ -96,10 +96,10 @@ impl<T: ToJSValConvertible> ToJSValConvertible for MozMap<T> {
		value.to_jsval(cx, js_value.handle_mut());

		assert!(JS_DefineUCProperty2(cx,

		@@ -443,7 +455,7 @@ unsafe fn define_name(cx: *mut JSContext, obj: HandleObject, name: &[u8]) {
		assert!(JS_DefineProperty2(cx,

		rval.set(JS_NewObjectWithUniqueType(cx, class, proto));
		assert!(!rval.ptr.is_null());

Use higher level handles from JS bindings #20246

Use higher level handles from JS bindings #20246

Conversation

marmistrz commented Mar 8, 2018 • edited Loading

jdm commented Mar 8, 2018

jdm commented Mar 8, 2018

marmistrz commented Mar 8, 2018 • edited Loading

jdm commented Mar 8, 2018

bors-servo commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmistrz Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Mar 27, 2018

bors-servo commented Mar 27, 2018

bors-servo commented Mar 27, 2018

bors-servo commented Mar 28, 2018

bors-servo commented Mar 28, 2018

marmistrz commented Mar 28, 2018 • edited Loading

jdm commented Mar 28, 2018

bors-servo commented Mar 28, 2018

bors-servo commented Mar 28, 2018

bors-servo commented Mar 28, 2018

jdm commented Mar 28, 2018

marmistrz commented Mar 8, 2018 •

edited

Loading

marmistrz commented Mar 8, 2018 •

edited

Loading

marmistrz Mar 27, 2018 •

edited

Loading

marmistrz commented Mar 28, 2018 •

edited

Loading