MessagePort: release an explicit ref() when the last 'message' listener is removed - #38009
MessagePort: release an explicit ref() when the last 'message' listener is removed#38009robobun wants to merge 5 commits into
Conversation
…er is removed
Node's setupPortReferencing unref()s a port whenever its last 'message'
listener goes away, regardless of an earlier port.ref(). Bun kept the
explicit ref (m_hasRef) in a separate slot that only unref()/close()/a
peer close released, so `port.ref(); port.on("message", f);
port.off("message", f)` left hasRef() true and the process (or worker
thread) alive.
Release it from onDidChangeListenerImpl when the 'message' listener count
drops to zero. That covers off()/removeListener(), removeEventListener(),
removeAllListeners(), once() listeners firing, AbortSignal removal and
onmessage = null, for MessageChannel ports and parentPort alike.
The onmessage setter no longer calls jsUnref() for a non-callable value:
the removal itself now decides, so clearing a handler that was never set,
or while other 'message' listeners remain, no longer unrefs a port that
is still listening (node leaves it alone in both cases).
The three copies of the m_hasRef release (close(), disentangle(),
jsUnref()) become releaseJsRef(); jsUnref() loses its now unused
JSGlobalObject parameter.
|
Warning Review limit reached
Next review available in: 9 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 (6)
Comment |
|
Status: fixed, review feedback folded in, waiting on CI for 10b6fde.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. Because it changes when deref() runs on the native MessagePort (now reachable from the onDidChangeListener hook, whose call sites in EventTarget touch this afterwards) and reroutes the event-loop unref through ScriptExecutionContext, a human look would still be worthwhile.
What was checked:
ScriptExecutionContext::unrefEventLoop()isBun__eventLoop__refKeepAlive(m_bunVM, -1), soreleaseJsRef()releases the same counter the oldjsUnref(global)did.- Every path that reaches
releaseJsRef()via the listener hook holds an independent ref (innerInvokeEventListeners'protectedThis, the abort-signalRef{*weakThis},disentanglePorts'RefPtr, the JS wrapper for directremoveEventListener), so thederef()of the self-ref is never the last one beforeeventListenersDidChange()runs. disentangle()now hitsreleaseJsRef()twice (once viaremoveAllEventListeners()→Clear, once directly); the second is a guarded no-op.
Extended reasoning...
Overview
The PR makes MessagePort::onDidChangeListenerImpl release the explicit m_hasRef self-ref + event-loop ref when the last 'message' listener is removed, matching Node's setupPortReferencing behavior. It also stops the onmessage setter from calling jsUnref() on non-callable assignment (the removal hook now handles it correctly), and consolidates three inline copies of the m_hasRef release into a new releaseJsRef() helper. jsUnref() loses its JSGlobalObject* parameter since the unref now goes through scriptExecutionContext()->unrefEventLoop(). Five new tests cover the hasRef() matrix, onmessage = null cases, and end-to-end process/worker exit.
Security risks
None. This is event-loop-keepalive bookkeeping for MessagePort; no untrusted input parsing, auth, or data exposure is involved.
Level of scrutiny
High. This is native C++ that changes when deref() is called on a ThreadSafeRefCounted object, reachable from a hook (onDidChangeListener) whose EventTarget call sites (removeEventListener, the Clear loop in removeAllEventListeners) touch this after the hook returns. The safety argument — that m_hasRef == true implies a self-ref was taken by jsRef(), so releasing it can never be the last ref while any caller holds one — is sound, and I verified each hook caller holds an independent ref (innerInvokeEventListeners takes Ref<EventTarget> protectedThis, the abort algorithm wraps in Ref{*weakThis}, disentanglePorts holds RefPtr<MessagePort>, the close task uses queueTaskKeepingObjectAlive, and JS-initiated removals hold via the wrapper). But ref-counting changes in this file have historically been subtle (per the extensive existing comments about GC-thread reads and m_hasRef release ordering), and a mistake here is a UAF or a process hang.
The event-loop-unref rerouting is behavior-preserving: I confirmed ScriptExecutionContext::unrefEventLoop() is exactly Bun__eventLoop__refKeepAlive(m_bunVM, -1), matching what the old jsUnref(global) did via clientData(vm)->bunVM — same per-thread VM.
Other factors
The PR intentionally diverges from Node on removeAllListeners() (Node bypasses its hooks and leaves the port ref'd; Bun releases both refs), documented and pinned by a test. The unref(); on() divergence is pre-existing (#36434). The test coverage is thorough — matrix over all removal paths plus negative cases, spawned-process and worker exit tests — and the PR description includes a full Node-vs-main-vs-branch comparison table. No prior human review comments to address. Given the memory-safety-adjacent nature and the intentional Node divergences, a maintainer familiar with the MessagePort ref model should confirm the design choice.
…t close() during context destruction makePortReadable dropped its 'message' listener as soon as the EOF message arrived. Now that removing the last listener also releases the port's read-time ref(), a worker whose stdin consumer was paused exited before the buffered data and 'end' were delivered. The 'close' handler already removes the listener once the stream has drained, so the EOF path only pushes null. contextDestroyed() is the one close() caller without an outside reference: a context torn down without a stop phase (a collected ShadowRealm global, a retired test-isolation global) closes a port that may be alive only through the self-ref close() releases, and close() kept using the freed port. Hold a reference across the teardown, and assert in releaseJsRef() that the self-ref is never the last one. Tests: a paused-stdin delivery test, the ShadowRealm teardown under ASAN with Malloc=1, and the non-callable onmessage values in the hasRef matrix.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes native MessagePort ref-counting — adding a deref() reachable from the listener-change hook and reworking when m_hasRef is released across close/disentangle/context-destroy — a human look at the lifetime reasoning would still be worthwhile.
What was reviewed:
- The new
releaseJsRef()path fromonDidChangeListenerImpland whether every hook caller holds its own ref across thederef()— the callers enumerated in the description (EventTarget removal, once-fire, abort algorithm, close task, disentangle) each do. - The
onmessage = nullsetter change: removingjsUnref()there is safe becausesetEventHandlerAttribute→setAttributeEventListenerfunnels through the same hook when a handler was actually removed. - The stdio readable in
worker_threads.tsdeferringport.off("message", ...)from EOF to 'close' — checked that 'close' fires on both natural end and destroy, so the listener is not leaked. - The
contextDestroyed()protectedThisaddition — needed now thatclose()can drop the last (self-)ref mid-teardown; the new ASAN test covers it.
Extended reasoning...
Overview
This PR aligns Bun's MessagePort ref semantics with Node's setupPortReferencing: when the last 'message' listener is removed, an explicit .ref() (or onmessage=) keepalive is released too. The mechanism is a new releaseJsRef() helper (consolidating three existing copies in close(), disentangle(), and jsUnref()) that is now also invoked from onDidChangeListenerImpl on the >0→0 transition. jsUnref() drops its JSGlobalObject* parameter and unrefs via scriptExecutionContext() (same per-thread VM). The onmessage setter no longer calls jsUnref() for non-callable values (the removal hook handles it). contextDestroyed() gains a Ref protectedThis. On the JS side, the stdio readable keeps its port listener until stream 'close' rather than dropping it at EOF, so buffered data still keeps the thread alive. Six new tests cover the hasRef() matrix, process/worker exit, and the ASAN context-destroy case.
Security risks
None. This is event-loop ref bookkeeping and native object lifetime; no untrusted-input parsing, auth, or boundary crossings.
Level of scrutiny
High. This is native C++ ref-counting on a ThreadSafeRefCounted object where a deref() is now reachable from a new call site (the EventTarget listener hook). Per the repo's review guidance, ref/deref balance and "can this be the last reference" reasoning across every terminal path is exactly what maintainers scrutinize most closely. The PR's argument that every hook caller holds its own reference is sound and is backed by an ASSERT(!hasOneRef()), but that assert is debug-only and the callers span EventTarget internals (once-listener removal inside innerInvokeEventListeners, AbortSignal removal, removeAllEventListeners from disentangle() and the close task). A maintainer should confirm they agree with that enumeration.
Other factors
- CI on the earlier commit (82e4d92) reported a Windows-x64 segfault in
test/bake/deinitialization.test.ts; the latest commit (b791a81) added theprotectedThisincontextDestroyed(). Whether that failure was related and is now resolved should be confirmed on the current build. - The automated
comment-copbot has flagged several of the new/edited comments as too long; those are outstanding on the thread. - Test coverage is thorough (removal-path matrix, negative cases, spawned process + worker exit, ASAN UAF repro), and the description documents verification against the upstream Node test suite. The change is well-motivated and well-argued — it's the native lifetime aspect, not the correctness of the Node semantics, that warrants a human pass.
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. Because it changes native MessagePort refcounting — adding a new deref() site reachable from the listener-change hook and relying on a caller-holds-a-ref invariant across several paths (bindings, peerClosed, disentangle, the close task, contextDestroyed) — a maintainer should confirm the lifetime reasoning before this lands.
What was checked:
releaseJsRef()'scontext->unrefEventLoop()is the sameBun__eventLoop__refKeepAlive(..., -1)the oldjsUnref(JSGlobalObject*)used, so the event-loop bookkeeping is unchanged.disentangle()now releases via theClearhook insideremoveAllEventListeners()one line earlier than before;disentanglePorts()'sRefPtrstill covers it, and the follow-upreleaseJsRef()handles the no-listenerref()-only case.- The
makePortReadablechange only defers listener removal to the existing'close'handler (which alreadyoff()s andunref()s), soendFromOwner()and destroy paths still clean up.
Extended reasoning...
Overview
The PR fixes a Node-compat bug where port.ref(); port.on('message', f); port.off('message', f) leaves the port ref'd (Node unrefs). It touches MessagePort.{cpp,h} (the listener-change hook now calls a new releaseJsRef() when the last 'message' listener is removed; three copies of the m_hasRef release are consolidated into that helper; contextDestroyed() gains a Ref protectedThis), JSMessagePort.cpp (the onmessage setter no longer calls jsUnref() for non-callable values; jsUnref() loses its JSGlobalObject* parameter), worker_threads.ts (makePortReadable keeps its listener until the stream closes rather than at EOF), plus six new tests and one new ASAN-gated UAF regression test.
Security risks
None in the traditional sense. The risk here is memory safety: releaseJsRef() calls deref() on this, and the new hook call site means a listener removal can now drop the port's self-reference. The PR asserts !hasOneRef() before the deref() and enumerates the reference each caller holds, and it fixes a pre-existing UAF in contextDestroyed() (reproduced under ASAN with a ShadowRealm). But this is exactly the class of change REVIEW.md calls out as most-blocked: a new early release on a refcounted object, whose safety depends on every reachable caller holding an independent reference.
Level of scrutiny
High. This is native lifecycle code on a ThreadSafeRefCounted object with a self-reference, reachable from user JS through off()/removeEventListener()/removeAllListeners()/once() firing/AbortSignal/onmessage= — i.e., user code can synchronously trigger the new deref(). The ASSERT compiles out in release, so a missed caller would be a UAF in production rather than a crash. The PR description's caller enumeration is thorough and I spot-checked disentangle() and the deferred-close task, but a maintainer familiar with EventTarget's ref discipline (particularly innerInvokeEventListeners and the abort-algorithm path) should confirm.
Other factors
- Test coverage is strong: a full
hasRef()matrix, process- and worker-exit end-to-end tests, a paused-stdin regression for themakePortReadablechange, and an ASAN-onlyShadowRealmteardown test for the UAF. The PR verified the new tests fail on main and that 40 upstream Node tests still pass. - The change intentionally diverges from Node on two edge cases (
removeAllListeners()andonmessage = {}transitions), documented in the description's table; these are pinned by tests but are the kind of compat decision a maintainer should sign off on. - The comment-cop bot threads are all resolved (comments were shortened in 99cf238/f2498e9).
- No prior human review; CI on the latest commit is still building.
1 participant
Problem
port.ref(); port.on("message", f); port.off("message", f)leavesport.hasRef()true and the process alive. Node printsfalseand exits. The same happens for a worker doing this onparentPort(the thread never exits), and forremoveEventListener(),removeAllListeners()and aonce()listener firing.m_hasRef, taken byjsRef()(.ref()or a callableonmessage =), and the listener ref, held whilem_isRefd && m_messageEventCount > 0.onDidChangeListenerImpl()(MessagePort.cpp) only reconciled the second one, so an explicit ref outlived the listeners. Node has one handle ref flag, andsetupPortReferencing(lib/internal/worker/io.js) callsport.unref()unconditionally when the last'message'listener is removed.onmessagesetter (JSMessagePort.cpp) calledjsUnref()for every non-callable assignment as its own approximation of that release.port.on("message", f); port.onmessage = nulltherefore unref'd a port that is still listening, andport.ref(); port.onmessage = nulldropped the explicit ref. Node changes nothing in either case.MessagePort::contextDestroyed()->close()releasedm_hasRef(a self-reference) and kept using the port. When the context dies without a stop phase (a collectedShadowRealmglobal, a retired test-isolation global) and the port's wrapper is already gone, that self-reference is the last one, andclose()reads freed memory (heap-use-after-freeinMessagePort::close()fromMessagePort::contextDestroyed()<-~ScriptExecutionContext<-Zig::GlobalObject::~GlobalObject, reproduced on main under ASAN withMalloc=1).Fix
onDidChangeListenerImpl()releasesm_hasRefwhen the'message'listener count goes from non-zero to zero (RemoveorClear). Every removal path funnels through this hook:off()/removeListener(),removeEventListener(),removeAllListeners()(one removal per listener),once()listeners being removed before they fire,AbortSignalremoval, andonmessage = <non-object>viasetAttributeEventListener.parentPortis the same class, so workers are covered.m_isRefdis left alone on that transition: with no listeners it has no effect, and keeping it meanson(); off(); on()refs the port again, as in node.onmessagesetter keepsjsRef()for a callable value and no longer callsjsUnref()otherwise; the removal itself (if there was one, and it was the last listener) now releases, which is node's rule. A non-callable object is installed as the handler by the attribute setter (and returned by the getter), so it counts as a listener and keeps the port ref'd until it is cleared; node agrees foronmessage = {}on an empty port and differs only on replacing a function with an object (it unrefs there) and on clearing an object (it stays ref'd there), because it only counts functions. Pinned by the test either way.m_hasRefrelease inclose(),disentangle()andjsUnref()becomereleaseJsRef(), which the hook also uses. It unrefs through the port'sScriptExecutionContextasclose()already did (the same per-thread VMjsRef()ref'd), sojsUnref()no longer needs aJSGlobalObjectand loses the parameter. Every caller keeps using the port after the release, so the self-reference must never be the last one;releaseJsRef()now asserts that (ASSERT(!hasOneRef())), and the callers hold one: the JS wrapper for the bindings,protectedThisinpeerClosed(),disentanglePorts()'sRefPtr,forEachActiveDOMObject()'sRefPtrforstop(), and for the hookEventTarget::removeEventListener(wrapper or theReftaken byinnerInvokeEventListeners/ the abort algorithm), the close task (queueTaskKeepingObjectAlive) anddisentangle().contextDestroyed()was the one caller without such a reference; it now holds one acrossclose()andActiveDOMObject::contextDestroyed()(the wrapper-less port is then destroyed when that reference goes away, after its context pointer has been cleared, which is the order~ActiveDOMObjectexpects).makePortReadable(a worker'sprocess.stdin, and capturedworker.stdout/stderr) dropped its port listener as soon as the EOF message arrived. With the release above that also dropped theref()it takes when reading starts, so a worker whose stdin consumer was paused exited before the buffered data and'end'were delivered (node and main deliver them). The EOF branch now only pushesnull; the stream's'close'handler already removes the listener and unrefs once the data has been consumed, which is node'skWaitingStreamsbehavior. The other internalref()users are unaffected: theWorkerpublic port and the messaging hub port never remove their listener, and the stdio writable unrefs before it drops its ack listener.setupPortReferencingin v26.3.0:removeListener(size) { if (size === 0) { stopMessagePort(port); port.unref(); } }, called with the count left after an actual removal), and it leaves no way for an explicit ref to outlive the listeners except an explicitref()taken afterwards. The stdio change keeps the one internal user that relied on the old timing at its previous (and node's) semantics, and thecontextDestroyed()reference makes the lifetime rule the new hook relies on hold for every caller.hasRef()matrix over the removal paths and the cases that must not release, theremoveAllListeners()cases, theonmessagecases (null, string, object, nothing set, other listeners remaining), a spawned process and a worker that must exit on their own, and the paused-stdin delivery test. The first five fail on main (release and debug+ASAN builds); the stdin test passes on main, fails with the native change alone, and passes with themakePortReadablechange.ShadowRealmteardown under ASAN withMalloc=1(it collects until the realm's global is gone, checked throughheapStats(), so the teardown path is known to have run). Against main'sMessagePortfiles it fails on itsstderrassertion with the ASAN report above; with thecontextDestroyed()change it passes, 10/10 runs.test-messagechannel,test-worker-message-port*,test-worker-message-channel*,test-worker-workerdata-messageport,test-worker-ref*,test-worker-parent-port-ref,test-worker-terminate-ref-public-port,test-worker-unref-from-message-during-exit,test-worker-onmessage*,test-worker-stdio*,test-worker-no-stdin-stdout-interaction,test-worker-terminate-unrefed,test-worker-event,test-worker-message-event,test-worker-cleanexit*,test-worker-exit-code. The new assertion did not fire anywhere in these.removeAllListeners()in node bypasses its listener hooks and leaves a listener-less port ref'd (the process hangs); bun already released the listener ref there and now releases the explicit one the same way.unref(); on("message", f)re-refs the port in node and not in bun (the add side of the same hook; MessagePort: pin the entangled peer while a side holds a loop ref #36434 addresses it).Background
ref()/unref()set it, andsetupPortReferencingsets it when the first'message'listener is added and clears it when the last one is removed.hasRef()reads it.MessagePort:m_hasRef(an event-loop ref plus a self-reference, so the native object outlives its JS wrapper while explicitly ref'd) and a listener-driven event-loop ref.hasRef()reports either being held.MessagePortis refcounted; its JS wrapper holds one reference,m_hasRefholds one, and code paths that run on a port take a temporary one (Ref protectedThis) when they may drop one of those. Dropping the last reference destroys the object immediately, so a method that drops a reference and continues needs the caller to hold another.onDidChangeListeneris a hookEventTargetinvokes after a listener was actually added (Add), actually removed (Remove), or the whole list was cleared (Clear); duplicates and removals of absent listeners do not invoke it.MessagePortuses it to count'message'listeners.ScriptExecutionContextis the native per-global object that ports, channels and workers register on. Normal teardown (process exit, worker exit) runs a stop phase over those objects before the context goes away; a global collected on a live VM (ShadowRealm, a test-isolation global) skips that and only notifies them from the context's destructor (contextDestroyed()).on()/off()/removeAllListeners()on a port are a shim in src/js/node/worker_threads.ts over the nativeaddEventListener/removeEventListener, which is why one native hook covers the node-style and web-style APIs. The worker stdio streams in the same file areReadable/Writableobjects fed by internal MessagePorts; the readable refs its port while it has a consumer so unconsumed data keeps the thread alive.hasRef() / exit probe: node v26.3.0, bun 1.4.0 (main), this branch
Each row is a fresh
MessageChannelwith the peer kept reachable;f/gare no-op functions. "exits" means the process exited on its own; "alive" means it was still running 400ms later.Worker stdin probe (parent writes two chunks and ends; the worker pauses in the first
'data'handler and resumes from an unref'd timer once EOF has been received): node and main report both chunks and'end'and exit 0; the native change alone made the worker exit 0 after the first chunk; with themakePortReadablechange it matches node and main again.Earlier revision of this description
The first revision of this PR contained only the hook release, the setter change and the
releaseJsRef()consolidation. Review of that revision turned up the stdio readable's dependence on the old release timing, the unprotectedcontextDestroyed()path, and the unpinned non-callable-objectonmessagebehavior; the current revision adds the fixes and tests for all three. Its internal-caller analysis claimed the stdio readable was "done with the port" when it dropped its listener at EOF, which was wrong for a consumer that still has buffered data; that is themakePortReadablebullet above.