Promise rooting is not sound

[jdm]

The second commit in #15080 exposes a crash that involves a TrustedPromise value that has been garbage collected by the time we try to turn it into a stack root once more. The backtrace shows a nursery GC occurring, which means that our trace hooks don't get called. The promise object is nursery-allocated, I guess, which would explain why it's GCed during the minor collection.

[jdm]

It may also be a case of the Promise object being moved out of the nursery, rather than collected. I don't know enough about SM to determine that yet.

[dati91]

If I can help you with this anyway just let me know!

[Ms2ger]

The js-side Promise object is indeed in the nursery when the dom::promise::Promise is created.

[tschneidereit]

If the underlying JS Promise instances are created via JSAPI functions, we can add support for allocating them in the tenured heap easily. If not, then a more fundamental fix is needed.

I'm pretty sure that this will become relevant for ReadableStream and WritableStream, too once we have those.

[jdm]

Yes, the instances are created via Promise::new which uses NewPromiseObject.

[jdm]

That sounds like it wouldn't work for WebIDL APIs that accept existing Promises, though, which could be created via new Promise() in JS.

[tschneidereit]

That sounds like it wouldn't work for WebIDL APIs that accept existing Promises, though, which could be created via new Promise() in JS.

It wouldn't, no. Since that needs to be supported, TrustedPromise will have to be able to deal with tenuring, I'm afraid.

Oh, and I just realize that pre-tenuring wouldn't fully solve this, because compacting GC would still cause Promise instances to be moved.

[jdm]

https://gist.github.com/jdm/b36d32d887e1fd6887b6d60c6cae218d is the testcase I'm using to try to reproduce this outside of Servo. I'm triggering a minor GC via DumpHeap, but it doesn't crash like I would expect it to.

[jdm]

It turns out my testcase wasn't replicating the issue from Servo correctly - specifically, it was stashing the promise object in a Heap value, which correctly uses post-barriers to ensure that the pointer gets updated when the promise object is moved out of the nursery. Servo's Reflector type contains an UnsafeCell<*mut JSObject>, rather than a Heap<*mut JSObject> as one might expect. The set_jsobject method of Reflector sets the pointer without using a post-barrier (unlike Heap<*mut JSObject>'s implementation), and the DOM Promise constructor calls that (via init_reflector) to initialize the promise's reflector field. Since Promises are one of the very few (only?) non-tenured objects stored in Reflector structures, we haven't encountered this problem until now.

[jdm]

There are two possible solutions that I see:

• add a post-barrier to Reflector::set_jsobject (this fixes the crash from #15080)
• replace the UnsafeCell in Reflector with Heap instead

I'm not sure why we aren't using Heap already. I'm guessing either performance concerns or historical artifact of when Heap wasn't supported from Rust code yet?

[jdm]

diff --git a/components/script/dom/promise.rs b/components/script/dom/promise.rs
index 89bbd08..a727e50 100644
--- a/components/script/dom/promise.rs
+++ b/components/script/dom/promise.rs
@@ -90,12 +90,12 @@ impl Promise {
     #[allow(unsafe_code, unrooted_must_root)]
     unsafe fn new_with_js_promise(obj: HandleObject, cx: *mut JSContext) -> Rc<Promise> {
         assert!(IsPromiseObject(obj));
-        let mut promise = Promise {
+        let promise = Promise {
             reflector: Reflector::new(),
             permanent_js_root: MutHeapJSVal::new(),
         };
-        promise.init_reflector(obj.get());
-        let promise = Rc::new(promise);
+        let mut promise = Rc::new(promise);
+        Rc::get_mut(&mut promise).unwrap().init_reflector(obj.get());
         promise.initialize(cx);
         promise
     }
diff --git a/components/script/dom/bindings/reflector.rs b/components/script/dom/bindings/reflector.rs
index eebea3a..6f93745 100644
--- a/components/script/dom/bindings/reflector.rs
+++ b/components/script/dom/bindings/reflector.rs
@@ -8,6 +8,7 @@ use dom::bindings::conversions::DerivedFrom;
 use dom::bindings::js::Root;
 use dom::globalscope::GlobalScope;
 use js::jsapi::{HandleObject, JSContext, JSObject};
+use js::rust::GCMethods;
 use std::cell::UnsafeCell;
 use std::ptr;

@@ -58,6 +59,7 @@ impl Reflector {
             assert!((*obj).is_null());
             assert!(!object.is_null());
             *obj = object;
+            GCMethods::post_barrier(obj, ptr::null_mut(), object);
         }
     }

[jdm]

And a straightforward testcase, now that the problem is fully clear:

diff --git a/tests/wpt/mozilla/tests/mozilla/promise.html b/tests/wpt/mozilla/tests/mozilla/promise.html
index fa16edc..729e878 100644
--- a/tests/wpt/mozilla/tests/mozilla/promise.html
+++ b/tests/wpt/mozilla/tests/mozilla/promise.html
@@ -58,6 +58,7 @@
       var p = new Promise(function() {});
       var start = Date.now();
       t.resolvePromiseDelayed(p, 'success', 100);
+      test.step_timeout(function() { window.gc() }, 0);
       return p.then(function(v) {
           var end = Date.now();
           assert_greater_than_equal(end - start, 100);