MessagePort: implement NodeEventTarget on the native listener list - #35811
MessagePort: implement NodeEventTarget on the native listener list#35811robobun wants to merge 13 commits into
Conversation
MessagePort's EventEmitter surface (.on/.once/.off/.emit/listenerCount/
eventNames/removeAllListeners/set|getMaxListeners) was a JS shim grafted
onto the prototype when node:worker_threads first loaded. It kept its
own (event -> userFn -> wrapper) registry alongside WebCore's EventTarget
listener list, so:
- the surface was absent until worker_threads was imported somewhere
- .on(t, f) registered a wrapper via addEventListener, so the same
function added via both APIs ran twice, listenerCount/eventNames
didn't see addEventListener listeners, removeEventListener(f) couldn't
remove a .on(f), removeAllListeners left addEventListener listeners
live, and getEventListeners returned the wrapper not f
- emit('message', payload) built MessageEvent(event, payload) with the
payload in the init-dict slot, so listeners got null, a primitive
payload threw, and emit returned this instead of a boolean
Node's model (lib/internal/event_target.js): MessagePort IS a
NodeEventTarget. .on/.addListener is addEventListener with
kIsNodeStyleListener storing f itself in the one listener list; once is
the same + once:true; listenerCount/eventNames/removeAllListeners read
or clear that map; emit(type, arg) = had = listenerCount > 0, hybrid
dispatch (node-style listeners get arg by identity, EventTarget-style
get a lazily-built MessageEvent/CustomEvent), returns had.
This change:
- adds an isNodeStyleListener bit to RegisteredEventListener /
AddEventListenerOptions, keyed off a private kIsNodeStyleListener
symbol so only internal code can set it
- in innerInvokeEventListeners, flagged listeners are invoked via
JSEventListener::handleEventNodeStyle, which passes event.data /
.detail / .error instead of the Event wrapper
- puts the NodeEventTarget methods natively on an intermediate prototype
between MessagePort.prototype and EventTarget.prototype (so
Object.getOwnPropertyNames(MessagePort.prototype) matches node) and
implements them over the one native listener list, including
removeAllEventListenersForType
- emit() builds a MessageEvent{data:arg} or CustomEvent{detail:arg} and
dispatches it; node-style listeners recover arg by identity
- wires CustomEventInterfaceType into EventFactory so a natively-created
CustomEvent is wrapped as JSCustomEvent (its .detail was unreachable)
- deletes the ~180-line injectFakeEmitter shim; parentPort's stand-in
forwards on/once/off to self.addEventListener with the private flag
BroadcastChannel is a plain EventTarget in node too; it keeps no shim
(unchanged).
WalkthroughChangesAdds Node-compatible MessagePort Node event support
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:15 AM PT - Jul 26th, 2026
❌ @robobun, your commit a367520 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35811That installs a local version of the PR into your bun-35811 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
The comment-cop threads are resolved: trimmed the multi-line comments in c53eb03. The remaining two-line blocks it still flags are doc comments that match the existing neighbor pattern (e.g. Also: the duplicate-PR bot is right that #35799 and #35796 cover subsets of this. Both are from parallel sessions on the same brief; this one is the full single-list design (native |
![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/js/node/worker_threads.ts:653-661—fakeParentPort()buildsparentPortasObject.create(MessagePort.prototype)(not a realJSMessagePort), and only shadowson/once/off/addListener/removeListener— solistenerCount/eventNames/removeAllListeners/getMaxListeners/setMaxListenersfall through to the new native prototype methods, whichcastThisValue<JSMessagePort>and throwTypeErroron this receiver. Before this PR the JS-shim versions of those methods worked on the fake object (e.g.parentPort.on('message', fn); parentPort.listenerCount('message') === 1), so this is a regression inside every worker thread. Add own-property forwarders for these methods onfake(readingself's listener list) alongside the existingon/off/onceshadows.Extended reasoning...
What the bug is
Inside a worker thread,
parentPortfromnode:worker_threadsis not a realMessagePort— it is a stand-in built byfakeParentPort()at src/js/node/worker_threads.ts:565 asconst fake = Object.create(MessagePort.prototype). That produces a plainJSFinalObjectwhose prototype chain isMessagePort.prototype → <intermediate NodeEventTarget prototype> → EventTarget.prototype, but which has noJSMessagePortwrapper cell backing it.This PR moves the NodeEventTarget surface (
on,once,off,addListener,removeListener,emit,listenerCount,eventNames,removeAllListeners,setMaxListeners,getMaxListeners) from the deleted JSinjectFakeEmittershim onto a native intermediate prototype (JSMessagePortNodeEventTargetTableValuesin JSMessagePort.cpp). Each of those native host functions is dispatched viaIDLOperation<JSMessagePort>::call<...>, which callscastThisValue<JSMessagePort>(callFrame.thisValue())and, when the receiver is not aJSMessagePort, executesthrowThisTypeError(lexicalGlobalObject, throwScope, "MessagePort", operationName)(JSDOMOperation.h:45,56).fakeParentPort()was updated in this PR to add own-property forwarders foron/addListener/once/off/removeListener(worker_threads.ts:653-661), forwarding toself.addEventListener/self.removeEventListenerwith the$kIsNodeStyleListenerflag. But it does not shadowlistenerCount,eventNames,removeAllListeners,getMaxListeners,setMaxListeners, oremit. Those fall through to the native intermediate-prototype methods and now throw aTypeErroron the fake receiver.Why this is a regression
Before this PR,
injectFakeEmitterinstalled plain JS functions on the intermediate prototype. Those functions read a per-thissymbol-keyedSafeMapregistry (this[kListenerRegistry]) and worked on any receiver — includingfake:- The old
on()calledregister(this, ...)which wrote toregistryFor(fake, true)and calledfake.addEventListener(...)(bound toself), so listeners were registered on the global scope and counted infake's registry. - The old
listenerCount(type)returnedregistryFor(this, false)?.get(type)?.size ?? 0→ readfake's registry, returned the correct count. - The old
eventNames()iteratedfake's registry keys. - The old
removeAllListeners()iterated the registry and calledthis.removeEventListener(t, w)— which onfakeis the own-propertyself.removeEventListener.bind(self), so it actually removed the listeners from the global scope. - The old
getMaxListeners()/setMaxListeners()read/wrotethis[kMaxListeners]— worked on any object.
All of those now throw. (
emitwas already broken pre-PR because the old JSemitcalledthis.dispatchEvent(...), which is inherited fromEventTarget.prototypeand also required a real EventTarget receiver — soemitalone is not a regression here.)Step-by-step proof
Inside a worker:
const { parentPort } = require('node:worker_threads')→parentPort = fakeParentPort()→fake = Object.create(MessagePort.prototype), aJSFinalObject.parentPort.on('message', fn)→ hits the own-propertyonshadow at worker_threads.ts:641 →self.addEventListener('message', fn, {$kIsNodeStyleListener: true}). Works.parentPort.listenerCount('message')→ no own property; prototype chain lookup reaches the nativelistenerCounton the intermediate prototype →jsMessagePortPrototypeFunction_listenerCount→IDLOperation<JSMessagePort>::call→castThisValue<JSMessagePort>(fake)returnsnullptr(fake is aJSFinalObject, not aJSMessagePort) →throwThisTypeError(..., "MessagePort", "listenerCount").- Before this PR, step 3 would have reached the JS
listenerCountfrominjectFakeEmitter, which returnedregistryFor(fake, false)?.get('message')?.size→1.
Same for
parentPort.eventNames(),parentPort.removeAllListeners(),parentPort.getMaxListeners(),parentPort.setMaxListeners(n).Impact
parentPortis the primary communication object in everynode:worker_threadsworker.listenerCount,eventNames,removeAllListeners, andget/setMaxListenersare documented Node.jsNodeEventTargetmethods that user code and libraries call on it. Turning them from working calls into hardTypeErrors is a user-visible regression that will break worker code at runtime.Fix
Add own-property forwarders on
fakefor the remaining NodeEventTarget methods, reading/writingself's listener list (which is whereon/offnow register). For example:function listenerCount(type) { return getEventListeners(self, type).length; } function eventNames() { /* enumerate self's listener types */ } function removeAllListeners(type) { /* iterate getEventListeners(self, t) and self.removeEventListener each */ return this; } function getMaxListeners() { return 10; } function setMaxListeners() { return this; }
and include them in the
Object.definePropertyloop alongsideon/once/off. Alternatively, makefakeParentPort()return an object backed by a realEventTarget/MessagePortso the native prototype methods accept it — but that's a larger change than this PR's scope. - The old
… parentPort shadows - setMaxListeners: toUInt32 can enter user JS; declare a ThrowScope and RETURN_IF_EXCEPTION before mutating state. - onDidChangeListenerImpl: reset m_hasMessageEventListener / m_hasCloseEventListener on Clear so removeAllListeners() pauses delivery (re-buffers) instead of leaving the flags stale and draining to zero listeners. - fakeParentPort: shadow listenerCount / eventNames / removeAllListeners / emit / set|getMaxListeners so they don't fall through to the native prototype (which requires a real MessagePort receiver). Tracks parentPort-registered listeners in a local map so removeAllListeners() only removes what parentPort added (self carries internal listeners). Two new tests cover the re-buffer path and the parentPort surface.
|
Addressed all three review findings in 54bac10:
The remaining comment-cop flags are on two-line doc comments that match existing neighbor patterns; the comment-cop CI check itself passes. |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js/node/worker_threads.ts:631-655—fakeParentPort()shimson/once/off/addListener/removeListenerlocally (with the comment "MessagePort.prototype.on/etc. require a real MessagePort receiver") but leaveslistenerCount/eventNames/removeAllListeners/getMaxListeners/setMaxListeners/emitinherited from the new native intermediate prototype — those go throughIDLOperation<JSMessagePort>::call, whichjsDynamicCast<JSMessagePort*>s the receiver and throws on the plainObject.create(MessagePort.prototype)fake. The deleted JS shim's versions read symbol-keyed state offthisand worked on any receiver (returned0/[]/10/this), so worker code likeparentPort.listenerCount('message')orparentPort.removeAllListeners()that previously returned a value now throwsTypeError: Can only call MessagePort.listenerCount on instances of MessagePort. Add local overrides onfakefor these six methods the same way the other five were shimmed.Extended reasoning...
What the bug is
fakeParentPort()(worker_threads.ts:562) builds the worker-sideparentPortasconst fake = Object.create(MessagePort.prototype)— a plainJSFinalObjectwhose prototype chain isMessagePort.prototype → <intermediate NodeEventTarget prototype> → EventTarget.prototype, but which is not aJSMessagePortwrapper. The PR moved the NodeEventTarget surface from JS (the deletedinjectFakeEmitter) to native host functions on that intermediate prototype, and each of those host functions is dispatched viaIDLOperation<JSMessagePort>::call, which doescastThisValue<JSMessagePort>(..., callFrame.thisValue())and, when the cast fails,throwThisTypeError(..., "MessagePort", operationName)(JSDOMOperation.h:43-56).fakeParentPort()was updated in this PR to shim five of the eleven NodeEventTarget methods locally —on,addListener,once,off,removeListener— with the explicit comment "MessagePort.prototype.on/etc. require a real MessagePort receiver; forward to self." But the other six —listenerCount,eventNames,removeAllListeners,getMaxListeners,setMaxListeners,emit— were not shimmed and now resolve up the prototype chain to the native host functions that reject the fake receiver.Why the old code didn't have this problem
Before this PR,
injectFakeEmitterplaced pure-JS implementations of these methods on the intermediate prototype. Those implementations read symbol-keyed state offthisand returned benign defaults on any receiver:listenerCount(type)→registryFor(this, false)?.get(type)?.size ?? 0→0eventNames()→registryFor(this, false)isundefined→[]removeAllListeners()→registryFor(this, false)isundefined→return thisgetMaxListeners()→this[kMaxListeners] ?? 10→10setMaxListeners(n)→this[kMaxListeners] = n; return this→ worked
(
emitis a partial exception: the old shim calledthis.dispatchEvent(...), which was already a nativeEventTargetmethod that would have rejected the fake receiver too — soemiton the fake parentPort was likely already broken. The other five definitively worked.)Step-by-step proof
Inside a worker:
const { parentPort } = require('node:worker_threads'); parentPort.listenerCount('message');
parentPortis the object returned byfakeParentPort(). It isObject.create(MessagePort.prototype); its[[Class]]isJSFinalObject, notJSMessagePort..listenerCountis not an own property offake(onlyon/addListener/once/off/removeListener/addEventListener/removeEventListener/postMessage/close/start/ref/unref/hasRef/setEncoding/onmessage/onmessageerrorare). Lookup walks toMessagePort.prototype(nolistenerCountthere — it's on the intermediate prototype perJSMessagePortNodeEventTargetTableValues), then to the intermediate prototype, where it finds the nativejsMessagePortPrototypeFunction_listenerCount.- That host function calls
IDLOperation<JSMessagePort>::call<...listenerCountBody>(*lexicalGlobalObject, *callFrame, "listenerCount"). IDLOperation<JSMessagePort>::castcallscastThisValue<JSMessagePort>(lexicalGlobalObject, callFrame.thisValue()), whichjsDynamicCast<JSMessagePort*>s the fake object →nullptr.- Line 55-56:
return throwThisTypeError(lexicalGlobalObject, throwScope, "MessagePort", "listenerCount")→TypeError: Can only call MessagePort.listenerCount on instances of MessagePort.
Before this PR, step 2 would have found the JS
listenerCounton the intermediate prototype, which evaluatedregistryFor(fake, false)?.get('message')?.size ?? 0→0.Impact
This is a user-facing regression on
parentPortin every worker thread.parentPort.removeAllListeners()in particular is a common cleanup idiom (e.g. before re-registering handlers, or in worker-pool teardown), andparentPort.listenerCount('message')is used to check whether anyone is listening. Both now throw instead of returning. Any worker code that calls one of these five methods — code that ran fine on the previous release — will now crash with a TypeError.Fix
Extend the local-override loop in
fakeParentPort()to also cover the remaining six methods, e.g.:function listenerCount() { return 0; } // or forward to self if a global-scope count is meaningful function eventNames() { return []; } function removeAllListeners() { return this; } let maxListeners = 10; function getMaxListeners() { return maxListeners; } function setMaxListeners(n: number) { maxListeners = n; return this; } function emit() { return false; } for (const [name, fn] of [ ["on", on], ["addListener", on], ["once", once], ["off", off], ["removeListener", off], ["listenerCount", listenerCount], ["eventNames", eventNames], ["removeAllListeners", removeAllListeners], ["getMaxListeners", getMaxListeners], ["setMaxListeners", setMaxListeners], ["emit", emit], ] as const) { Object.defineProperty(fake, name, { value: fn, enumerable: false, configurable: true, writable: true }); }
(Longer term the fake parentPort should probably become a real
EventTarget/JSMessagePortso none of this shimming is needed — the file already carries a// TODO: parent port emulation is not complete— but restoring the previously-working surface is the minimal fix for this PR.)
|
The second |
Only the no-import row and the parentPort row need subprocess isolation; the rest now run directly against the debug build, which avoids 15 concurrent bun-debug spawns exceeding the default 5s timeout under ASAN.
…e error for once
- oxlint: Array.from(byType.keys()) instead of spread.
- fakeParentPort.once(): register a wrapper that deletes the listener
from byType when it fires so listenerCount/eventNames/emit are
accurate afterwards; off() looks up the wrapper.
- messagePortNodeAddListener: name the function in the listener-arg
TypeError ('once' vs 'on').
track() now returns false when the listener is already tracked and the caller skips self.addEventListener, so a second once()/on() for the same function is a no-op (matching node) and off() can't orphan an earlier registration.
Avoids the subprocess + worker double-spawn that sat at the 5s default timeout boundary under debug+ASAN.
…tale comment - once() wrapper returns listener(arg) so a promise return reaches handleEvent's addCatch path. - removeAllListeners iterates byType.keys() directly; SafeMap's iterator tolerates deleting the just-yielded key so no snapshot is needed. - Drop the test-file comment that referenced the deleted JS shim.
So .on(fn) -> removeEventListener(fn) -> .on(fn) re-registers instead of being swallowed by track()'s first-wins check.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/jsc/bindings/webcore/EventHeaders.h`:
- Around line 81-82: Add CustomEvent.h and JSCustomEvent.h to the generator
input that produces EventHeaders.h, rather than modifying the generated header
directly, then regenerate EventHeaders.h and verify both includes are preserved.
In `@src/jsc/bindings/webcore/JSMessagePort.cpp`:
- Around line 593-601: Update jsMessagePortPrototypeFunction_setMaxListenersBody
to validate the original argument as a finite, non-negative integer before
converting or storing it. Throw for negative, NaN, Infinity, fractional, or
otherwise invalid values, and only call setNodeMaxListeners after validation
succeeds.
In `@test/js/node/worker_threads/message-port-node-event-target.test.ts`:
- Around line 87-101: The negative assertion in “removeAllListeners clears
addEventListener listeners too” relies on a single setImmediate tick and can
pass before delayed delivery occurs. Replace it with a short bounded polling
window, following the established negative-condition polling pattern in this
test file, and assert that n remains zero throughout the window.
- Around line 18-24: Replace the require.cache check in the embedded
worker_threads preload test with a sentinel that detects the module’s bootstrap
side effect, and throw when that sentinel indicates node:worker_threads was
preloaded. Keep the existing MessageChannel API validation unchanged.
🪄 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: d0a25aca-f740-4c83-bfb7-7848192fc934
📒 Files selected for processing (19)
src/js/builtins.d.tssrc/js/builtins/BunBuiltinNames.hsrc/js/node/worker_threads.tssrc/jsc/bindings/webcore/AddEventListenerOptions.hsrc/jsc/bindings/webcore/EventFactory.cppsrc/jsc/bindings/webcore/EventHeaders.hsrc/jsc/bindings/webcore/EventListener.hsrc/jsc/bindings/webcore/EventListenerMap.cppsrc/jsc/bindings/webcore/EventListenerMap.hsrc/jsc/bindings/webcore/EventTarget.cppsrc/jsc/bindings/webcore/EventTarget.hsrc/jsc/bindings/webcore/JSAddEventListenerOptions.cppsrc/jsc/bindings/webcore/JSEventListener.cppsrc/jsc/bindings/webcore/JSEventListener.hsrc/jsc/bindings/webcore/JSMessagePort.cppsrc/jsc/bindings/webcore/MessagePort.cppsrc/jsc/bindings/webcore/MessagePort.hsrc/jsc/bindings/webcore/RegisteredEventListener.htest/js/node/worker_threads/message-port-node-event-target.test.ts
…ventListener forwards options and wrapper - removeAllListeners treats an undefined/null argument the same as no argument (node checks type !== undefined && type !== null). - fakeParentPort.removeEventListener looks up the once-wrapper via byType and forwards the options/useCapture argument, mirroring off().
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/js/node/worker_threads.ts (3)
681-690: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat
undefinedandnullas “remove all.”With one argument,
removeAllListeners(undefined)andremoveAllListeners(null)callclear()for those values instead of clearing the registry. The native implementation explicitly treats both values as the no-argument form, so fakeparentPortretains listeners and reports stale state.Use an explicit
arguments.length === 0 || type === undefined || type === nullcheck before type-specific removal.As per coding guidelines, deliberately enumerate empty, unset, and null input variants rather than relying only on argument count.
🤖 Prompt for 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. In `@src/js/node/worker_threads.ts` around lines 681 - 690, The removeAllListeners function must treat no argument, undefined, and null as requests to remove all listeners. Update its branching condition to explicitly check arguments.length === 0, type === undefined, or type === null before invoking type-specific clear logic, while preserving targeted removal for valid event types.Source: Coding guidelines
673-679: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftInclude direct EventTarget listeners in shared bookkeeping.
fake.addEventListener()registers onselfbut never updatesbyType, whilelistenerCount(),eventNames(), andemit()consult onlybyType. A listener added throughparentPort.addEventListener()can therefore run duringemit()whilelistenerCount()returns0,eventNames()omits the event, andemit()returnsfalse. The nativeMessagePortimplementation reads the shared listener list for these operations.Track fake-owned direct registrations separately from internal
selflisteners, or route all listener APIs through one registry.Also applies to: 693-700
🤖 Prompt for 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. In `@src/js/node/worker_threads.ts` around lines 673 - 679, The fake EventTarget listener registrations are not reflected in the shared bookkeeping used by listenerCount, eventNames, and emit. Update the add/remove listener handling associated with fake.addEventListener and the byType registry so direct parentPort registrations are tracked alongside the relevant event type without counting internal self listeners, and ensure listenerCount, eventNames, and emit observe the same registry and return accurate results.
658-664: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
handleEventsupport inonce()
This wrapper callslistener(arg)directly, so{ handleEvent() {} }listeners throw when the event fires even thoughaddEventListeneraccepts them. Dispatch callable listeners and EventListener objects the same way here.🤖 Prompt for 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. In `@src/js/node/worker_threads.ts` around lines 658 - 664, The once function’s wrapper must support EventListener objects with handleEvent in addition to callable listeners. Update the listener invocation inside once to dispatch through the existing listener-handling mechanism, preserving cleanup and callable-listener behavior.src/jsc/bindings/webcore/JSMessagePort.cpp (2)
445-462: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSkip nullish listeners before
track()insrc/js/node/worker_threads.ts#L644-L665.addEventListener(null)is a no-op, but the shim still records the entry;once()also closes over a null listener and can throw when it fires.🤖 Prompt for 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. In `@src/jsc/bindings/webcore/JSMessagePort.cpp` around lines 445 - 462, Skip nullish listeners before registering or tracking them. In messagePortNodeAddListener, preserve the existing no-op behavior for a null listener and ensure the worker_threads.ts shim does not call track() or create a once() wrapper for nullish listeners; apply the corresponding change at src/jsc/bindings/webcore/JSMessagePort.cpp lines 445-462 and src/js/node/worker_threads.ts lines 644-655.Source: Coding guidelines
486-499: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the registered capture on
fake.removeEventListener().
JSMessagePort.off()/removeListener()only take(type, listener), so the capture concern doesn’t apply there. Insrc/js/node/worker_threads.ts#L638-L641,byTypeis deleted unconditionally, so a capture-mismatchedremoveEventListener()can desynclistenerCount()/eventNames()from the actual listener list. Store capture with the registration, or skip the delete when the requested capture doesn’t match.🤖 Prompt for 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. In `@src/jsc/bindings/webcore/JSMessagePort.cpp` around lines 486 - 499, Update the listener bookkeeping in worker_threads.ts at lines 638-641 and 667-670 so byType entries are removed only when the requested capture matches the registered listener, or store the registration’s capture value for comparison; preserve the actual listener removal behavior. The JSMessagePort.cpp function jsMessagePortPrototypeFunction_offBody at lines 486-499 requires no direct change because off/removeListener only accept type and listener.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/js/node/worker_threads.ts`:
- Around line 681-690: The removeAllListeners function must treat no argument,
undefined, and null as requests to remove all listeners. Update its branching
condition to explicitly check arguments.length === 0, type === undefined, or
type === null before invoking type-specific clear logic, while preserving
targeted removal for valid event types.
- Around line 673-679: The fake EventTarget listener registrations are not
reflected in the shared bookkeeping used by listenerCount, eventNames, and emit.
Update the add/remove listener handling associated with fake.addEventListener
and the byType registry so direct parentPort registrations are tracked alongside
the relevant event type without counting internal self listeners, and ensure
listenerCount, eventNames, and emit observe the same registry and return
accurate results.
- Around line 658-664: The once function’s wrapper must support EventListener
objects with handleEvent in addition to callable listeners. Update the listener
invocation inside once to dispatch through the existing listener-handling
mechanism, preserving cleanup and callable-listener behavior.
In `@src/jsc/bindings/webcore/JSMessagePort.cpp`:
- Around line 445-462: Skip nullish listeners before registering or tracking
them. In messagePortNodeAddListener, preserve the existing no-op behavior for a
null listener and ensure the worker_threads.ts shim does not call track() or
create a once() wrapper for nullish listeners; apply the corresponding change at
src/jsc/bindings/webcore/JSMessagePort.cpp lines 445-462 and
src/js/node/worker_threads.ts lines 644-655.
- Around line 486-499: Update the listener bookkeeping in worker_threads.ts at
lines 638-641 and 667-670 so byType entries are removed only when the requested
capture matches the registered listener, or store the registration’s capture
value for comparison; preserve the actual listener removal behavior. The
JSMessagePort.cpp function jsMessagePortPrototypeFunction_offBody at lines
486-499 requires no direct change because off/removeListener only accept type
and listener.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22bdefe0-d0ec-4609-b430-fddf3265f846
📒 Files selected for processing (2)
src/js/node/worker_threads.tssrc/jsc/bindings/webcore/JSMessagePort.cpp
|
Re the five outside-diff coderabbit findings on
The stand-in is explicitly |
There was a problem hiding this comment.
No new findings on d2147a7. This is a substantial rework of the MessagePort event surface (native NodeEventTarget prototype, EventTarget dispatch hook, ~200 lines of new C++ bindings), so it's worth a human pass.
What was reviewed across iterations: exception-scope coverage in every new JSMessagePort body; EventListenerMap::removeAll marking listeners removed before erase (matches clear()); onDidChangeListenerImpl reconciling m_hasMessageEventListener/m_hasCloseEventListener on Clear so re-buffering and GC liveness stay correct; SetForScope keeping m_invokeAsNodeStyle transient; the intermediate-prototype construction in createPrototype. The remaining acknowledged nits (fakeParentPort removeAllListeners(undefined), setMaxListeners range validation) are pre-existing gaps deferred to a follow-up.
Extended reasoning...
Overview
This PR replaces the ~180-line injectFakeEmitter JS shim with a native NodeEventTarget implementation on MessagePort. It touches 19 files: an isNodeStyleListener bit threaded through AddEventListenerOptions → RegisteredEventListener → EventTarget::innerInvokeEventListeners; a handleEventNodeStyle override on JSEventListener that passes .data/.detail/.error instead of the Event wrapper; ~200 lines of new host functions in JSMessagePort.cpp (on/once/off/emit/listenerCount/eventNames/removeAllListeners/get|setMaxListeners) reified onto an intermediate prototype inserted between MessagePort.prototype and EventTarget.prototype; EventListenerMap::removeAll + EventTarget::removeAllEventListenersForType; wiring CustomEventInterfaceType into EventFactory; and a rewritten fakeParentPort() stand-in in worker_threads.ts. 15-row test matrix added.
Security risks
None identified. The kIsNodeStyleListener flag is keyed off a JSC private name (unforgeable from userland), matching the existing kResistStopPropagation pattern. No auth/crypto/permission code is touched.
Level of scrutiny
High. innerInvokeEventListeners is on the hot path for every event dispatch in the runtime, and inserting a prototype into a WebCore class chain is a structural change. The new C++ host functions do argument coercion (which enters user JS) and dispatch events (which run user callbacks that can synchronously close()/removeAllListeners() the port). The onDidChangeListener(Clear) path now feeds into m_hasMessageEventListener/m_hasCloseEventListener, which gate GC liveness (hasPendingActivity) and drain scheduling.
Other factors
Over the course of ~7 earlier passes I flagged and the author fixed: missing ThrowScope in setMaxListeners; stale m_has*EventListener after removeAllListeners (now handled in onDidChangeListenerImpl for Clear); several fakeParentPort desync/tamper-resistance issues; removeAllListeners(undefined|null) on the native path; removeEventListener options/wrapper forwarding on the fake. Two acknowledged-not-regressions remain for a follow-up (fake-side removeAllListeners(undefined) on the TODO-marked stub, setMaxListeners input validation). The change is well-tested and the design mirrors Node's lib/internal/event_target.js, but the surface area — prototype-chain surgery, event-dispatch hook, GC-liveness bookkeeping — is large enough that a maintainer should sign off.
|
CI status: the diff is green on every lane that ran my tests (gate passed on ASAN and release; Build 81904 failed only on a
Neither touches MessagePort/EventTarget/worker_threads. Ready for review; needs a fresh CI run once the build fleet recovers. |
2 participants
Repro
Cause
injectFakeEmitterinsrc/js/node/worker_threads.tsgrafted the emitter surface ontoMessagePort.prototypeat module top level and kept its own (event → userFn → wrapper) registry alongside WebCore's listener list..on(t, f)registered a wrapper viaaddEventListener, so the two APIs couldn't see each other:listenerCount/eventNameswere blind toaddEventListenerlisteners,removeEventListener(f)couldn't remove a.on(f),removeAllListenersleftaddEventListenerlisteners live,getEventListenersreturned the wrapper, andemit('message', x)builtnew MessageEvent('message', x)withxin the init-dict slot (so a primitive threw and the listener gotnull). The surface was also absent until something importednode:worker_threads.Node's model (
lib/internal/event_target.js):MessagePortis aNodeEventTarget; there is one listener list..on/.addListenerisaddEventListener(type, f, {[kIsNodeStyleListener]: true})storingfitself;onceis the same withonce: true;listenerCount/eventNames/removeAllListenersread/clear that list;emit(type, arg)computeshad = listenerCount(type) > 0, dispatches so node-style listeners receiveargby identity and EventTarget-style listeners receive aMessageEvent{data:arg}/CustomEvent{detail:arg}, and returnshad.Fix
RegisteredEventListener/AddEventListenerOptionsgain anisNodeStyleListenerbit, keyed off a privatekIsNodeStyleListenersymbol.EventTarget::innerInvokeEventListenersinvokes flagged listeners viaJSEventListener::handleEventNodeStyle, which passesevent.data/.detail/.errorinstead of the Event wrapper.JSMessagePort's prototype chain now has an intermediate prototype (betweenMessagePort.prototypeandEventTarget.prototype) carrying nativeon/once/off/addListener/removeListener/emit/listenerCount/eventNames/removeAllListeners/set|getMaxListeners, all reading the one native listener list. The intermediate prototype keepsObject.getOwnPropertyNames(MessagePort.prototype)matching node.emit()buildsMessageEvent{data:arg}(formessage/messageerror) orCustomEvent{detail:arg}(otherwise) and dispatches; node-style listeners recoverargby identity at invoke.CustomEventInterfaceTypeis wired intoEventFactoryso a natively-createdCustomEventis wrapped asJSCustomEvent(its.detailwas previously unreachable from such events).injectFakeEmittershim is deleted. The worker-sideparentPortstand-in forwardson/once/offtoself.addEventListenerwith the private flag.BroadcastChannelis a plainEventTargetin node too; no shim there (unchanged).Verification
test/js/node/worker_threads/message-port-node-event-target.test.tsruns a 15-row coherence matrix (same fn via.on+ael invoked once;listenerCount/eventNamescount both; cross-removeEventListener;removeAllListenersclears both;emitpayload identity + boolean return + primitive payload + error identity;oncededupe;getEventListenersidentity; hybrid dispatch; surface present with noworker_threadsimport). Before: 14/15 fail. After: 15/15 pass.Existing coverage stays green:
test/js/node/worker_threads/worker_threads.test.ts(91 tests),test/js/web/workers/message-channel.test.ts,test-worker-message-port*.js,test-eventtarget*.js,test-events-customevent.js.This supersedes #33856 (which only moved the shim to load earlier but kept the separate registry), #35799 (shared registry only), and #35796 (emit() payload only).
[review] gate passed · iteration 2 · 19 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 6 passed · 1 rejected · iteration 2
evidence per changed file