webstreams: hold the native-source adapter's controller as a WriteBarrier - #36337
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesNative stream adapter lifecycle
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Gate note: the fail-before check will not pass on this PR. The crash requires a GC to land in the window between FetchTasklet's Strong release and the The fix is verifiable from the type change: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/jsc/JSRef.rs:175-180— This change introduces a newtry_get() == Nonestate — "Weak handle was populated but GC-cleared, wrapper not yet swept" — thatNewSocket::get_this_value(src/runtime/socket/socket_body.rs:1548) does not distinguish from "wrapper never created", so it falls through toself.to_js(global)and creates a second JS wrapper over the samem_ctxwithout a ref bump; when the first (white) wrapper is swept both wrappers finalize the same native → double-deref/UAF. The gate-note claim that "every existing None branch at the 29 holder sites covers the cleared case" is false for this site — either exposeWeakHandle(Some(_)) && get()==Noneas a distinguishable state (or add aJsRef::was_ever_set()) and haveget_this_valuetreat it the same asFinalized.Extended reasoning...
What changed and what it broke
Before this PR,
JsRef::Weakheld a rawJSValue. Once a wrapper had been stored (viainit_strong→downgradeinmark_inactiveat socket_body.rs:1319-1320, or in the connect-error path),try_get()on that Weak returnedSome(value)right up until the wrapper's codegen finalizer flipped the ref toJsRef::Finalized. Callers could therefore rely on:try_get() == None && !Finalized⟹ no wrapper was ever created.After this PR,
JsRef::Weak(WeakHandle)wraps a realJSC::Weak<JSObject>. JSC clears Weak handles at the end of marking, before the pointee's lazy destructor (sweep) runs. So in the [end-of-marking, sweep] window — the exact window this PR's own description documents — the ref is stillJsRef::Weak(..)(not yetFinalized) buttry_get()already returnsNone. The invariant above no longer holds:try_get() == None && !Finalizedis now also reachable for a wrapper that did exist and is white-and-unswept.The affected call site
NewSocket::get_this_value(src/runtime/socket/socket_body.rs:1548-1567) depends on the old invariant:if let Some(value) = self.this_value.get().try_get() { return value; } if matches!(self.this_value.get(), JsRef::Finalized) { // The JS wrapper was already garbage-collected. Creating a new one // here would result in a second `finalize` (and double-deref) later. return JSValue::UNDEFINED; } let value = self.to_js(global); // adopts ownership into a NEW JSCell wrapper ... self.this_value.with_mut(|r| r.set_strong(value, global));
If this runs while
this_valueis a GC-cleared Weak, both guards fall through andself.to_js(global)is called.to_js(&self)at socket_body.rs:446-458 passesself.as_ctx_ptr()straight to the codegenjs_{TCP,TLS}Socket::to_jswithout aref_()— its comment says "ownership is adopted by the C++ JSCell wrapper, which callsfinalizeon GC". So a second JS wrapper is created over the same nativeNewSocketwith no extra +1 on the refcount.Step-by-step proof of double-finalize
- Socket is created;
this_value=Strong(wrapper₁). Native refcount includes the +1 adopted by wrapper₁. - Socket closes →
mark_inactive()(socket_body.rs:1303-1321) runsthis_value.downgrade()→this_value=Weak(WeakHandle(Some(h))),hpoints at wrapper₁. - User code drops the last JS reference to wrapper₁. GC marking runs; wrapper₁ is unreachable (white). At end-of-marking JSC clears
h→WeakHandle::get()returnsNone. Sweep is lazy; wrapper₁'s block has not been swept, sofinalize()has not run andthis_valueis stillJsRef::Weak(..). - A native event dispatch (e.g.
on_close,on_writable, or the Listener reuse-prev path at Listener.rs:1520 whose comment explicitly discusses "prev.this_value was downgraded to Weak") callsget_this_value(global):try_get()→None(handle cleared).matches!(.., Finalized)→false(stillWeak).- Falls through:
self.to_js(global)creates wrapper₂ over the samem_ctx(no ref bump);this_value.set_strong(wrapper₂).
- wrapper₁'s block is later swept → codegen finalizer →
NewSocket::finalize()→this_value.finalize()(drops the Strong holding wrapper₂) +deref()(refcount −1). - wrapper₂ is now unrooted; when it's collected → codegen finalizer →
NewSocket::finalize()again → secondderef()on the same native → refcount underflow / use-after-free.
The
Finalizedguard's own comment at line 1553-1554 ("Creating a new one here would result in a secondfinalize(and double-deref) later") names precisely this hazard. This PR introduces a second "wrapper existed and is dead" state that bypasses that guard.Why this belongs in this PR
The PR's gate-note claims "Every existing None branch at the 29 holder sites covers the cleared case" — this site's
None-and-not-Finalizedbranch does not cover it; it creates a duplicate wrapper. This is the same class of timing window (native callback firing between end-of-marking and sweep) that the PR is fixing everywhere else, and the PR already added an equivalentclosed-flag guard for the identical shape innode_fs_stat_watcher.rs. Per REVIEW.md ("Fix the whole class in the same PR", "Fix bugs at the layer that owns the violated invariant"), the socket site needs the same treatment.Fix
The information is already there:
WeakHandle(Some(_))withget() == Noneis "was populated, now cleared" and is distinguishable fromWeakHandle(None)("never populated"). Expose that (e.g.JsRef::was_cleared()/WeakHandle::was_set()) and haveget_this_valuereturnJSValue::UNDEFINEDfor the cleared-Weak case exactly as it does forFinalized. Alternatively, gate the fall-through on the socket's own explicit protocol flags (mirroring theclosedcheck added forStatWatcherin this PR). - Socket is created;
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This is neither correct nor necessary.
|
Could you say which part? The approach here follows two Slack follow-ups attributed to you ("convert What I have so far on the pipe side: every If the fix belongs in the streams layer instead (barrier ordering on |
|
Backing this approach out per the review above. The What I can say about the pipe-side rooting chain:
None of that accounts for the core's post-hoc state (pipe cluster Moving this to draft and leaving the branch as-is for reference. If you have a hypothesis for the actual mechanism I'm happy to take another run; otherwise this should probably go back to the fuzz ledger as "confirmed state, mechanism not identified". |
…rier JSNativeStreamSourceAdapter::m_controller was a JSC::Weak<>. When the native pull promise is rejected (socket fault on a fetch body) the adapter is queued as the onNativePullRejected reaction context, which roots the adapter but not the controller: the adapter's only edge to it was the Weak. FetchTasklet releases both native Strong<>s to the body stream before that microtask drains, so a GC in between could leave the entire consumer graph (controller -> stream -> reader -> pipe op -> destination -> writer -> readyPromise) white. The subsequent error cascade then enqueues the pipe's writes-drained shutdown deferral against a corpse op, and performPipeShutdownAction(AbortDestination) dereferences a swept readyPromise (RELEASE_ASSERT in JSObject::realm()). MarkedSpace::reapWeakSets() only reaps m_activeWeakSets on a Full collection, so a Weak in an old block can read Live across an eden cycle that left its pointee unmarked; the Weak edge is not the safe null read the design assumed. Hold m_controller as a visited WriteBarrier so a queued adapter roots the controller directly. The edge is cleared on every terminal path (nativeSourcePullRejected, nativeSourceCallClose, nativeSourceCancel); controller->algorithmContext is cleared by clearAlgorithms, so the abandoned case is an ordinary intra-heap cycle. With no Weak member the adapter becomes JSNonFinalObject (no destructor). native-source-onclose-leak.test.ts (the partial-read + releaseLock abandonment test) continues to pass, confirming the cycle does not pin. The committed pipeTo-shutdown-gc stress test exercises the shape (native body source, socket fault mid-stream, fire-and-forget pipeTo under collectContinuously, AbortDestination shutdown arm). The crash itself is 0/1800 standalone; it reproduces ~1/3 only under a fault-injected tracer replay.
7623250 to
80bf40f
Compare
robobun
changed the title
The five JSValue members (handle, pendingView, closer, drainValue, controller) become internal fields; the base class visits them. Scalar members (chunkSize, the flag bitfield, textState) stay as plain members. Typed accessors keep call-site readability.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
this appends the edges twice to heap analyzer since internalfieldobjectimpl does appendValues and not appendValuesHidden
Base::visitChildren uses appendValues (not hidden), so the internal fields already surface as edges in heap snapshots; analyzeHeap was reporting them a second time with labels.
|
Dropped The only |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/js/web/streams/pipeTo-shutdown-gc.test.ts`:
- Around line 4-20: Replace the speculative multi-line crash narrative above the
regression test with a concise statement of the durable invariant or execution
shape being exercised. Remove asserted causal details about Weak references, GC
timing, swept objects, and specific shutdown internals while retaining only the
stable regression intent.
- Around line 36-47: Synchronize the abrupt socket failure in
test/js/web/streams/pipeTo-shutdown-gc.test.ts:36-47 with an observable
milestone such as the sink’s first write instead of elapsed timers. In
test/js/web/streams/pipeTo-shutdown-gc.test.ts:67-103, remove or sequence the
competing ac.abort() path and assert the observable native source-error and
abort-sink outcome, ensuring the test fails specifically when the protected
controller edge is absent.
🪄 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: af766ff8-dd78-4531-99e0-342543e6d36c
📒 Files selected for processing (4)
src/jsc/bindings/webcore/streams/BunStreamSource.cppsrc/jsc/bindings/webcore/streams/BunStreamSource.hsrc/jsc/bindings/webcore/streams/ReadableStreamOperations.cpptest/js/web/streams/pipeTo-shutdown-gc.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
test/js/web/streams/pipeTo-shutdown-gc.test.ts:96-102—BUN_JSC_collectContinuously: "1"is set unconditionally with a 30s non-ASAN timeout, but every one of the ~11 existing tests in the repo that use this flag gates it on!isWindows(documented as "brutally slow on Windows … >60s for a single subprocess on x64-baseline"). This test's workload — 320 concurrentfetch()calls through the full native-body/pipeTo/WritableStream stack plus 54Bun.gc(true)cycles under a per-allocation collector — is heavier than every gated precedent, so it will time out on Windows CI. ImportisWindowsand eithertest.skipIf(isWindows)(...)(matchingfetch-response-finalizer-sweep.test.ts, the closest structural analog) or gate the env var:...(isWindows ? {} : { BUN_JSC_collectContinuously: "1" })— the script already does explicitBun.gc(true)storms so it retains value ungated.Extended reasoning...
What the finding is
test/js/web/streams/pipeTo-shutdown-gc.test.ts:96sets:env: { ...bunEnv, BUN_JSC_collectContinuously: "1" },
unconditionally, and the test timeout at line 102 branches only on
isASAN(isASAN ? 90_000 : 30_000), never onisWindows.isWindowsis not imported from harness at all.Why this is a repo convention, not a guess
I grepped
test/forcollectContinuouslyand found ~11 existing tests that setBUN_JSC_collectContinuously. All of them gate onisWindows, most with a near-identical comment:test/js/bun/resolve/require-esm-gc-roots.test.ts:42-49— "collectContinuously is brutally slow on Windows (every allocation triggers a full GC; >60s for a single subprocess on x64-baseline)" →isWindows ? forceRAMSize : collectContinuously.test/js/bun/transpiler/transpiler-error-gc-uaf.test.ts:38-44— "Windows + collectContinuously is prohibitively slow in CI and the code path is platform-agnostic" →if (!isWindows) gcEnv.BUN_JSC_collectContinuously = "1".test/js/bun/test/jest-each-gc-root.test.ts:93-99— same comment, same gate.test/regression/issue/29519.test.ts:12-16,test/regression/issue/30205.test.ts:90-93—describe.skipIf(isWindows)/test.skipIf(isWindows).test/js/web/fetch/fetch-response-finalizer-sweep.test.ts:86-88— the closest structural analog (a spawned subprocess doingfetch()against anet.createServerundercollectContinuously) →describe.skipIf(isWindows)with "the code path is identical across platforms".- Also gated:
abort-controller-gc-reason.test.ts,message-event-init-gc.test.ts,sourcetextmodule-link-gc.test.ts,module-children-concurrent-gc.test.ts,esm-registry-concurrent-gc.test.ts.
REVIEW.md, Tests reviewers reject: "Copy harness conventions exactly"; "Check
harness.tsfor platform helpers before writing your own"; "A correct but slow test still gets changes-requested."Why this test will exceed the timeout on Windows
collectContinuouslytriggers a full GC on every allocation.require-esm-gc-roots.test.tsdocuments >60s on Windows x64-baseline for a single trivially-loading subprocess. This test's subprocess workload is heavier than any gated precedent:- 8 outer iterations × 40 concurrent
once()calls = 320fetch()requests, each materializing the full HTTP client + native body ReadableStream +JSNativeStreamSourceAdapter+ controller +pipeToop +WritableStream+ writer + readyPromise object graph. - 8 × 6 + 6 = 54 explicit
Bun.gc(true)+await Bun.sleep(1)cycles on top of the per-allocation collector. - A
net.createServerdoing threesock.writes + two nestedsetTimeouts +sock.destroyper connection.
On the non-ASAN Windows lanes this runs under a 30-second timeout. Even if the debug lane happens to be x64 (not x64-baseline) and squeaks under once, this is precisely the flake shape the harness convention was introduced to prevent.
Why Windows coverage adds nothing
The code path this PR fixes is platform-agnostic C++ (JSC
WriteBarriervisiting viaJSInternalFieldObjectImpl<5>). Every one of the precedent tests states the same rationale — "the code path is platform-agnostic" — for skipping/gating on Windows. The POSIX lanes fully exercise the fix.Step-by-step proof
- Windows x64-baseline CI runs
pipeTo-shutdown-gc.test.ts;isASANis false → timeout = 30 000 ms. Bun.spawnlaunches the subprocess withBUN_JSC_collectContinuously=1(line 96 does not gate on platform).- Under the per-allocation collector, the subprocess begins iteration 0: 40 concurrent
fetch()calls each allocate the full native-body/adapter/controller/pipeTo/writer graph — thousands of JSCell allocations, each triggering a synchronous full GC. - Per
require-esm-gc-roots.test.ts's in-tree measurement, a lighter single-subprocess workload takes >60s on this lane; this workload is strictly heavier. - Wall clock reaches 30 000 ms before the subprocess prints "OK"; the test runner fails the test on timeout.
- CI is red on a lane this diff does not otherwise touch.
Fix
Import
isWindowsfrom"harness"and either:- (a)
test.skipIf(isWindows)("abandoned pipeTo …", …)— matchesfetch-response-finalizer-sweep.test.ts, the closest structural analog; or - (b) gate the env var:
env: { ...bunEnv, ...(isWindows ? {} : { BUN_JSC_collectContinuously: "1" }) }— matchestranspiler-error-gc-uaf.test.ts/jest-each-gc-root.test.ts. The script already does 54 explicitBun.gc(true)storms, so it retains regression value on Windows without the per-allocation collector.
Either is a two-line change.
There was a problem hiding this comment.
No new findings this pass; all prior nits are addressed. Deferring to a human because this reverses a documented GC-rooting invariant (the old header called m_controller "THE one sanctioned JSC::Weak in the subsystem" specifically so an external Strong<> on the native handle could never pin the abandoned consumer graph). The new design instead relies on the Rust side's this_jsvalue only being Strong during in-flight I/O and on ClearAlgorithms breaking the reverse edge — that reasoning looks sound and native-source-onclose-leak.test.ts passes, but it's a memory-safety trade-off a maintainer should sign off on, especially since the crash itself has no fail-before reproducer.
What was reviewed:
JSInternalFieldObjectImpl<5>migration:finishCreationinitializes all 5 fields,Base::visitChildrenvisits them,needsDestructionflipped toDoesNotNeedDestruction, destructor/destroyremoved.- Semantic equivalence of the accessor rewrite:
drainValueempty→undefined sentinel swap,.getObject()/dynamicDowncastonjsUndefinedreturningnullptrmatching the oldWriteBarrier<JSObject>::get()/Weak::get()behavior at every call site. clearControllernow fires on all three named terminal paths (pullRejected, callClose, cancel) — matches the header comment.- Leak-cycle argument:
releaseLockdoes not runClearAlgorithms, so the abandoned case leans on the RustStrong<>being dropped when I/O settles; the cited leak test covers that shape.
Extended reasoning...
Overview
JSNativeStreamSourceAdapter is rebased from JSDestructibleObject + one JSC::Weak<JSReadableStreamDefaultController> onto JSInternalFieldObjectImpl<5>, with the controller now held as a visited internal field. Every m_* access site in BunStreamSource.cpp and the one in ReadableStreamOperations.cpp is mechanically rewritten to typed accessors; clearController(vm) is added to nativeSourceCallClose and nativeSourceCancel (previously only nativeSourcePullRejected cleared it). destroy/dtor/analyzeHeap/WeakInlines.h are removed. A new GC-storm regression test exercises the pipeTo-over-faulting-native-body shape under collectContinuously.
Security risks
None in the injection/auth sense. The risk surface here is memory safety: an under-rooted edge is a UAF (the bug being fixed), and an over-rooted edge is a leak (what the old Weak was guarding against). The PR trades the former for a design that must argue the latter cannot happen.
Level of scrutiny
High. This is native JSC GC-edge code — REVIEW.md's most-blocked category — and it deliberately reverses an invariant the previous author documented in the header ("a STRONG back-edge would let Rust's external Strong root on the native handle pin the entire abandoned JS consumer graph forever"). The counter-argument (cycle is intra-heap once I/O settles; ClearAlgorithms breaks the reverse edge on close/cancel/error; this_jsvalue is only Strong during FileReader I/O where pinning is correct) is coherent and backed by native-source-onclose-leak.test.ts passing, but it depends on Rust-side Strong<> discipline that a reviewer with the full NewSource/FetchTasklet picture should confirm. The base-class swap also changes the cell's IsoSubspace destruction mode, which is subtle enough to want a second pair of eyes.
Other factors
- The crash has no deterministic fail-before test (0/1800 standalone; only reproduces under fault-injected tracer replay). The added test is explicitly a regression surface, not a reproducer — acknowledged in the file comment and accepted by CodeRabbit after discussion, but it means REVIEW.md's "confirm deleting each load-bearing clause of your fix breaks at least one test" is not satisfied for the core change.
- All four prior review rounds (dead scaffolding, stale include, comment/implementation mismatch on
clearController, stalewscomment) were addressed in follow-up commits and are resolved. - CI on the previous build was green except one unrelated flaky proxy test.
- The mechanical accessor rewrite checks out: the
isEmpty→isUndefinedsentinel change fordrainValueis behavior-preserving givenfinishCreationnow seeds all fields tojsUndefined, and everyhandle()/pendingView()/controller()read site toleratesnullptrthe same way it tolerated a clearedWriteBarrier/Weak.
There was a problem hiding this comment.
No issues found in this pass, but I'd like a human to sign off on the core design change: this reverses the explicitly-documented "one sanctioned JSC::Weak" on m_controller to a visited edge, and swaps the cell's base class from JSDestructibleObject to JSInternalFieldObjectImpl<5>. The mechanical refactor and terminal-path clearing look correct, and native-source-onclose-leak.test.ts reportedly still passes, but the Weak→visited tradeoff (UAF fix vs. the original pin-the-consumer-graph concern) is a GC-rooting judgment call worth a maintainer's eyes.
What was reviewed:
- All former
m_*member-access sites migrated to the typed accessors; no stragglers in the tree. drainValuesentinel change (isEmpty()→isUndefined()) is behavior-preserving giveninitialValues()and the!drainValue.isUndefined()set-guard inmaterializeNativeSource.clearControllernow fires on all three named terminal paths (pullRejected, callClose, cancel); reverse edge cleared viaClearAlgorithmson close/error.Base::visitChildrencovers the five internal fields; destructor/destroy/WeakInlines removal is consistent.
Extended reasoning...
Overview
This PR changes JSNativeStreamSourceAdapter from a JSDestructibleObject with four WriteBarrier members plus one JSC::Weak<JSReadableStreamDefaultController> into a JSInternalFieldObjectImpl<5> where all five slots (handle, pendingView, closer, drainValue, controller) are visited internal fields. Every call site in BunStreamSource.cpp and the one in ReadableStreamOperations.cpp is rewritten to typed accessors; the destructor, destroy(), analyzeHeap, and the WeakInlines.h include are removed. clearController(vm) is added to nativeSourceCallClose and nativeSourceCancel (in addition to the existing nativeSourcePullRejected). A new GC-stress test exercises the fetch-body → pipeTo → socket-fault → AbortDestination shutdown shape under collectContinuously.
Security risks
None. This is internal JSC cell layout and GC-edge bookkeeping; no user-facing API surface, no parsing of untrusted input, no auth/crypto.
Level of scrutiny
High. This is C++ JSC GC integration in the WebStreams native-source path — REVIEW.md's most-blocked category. The original header comment for m_controller said it was "THE ONE SANCTIONED JSC::Weak in the whole subsystem" and that "a STRONG back-edge would let Rust's external Strong root on the native handle pin the entire abandoned JS consumer graph forever." This PR deliberately reverses that decision, arguing (a) the adapter↔controller cycle is intra-heap and collectable once no external Strong<> reaches it, (b) readableStreamDefaultControllerClearAlgorithms breaks the reverse edge on close/cancel/error, and (c) NewSource::this_jsvalue is only Strong during FileReader I/O where pinning is correct. That reasoning is coherent and native-source-onclose-leak.test.ts reportedly passes, but per REVIEW.md ("Don't add Strong refs … you can't justify — and don't silently delete existing ones"), overturning a documented load-bearing Weak in favor of a visited edge is exactly the kind of GC-rooting tradeoff a maintainer familiar with the FetchTasklet/NewSource lifetime should confirm.
Other factors
- The mechanical refactor is clean: I grepped for any remaining
adapter->m_handle/m_pendingView/m_closer/m_drainValue/m_controlleracrosssrc/jsc/bindings/webcore/streams/— none remain on this class. The.getObject()accessors returnnullptrforjsUndefined(), matching the old cleared-WriteBarrier<JSObject>semantics;drainValue()'sisUndefined()check matches becauseinitialValues()seeds all five slots tojsUndefined()andsetDrainValueis only called when the value is not undefined. - All prior inline findings from earlier review rounds (dead
connId/once(i)scaffolding, staleWeakInlines.hinclude, single-call-siteclearControllervs. "every terminal path" comment, stalewsreference) have been addressed in follow-up commits and the threads are resolved. - The new test is acknowledged as a regression surface rather than a fail-before reproducer (the crash needs a fault-injected tracer replay to hit ~1/3; standalone probes went 0/1800). CodeRabbit raised and then withdrew an objection on this point. That's a reasonable compromise for an instrumentation-gated GC window, but it does mean the fix's correctness rests on the PR body's mechanism analysis rather than a test that fails-before/passes-after.
- CI on 71b9383 was green except one
[flaky]-tagged proxy test unrelated to this diff.
2 participants
JSNativeStreamSourceAdapter::m_controllerwas aJSC::Weak<JSReadableStreamDefaultController>. When the native pull promise is rejected (socket fault on a fetch body) the adapter is queued as theonNativePullRejectedreaction context, which roots the adapter but not the controller: the adapter's only edge to it was theWeak.FetchTaskletreleases both nativeStrong<>s to the body stream before that microtask drains, so a GC in between can leave the entire consumer graph (controller -> stream -> reader -> pipe op -> destination -> writer -> readyPromise) white. The subsequent error cascade then enqueues the pipe's writes-drained shutdown deferral against a corpseop, andperformPipeShutdownAction(AbortDestination)dereferences a sweptreadyPromise:On builds without the assert the same path is a silent write into freed/reused promise memory.
Fix
Hold
m_controlleras a visited internal field so a queued adapter roots the controller directly. The edge is cleared on every terminal path (nativeSourcePullRejected,nativeSourceCallClose,nativeSourceCancel);controller->algorithmContextis cleared byreadableStreamDefaultControllerClearAlgorithms, so the abandoned case is an ordinary intra-heap cycle mark-sweep collects.NewSource::this_jsvalueis onlyStrongduring FileReader I/O, where pinning the consumer graph is the correct behavior anyway.With the
Weakgone the adapter no longer needs a destructor, so it is now aJSInternalFieldObjectImpl<5>: the five JSValue members (handle, pendingView, closer, drainValue, controller) are internal fields visited by the base class, with typed accessors at call sites. The scalar members (chunkSize, flag bitfield, text-decode state) stay as plain members.Verification
native-source-onclose-leak.test.ts(the partial-read +releaseLockabandonment tests for Blob/fetch/File sources) continues to pass, confirming the cycle does not pin.streams.test.js,pipeTo-signal-leak.test.ts,compression.test.ts,blob.test.tsall pass.The crash itself is 0/1800 standalone; it reproduces ~1/3 only under a fault-injected tracer replay.
pipeTo-shutdown-gc.test.tsexercises the shape (native body source, socket fault mid-stream, fire-and-forgetpipeToundercollectContinuously,AbortDestinationshutdown arm) as a regression surface.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/streams/pipeTo-shutdown-gc.test.ts