Traceable is unsound

[gmorenz]

I'll probably fix this at the same time as fixing #520, but just in case creating an issue for it.

Traceable takes an &self and eventually calls into spidermonkey's tracing functions. Spidermonkey may then mutate the thing being traced to point to a new location where it moved the object. Per this comment.

Traceable is defined as

pub unsafe trait Traceable {
    /// Trace `self`.
    unsafe fn trace(&self, trc: *mut JSTracer);
}

It takes self as a immutable reference, despite mutating it. Short of interior mutability, this is unsound. It is implemented for Value and ValueArray, which do not (and IMHO should not) have interior mutability. So the type signature of trace needs to change to either &mut self, self: &UnsafeCell<Self>.

[jdm]

Thinking out loud: is this is only unsound when implemented against types that can mutate due to GC: JSVal, *mut JSObject, etc? Or is the presence of any field that can be mutated (eg. PropertyDescriptor) enough to make this unsound?

[jdm]

I'm pretty sure I attempted to use a &mut self type signature back when I first implemented this 10 years ago, but there was no way to guarantee exclusive mutable access since GCs can be triggered at just about any time.

[sagudev]

Thinking out loud: is this is only unsound when implemented against types that can mutate due to GC: JSVal, *mut JSObject, etc? Or is the presence of any field that can be mutated (eg. PropertyDescriptor) enough to make this unsound?

I think it's both.

I'm pretty sure I attempted to use a &mut self type signature back when I first implemented this 10 years ago, but there was no way to guarantee exclusive mutable access since GCs can be triggered at just about any time.

I think that is exactly the problem. When we are doing GC there should be no & to js managed (as it can be mutated/moved). Handles/MutableHandles are OK (because they are effectively JS pointers that will be corrected by SM when GC is done).

[gmorenz]

I think it's both.

From an &mut point of view I agree, from a interior mutability point of view, only the things that the GC modifies would need to be wrapped in a Cell type.

I think we could narrowly fix this issue, and not the more structural issues issues in #520, by replacing the definition of Value with struct Value(Cell<OldValue>). Wrap things like *mut JSObject into Cell<*mut JSObject> everywhere. Which, so long as rust code never meaningfully de-references anything the GC mutates, would be a relatively minor API change. I'm not sure if that's the case? Dereferencing *mut JSObject seems unlikely, but I remember seeing a reference to updating hash values and so on that made me think structs with their internals defined on the rust side might need to be mutated as well?

I think that is exactly the problem. When we are doing GC there should be no & to js managed (as it can be mutated/moved). Handles/MutableHandles are OK (because they are effectively JS pointers that will be corrected by SM when GC is done).

This line of thought is why I was suggesting that this is solved by #520. If we fix MutableHandle/RootedGuard/... so that they acknowledge that there's a GC that mutates the data, that acknowledgment probably defines what the method that the GC can use to mutate the data looks like. This issue then just turns into "and use that method".

[sagudev]

One of interesting cases is servo's dom_struct, that is actually allocated via Box, but is actually lifetime managed by SM (SM finalizes it by Box::from_raw then drop), but to my knowledge not moved. All fields are implemented via interior mutability. DOM methods are then implemented as normal &self methods and they occasionally also call internal SM functions (that might trigger GC; there is no way around that). So I think having &mut self on trace would definitely brake this, but having &self is actually ok, at least for dom_structs.

[gmorenz]

I think you're right. It looks like that, including in servo, the only things that are modified in Trace, are either

• Using interior mutability.
• In a Heap<T>, which is a wrapper around UnsafeCell<T>. I'm not sure that how we currently cast a *const UnsafeCell<T> to a *mut T is valid (without going through UnsafeCell methods), but that's an easy fix.
• One of Value, ValueArray, and PropertyDescriptor,

And I'm confused as to why it's ok to have a Value (or friends) not in a Heap<T> in a place where trace is ever called. Shouldn't it need the same GC read/write barriers that a *mut JSObject needs, in case the stored value is a *mut JSOjbect? The fix might just be to remove those 3 impls methods, and deal with the fallout of moving everything that needs trace into a Heap<T>?

[gmorenz]

I'm not sure that how we currently cast a *const UnsafeCell<T> to a *mut T is valid (without going through UnsafeCell methods), but that's an easy fix.

I take this back, it's suspicious looking but I think it's fine.

Context:

The UnsafeCell docs explicitly forbid

unsafe fn not_allowed<T>(ptr: &UnsafeCell<T>) -> &mut T {
  let t = ptr as *const UnsafeCell<T> as *mut T;
  // This is undefined behavior, because the `*mut T` pointer
  // was not obtained through `.get()` nor `.raw_get()`:
  unsafe { &mut *t }
}

The code looks like

CallValueTracer(trc, self as *const _ as *mut Self, c"value".as_ptr());

I read this as &Heap<T> -> *mut T (which would be the forbidden pattern), but it's actually &Heap<T> -> *mut Heap<T>, and while you're not allowed to write through that pointer, you are allowed to read through it, to call .get() on the UnsafeCell, and write through the returned *mut T. The *mut Heap<T> is immediately passed off to C and the rust compiler can't "prove" that the foreign C code is writing directly to it without calling .get on an UnsafeCell to get a valid pointer first. So this shouldn't be able to result in undefined behavior.

[gmorenz]

Branch removing impl Traceable for Value here, this went very well (though I'm not sure it's correct... will look at this more later). The servo changes are here and tiny.

The Codegen.py case looks to me exactly like the GC barriers enforced by Heap being missed because we aren't using it. We're storing these Values across calls to Call (which I presume can GC), and they contain Objects, and we're mutating them as we iterate through the loop... I don't think I 100% understand GC barriers though, so maybe someone else can take a look and tell me if this is actually an issue.

I haven't tested removing impl Traceable for ValueArray and PropertyDescriptor yet.

[sagudev]

I checked in firefox and they always use Heap<JS::Value> in dom objects (as fields), but they use RootedValue = JS::Rooted<JS::Value> for local rooted values. I think this boils down to duality of our Traceable, that is used in rooting and in DOM GC (I think firefox has separate impl of trace for CC). From what I gathered Value should be either rooted or heaped.

As for ValueArray and PropertyDescriptor they simply are not wrapped in heap in firefox, but that's probably because they are also not used as field in dom objects.

[jdm]

From what I gathered Value should be either rooted or heaped.

Correct.