Skip to content

Added traceable for std::rc::Weak #659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Added traceable for std::rc::Weak #659

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion mozjs-sys/src/trace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use std::num::{
};
use std::ops::Range;
use std::path::PathBuf;
use std::rc::Rc;
use std::rc::{Rc, Weak};
use std::sync::atomic::{
AtomicBool, AtomicI16, AtomicI32, AtomicI64, AtomicI8, AtomicIsize, AtomicU16, AtomicU32,
AtomicU64, AtomicU8, AtomicUsize,
Expand Down Expand Up @@ -137,6 +137,17 @@ unsafe impl<T: Traceable> Traceable for Rc<T> {
}
}

unsafe impl<T: Traceable> Traceable for Weak<T> {
#[inline]
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);
}
}
Comment on lines +142 to +149
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.

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 •
edited
Loading

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

@gmorenz gmorenz Nov 17, 2025

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 •
edited
Loading

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.


unsafe impl<T: Traceable + ?Sized> Traceable for Box<T> {
#[inline]
unsafe fn trace(&self, trc: *mut JSTracer) {
Expand Down
Loading