Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughChangesThe zlib, Brotli, and Zstd native wrappers now use Zlib JsRef lifecycle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:43 PM PT - Jul 19th, 2026
❌ @Jarred-Sumner, your commit 60625ef has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34671That installs a local version of the PR into your bun-34671 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Review feedback addressed in 7798d32:
This conversion is behavior-preserving: the previous CI on 7798d32: the diff is green on every lane. The two failures are unrelated to this change ( |
NativeZlib/NativeBrotli/NativeZstd each keep a back-reference to their own JS wrapper so the completion callback on run_from_js_thread can reach the cached writeResult/writeCallback/pendingInput/pendingOutput values after a work-pool write. That slot was a bare StrongOptional, which is the type the repo flags as a self-ref hazard for JsClass payloads; the canonical type for a wrapper back-ref that is strong only while pending work exists is JsRef (see JSRef.rs, timer_object_internals.rs, Terminal.rs, cron.rs). This converts the shared this_value slot to JsRef across the CompressionStream mixin trait and all three implementations. The lifecycle is preserved exactly: write() upgrades the ref to strong before scheduling the work-pool task, run_from_js_thread() takes the value and drops the strong ref before invoking callbacks, close() resets it, and finalize() marks it terminal. The old StrongOptional already cleared its HandleSlot in try_swap(), so there was no steady-state wrapper leak; the JsRef spelling is the correct one for a self-ref and removes the TODO in NativeBrotli.rs. Adds a subprocess test that drives the native handle directly (bypassing the Transform wrapper) to force a full GC while an async zlib/brotli/zstd write is in flight and then to verify the wrapper count stays bounded across batches. It is a regression guard for both halves of the lifecycle and also passes on the prior build.
- Move the handle into its own frame and return a WeakRef so the forced GC runs
after the allocating frame has unwound; the previous shape left `h` in a
live bytecode register at the GC call, making the rooting assertion vacuous.
- Give each concurrent write in batch() its own output buffer so worker threads
do not race on the same 512-byte allocation.
- Use a combined {stdout, stderr, exitCode} assertion.
23abfbf to
7798d32
Compare
There was a problem hiding this comment.
Both earlier findings (shared out buffer race in batch(), stderr assertion form) are addressed in the current diff, and no new issues turned up this pass. The StrongOptional → JsRef conversion maps 1:1 (set → set_strong, try_swap → try_get + reset, deinit → empty()) and matches the pattern in Terminal.rs / timer_object_internals.rs. That said, this is GC-rooting code that keeps the wrapper and its pinned buffers alive across a work-pool thread hop, so it's worth a human look before landing.
Extended reasoning...
Overview
This PR converts the this_value back-reference field on NativeZlib / NativeBrotli / NativeZstd from StrongOptional to JsRef, the repo's canonical type for a JsClass payload's back-reference to its own JS wrapper. The change touches the CompressionStream mixin trait in node_zlib_binding.rs (the trait accessor signature, write(), run_from_js_thread(), close_internal(), finalize(), and the __impl_compression_stream! macro) plus the three struct definitions and constructors. It also adds a subprocess GC-stress test to zlib.test.js.
Security risks
No new attack surface. The change is a type substitution in an internal GC-rooting slot. The risk class is memory safety (UAF on the JS wrapper or its cached pendingInput/pendingOutput array buffers if the strong root were lost mid-write), not injection/auth/data exposure.
Level of scrutiny
High. This is native GC-lifecycle code on a hot path (node:zlib async writes) that hands raw buffer pointers to a worker thread and relies on the this_value root to keep the wrapper (and via its cached WriteBarrier slots, the pinned input/output buffers) alive across the thread hop. The conversion itself is mechanical and mirrors src/jsc/JSRef.rs's documented pattern and existing users (Terminal.rs:1904, timer_object_internals.rs, cron.rs), and the removed TODO in NativeBrotli.rs explicitly called for this exact change. I verified the API mapping against JSRef.rs: set_strong(value, global) argument order is correct, try_get() + *v = JsRef::empty() replicates try_swap()'s take-and-clear semantics, and finalize() only ever runs when the slot is already Weak(UNDEFINED) (a live Strong self-ref would prevent the wrapper from being collected in the first place), so the new terminal-state marking is defensive rather than load-bearing.
Other factors
The two issues I flagged on the prior revision are both fixed in the current diff (per-iteration out buffer in batch(); combined {stdout, stderr, exitCode} assertion), and both inline threads are marked resolved. The PR body honestly notes the new test also passes on main since the change is behavior-preserving, so the test guards against regression rather than proving the fix. Given it's GC-rooting code across a thread boundary — the exact category REVIEW.md's memory-safety section calls out — I'm deferring rather than approving so a maintainer familiar with the JsRef migration can confirm.
There was a problem hiding this comment.
LGTM — the two test issues from the earlier pass (shared out buffer across concurrent work-pool writes, and the stderr assertion form) are both addressed, and the StrongOptional → JsRef conversion follows the documented back-ref pattern used by timers/Terminal/cron.
What was reviewed:
set_strong/try_get+*v = JsRef::empty()/finalize()semantics againstsrc/jsc/JSRef.rs— behavior matches the previousStrongOptional::set/try_swap/deinitlifecycle.JsCell::set/with_mutdrop the oldJsRefvariant, so the StrongHandleSlotis released on every reset path (close, run_from_js_thread, deinit).- Ruled out:
weak.deref()incheckRootingbeing vacuous under WeakRef [[KeptAlive]] — theschedule()frame has returned before the check, and the load-bearing assertion isawait promise+ output length anyway.
Extended reasoning...
Overview
This PR converts the this_value back-reference on NativeZlib / NativeBrotli / NativeZstd from StrongOptional to JsRef, the repo's canonical type for a JsClass payload holding a reference to its own JS wrapper that is strong only while pending work exists. The change touches the shared CompressionStream mixin in node_zlib_binding.rs and the three field declarations/initializers, plus a new subprocess-based lifecycle test in zlib.test.js. It removes the pre-existing TODO in NativeBrotli.rs that flagged exactly this hazard.
Security risks
None. This is an internal GC-rooting mechanism change with no user-facing surface, no parsing of untrusted input, and no auth/crypto/permission code.
Level of scrutiny
Medium-high — GC lifecycle in native code is the most-blocked category per REVIEW.md, so I traced each of the four state transitions against src/jsc/JSRef.rs:
write():set_strong(this_value, global)creates/reuses aStronghandle rooting the wrapper across the work-pool hop — equivalent to the oldStrongOptional::set.run_from_js_thread():try_get()reads the JSValue from theStrongvariant, then*v = JsRef::empty()drops it (releasing theHandleSlotviaStrong::Drop), thenensure_still_alivekeeps the value on the native stack — equivalent totry_swap().close():*v = JsRef::empty()drops any held Strong — equivalent todeinit().finalize(): newv.finalize()sets the terminalFinalizedstate beforeT::deref. When finalize runs the slot is empty (a live Strong would have rooted the wrapper and prevented finalize), so this is defensive and matches the pattern intimer_object_internals.rs,Terminal.rs,cron.rs.
I also confirmed JsCell::set (used in NativeBrotli::deinit) does *slot = value, dropping the previous JsRef and releasing any Strong it held.
Other factors
Both of my earlier findings are resolved: batch() now allocates a per-iteration output buffer so concurrently scheduled deflates no longer race on shared bytes, and the subprocess assertion uses the combined {stdout, stderr, exitCode} form. The verifier ruled out the WeakRef [[KeptAlive]] concern on checkRooting — the constructing frame unwinds before Bun.gc(true), and the substantive assertion is that the write completes with output. robobun reports the diff green on every lane with only pre-existing/unrelated flakes. This is a mechanical, behavior-preserving type conversion to the documented pattern, with a lifecycle regression guard covering all three implementations.
This branch has not been deployed
2 participants
What this does
NativeZlib/NativeBrotli/NativeZstdeach keep a back-reference to their own JS wrapper so the completion path inrun_from_js_threadcan reach the cachedwriteResult/writeCallback/pendingInput/pendingOutputvalues after a work-pool write. That slot was a bareStrongOptional, which is the type the repo flags as a self-reference hazard forJsClasspayloads (a Strong held by the payload to its own wrapper is a GC cycle). The canonical type for a wrapper back-ref that is strong only while pending work exists isJsRef;src/jsc/JSRef.rsdocuments the pattern and timers,Terminal, andCronalready use it. This PR makes the same mechanical conversion across theCompressionStreammixin trait and all three implementations, removing the TODO inNativeBrotli.rs.The lifecycle is preserved exactly:
write()upgrades the ref to strong (set_strong) before scheduling the work-pool task. The wrapper has nohasPendingActivityhook, so this strong ref is the sole GC root for the wrapper (and the pinned pending-buffer values it caches) across the thread hop.run_from_js_thread()reads the value and resets the slot toJsRef::empty()before invoking callbacks.close()resets the slot.finalize()now marks the slotFinalized, matching the pattern intimer_object_internals.rs/Terminal.rs/cron.rs.Why this is correct to have
This is a type-level refactor, not a leak fix. The previous
StrongOptionalalready cleared itsHandleSlotintry_swap()on every write completion (verified withheapStats().objectTypeCounts.NativeZlib: the live count stays bounded at 1-3 across thousands of dropped streams withoutclose()), so there was no steady-state wrapper leak on main.JsRefis still the right type here because these classes have afinalize: truehook:JsRef::Finalizedis the terminal state designed for exactly that, and holding a self-Strong viaStrongOptionalleaves no structural signal that the slot is a wrapper back-ref rather than an ordinary root. #31843 attempted the same conversion earlier and was closed; this is a minimal redo against current main (4 src files, no unrelated reformatting).Verification
Adds a subprocess test to
test/js/node/zlib/zlib.test.jsthat drives the native handle directly (bypassing theTransformwrapper) so it exercises only theCompressionStreamlifecycle:writeResultstorage if the in-flight strong root were lost.close(), and assertsheapStats().objectTypeCounts.NativeZlibstays bounded rather than growing by 50 per batch. Would fail if the completion path stopped clearing the strong ref.Because the conversion is behavior-preserving the new test also passes on the prior build; the full
test/js/node/zlib/suite passes on this branch (modulo the unrelatedlistenerCountSlowReferenceError on main, tracked in #34667, which this PR's test avoids by not going through the readable-stream path).[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file