webcore: store ReadableStreamSource onClose in WriteBarrier slot instead of Strong - #32582
Jarred-Sumner merged 8 commits into
Conversation
…ead of Strong
NewSource.close_jsvalue was a jsc.Strong, which rooted a cycle through
the native heap: source wrapper -> m_ctx NewSource -> Strong(onClose)
-> bound #onClose -> NativeReadableStreamSource -> $stream -> source
wrapper. The cycle only broke when EOF ran callClose or cancel ran
#cancel, both of which clear $stream. A stream that is read partially
and then dropped (releaseLock without cancel) never hits either path,
so every JS{Blob,Bytes,File}InternalReadableStreamSource wrapper and
its NewSource leaked forever.
The codegen already declares an onCloseCallback WriteBarrier slot in
streams.classes.ts (onDrain already uses its equivalent). Switch
onClose to the same storage and delete the Strong field; the cycle
becomes an ordinary intra-heap cycle that mark-sweep collects.
Also take the across-read ref on the Windows non-lazy FileReader path
(fromPipe via Bun.spawn stdout/stderr), matching the existing POSIX
arm. Without the Strong cycle masking it, the source is now
collectable while a uv_read_start is pending.
This re-applies the fix from #29472 to the Rust port.
|
Updated 9:43 PM PT - Jun 22nd, 2026
✅ @robobun, your commit b894a15de8cd87fbc07bd8472621163dda474f34 passed in 🧪 To try this PR locally: bunx bun-pr 32582That installs a local version of the PR into your bun-32582 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Not linking #7272: that report is a 2GB streaming download via |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesReadableStream onClose GC leak fix
Windows FileReader IOCP refcount fix
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/FileReader.rs`:
- Around line 452-462: The Windows block starting at line 452 is missing an
idempotency guard that exists in the POSIX sibling block. Add a
`!self.started.get()` check as part of the condition in the Windows block
(alongside the existing checks on `self.reader().source.is_some()` and
`!self.reader().is_done()`) to prevent `increment_count()` from being called
multiple times if `on_start()` is invoked before `self.started.set(true)` is
executed. This guard ensures the refcount is only incremented once, preventing
the reference leak that would occur if decrement operations do not match the
number of increments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4e1de60-4670-46e1-a90c-aaaffe22564b
📒 Files selected for processing (3)
src/runtime/webcore/FileReader.rssrc/runtime/webcore/ReadableStream.rstest/js/web/streams/native-source-onclose-leak.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…/O is pending
Removing the close_jsvalue Strong also removed the only GC root that
transitively kept the JS{Blob,Bytes,File}InternalReadableStreamSource
wrapper alive across a pending FileReader read. this_jsvalue is a raw
heap-resident JSValue zeroed only at lazy sweep, so on_js_close (reached
from FileReader::on_reader_done off the event loop, with no JS frame on
the stack) could read m_onCloseCallback from a dead-but-unswept cell and
queue a dead bound function as a microtask.
Add hasPendingActivity: true to streams.classes.ts and implement
NewSource::has_pending_activity as ref_count > 1. Only FileReader bumps
ref_count (for the across-read ref taken in on_start), so Blob/Bytes
sources always return false and File sources return true exactly while
waiting_for_on_reader_done is held. ref_count becomes AtomicU32 so the
GC-thread read is sound.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/ReadableStream.rs`:
- Around line 917-929: The ref_count atomic operations use Release and Acquire
orderings without documented justification, which conflicts with the repo's
guideline to default to SeqCst ordering. Fix this by either replacing all
Release and Acquire orderings with SeqCst throughout the increment_count,
decrement_count, and has_pending_activity methods, or by adding an explicit
comment above the ref_count field documenting why the Release/Acquire pair is
safe for the cross-thread access pattern (GC thread reading via
has_pending_activity, JS thread writing via increment_count and
decrement_count).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c3d637a-6e3f-40e4-a889-4380bd517e62
📒 Files selected for processing (2)
src/runtime/api/streams.classes.tssrc/runtime/webcore/ReadableStream.rs
There was a problem hiding this comment.
All prior review feedback has been addressed and I have no further findings, but this changes GC rooting for every native ReadableStream source (Strong→WriteBarrier + new hasPendingActivity read from the GC thread) and re-applies a previously-reverted fix, so it warrants a human look before merge.
Extended reasoning...
Overview
This PR swaps NewSource.close_jsvalue: Strong for the existing codegen onCloseCallback WriteBarrier slot to break a GC root cycle that leaked one native source wrapper per abandoned (partially-read, releaseLock()-without-cancel()) stream. To compensate for losing the Strong as an implicit root across pending I/O, it adds hasPendingActivity: true to the {Blob,Bytes,File}InternalReadableStreamSource codegen and implements it as ref_count > 1 (with ref_count made AtomicU32 since it is now read from the GC thread). It also adds a Windows arm in FileReader::on_start to take the across-read ref on the non-lazy fromPipe path, mirroring the existing POSIX arm. Four regression tests cover collectability and onClose delivery.
Security risks
No auth/crypto/permissions surface. The risk class here is memory safety: GC-lifecycle changes that, if wrong, manifest as UAF or heap type-confusion (e.g. the dead-but-unswept this_jsvalue window I flagged earlier, now closed by hasPendingActivity). The fix as it stands looks correct to me, but this category of bug is notoriously hard to verify by inspection alone.
Level of scrutiny
High. This is production-critical runtime code on the hot path for Blob.stream(), fetch() response bodies, and Bun.spawn() stdout/stderr. It alters how three JSC wrapper classes are rooted, introduces a GC-thread read of native state, and re-applies a fix (#29472) that was previously reverted — all of which argue for a human reviewer (the PR description suggests dylan-conway) to confirm the rooting model and the ref_count > 1 ↔ waiting_for_on_reader_done correspondence.
Other factors
All inline review feedback (mine and CodeRabbit's) has been applied: the Windows !self.started.get() idempotency guard, the stale accessor-count doc comment, the hasPendingActivity rooting fix for the UAF window, and the SeqCst atomic ordering. The bug-hunting pass on the current head found nothing. CI on the most-recently-reported commit shows build failures on FreeBSD/Android build-rust and a Windows agent-creation failure that look infra-related rather than code-related, but the author should confirm a green build on the final commit. Given the change is well-tested and all bot feedback is resolved, the remaining gate is a human sanity-check on the GC-lifecycle reasoning.
There was a problem hiding this comment.
All earlier review feedback (including the dead-cell deref window) has been addressed; deferring to a human since this changes GC rooting/lifetime for every native ReadableStream source and re-applies a previously-reverted fix.
Extended reasoning...
Overview
The PR swaps NewSource.close_jsvalue: Strong for the codegen'd onCloseCallback WriteBarrier slot on JS{Blob,Bytes,File}InternalReadableStreamSource, breaking a Strong-rooted cycle that pinned one source wrapper per partially-read-then-abandoned stream. To compensate for losing that root while I/O is pending, it adds hasPendingActivity: true to streams.classes.ts, implements NewSource::has_pending_activity() as ref_count > 1, and converts ref_count to AtomicU32 (SeqCst) since the check runs on the GC thread. A new #[cfg(windows)] arm in FileReader::on_start mirrors the POSIX across-read ref for the non-lazy fromPipe (spawn stdout/stderr) path. Four regression tests cover the leak, full-consumption, and onClose delivery.
Security risks
No auth/crypto/permission surfaces. The relevant risk is memory safety: removing a GC root from a native↔JS cycle while async I/O completions still dereference this_jsvalue. My earlier 🔴 finding identified a dead-but-unswept-cell read in on_js_close; the author fixed it with hasPendingActivity keyed on ref_count > 1, which keeps the wrapper marked exactly while waiting_for_on_reader_done holds the extra ref. With that in place I don't see a remaining UAF window, but the correctness depends on the invariant that every path which can reach on_js_close from a non-JS frame holds an outstanding increment_count() — a property that's easy to break in future edits.
Level of scrutiny
High. This is GC-lifetime plumbing on a hot path (every Blob/fetch-body/spawn-pipe stream), it changes cross-thread state (has_pending_activity reads from the marker thread), it adds platform-specific behavior (Windows IOCP ref), and it re-applies a fix that was landed as #29472 and then reverted in bf2e2ce — the prior revert alone is reason enough for a human to confirm the failure mode that triggered it doesn't recur in the Rust port.
Other factors
All four inline review threads (mine and CodeRabbit's) are resolved on the current head: the !self.started.get() idempotency guard on the Windows arm, the stale macro doc-comment count, the hasPendingActivity addition, and the SeqCst ordering convention. The bug-hunting pass on the latest revision found nothing new. Test coverage looks reasonable for the leak itself, though the Windows IOCP path and the GC-thread hasPendingActivity interaction are not directly exercised by the new tests and rely on existing CI suites.
CI statusBuild 64082 (b894a15, the Earlier builds:
The new test file The diff is ready for review. Remaining CI red across builds is darwin-14 agent availability or unrelated single-lane flake. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Don't use hasPendingActivity for this. The sink generator code already uses JSC::Weak. What can we do to simplify the lifetime here? Can we use JSRef and then downgrade/upgrade conditionally?
… ref_count Per review: drop the hasPendingActivity codegen hook and the AtomicU32 ref_count in favor of the JsRef pattern already used by Subprocess, Image, Socket, and ResumableSink. this_jsvalue becomes a JsRef. increment_count() upgrades it to Strong so the wrapper is rooted while a native I/O ref is held (in practice the FileReader waiting_for_on_reader_done ref). decrement_count() downgrades back to Weak when only the wrapper's own ref remains. finalize() flips it to Finalized so on_js_close reads None instead of a dead-but-unswept cell. The redundant per-host-fn this_jsvalue = call_frame.this() writes are dropped: the wrapper JSValue is set once in to_readable_stream and never changes, and keeping them would have to preserve the Weak/Strong state explicitly.
|
@Jarred-Sumner done in b894a15. Dropped
Same pattern as
|
2 participants
Summary
NewSource.close_jsvaluewas ajsc.Strong, which rooted a cycle through the native heap:Because a
Strongis a global GC root, the source wrapper survives even after every JS reference (including the outerReadableStream) is dropped. The cycle only broke when EOF ran the JS-sidecallClose(which clears$stream) or the cancel algorithm ran#cancel. A stream that is read partially and then dropped (reader.releaseLock()withoutcancel()) never hits either path, so the source wrapper leaked one per abandoned stream until VM shutdown.Reproduction
The same pattern hits
fetch()response bodies whose body exceeds one pull buffer.Fix
The codegen already declares an
onCloseCallbackWriteBarrierslot instreams.classes.ts(values: ["pendingPromise", "onCloseCallback", "onDrainCallback"]);onDrainalready uses its slot. SwitchonCloseto the same storage and delete theStrongfield. The cycle becomes an ordinary intra-heap cycle that mark-sweep collects.Also take the across-read ref on the Windows non-lazy
FileReader.on_startpath (fromPipeviaBun.spawn().stdout/.stderr), matching the existing POSIX arm. Without the Strong cycle masking it, the source is now collectable while auv_read_startIOCP read is pending; the ref keeps it alive untilon_reader_done/on_reader_errorreleases it.Relation to #29472 / #29440
This re-applies the fix originally landed as #29472 (Zig, reverted in bf2e2ce) and re-opened as #29440 (still targets the uncompiled
.zigreference files), to the Rust port. TheWindowsBufferedReader.deinitordering fix that #29440 bundles is already present insrc/io/PipeReader.rs.Verification