Auto merge of #8026 - eefriedman:js-rooting, r=nox

Fix uses of JS<T> as a type on the stack

`JS<T>` belongs on the heap, and only on the heap.  This is a collection of fixes so that code uses either `Root<T>` or `&T` to pass around garbage-collected pointers.

Ideally, we could completely ban constructing a `JS<T>` outside of constructor functions, but we aren't quite there yet.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8026)
<!-- Reviewable:end -->

components/script/dom/bindings/js.rs

@@ -32,13 +32,19 @@ use js::jsapi::{Heap, JSObject, JSTracer};
 use js::jsval::JSVal;
 use layout_interface::TrustedNodeAddress;
 use script_task::STACK_ROOTS;
-use std::cell::{Cell, UnsafeCell};
+use std::cell::UnsafeCell;
 use std::default::Default;
 use std::ops::Deref;
+use std::ptr;
 use util::mem::HeapSizeOf;
 
-/// A traced reference to a DOM object. Must only be used as a field in other
-/// DOM objects.
+/// A traced reference to a DOM object
+///
+/// This type is critical to making garbage collection work with the DOM,
+/// but it is very dangerous; if garbage collection happens with a `JS<T>`
+/// on the stack, the `JS<T>` can point to freed memory.
+///
+/// This should only be used as a field in other DOM objects.
 #[must_root]
 pub struct JS<T> {
     ptr: NonZero<*const T>
@@ -54,12 +60,13 @@ impl<T> HeapSizeOf for JS<T> {
 
 impl<T> JS<T> {
     /// Returns `LayoutJS<T>` containing the same pointer.
-    pub unsafe fn to_layout(self) -> LayoutJS<T> {
+    pub unsafe fn to_layout(&self) -> LayoutJS<T> {
         LayoutJS {
             ptr: self.ptr.clone()
         }
     }
 }
+
 impl<T: Reflectable> JS<T> {
     /// Root this JS-owned value to prevent its collection as garbage.
     pub fn root(&self) -> Root<T> {
@@ -109,8 +116,6 @@ impl<T: Reflectable> LayoutJS<T> {
     }
 }
 
-impl<T> Copy for JS<T> {}
-
 impl<T> Copy for LayoutJS<T> {}
 
 impl<T> PartialEq for JS<T> {
@@ -205,73 +210,76 @@ impl MutHeapJSVal {
 
 
 /// A holder that provides interior mutability for GC-managed values such as
-/// `JS<T>`.
+/// `JS<T>`.  Essentially a `Cell<JS<T>>`, but safer.
+///
+/// This should only be used as a field in other DOM objects; see warning
+/// on `JS<T>`.
 #[must_root]
 #[derive(JSTraceable)]
-#[derive(HeapSizeOf)]
-pub struct MutHeap<T: HeapGCValue + Copy> {
-    val: Cell<T>,
+pub struct MutHeap<T: HeapGCValue> {
+    val: UnsafeCell<T>,
 }
 
-impl<T: HeapGCValue + Copy> MutHeap<T> {
+impl<T: Reflectable> MutHeap<JS<T>> {
     /// Create a new `MutHeap`.
-    pub fn new(initial: T) -> MutHeap<T> {
+    pub fn new(initial: &T) -> MutHeap<JS<T>> {
         MutHeap {
-            val: Cell::new(initial),
+            val: UnsafeCell::new(JS::from_ref(initial)),
         }
     }
 
     /// Set this `MutHeap` to the given value.
-    pub fn set(&self, val: T) {
-        self.val.set(val)
-    }
-
-    /// Set the value in this `MutHeap`.
-    pub fn get(&self) -> T {
-        self.val.get()
-    }
-}
-
-/// A mutable holder for GC-managed values such as `JSval` and `JS<T>`, with
-/// nullability represented by an enclosing Option wrapper. Must be used in
-/// place of traditional internal mutability to ensure that the proper GC
-/// barriers are enforced.
-#[must_root]
-#[derive(JSTraceable, HeapSizeOf)]
-pub struct MutNullableHeap<T: HeapGCValue + Copy> {
-    ptr: Cell<Option<T>>
-}
-
-impl<T: HeapGCValue + Copy> MutNullableHeap<T> {
-    /// Create a new `MutNullableHeap`.
-    pub fn new(initial: Option<T>) -> MutNullableHeap<T> {
-        MutNullableHeap {
-            ptr: Cell::new(initial)
+    pub fn set(&self, val: &T) {
+        unsafe {
+            *self.val.get() = JS::from_ref(val);
         }
     }
 
-    /// Set this `MutNullableHeap` to the given value.
-    pub fn set(&self, val: Option<T>) {
-        self.ptr.set(val);
-    }
-
-    /// Retrieve a copy of the current optional inner value.
-    pub fn get(&self) -> Option<T> {
-        self.ptr.get()
+    /// Set the value in this `MutHeap`.
+    pub fn get(&self) -> Root<T> {
+        unsafe {
+            ptr::read(self.val.get()).root()
+        }
     }
 }
 
+impl<T: HeapGCValue> HeapSizeOf for MutHeap<T> {
+    fn heap_size_of_children(&self) -> usize {
+        // See comment on HeapSizeOf for JS<T>.
+        0
+    }
+}
+
+/// A holder that provides interior mutability for GC-managed values such as
+/// `JS<T>`, with nullability represented by an enclosing Option wrapper.
+/// Essentially a `Cell<Option<JS<T>>>`, but safer.
+///
+/// This should only be used as a field in other DOM objects; see warning
+/// on `JS<T>`.
+#[must_root]
+#[derive(JSTraceable)]
+pub struct MutNullableHeap<T: HeapGCValue> {
+    ptr: UnsafeCell<Option<T>>
+}
+
 impl<T: Reflectable> MutNullableHeap<JS<T>> {
+    /// Create a new `MutNullableHeap`.
+    pub fn new(initial: Option<&T>) -> MutNullableHeap<JS<T>> {
+        MutNullableHeap {
+            ptr: UnsafeCell::new(initial.map(JS::from_ref))
+        }
+    }
+
     /// Retrieve a copy of the current inner value. If it is `None`, it is
     /// initialized with the result of `cb` first.
     pub fn or_init<F>(&self, cb: F) -> Root<T>
         where F: FnOnce() -> Root<T>
     {
         match self.get() {
-            Some(inner) => Root::from_rooted(inner),
+            Some(inner) => inner,
             None => {
                 let inner = cb();
-                self.set(Some(JS::from_rooted(&inner)));
+                self.set(Some(&inner));
                 inner
             },
         }
@@ -280,25 +288,45 @@ impl<T: Reflectable> MutNullableHeap<JS<T>> {
     /// Retrieve a copy of the inner optional `JS<T>` as `LayoutJS<T>`.
     /// For use by layout, which can't use safe types like Temporary.
     pub unsafe fn get_inner_as_layout(&self) -> Option<LayoutJS<T>> {
-        self.ptr.get().map(|js| js.to_layout())
+        ptr::read(self.ptr.get()).map(|js| js.to_layout())
+    }
+
+    /// Get a rooted value out of this object
+    pub fn get(&self) -> Option<Root<T>> {
+        unsafe {
+            ptr::read(self.ptr.get()).map(|o| o.root())
+        }
     }
 
     /// Get a rooted value out of this object
-    // FIXME(#6684)
     pub fn get_rooted(&self) -> Option<Root<T>> {
-        self.get().map(|o| o.root())
+        self.get()
+    }
+
+    /// Set this `MutNullableHeap` to the given value.
+    pub fn set(&self, val: Option<&T>) {
+        unsafe {
+            *self.ptr.get() = val.map(|p| JS::from_ref(p));
+        }
     }
 }
 
-impl<T: HeapGCValue + Copy> Default for MutNullableHeap<T> {
+impl<T: HeapGCValue> Default for MutNullableHeap<T> {
     #[allow(unrooted_must_root)]
     fn default() -> MutNullableHeap<T> {
         MutNullableHeap {
-            ptr: Cell::new(None)
+            ptr: UnsafeCell::new(None)
         }
     }
 }
 
+impl<T: HeapGCValue> HeapSizeOf for MutNullableHeap<T> {
+    fn heap_size_of_children(&self) -> usize {
+        // See comment on HeapSizeOf for JS<T>.
+        0
+    }
+}
+
 impl<T: Reflectable> LayoutJS<T> {
     /// Returns an unsafe pointer to the interior of this JS object. This is
     /// the only method that be safely accessed from layout. (The fact that
@@ -435,12 +463,6 @@ impl<T: Reflectable> Root<T> {
     pub fn r(&self) -> &T {
         &**self
     }
-
-    /// Generate a new root from a JS<T> reference
-    #[allow(unrooted_must_root)]
-    pub fn from_rooted(js: JS<T>) -> Root<T> {
-        js.root()
-    }
 }
 
 impl<T: Reflectable> Deref for Root<T> {