Repository navigation
Add jsg::Cell and jsg::RefCell for traced fields - #7982
Merged
Merged
Conversation
piscisaureus
force-pushed
the
piscisaureus/rust-jsg-rc-take
branch
from
October 25, 2026 16:41
ef1e43e to
d2e3d8a
Compare
piscisaureus
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7982 +/- ##
==========================================
- Coverage 68.84% 68.83% -0.02%
==========================================
Files 737 738 +1
Lines 193921 193988 +67
Branches 30088 30088
==========================================
+ Hits 133513 133532 +19
- Misses 42756 42805 +49
+ Partials 17652 17651 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
This overall direction makes a lot of sense to me in aligning with our C++ approach, which is typically the goal. It's an important correctness fix. Appreciated for the clear justification and comparison to alternatives. For defense-in-depth it could be worthwhile to maintain the boxing as a runtime protection, alongside the One suggestion from the AI would be to make Should we make |
littledivy
added a commit
that referenced
this pull request
Oct 30, 2026
A visited handle is traced: only its owner's next visit keeps the target alive. Moving a jsg::Data makes the destination strong again, but moving the kj::Own<T> of an object that holds handles moves only the pointer. The handles stay traced with nothing visiting them, and the GC can free their targets while the object still uses them. Code that hit this either left the handles unvisited, which leaks, or worked around it by hand. The stream queue did the first: it never visited the resolvers of pending reads, so a ReadableStream whose read never settles was never collected, along with its reader and everything they held. WritableStreamInternalController::drain() does the second (see 687b3a6). In this patch: - GcVisitor::makeStrong() makes every handle reachable through a value strong: jsg::Data, the Rust Global/Traced pair, and Refs, whose parent it clears so another owner can visit them. It matches make_strong in Rust JSG's jsg::Cell (#7982), so both languages follow one rule: whatever leaves a traced owner is strong. - jsg::TracedOwn<T> is an owning pointer that records the isolate of its last visit and strengthens the object when it moves or is released. MemoryTracker accepts it like kj::Own. - The stream queue holds pending reads in TracedOwn and visits them from the consumer and from the queue. The queue's visit keeps a pending read alive while its controller is reachable, as in the spec, where the controller holds its stream. cancel() drains the ring one element at a time instead of moving it. - The visits are behind a new autogate, gc-complete-tracing. With it disabled the resolvers stay unvisited strong roots, as before, so the rollout can be compared gated against ungated and turned off if code turns out to rely on such a stream staying alive. - A new clang-tidy check, jsg-traced-own, flags a visitForGc() that visits through a kj::Own, and kj::mv() of a field that is a container of TracedOwn. Every site it found now holds a TracedOwn: stream readables and their consumers, the internal stream's pipe events, EventTarget listeners, CryptoKey, WebSocket and the trace Request detail. None of them moves its Own today; the check keeps it that way. - The jsg README, AGENTS.md and the GcVisitor comment describe the rule. Shared ownership is not covered: queue buffer entries are kj::Rc shared between consumers, so a buffered chunk that refers to its own stream still keeps the stream alive. Tests: - tracing-test: a value and a Ref taken out of a traced queue survive the queue being collected, and a cycle through a queued item is collected. With strengthening disabled the first case fails ("a value taken out of a traced queue was collected"). - streams-pending-gc-test, with the autogate enabled: streams, readers and BYOB streams with a pending read are collected; a pending read still settles, including when only the controller is reachable. With the autogate disabled, the three collection cases fail, as on main. - traced-own-test covers the clang-tidy check. 1,000 streams with a never-settling read, after GC: 0 collected with the autogate disabled, 999 with it enabled. RSS of `workerd serve` for a worker whose every request leaves one such stream behind (macOS, fastbuild): requests disabled enabled 20,000 246 MB 246 MB 60,000 482 MB 261 MB 100,000 801 MB 261 MB With the autogate disabled, RSS grows like on main; with it enabled, it levels off.
littledivy
added a commit
that referenced
this pull request
Oct 30, 2026
A visited handle is traced: only its owner's next visit keeps the target alive. Moving a jsg::Data makes the destination strong again, but moving the kj::Own<T> of an object that holds handles moves only the pointer. The handles stay traced with nothing visiting them, and the GC can free their targets while the object still uses them. Code that hit this either left the handles unvisited, which leaks, or worked around it by hand. The stream queue did the first: it never visited the resolvers of pending reads, so a ReadableStream whose read never settles was never collected, along with its reader and everything they held. WritableStreamInternalController::drain() does the second (see 687b3a6). In this patch: - GcVisitor::makeStrong() makes every handle reachable through a value strong: jsg::Data, the Rust Global/Traced pair, and Refs, whose parent it clears so another owner can visit them. It matches make_strong in Rust JSG's jsg::Cell (#7982), so both languages follow one rule: whatever leaves a traced owner is strong. - jsg::TracedOwn<T> is an owning pointer that records the isolate of its last visit and strengthens the object when it moves or is released. MemoryTracker accepts it like kj::Own. - The stream queue holds pending reads in TracedOwn and visits them from the consumer and from the queue, along with a BYOB read's view. The queue's visit keeps a pending read alive while its controller is reachable, as in the spec, where the controller holds its stream. cancel() drains the ring one element at a time instead of moving it. - The visits, and TracedOwn making handles strong, are behind a new autogate, gc-complete-tracing. With it disabled the resolvers stay unvisited strong roots and TracedOwn behaves like kj::Own, as before, so the rollout can be compared gated against ungated and turned off if code turns out to rely on such a stream staying alive. - A new clang-tidy check, jsg-traced-own, flags a visitForGc(), or a helper handed the visitor, that visits through a kj::Own, and kj::mv() of a container field whose elements hold a TracedOwn, directly or in a struct, kj::Maybe or kj::OneOf. Every site it found now holds a TracedOwn: stream readables and their consumers, the internal stream's pipe events, EventTarget listeners, CryptoKey and WebSocket. None of them moves its Own today; the check keeps it that way. The trace Request's Detail is shared through kj::addRef, so it keeps its kj::Own and the site is suppressed. - The jsg README, AGENTS.md and the GcVisitor comment describe the rule. Shared ownership is not covered: queue buffer entries are kj::Rc shared between consumers, so a buffered chunk that refers to its own stream still keeps the stream alive. Tests: - tracing-test: a value and a Ref taken out of a traced queue survive the queue being collected, and a cycle through a queued item is collected. With strengthening disabled the first case fails ("a value taken out of a traced queue was collected"). - streams-pending-gc-test, with the autogate enabled: streams, readers and BYOB streams with a pending read are collected; a pending read still settles, including when only the controller is reachable, and a cycle through a BYOB read's view is collected. streams-pending-gc-gate-off-test runs the same cases with the autogate disabled and checks that none of these streams is collected, as on main. - traced-own-test covers the clang-tidy check. 1,000 streams with a never-settling read, after GC: 0 collected with the autogate disabled, 999 with it enabled. RSS of `workerd serve` for a worker whose every request leaves one such stream behind (macOS, fastbuild): requests disabled enabled 20,000 246 MB 246 MB 60,000 482 MB 261 MB 100,000 801 MB 261 MB With the autogate disabled, RSS grows like on main; with it enabled, it levels off.
littledivy
added a commit
that referenced
this pull request
Oct 30, 2026
A visited handle is traced: only its owner's next visit keeps the target alive. Moving a jsg::Data makes the destination strong again, but moving the kj::Own<T> of an object that holds handles moves only the pointer. The handles stay traced with nothing visiting them, and the GC can free their targets while the object still uses them. Code that hit this either left the handles unvisited, which leaks, or worked around it by hand. The stream queue did the first: it never visited the resolvers of pending reads, so a ReadableStream whose read never settles was never collected, along with its reader and everything they held. WritableStreamInternalController::drain() does the second (see 687b3a6). In this patch: - GcVisitor::makeStrong() makes every handle reachable through a value strong: jsg::Data, the Rust Global/Traced pair, and Refs, whose parent it clears so another owner can visit them. It matches make_strong in Rust JSG's jsg::Cell (#7982), so both languages follow one rule: whatever leaves a traced owner is strong. - jsg::TracedOwn<T> is an owning pointer that records the isolate of its last visit and strengthens the object when it moves or is released. MemoryTracker accepts it like kj::Own. - The stream queue holds pending reads in TracedOwn and visits them from the consumer and from the queue, along with a BYOB read's view. The queue's visit keeps a pending read alive while its controller is reachable, as in the spec, where the controller holds its stream. cancel() drains the ring one element at a time instead of moving it. - The visits, and TracedOwn making handles strong, are behind a new autogate, gc-complete-tracing. With it disabled the resolvers stay unvisited strong roots and TracedOwn behaves like kj::Own, as before, so the rollout can be compared gated against ungated and turned off if code turns out to rely on such a stream staying alive. - A new clang-tidy check, jsg-traced-own, flags a visitForGc(), or a helper handed the visitor, that visits through a kj::Own, and kj::mv() of a container field whose elements hold a TracedOwn, directly or in a struct, kj::Maybe or kj::OneOf. Every site it found now holds a TracedOwn: stream readables and their consumers, the internal stream's pipe events, EventTarget listeners, CryptoKey and WebSocket. None of them moves its Own today; the check keeps it that way. The trace Request's Detail is shared through kj::addRef, so it keeps its kj::Own and the site is suppressed. - The jsg README, AGENTS.md and the GcVisitor comment describe the rule. Shared ownership is not covered: queue buffer entries are kj::Rc shared between consumers, so a buffered chunk that refers to its own stream still keeps the stream alive. Tests: - tracing-test: a value and a Ref taken out of a traced queue survive the queue being collected, and a cycle through a queued item is collected. With strengthening disabled the first case fails ("a value taken out of a traced queue was collected"). - streams-pending-gc-test, with the autogate enabled: streams, readers and BYOB streams with a pending read are collected; a pending read still settles, including when only the controller is reachable. streams-pending-gc-gate-off-test runs the same cases with the autogate disabled and checks that none of these streams is collected, as on main. - traced-own-test covers the clang-tidy check. 1,000 streams with a never-settling read, after GC: 0 collected with the autogate disabled, 999 with it enabled. RSS of `workerd serve` for a worker whose every request leaves one such stream behind (macOS, fastbuild): requests disabled enabled 20,000 246 MB 246 MB 60,000 482 MB 261 MB 100,000 801 MB 261 MB With the autogate disabled, RSS grows like on main; with it enabled, it levels off.
littledivy
added a commit
that referenced
this pull request
Oct 30, 2026
A visited handle is traced: only its owner's next visit keeps the target alive. Moving a jsg::Data makes the destination strong again, but moving the kj::Own<T> of an object that holds handles moves only the pointer. The handles stay traced with nothing visiting them, and the GC can free their targets while the object still uses them. Code that hit this either left the handles unvisited, which leaks, or worked around it by hand. The stream queue did the first: it never visited the resolvers of pending reads, so a ReadableStream whose read never settles was never collected, along with its reader and everything they held. WritableStreamInternalController::drain() does the second (see 687b3a6). In this patch: - GcVisitor::makeStrong() makes every handle reachable through a value strong: jsg::Data, the Rust Global/Traced pair, and Refs, whose parent it clears so another owner can visit them. It matches make_strong in Rust JSG's jsg::Cell (#7982), so both languages follow one rule: whatever leaves a traced owner is strong. - jsg::TracedOwn<T> is an owning pointer that records the isolate of its last visit and strengthens the object when it moves or is released. MemoryTracker accepts it like kj::Own. - The stream queue holds pending reads in TracedOwn and visits them from the consumer and from the queue, along with a BYOB read's view. The queue's visit keeps a pending read alive while its controller is reachable, as in the spec, where the controller holds its stream. cancel() drains the ring one element at a time instead of moving it. - The visits, and TracedOwn making handles strong, are behind a new autogate, gc-complete-tracing. With it disabled the resolvers stay unvisited strong roots and TracedOwn behaves like kj::Own, as before, so the rollout can be compared gated against ungated and turned off if code turns out to rely on such a stream staying alive. - A new clang-tidy check, jsg-traced-own, flags a visitForGc(), or a helper handed the visitor, that visits through a kj::Own, and kj::mv() of a container field whose elements hold a TracedOwn, directly or in a struct, kj::Maybe or kj::OneOf. Every site it found now holds a TracedOwn: stream readables and their consumers, the internal stream's pipe events, EventTarget listeners, CryptoKey and WebSocket. None of them moves its Own today; the check keeps it that way. The trace Request's Detail is shared through kj::addRef, so it keeps its kj::Own and the site is suppressed. - The jsg README, AGENTS.md and the GcVisitor comment describe the rule. Shared ownership is not covered: queue buffer entries are kj::Rc shared between consumers, so a buffered chunk that refers to its own stream still keeps the stream alive. Tests: - tracing-test: a value and a Ref taken out of a traced queue survive the queue being collected, and a cycle through a queued item is collected. With strengthening disabled the first case fails ("a value taken out of a traced queue was collected"). - streams-pending-gc-test, with the autogate enabled: streams, readers and BYOB streams with a pending read are collected; a pending read still settles, including when only the controller is reachable. streams-pending-gc-gate-off-test runs the same cases with the autogate disabled and checks that none of these streams is collected, as on main. - traced-own-test covers the clang-tidy check, memory-test the MemoryTracker overload, and standard-test detaching a readable stream now held in a TracedOwn. 1,000 streams with a never-settling read, after GC: 0 collected with the autogate disabled, 999 with it enabled. RSS of `workerd serve` for a worker whose every request leaves one such stream behind (macOS, fastbuild): requests disabled enabled 20,000 246 MB 246 MB 60,000 482 MB 261 MB 100,000 801 MB 261 MB With the autogate disabled, RSS grows like on main; with it enabled, it levels off.
littledivy
added a commit
that referenced
this pull request
Oct 30, 2026
A visited handle is traced: only its owner's next visit keeps the target alive. Moving a jsg::Data makes the destination strong again, but moving the kj::Own<T> of an object that holds handles moves only the pointer. The handles stay traced with nothing visiting them, and the GC can free their targets while the object still uses them. Code that hit this either left the handles unvisited, which leaks, or worked around it by hand. The stream queue did the first: it never visited the resolvers of pending reads, so a ReadableStream whose read never settles was never collected, along with its reader and everything they held. WritableStreamInternalController::drain() does the second (see 687b3a6). In this patch: - GcVisitor::makeStrong() makes every handle reachable through a value strong: jsg::Data, the Rust Global/Traced pair, and Refs, whose parent it clears so another owner can visit them. It matches make_strong in Rust JSG's jsg::Cell (#7982), so both languages follow one rule: whatever leaves a traced owner is strong. - jsg::TracedOwn<T> is an owning pointer that records the isolate of its last visit and strengthens the object when it moves or is released. MemoryTracker accepts it like kj::Own. - The stream queue holds pending reads in TracedOwn and visits them, along with a BYOB read's view, from the queue alone: a handle visited from two parents is strong or traced depending on which visited last. Every stream holds its controller, so the queue's visit keeps a pending read alive while its stream or its controller is reachable, as in the spec, where the controller holds its stream. close() and error() make the reads strong before the queue stops being visited, and cancel() drains the ring one element at a time instead of moving it. - The visits, and TracedOwn making handles strong, are behind a new autogate, gc-complete-tracing. With it disabled the resolvers stay unvisited strong roots and TracedOwn behaves like kj::Own, as before, so the rollout can be compared gated against ungated and turned off if code turns out to rely on such a stream staying alive. - A new clang-tidy check, jsg-traced-own, flags a visitForGc(), or a helper handed the visitor, that visits through a kj::Own, and kj::mv() of a container field whose elements hold a TracedOwn, directly or in a struct, kj::Maybe or kj::OneOf. Every site it found now holds a TracedOwn: stream readables and their consumers, the internal stream's pipe events, EventTarget listeners, CryptoKey and WebSocket. None of them moves its Own today; the check keeps it that way. The trace Request's Detail is shared through kj::addRef, so it keeps its kj::Own and the site is suppressed. - The jsg README, AGENTS.md and the GcVisitor comment describe the rule. Shared ownership is not covered: queue buffer entries are kj::Rc shared between consumers, so a buffered chunk that refers to its own stream still keeps the stream alive. Tests: - tracing-test: a value and a Ref taken out of a traced queue survive the queue being collected, and a cycle through a queued item is collected. With strengthening disabled the first case fails ("a value taken out of a traced queue was collected"). - streams-pending-gc-test, with the autogate enabled: streams, readers and BYOB streams with a pending read are collected; a pending read still settles, including when only the controller is reachable and on both branches of a tee when the stream closes. streams-pending-gc-gate-off-test runs the same cases with the autogate disabled and checks that none of these streams is collected, as on main. - traced-own-test covers the clang-tidy check, memory-test the MemoryTracker overload, and standard-test detaching a readable stream now held in a TracedOwn. 1,000 streams with a never-settling read, after GC: 0 collected with the autogate disabled, 999 with it enabled. RSS of `workerd serve` for a worker whose every request leaves one such stream behind (macOS, fastbuild): requests disabled enabled 20,000 246 MB 246 MB 60,000 482 MB 261 MB 100,000 801 MB 261 MB With the autogate disabled, RSS grows like on main; with it enabled, it levels off.
littledivy
added a commit
that referenced
this pull request
Oct 30, 2026
A visited handle is traced: only its owner's next visit keeps the target alive. Moving a jsg::Data makes the destination strong again, but moving the kj::Own<T> of an object that holds handles moves only the pointer. The handles stay traced with nothing visiting them, and the GC can free their targets while the object still uses them. Code that hit this either left the handles unvisited, which leaks, or worked around it by hand. The stream queue did the first: it never visited the resolvers of pending reads, so a ReadableStream whose read never settles was never collected, along with its reader and everything they held. WritableStreamInternalController::drain() does the second (see 687b3a6). In this patch: - GcVisitor::makeStrong() makes every handle reachable through a value strong: jsg::Data, the Rust Global/Traced pair, and Refs, whose parent it clears so another owner can visit them. It matches make_strong in Rust JSG's jsg::Cell (#7982), so both languages follow one rule: whatever leaves a traced owner is strong. - jsg::TracedOwn<T> is an owning pointer that records the isolate of its last visit and strengthens the object when it moves or is released. MemoryTracker accepts it like kj::Own. - The stream queue holds pending reads in TracedOwn and visits them, along with a BYOB read's view, from the queue alone: a handle visited from two parents is strong or traced depending on which visited last. Every stream holds its controller, so the queue's visit keeps a pending read alive while its stream or its controller is reachable, as in the spec, where the controller holds its stream. close() and error() make the reads strong before the queue stops being visited, and cancel() drains the ring one element at a time instead of moving it. - The visits, and TracedOwn making handles strong, are behind a new autogate, gc-complete-tracing. With it disabled the resolvers stay unvisited strong roots and TracedOwn behaves like kj::Own, as before, so the rollout can be compared gated against ungated and turned off if code turns out to rely on such a stream staying alive. - A new clang-tidy check, jsg-traced-own, flags a visitForGc(), or a helper handed the visitor, that visits through a kj::Own, and kj::mv() of a container field whose elements hold a TracedOwn, directly or in a struct, kj::Maybe or kj::OneOf. Every site it found now holds a TracedOwn: stream readables and their consumers, the internal stream's pipe events, EventTarget listeners, CryptoKey and WebSocket. None of them moves its Own today; the check keeps it that way. The trace Request's Detail is shared through kj::addRef, so it keeps its kj::Own and the site is suppressed. - The jsg README, AGENTS.md and the GcVisitor comment describe the rule. Shared ownership is not covered: queue buffer entries are kj::Rc shared between consumers, so a buffered chunk that refers to its own stream still keeps the stream alive. Tests: - tracing-test: a value and a Ref taken out of a traced queue survive the queue being collected, and a cycle through a queued item is collected. With strengthening disabled the first case fails ("a value taken out of a traced queue was collected"). - streams-pending-gc-test, with the autogate enabled: streams, readers and BYOB streams with a pending read are collected; a pending read still settles, including when only the controller is reachable and on both branches of a tee when the stream closes. streams-pending-gc-gate-off-test runs the same cases with the autogate disabled and checks that none of these streams is collected, as on main. - traced-own-test covers the clang-tidy check, memory-test the MemoryTracker overload, and standard-test detaching a readable stream now held in a TracedOwn. 1,000 streams with a never-settling read, after GC: 0 collected with the autogate disabled, 999 with it enabled. RSS of `workerd serve` for a worker whose every request leaves one such stream behind (macOS, fastbuild): requests disabled enabled 20,000 246 MB 246 MB 60,000 482 MB 261 MB 100,000 801 MB 261 MB With the autogate disabled, RSS grows like on main; with it enabled, it levels off.
guybedford
approved these changes
Nov 2, 2026
littledivy
added a commit
that referenced
this pull request
Nov 3, 2026
Main added Traced impls and std cells holding GC references after this branch was written. Each Traced impl now has make_strong, and each field that holds a jsg::Rc or a TracedGlobal and changes after construction is a jsg::Cell or jsg::RefCell: - make_strong for TracedAsyncContextFrame, promise reaction data, Function::new data, #[jsg_struct], #[jsg_oneof], NullOrUndefined, Dict, InspectorConnection and the io-context types. - jsg_cpp::Ref::make_strong makes its jsg::Ref strong as moving it does in C++. - Promise reaction data is a jsg::RefCell. - std::rc::Rc<T> is Traced only for T: HoldsNoGcReferences, so two owners can't trace the same reference, with a compile test.
littledivy
changed the title
littledivy
marked this pull request as ready for review
November 3, 2026 05:59
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
bd96796e.
This review, of af4d1527
💬 Approved with comments — A sound correctness fix with good tests. The two WARNINGs are documentation errors (a false claim about #[jsg_struct] and a README example that no longer compiles), plus two INFO notes. None of them affects runtime behaviour.
Findings: 2 warnings · 2 info (3 are inline comments on the diff)
Not opened by the correctness review: 1 changed file it was meant to read
types/rtti-fixture/main.rs
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #7982.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (2)
### [Code Quality] The `HoldsNoGcReferences` doc says `#[jsg_struct]` implements it, but the `jsg_struct` and oneof macros emit only `Traced`.
- **File:** src/rust/jsg/wrappable.rs:181
### [Documentation] The `DynamicResource` `custom_trace` example implements only `trace`. `make_strong` is now required, so the example doesn't compile.
- **File:** src/rust/jsg-macros/README.md:598
## INFO (2)
### [AGENTS.md] Clarify which `RefCell` the GC-related `borrow_mut()` restriction applies to.
- **File:** src/rust/AGENTS.md:38
### [Performance] `jsg::RefCell::borrow_mut` runs `make_strong` over the entire contents on every call, which is O(n) for collections.
- **File:** src/rust/jsg/cell.rs:145
📖 AGENTS.md: 💬 1 info
The RefCell GC caveat in src/rust/AGENTS.md reads as applying to jsg::RefCell too.
Recommendation: 💬 Optional Nits
- [INFO] Clarify which
RefCellthe GC-relatedborrow_mut()restriction applies to. (src/rust/AGENTS.md:38)
View full review
src/rust/AGENTS.md:38: the restriction that aborrow_mut()of a cell holding GC references must not cross an allocating call ("since tracing skips a mutably borrowedRefCell") reads as coveringjsg::RefCelltoo.jsg::RefCell::borrow_mutmakes the contents strong first, so skipping it during tracing is safe. Std cells can no longer hold GC references at all.
📋 Code Quality: ⚠️ 1 warning
No correctness bugs found. The HoldsNoGcReferences doc says #[jsg_struct] implements it, but no macro emits that impl.
Recommendation: 📝 Minor Comments
- [WARNING] The
HoldsNoGcReferencesdoc says#[jsg_struct]implements it, but thejsg_structand oneof macros emit onlyTraced. (src/rust/jsg/wrappable.rs:181)
View full review
src/rust/jsg/wrappable.rs:181: the doc saysjsg_traced!and#[jsg_struct]implementHoldsNoGcReferences. Injsg-macros, thejsg_structand oneof macros emit onlyTraced, neverHoldsNoGcReferences. So a plain-data#[jsg_struct]doesn't fit in a stdCell/RefCell/Rcwithout a hand-written impl, which is whatjsg-test/tests/gc.rsalready writes forNativeState.
Things checked that look correct:
CppRef::makeStrongmatches the C++jsg::Refmove semantics.Rc::make_strongresetsparent.jsg::RefCell::traceskipping a mutably borrowed cell is safe.- Every in-tree
Tracedimpl now providesmake_strong.
⚡ Performance: 💬 1 info
jsg::RefCell::borrow_mut walks the whole contents on every call.
Recommendation: 💬 Optional Nits
- [INFO]
jsg::RefCell::borrow_mutrunsmake_strongover the entire contents on every call, which is O(n) for collections. (src/rust/jsg/cell.rs:145)
View full review
src/rust/jsg/cell.rs:borrow_mutrunsmake_strongover the whole contents. For aRefCell<Vec<jsg::Rc<_>>>that's O(n) per borrow, so pushing elements one borrow at a time is O(n²). Each element's check is cheap, so this only matters for large collections that are mutated often.
📝 Documentation: ⚠️ 1 warning
The custom_trace README example omits the now-required make_strong and no longer compiles.
Recommendation: 📝 Minor Comments
- [WARNING] The
DynamicResourcecustom_traceexample implements onlytrace.make_strongis now required, so the example doesn't compile. (src/rust/jsg-macros/README.md:598)
View full review
src/rust/jsg-macros/README.md:598-606: theDynamicResourceexample implements onlytrace.Traced::make_strongis now required with no default (src/rust/jsg/wrappable.rs:164), so copying the example gives a compile error. TheEventHandlersexample earlier in the same file was updated, but this one was missed.
✅ All other checks passed
🔒 Security: ✅ LGTM
No security issues found.
Reviewed commit: af4d1527
There was a problem hiding this comment.
Superseded by the review of
5ac69e4f.
This review, of bd96796e
💬 Approved with comments — A sound GC correctness fix with good tests. Both WARNINGs are documentation errors (a false claim that #[jsg_struct] implements HoldsNoGcReferences, and a README example that won't compile), and neither changes runtime behaviour.
Findings: 2 warnings · 2 info
Not opened by the correctness review: 1 changed file it was meant to read
types/rtti-fixture/main.rs
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #7982.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (2)
### [Code Quality] The `HoldsNoGcReferences` doc says `#[jsg_struct]` implements it, but the `jsg_struct` and oneof macros emit only `Traced`.
- **File:** src/rust/jsg/wrappable.rs:182
### [Documentation] The `DynamicResource` `custom_trace` example implements only `trace`. `make_strong` is now required, so the example doesn't compile.
- **File:** src/rust/jsg-macros/README.md:598
## INFO (2)
### [AGENTS.md] Say which `RefCell` the GC-related `borrow_mut()` restriction applies to.
- **File:** src/rust/AGENTS.md:38
### [Performance] `jsg::RefCell::borrow_mut` runs `make_strong` over the whole contents on every call, which is O(n) for collections.
- **File:** src/rust/jsg/cell.rs:147
📖 AGENTS.md: 💬 1 info
The GC-related borrow_mut() restriction doesn't say which RefCell it means.
Recommendation: 💬 Optional Nits
- [INFO] Say which
RefCellthe GC-relatedborrow_mut()restriction applies to. (src/rust/AGENTS.md:38)
View full review
src/rust/AGENTS.md:38: the rule against holding aborrow_mut()across an allocating call, "since tracing skips a mutably borrowedRefCell", reads as if it coversjsg::RefCell.jsg::RefCell::borrow_mutmakes the contents strong first, and std cells can no longer hold GC references, so the reason given no longer matches either type.
📋 Code Quality: ⚠️ 1 warning
The HoldsNoGcReferences doc says #[jsg_struct] implements it, but the macro doesn't. The isolates.rs change is correct.
Recommendation: 📝 Minor Comments
- [WARNING] The
HoldsNoGcReferencesdoc says#[jsg_struct]implements it, but thejsg_structand oneof macros emit onlyTraced. (src/rust/jsg/wrappable.rs:182)
View full review
src/rust/jsg/wrappable.rs:181-182: the doc saysjsg_traced!and#[jsg_struct]implementHoldsNoGcReferences. Thejsg_structand oneof macros injsg-macrosemit onlyimpl jsg::Traced. So a plain-data#[jsg_struct]can't go in a stdCell,RefCellorRcwithout a hand-written impl, which is whatjsg-test/tests/gc.rsalready writes forNativeState.src/rust/jsg-test/tests/isolates.rs: the move tojsg::Cellandtake(lock)is correct.
⚡ Performance: 💬 1 info
jsg::RefCell::borrow_mut takes O(n) time per call for collections.
Recommendation: 💬 Optional Nits
- [INFO]
jsg::RefCell::borrow_mutrunsmake_strongover the whole contents on every call, which is O(n) for collections. (src/rust/jsg/cell.rs:147)
View full review
src/rust/jsg/cell.rs:145-149:borrow_mutandtry_borrow_mutrunmake_strongover the whole contents on every call. This only matters for large collections that are mutated often.
📝 Documentation: ⚠️ 1 warning
The DynamicResource custom_trace example has no make_strong, which is now required.
Recommendation: 📝 Minor Comments
- [WARNING] The
DynamicResourcecustom_traceexample implements onlytrace.make_strongis now required, so the example doesn't compile. (src/rust/jsg-macros/README.md:598)
View full review
src/rust/jsg-macros/README.md:598-606: theDynamicResourceexample implements onlytrace.Traced::make_strongis now required, so copying the example gives a compile error. TheEventHandlersexample earlier in the same file was updated.
✅ All other checks passed
🔒 Security: ✅ LGTM
No security issues found.
Reviewed commit: bd96796e
There was a problem hiding this comment.
Superseded by the review of
ddce94a0.
This review, of 5ac69e4f
✅ Approved — Both documentation warnings from the previous review are fixed, and nothing new came up in the code changed since. The GC correctness fix and its tests look sound.
Not opened by the correctness review: 1 changed file it was meant to read
types/rtti-fixture/main.rs
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No security issues found.
📖 AGENTS.md: ✅ LGTM
The guidance in src/rust/AGENTS.md matches how the new cells behave.
📋 Code Quality: ✅ LGTM
The HoldsNoGcReferences doc in src/rust/jsg/wrappable.rs now matches the macros. Every make_strong impl passes the call on to each element, and jsg::Cell and jsg::RefCell make values strong before they leave the cell. CppRef::makeStrong uses the C++ move constructor to make the reference strong. The RTTI fixture's move to jsg::Cell is correct.
⚡ Performance: ✅ LGTM
take and replace each make a single make_strong call. The collection make_strong impls visit each element once, which costs the same as tracing.
📝 Documentation: ✅ LGTM
The DynamicResource example in src/rust/jsg-macros/README.md now implements make_strong, so it compiles.
Reviewed commit: 5ac69e4f
jamesopstad
approved these changes
Nov 3, 2026
piscisaureus
force-pushed
the
piscisaureus/rust-jsg-rc-take
branch
from
November 3, 2026 17:19
5ac69e4 to
ddce94a
Compare
Contributor
Author
|
Rebased onto main and squashed (Divy's follow-ups are folded in, with a One design change while rebasing: #8153 added
Not included: an isolate check for a @guybedford could you take another look? |
There was a problem hiding this comment.
Superseded by the review of
515070ea.
This review, of ddce94a0
✅ Approved — No issues found. The detach-based jsg::Cell/jsg::RefCell and the HoldsNoGcReferences bounds look correct and come with tests.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
The HoldsNoGcReferences bounds tighten the memory-safety boundary: a GC reference held in a std cell or a std Rc no longer compiles.
📖 AGENTS.md: ✅ LGTM
The guidance in src/rust/AGENTS.md matches how the cells behave.
📋 Code Quality: ✅ LGTM
take and replace move the old value out before detaching it, and borrow_mut detaches while the borrow is held. The isolate check runs after the early return for a closed isolate. The call sites, including types/rtti-fixture/main.rs, are updated correctly, and jsg-test/tests/cell.rs and the compile tests cover the new behaviour.
⚡ Performance: ✅ LGTM
Each take, replace or borrow_mut runs one detaching visit, which costs about the same as a trace. The marker traits add no runtime cost.
📝 Documentation: ✅ LGTM
The documentation matches the detach-based design, and nothing refers to make_strong any more.
Reviewed commit: ddce94a0
piscisaureus
force-pushed
the
piscisaureus/rust-jsg-rc-take
branch
from
November 3, 2026 17:30
ddce94a to
4ef0fd8
Compare
When GC visits a jsg::Rc or a TracedGlobal in a field, it stops being strong and relies on the field's owner to trace it. Moving it out of the field (Cell::take, popping a Vec, ...) leaves it neither strong nor traced, so the next GC can collect its wrapper or value while Rust still uses it (#7976). C++ jsg::Ref avoids this in its move constructor, but Rust has no hook on moves. * jsg::Cell and jsg::RefCell hold traced state that changes after construction. take, replace and borrow_mut take the lock and first detach the contents from their owner with GcVisitor::detach, as a typed promise's reaction does with its value: every jsg::Rc and TracedGlobal in them is strong again and forgets its parent. A detaching visit now checks that each TracedGlobal belongs to the lock's isolate, as other TracedGlobal operations do. * std::cell::Cell, std::cell::RefCell and std::rc::Rc are Traced only for types that hold no GC references (jsg::HoldsNoGcReferences), so a jsg::Rc or TracedGlobal in a std cell, or shared by two owners, doesn't compile. jsg_traced! and the built-in wrappers and collections implement it. This replaces the debug-build abort when tracing a std RefCell that can hold GC references. * Fields that held a GC reference in a std cell, including the data of promise reactions and the value of an opaque wrapper, now use jsg::Cell or jsg::RefCell. * Compile tests for a GC reference in a std cell or a std Rc, and tests in jsg-test/tests/cell.rs that take a task out of a cell after a GC has made it weak, then GC again. Co-authored-by: Divy Srivastava <divy@cloudflare.com>
piscisaureus
force-pushed
the
piscisaureus/rust-jsg-rc-take
branch
from
November 3, 2026 17:30
4ef0fd8 to
515070e
Compare
There was a problem hiding this comment.
✅ Approved — No issues found. The only change since the last review is the isolate check in v8.rs. It is correct, it keeps the early return for a closed isolate, and a test covers it.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No security concerns. The new isolate check makes the detaching path stricter.
📖 AGENTS.md: ✅ LGTM
The guidance in src/rust/AGENTS.md still matches how the cells behave.
📋 Code Quality: ✅ LGTM
VisitMode::Detach now carries the lock's isolate pointer, and visit_traced_global asserts global.state.belongs_to(isolate) after the is_closed() early return. detach now takes &Lock, and its callers in cell.rs are updated to match. isolates.rs tests the panic and its message.
⚡ Performance: ✅ LGTM
The new check adds one pointer comparison per detaching visit, which costs almost nothing.
📝 Documentation: ✅ LGTM
The doc comments on detach and visit_traced_global match what the code does.
Reviewed commit: 515070ea
guybedford
approved these changes
Nov 3, 2026
bartlomieju
added a commit
that referenced
this pull request
Nov 3, 2026
Once a GC has visited a resource that keeps a #[jsg_struct] with a TracedGlobal field, the handle is weak and relies on the resource's trace. Taken out of the field with a plain move, nothing traces it and the next GC can collect the value, after which to_js panics with "C++ passed an empty v8::Local". With jsg::RefCell (#7982), borrow_mut detaches the struct before it can leave, so it is strong again. * A test takes such a struct out of a resource's jsg::RefCell after a GC, runs another GC and converts it back. Without the detach in borrow_mut, it fails with that panic. * struct_js_value_field_outlives_its_scope runs a GC between its two scopes, and holds objects rather than a string literal, so it checks that the struct's handles keep their values. * The #[jsg_struct] docs say that a resource changing such a struct after construction keeps it in a jsg:: cell.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When GC visits a
jsg::Rcor aTracedGlobalin a field, it stopsbeing strong and relies on the field's owner to trace it. Moving it out
of the field (
Cell::take, popping aVec, ...) leaves it neitherstrong nor traced, so the next GC can collect its wrapper or value
while Rust still uses it (#7976). C++
jsg::Refavoids this in itsmove constructor, but Rust has no hook on moves.
In this patch:
jsg::Cellandjsg::RefCellfor traced fields that change afterconstruction.
take,replaceandborrow_muttake the lock andfirst detach the contents from their owner with
GcVisitor::detach,as a typed promise's reaction does with its value (Rust JSG: typed promises #8153): every
jsg::RcandTracedGlobalin them is strong again and forgets itsparent. This is the same rule as C++
jsg::TracedOwn(Add jsg::TracedOwn so visited handles survive their owner moving #8103). Adetaching visit checks that each
TracedGlobalbelongs to thelock's isolate.
std::cell::Cell/RefCellandstd::rc::RcareTracedonly fortypes that hold no GC references (
jsg::HoldsNoGcReferences), so ajsg::RcorTracedGlobalin a std cell, or shared by two owners,doesn't compile. This replaces the debug-build abort when tracing a
std
RefCellthat can hold GC references.reactions and the value of an opaque wrapper, now use
jsg::Cell/RefCell.Take a task out of a cell after a GC has made it weak, then GC again
(
jsg-test/tests/cell.rs):jsg::CellTracedGlobal