Heap constructor is unsafe

[jdm]

The Heap constructor calls Heap::set which triggers a post barrier. This stores the current address of the value stored inside the Heap, which will be moved once the Heap constructor returns. I have not yet been able to think of a way to prevent people from calling Heap::set before the Heap value has stopped being moved, short of making the Heap constructor return a Box instead.

[MortimerGoro]

I think that I got a new instance of this issue. The gamepad implementation worked great until I moved the typedarray from a setter to the heap constructor after a suggestion in the PR code review. Now there is a GC related crash in about 30 seconds after running the app.

Here you can see the commit with the failing and working version: https://github.com/MortimerGoro/servo/commit/644114df47e46372990c2d6e28911b3b8543029e

Do you think that the crash is related to this issue? I want to understand why the crash happens.

[jdm]

Yes, that's the exact same symptom and solution as https://github.com/servo/servo/issues/15445.

[asajeffrey]

Rather drastic suggestion: can we remove the constructor? Or mark it as unsafe? The fix in (e.g.) https://github.com/servo/servo/pull/16500/files#diff-ee0d03fc9e0cdadcde40c50bd0f8369eL38 is to replace the constructor by a use of Heap::default() followed by an explicit set after the heap object has finished moving.

[jdm]

Yes, we should remove the constructor.

[jdm]

I want to apply this patch:

diff --git a/src/conversions.rs b/src/conversions.rs
index 2b2f6a9..bd114ce 100644
--- a/src/conversions.rs
+++ b/src/conversions.rs
@@ -216,7 +216,9 @@ impl FromJSValConvertible for Heap<JSVal> {
                          value: HandleValue,
                          _option: ())
                          -> Result<ConversionResult<Self>, ()> {
-        Ok(ConversionResult::Success(Heap::new(value.get())))
+        // FIXME: This is horribly unsound and has no guarantee of working.
+        // https://github.com/servo/rust-mozjs/issues/334
+        Ok(ConversionResult::Success(Heap::do_not_use_this(value.get())))
     }
 }

diff --git a/src/rust.rs b/src/rust.rs
index 8faab9b..8f495f2 100644
--- a/src/rust.rs
+++ b/src/rust.rs
@@ -642,7 +642,7 @@ impl GCMethods for Value {
 }

 impl<T: GCMethods + Copy> Heap<T> {
-    pub fn new(v: T) -> Heap<T>
+    pub(crate) unsafe fn do_not_use_this(v: T) -> Heap<T>
         where Heap<T>: Default
     {
         let ptr = Heap::default();
@@ -680,14 +680,6 @@ impl<T: GCMethods + Copy> Heap<T> {
     }
 }

-impl<T: GCMethods + Copy> Clone for Heap<T>
-    where Heap<T>: Default
-{
-    fn clone(&self) -> Self {
-        Heap::new(self.get())
-    }
-}
-
 impl<T> Default for Heap<*mut T>
     where *mut T: GCMethods + Copy
 {

but I need to wait until https://github.com/servo/servo/pull/17056 merges first, since that removes all uses of Heap::new from Servo's code.

[asajeffrey]

If moving heap values after they are set, then growing types like Vec<Heap<JSVal>> are unsafe, and need to be Vec<Box<Heap<JSVal>>>. As @jdm said, the only obvious fix is to mark the constructor as unsafe, and provide a safe constructor which returns a Box<Heap<T>>. Rather annoyingly, this means Heap can't implement Default, since default is required to be safe.

[jdm]

No, the existing heap default is safe. Undefined and null values do not cause any issues.

[jdm]

By itself, Vec<Box<Heap<JSVal>>>> is also unsafe, since the engine has no idea that the vector exists. We would need to use Servo's RootedVec for that.

[Xanewok]

Since I touched Heap construction/code recently, I intend to work on this over the next couple of days if possible.

I tried to comment the FromJSValConvertible for Heap<JSVal> impl and it seems that only dict members of Heap<{JSVal, *mut JSObject}> type seem to be the offenders here? I can try to combine this with https://github.com/servo/servo/pull/20265#issuecomment-372838379 and see where it goes.

---

Re Vec<Box<Heap<JSVal>>> unsafety - from what I understand:

• Heaps are only for allowing values to cross different SM-managed heaps (they still need to be traced as per this (still relevant?) guide
• We call barriers using &Heap<T>

So, if I'm not mistaken, moving Box elems in Vec will still be safe, even if we reallocate to grow the container? We still need to additionally trace underlying values, so something along the lines of RootedTraceableBox<Vec<Heap<JSVal>>> would be the 'complete', safe answer, assuming we impl JSTraceable on Vec where T: JSTraceable?

[jdm]

Yes, as far as I know a vector of boxed heap values would be safe.