Auto merge of #15118 - jdm:reflector-barrier-crash, r=<try>

bors-servo · web-flow · commit 469a3f45aedd · 2017-01-20T08:32:46.000-08:00
Use Heap instead of UnsafeCell in DOM reflectors The previous `Reflector` implementation did not use post barriers, so we could crash when storing nursery objects in a `Reflector` structure that were later moved out of the nursery. - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15085 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15118)
diff --git a/components/script/dom/bindings/reflector.rs b/components/script/dom/bindings/reflector.rs
@@ -7,9 +7,8 @@
 use dom::bindings::conversions::DerivedFrom;
 use dom::bindings::js::Root;
 use dom::globalscope::GlobalScope;
-use js::jsapi::{HandleObject, JSContext, JSObject};
-use std::cell::UnsafeCell;
-use std::ptr;
+use js::jsapi::{HandleObject, JSContext, JSObject, Heap};
+use std::default::Default;
 
 /// Create the reflector for a new DOM object and yield ownership to the
 /// reflector.
@@ -34,44 +33,41 @@ pub fn reflect_dom_object<T, U>(
 // If you're renaming or moving this field, update the path in plugins::reflector as well
 pub struct Reflector {
     #[ignore_heap_size_of = "defined and measured in rust-mozjs"]
-    object: UnsafeCell<*mut JSObject>,
+    object: Heap<*mut JSObject>,
 }
 
 #[allow(unrooted_must_root)]
 impl PartialEq for Reflector {
     fn eq(&self, other: &Reflector) -> bool {
-        unsafe { *self.object.get() == *other.object.get() }
+        self.object.get() == other.object.get()
     }
 }
 
 impl Reflector {
     /// Get the reflector.
     #[inline]
     pub fn get_jsobject(&self) -> HandleObject {
-        unsafe { HandleObject::from_marked_location(self.object.get()) }
+        self.object.handle()
     }
 
     /// Initialize the reflector. (May be called only once.)
     pub fn set_jsobject(&mut self, object: *mut JSObject) {
-        unsafe {
-            let obj = self.object.get();
-            assert!((*obj).is_null());
-            assert!(!object.is_null());
-            *obj = object;
-        }
+        assert!(self.object.get().is_null());
+        assert!(!object.is_null());
+        self.object.set(object);
     }
 
     /// Return a pointer to the memory location at which the JS reflector
     /// object is stored. Used to root the reflector, as
     /// required by the JSAPI rooting APIs.
-    pub fn rootable(&self) -> *mut *mut JSObject {
-        self.object.get()
+    pub fn rootable(&self) -> &Heap<*mut JSObject> {
+        &self.object
     }
 
     /// Create an uninitialized `Reflector`.
     pub fn new() -> Reflector {
         Reflector {
-            object: UnsafeCell::new(ptr::null_mut()),
+            object: Heap::default(),
         }
     }
 }
diff --git a/components/script/dom/bindings/trace.rs b/components/script/dom/bindings/trace.rs
@@ -20,7 +20,7 @@
 //!    calls `trace()` on the field.
 //!    For example, for fields of type `JS<T>`, `JS<T>::trace()` calls
 //!    `trace_reflector()`.
-//! 4. `trace_reflector()` calls `JS_CallUnbarrieredObjectTracer()` with a
+//! 4. `trace_reflector()` calls `JS::TraceEdge()` with a
 //!    pointer to the `JSObject` for the reflector. This notifies the GC, which
 //!    will add the object to the graph, and will trace that object as well.
 //! 5. When the GC finishes tracing, it [`finalizes`](../index.html#destruction)
@@ -54,7 +54,7 @@ use hyper::method::Method;
 use hyper::mime::Mime;
 use hyper::status::StatusCode;
 use ipc_channel::ipc::{IpcReceiver, IpcSender};
-use js::glue::{CallObjectTracer, CallUnbarrieredObjectTracer, CallValueTracer};
+use js::glue::{CallObjectTracer, CallValueTracer};
 use js::jsapi::{GCTraceKindToAscii, Heap, JSObject, JSTracer, TraceKind};
 use js::jsval::JSVal;
 use js::rust::Runtime;
@@ -139,12 +139,8 @@ pub fn trace_jsval(tracer: *mut JSTracer, description: &str, val: &Heap<JSVal>)
 /// Trace the `JSObject` held by `reflector`.
 #[allow(unrooted_must_root)]
 pub fn trace_reflector(tracer: *mut JSTracer, description: &str, reflector: &Reflector) {
-    unsafe {
-        trace!("tracing reflector {}", description);
-        CallUnbarrieredObjectTracer(tracer,
-                                    reflector.rootable(),
-                                    GCTraceKindToAscii(TraceKind::Object));
-    }
+    trace!("tracing reflector {}", description);
+    trace_object(tracer, description, reflector.rootable())
 }
 
 /// Trace a `JSObject`.
diff --git 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/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);
