MutableHandle are clonable breaking safety provided by &mut

[sagudev]

mut is required for https://doc.servo.org/mozjs/gc/root/struct.MutableHandle.html#tymethod.set, but this is still weird as MutableHandle is clonable so &mut does not help a lot with safety.

Originally posted by @sagudev in servo/servo#34087 (comment)

When cloning MutableHandle we still have underlying pointer point to same location, so we can have two &mut that points to same location.

[sagudev]

I think the proper solution would be to not impl Clone/Copy for *Handle or at least mark it as unsafe.

[sagudev]

I think the first action point would be to remove all mozjs::rust::wrappers usages, then we can remove clone,copy in followup.

[gmorenz]

I'm going to take a go at this.

[sagudev]

I'm going to take a go at this.

Ok. I assigned you. Do note that big changes such as this require companion PRs in servo.

[gmorenz]

There is a larger problem here, RootedGuard is unsound, as is basically everything derived from it.

Specifically RootedGuard is

pub struct RootedGuard<'a, T: 'a + RootKind> {
    root: &'a mut Rooted<T>,
}

Which requires that it have unique access to root. Meanwhile the entire purpose of this type is that it stores a root that spidermonkey is aware of, and can read and update, as part of its gc (which is enabled by adding it to a root stack during RootedGuard::new). Spidermonkey performing GC results in undefined behavior as long as any of these exist.

The same problem exists with anything that creates a &T or &mut T pointing to the value stored in Rooted<T>. This includes the Deref/DerefMut impl for RootedGuard/Handle/MutableHandle, and struct Handle itself. Curiously struct MutableHandle escapes this because it uses a raw pointer (though it's Deref/DerefMut impls do not).

How can we fix this? There are a few paths. In all cases we can't have an impl Deref { type Target = T }, we will have to use get and set functions like Cell does to safely access the value. Because we can't guarantee that the user won't do something to trigger a GC while they have the &T returned from Deref hanging around.

1. Lean into interior mutability

RootedGuard contains an &'a UnsafeCell<Rooted<T>> (or a &'a Cell<Rooted<T>> since I think we're only using this with Copy types). The rust definition of the C++ GC types consider those to operate on Cell's.

Handle and MutableHandle become wrappers around &UnsafeCell<T>/&Cell<T>. Or they potentially are just outright replaced with &Cell<T>. If they aren't replaced with &Cell<T>, they are still both Copy like Cell<T> is.

2. Lean into raw pointers

Everything can be transformed to look like the existing

pub struct MutableHandle<'a, T: 'a> {
    pub(crate) ptr: *mut T,
    anchor: PhantomData<&'a mut T>,
}

Instead of storing references, store raw pointers that don't have aliasing guarantees.

There are suboptions here, as to what the API looks like:

1. MutableHandle and Handle mimic Cell in taking a &T for writes. In this case I don't see why we wouldn't prefer the Cell option.

2. MutableHandle requires a &mut T for writes and we pass around a &mut MutableHandle<T>. This is what I am in the middle of implementing in Remove Copy/Clone from MutableHandle #559. The benefit here is to present a rust-facing API with nearly unique writer semantics (up to the GC). The drawback is that it introduces an extra level of indirection - we now have &mut *mut RootedValue. That extra level of indirection means we need to store the extra pointer somewhere, which ends up adding line-noise in the form of extra &mut statements wherever we create the &mut MutableHandle<T> (for example). These can't be abstracted away because they must be stored in the callers stack frame.

3. MutableHandle requires a &mut self (or even self) for writes, and we pass around a MutableHandle<'a, T> via explicit reborrowing. This works much like a &mut T works, except there the compiler implicitly handles reborrowing for the user. Concretely this means adding a method as follows and using it to create copies whenever we can't just move the handle.

   impl<'a, T> MutableHandle<'a, T> {
       pub fn re<'b>(&'b mut self) -> MutableHandle<'b, T>
       where 'a: 'b
       {
           MutableHandle {
               ptr: self.ptr,
               anchor: PhantomData,
           }
       }
   }
   
   fn uses_reborrowing(mut handle: MutableHandleValue<T>) {
       call_once(handle.re()); // Reborrow to make a copy of the handle
       call_two(handle);
   }

   In exchange for the .re() call we avoid the extra indirection in option 2, and thus the &mut syntactic noise required to create places to store the extra pointer.

   PS. Rust issue related to this

3. Bypass rust's aliasing and provenance assumptions

Replace pointers to T with pointers to a zero-sized Tracked<T> (bikesheddable name). For example Handle<T> because &Tracked<T> and HandleMut<T> becomes &mut Tracked<T>.

Because Tracked<T> is zero-sized, it doesn't have aliasing guarantees. The T it "points to" is actually right after it's zero sized slice of memory, and is free to change.

By the same token we can't access the T it points to with the provenance of a &Tracked<T> or a &mut Tracked<T>. Instead we have to bypass rust's provenance assumptions and rely on exposed provenance. When we create a Tracked<T> we expose the provenance of the memory holding T. When we access the value stored in the Tracked<T> we do so with pointers created via that exposed provenance, instead of directly via the reference.

impl<T> Tracked<T> {
    // Note that it isn't possible to create a `Tracked<T>` directly. That has
    // no meaning. We will only ever allow the creation of references, and
    // pointers, to a `Tracked<T>`.
    fn new(val: &mut T) -> &mut Self {
        let ptr = val as *mut T;
        ptr.expose_provenance();
        &mut *(ptr as *mut Self),
    }

    pub fn as_ptr(&self) -> *const T {
        ptr::with_exposed_provenance((self as *const Self).addr())
    }

    pub fn as_mut_ptr(&mut self) -> *mut T {
        ptr::with_exposed_provenance_mut((self as *mut Self).addr())
    }

    pub fn get(&self) -> T
    where
        T: Copy
    {
        // SAFETY: Any accesses come from the same thread as the &mut self lives on,
        // so there can be no sychronization issues.
        unsafe{ *self.as_ptr() }
    }

    pub fn set(&mut self, v: T)
    where
        T: Copy
    {
        // SAFETY: Any reads come from the same thread as the &mut self lives on,
        // so there can be no sychronization issues.
        unsafe{ *self.as_mut_ptr() = v }
    }
}

Note

Maybe some day rust will allow us to store provenance in a 0 sized struct and we could pass the correct provenance along. I don't believe that day is today.

By implementing it with Tracked<T>: !Send we have that &mut Tracked<T> pointers (like MutableHandle<T>) always remain on the correct thread and they can't conflict with the GC's reads or writes, and by requiring that Tracked<T>: !Sync we get the same for &Tracked<T> and the GC's writes.

This option is similar to 2. and 3. in the "lean into raw pointers" proposals, except it avoids the syntactic noise, because we don't have an extra layer of indirection, and we get to take advantage of the compilers implicit reborrowing. Instead it comes with the drawback that we are relying on exposed provenance, which is not fully defined. It is also as far as I know novel, which is not really a good thing with unsafe code.

---

I'm leaning towards interior mutability as the best option, with the Tracked<T> option in second place.

I came up with the Tracked<T> idea before I realized that the spidermonkey GC writes as well as reads to rooted values. With a GC that only read from rooted values it would be more advantageous. In particular it could impl Deref { target = T }, just not DerefMut, and as far as all the rust code was concerned we would have the normal rust semantics, just with a different syntax for writing.

As it stands though, the GC we have mutates values. Which means that while on the rust side we can mimic the "must have a unique pointer to write" semantics, we can't take advantage of any of the properties that provides us (like values not changing if the borrow checker says they don't). Considering that, it doesn't seem worthwhile to track whether or not a pointer is unique and only allow writing through unique pointers. Both because satisfying the borrow checker is actually a slight amount of extra work, and because all the ways to do that have drawbacks - either in terms of friction for users of the API or by using exposed provenance.

I'd appreciate some feedback on this before going ahead and implementing that, both because I'm a newcomer to the project and it's a non-trivial change, and because this is a decision that feels like it may have subtle implications.

Edit: I've re-evaluated as I've experimented over the last couple of days. I think I now favor the .reborrow approach. Tracked seems to fancy. Cell<T> ends up being an awkward abstraction at this level, and we do end up working with a lot of non-Copy data even if currently isn't used much in handles, which makes trying to base everything on Cell involve a lot of casting back and forth from pointers (e.g. trying to do this with trace ended up with a lot of casts).

[gmorenz]

Two further thoughts that I didn't end up capturing in the above:

1. Any of the non-interior mutability options are effectively betting on us being able to reliably prevent spidermonkey from handing us multiple copies of the same object. I think we do actually have that... but there are a few callbacks and spidermonkey probably isn't really designed for that the same way rust is. With all of the current proposals spidermonkey violating this wouldn't actually end up violating soundness, but it would make the uniqueness guarantees meaningless.
2. An under-analyzed benefit/drawback of these proposals is how they end up interacting with an eventual scripting engine API. If we don't track uniqueness in the servo code we may well end up requiring some form of interior mutability in the API. However it feels to early to worry about that. We can make an API that generalizes over interior mutability and unique-pointer-mutability later, if we need it and with the actual requirements in sight.

[sagudev]

cc @jdm

[sagudev]

I've suspected we were more unsound issues due to aliasing rules, but I never really dive into it. Thanks for all the investigation work.

I think we would still need to access &T/&mut sometimes, so we would need something like this:

handle.ref(|r: &T| {
    // use static analysis lint to ensure no GC happens here
});

or some unsafe function where users would need too ensure that themself. It all boils down to notion of no_gc/can_gc that can only be provided via static analysis in the current model.

I am questioning if it's really worth fixing current model (there are also other problems and ergonomics) or should we throw our resources at different model: https://github.com/asajeffrey/josephine where all &T have lifetime bounded to ctx, so when you try to do GC (or call any function that can call GC) you need &mut ctx, so you get no_gc via lifetimes (rough idea). This is still under construction and not battle tested, so we might not be able to afford experimenting with it due to limited resources.

[sagudev]

related: servo/rust-mozjs#273

[jdm]

Thank you for all this analysis! I would like to take the time to read it closely and think about it, but I'm currently sick and also solo parenting a toddler, so I can't promise if I will have time before the weekend (I really hope I'm less sick by this weekend).

[gmorenz]

Thank you for all this analysis! I would like to take the time to read it closely and think about it, but I'm currently sick and also solo parenting a toddler, so I can't promise if I will have time before the weekend (I really hope I'm less sick by this weekend).

Take your time and get better! These problems have waited a decade, they can definitely wait a bit longer (not that you need permission from a stranger on the internet).

I think we would still need to access &T/&mut sometimes, so we would need something like this:

It occurs to me that the slash makes a big difference here. Both of these require preventing the GC from running (e.g. by borrowing context), but the &mut side also requires knowing that either we're the only thing currently using this function for this handle. The latter could be done with type definitions that prevent accessing more than one variable at a time, or by using the approaches above that do track uniqueness of handles instead of the Cell approach.

Potentially I should try and implement this enough to hunt down the cases where this is necessary in servo and see what they look like. I'm sort of surprised I didn't see them when porting everything to use &mut MutableHandle - but that change wouldn't guarantee running into them.

I am questioning if it's really worth fixing current model (there are also other problems and ergonomics) or should we throw our resources at different model: https://github.com/asajeffrey/josephine where all &T have lifetime bounded to ctx, so when you try to do GC (or call any function that can call GC) you need &mut ctx, so you get no_gc via lifetimes (rough idea). This is still under construction and not battle tested, so we might not be able to afford experimenting with it due to limited resources.

Josephine looks interesting, I'll find some time to look at it more closely for inspiration if nothing else.

My initial gut feeling is that we're more likely to settle on a good API by iterating on the thing with real use (this), than starting from scratch.

... I'll have more thoughts on this &mut ctx approach later.

[gmorenz]

where all &T have lifetime bounded to ctx, so when you try to do GC (or call any function that can call GC) you need &mut ctx, so you get no_gc via lifetimes (rough idea)

So I don't think this quite works. Spidermonkeys docs say

SpiderMonkey can trigger a GC at almost any time and in ways that are not always clear. For example, the following innocuous-looking actions can cause a GC: allocation of any new GC thing; [...]

Which I think means this proposal would prevent you from ever having 2 values you allocated at the same time. Which seems devastating in terms of usability.

However if instead of mutably borrowing the context GC triggering ops immutably borrow it, and handles borrow it with the same mutability as the handle, then we get that you never have a MutableHandle at the same time as the context is borrowed, so MutableHandles can implement Deref and DerefMut.

That's such a restrictive property on MutableHandles that I imagine this is only useful in conjunction with a Handle::set function on the non-immutable ones, that is used when we don't need DerefMut. If it's useful at all, it might just not be worth the complexity, it's an interesting possibility though.

[sagudev]

I think my explanation wasn't good enough. Here is how I imagined it:

RootedGuard/Handle/MutableHandle will mostly remain the same (their lifetime is independent of ctx, as they can be held across GC), but their deref/deref_mut would return &'ctx (it's bounded to lifetime of ctx) and we also want it to bound to self as we do not want to outlive it (outlive the root):

impl ___<T> {
    fn deref<'root: 'r, 'cx: 'r, 'r>(&'root self, &'cx cx) -> &'r T {
        // ...
    }

    fn deref_mut<'root: 'r, 'cx: 'r, 'r>(&'root mut self, &'cx cx) -> &'r mut T {
        // ...
    }
}

Now each SM function that can trigger GC should accept &mut ctx, that will prevent holding any & across (but it will not prevent holding handles/roots). I think all such functions already accept cx, but not all functions that accept cx can trigger GC.

EDIT: This model assumes good SafeJSContext that is not clonable.

[sagudev]

mut is required for https://doc.servo.org/mozjs/gc/root/struct.MutableHandle.html#tymethod.set, but this is still weird as MutableHandle is clonable so &mut does not help a lot with safety.

Originally posted by @sagudev in servo/servo#34087 (comment)

When cloning MutableHandle we still have underlying pointer point to same location, so we can have two &mut that points to same location.

Actually this is not a problem. While we can have two MutableHandles, we cannot use neither until one is dropped:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=0bc4e0119bc67b45ef41c304fec541fa
so at least on that front everything is OK.

[mukilan]

Actually this is not a problem. While we can have two MutableHandles, we cannot use neither until one is dropped:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=0bc4e0119bc67b45ef41c304fec541fa
so at least on that front everything is OK.

This code still compiles, wouldn't that be a problem?

fn main() {
    let mut root = Root(5);
    let h = root.handle_mut();
    let h2 = h.clone();
    use_handle(&h);
    use_handle(&h2);
}

[gmorenz]

I think my explanation wasn't good enough. Here is how I imagined it:

Without assigning blame I definitely misunderstood. I agree, this looks like a reasonable and useful API.

Actually this is not a problem. While we can have two MutableHandles, we cannot use neither until one is dropped: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=0bc4e0119bc67b45ef41c304fec541fa so at least on that front everything is OK.

This code still compiles, wouldn't that be a problem?

Yes, sort of. Not when use_handle only takes an &HandleMut and not a &mut HandleMut.

Here's my toy example with copy and pasted definitions that miri will complain about https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=632c9a86d7c1a90b48f9577848e5a450

[sagudev]

mut is required for https://doc.servo.org/mozjs/gc/root/struct.MutableHandle.html#tymethod.set, but this is still weird as MutableHandle is clonable so &mut does not help a lot with safety.
Originally posted by @sagudev in servo/servo#34087 (comment)
When cloning MutableHandle we still have underlying pointer point to same location, so we can have two &mut that points to same location.

Actually this is not a problem. While we can have two MutableHandles, we cannot use neither until one is dropped: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=0bc4e0119bc67b45ef41c304fec541fa so at least on that front everything is OK.

Ignore that. MutableHandle is like &mut and &mut doesn't implement Clone so it's not OK to clone MutableHandle.

[gmorenz]

I've updated #559 to remove the Deref[Mut] impls for Handle[Mut] (the Copy/Clone impls on MutableHandle are already gone in that branch).

This is the accompanying servo commit, turns out that we aren't meaningfully using the impls almost anywhere. The one place where we actually held a reference, we did so across a GC, so that had to go... but it only required duplicating a few lines of code to avoid it.

The PR currently only addresses gc::root::[Mutable]Handle, not JS::[Mutable]Handle, which has the same issue. I'm hopeful that there won't be any more difficult there.