Skip to content

Conversation

@Narfinger
Copy link
Contributor

std::rc::Weak need to be reached by the Gc for memory safety reasons.
Signed-off-by: Narfinger [email protected]

Comment on lines +142 to +149
unsafe fn trace(&self, trc: *mut JSTracer) {
// A rc::Weak needs to be reachable by the JS.
// It could happen that we upgrade a `Weak<Foo>` on the stack while the original `Rc<Foo>`
// is getting dropped. Then this could lead to the Gc cleaning up the upgraded `Rc<Foo>`
// while it is still in use via the upgraded `Weak<Foo>`.
self.upgrade().trace(trc);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that I understand the motivation. Upgraded weak that is held across GC should prevent drop of inner, at least until the the drop of upgrade (which happens after GC is done). Also if we would do this weakref somehow losses the point of being weakref.

Copy link
Contributor Author

@Narfinger Narfinger Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it is something that I missunderstood how the Gc works but here is the scenario without this.
Let us have a struct Foo, and Rc<Foo> in Struct2 and a Weak<Foo>. Weak<Foo> is annotated with #[no_trace] because the developer believes that the Gc can always trace Foo via the Rc.

Now somewhere in the code we call let bar = Weak<Foo>::upgrade(), giving us the Rc<Foo> on the stack. While we are doing something with bar the struct Struct2 goes out of scope, making Foo only be reachable by bar.
bar is, however, not traced by the Gc, so it can be cleaned up in the next garbage collection.

I am not sure if weakref is the same or a different thing. The documentation is not really clear to me.
I am new to Gc stuff, so this might already be covered by something else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the worst case we can even imagine that someone just ran mem::forget on a strong ref. So I think we definitely need to treat these the same way as strong refs.

Copy link
Member

@sagudev sagudev Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now somewhere in the code we call let bar = Weak::upgrade(), giving us the Rc on the stack. While we are doing something with bar the struct Struct2 goes out of scope, making Foo only be reachable by bar.
bar is, however, not traced by the Gc, so it can be cleaned up in the next garbage collection.

In this case you should root the upgrade (this should be enforced by crown as no unrooted stuff should be allowed on the stack). Something similar is done by weakref, where weakref itself is not traced (it actually implements nop trace), but when you upgrading you get the rooted inner.

With that being said, in servo we occasionally use the fact that when the method on dom_struct is called, whole doe_struct is rooted, so one does not need to root it's fields (Dom) as they are rooted from the dom_struct (for the lifetime of dom_struct - &self), but one returning values (refs) to those fields one needs to root them. Is that motivation for this, or you have any other concrete case in servo to consider here?

In the worst case we can even imagine that someone just ran mem::forget on a strong ref.

This causes a memory leak (even if temporary) and we do not want those in servo. Although that might not seem that problematic at first as at least one of uses must be rooted; I think it actually is as most weak uses are in globalscope which is basically perma-rooted (for the time of lifetime of the page). I know we had problems with such leaks before, but do not have link on hand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants