worker_threads: deliver the emit() payload to MessagePort listeners - #35796
worker_threads: deliver the emit() payload to MessagePort listeners#35796robobun wants to merge 12 commits into
Conversation
MessagePort's NodeEventTarget emit() shim did
`new MessageEvent("message", payload)` / `new ErrorEvent("error", err)`,
so the payload was read as an init dict and listeners received
null/undefined instead of the value. It also returned `this` where
node returns a boolean.
Wrap the arg in the correct init dict (MessageEvent {data} for
message/messageerror, CustomEvent {detail} for everything else, matching
node's MessagePort[kCreateEvent]), return listenerCount(type) > 0, and
fix the messageerror extractor to read .data (native messageerror is a
MessageEvent, not an ErrorEvent).
WalkthroughUpdates the worker thread fake emitter so ChangesMessagePort emit behavior
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused Node-compat fix with a clear root cause and thorough test coverage. The one inline nit is a pre-existing listenerCount() gap that the new boolean return exposes; fine as a follow-up.
What was reviewed:
emit()event construction now round-trips through the.on()extractors for all four event types; verified against Node'sNodeEventTarget.prototype.emitsemantics.messageerrorextractor switch to.datais correct — native dispatch (MessageEvent.cpp:112) builds aMessageEvent, and Node'sMessagePort[kCreateEvent]does the same.injectFakeEmitteris only applied toMessagePort, so the"error"→CustomEventchange has no other consumers.
Extended reasoning...
Overview
The PR fixes injectFakeEmitter's emit() in src/js/node/worker_threads.ts, which was constructing MessageEvent/ErrorEvent with the user's raw payload as the init dictionary instead of wrapping it in {data: arg} / {detail: arg}. As a result, .on() listeners received null/undefined and emit() returned this instead of a boolean. It also fixes the messageerror extractor to read .data (native messageerror is a MessageEvent, not an ErrorEvent), removes the now-dead errorEventHandler/EventClass, and returns listenerCount(type) > 0 per Node's NodeEventTarget.prototype.emit. A ~85-line test in the existing worker_threads.test.ts covers message/error/messageerror/custom for both .on() (identity) and addEventListener (event class + payload field), plus the boolean return in listener/no-listener cases.
Security risks
None. This is JS-side event-shape plumbing on MessagePort's emitter shim; no untrusted input parsing, no privilege boundaries, no native code.
Level of scrutiny
Low-to-moderate. The change is ~30 net source lines in a single function plus its extractor table, entirely within the Node-compat layer. injectFakeEmitter is applied only to MessagePort (line 278), so scope is contained. I confirmed native messageerror dispatch creates a MessageEvent (src/jsc/bindings/webcore/MessageEvent.cpp:112), so switching that extractor from .error to .data is correct for native events, not just synthetic emit() — this actually fixes a second, independent bug where .on('messageerror', cb) would have missed real deserialization failures. The "error" case moving from ErrorEvent/.error to CustomEvent/.detail is safe because MessagePort never fires error natively; only emit() produces it, and emit() now constructs the matching CustomEvent.
Other factors
The PR description reports the full worker_threads.test.ts suite plus the vendored Node compat tests (test-worker-message-port.js, test-event-target.js, test-messagechannel.js) pass. Dead code is deleted in the same PR per the review guidelines. The one flagged nit — emit() returning false when only addEventListener listeners exist — is a pre-existing listenerCount() limitation that was already user-observable before this PR (p.addEventListener(...); p.listenerCount(...) already returned 0); the new return value adds a second observation point but is still a strict improvement over return this for the common cases. It's correctly scoped as a follow-up.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?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:133-145— The "MessagePort never fireserrornatively" justification for routingerrorthroughcustomEventHandlerdoesn't hold for the fakeparentPort: itsaddEventListeneris bound toself, andWebWorker__dispatchErrordispatches a realErrorEvent(with.error, no.detail) onselffor uncaught worker exceptions. SoparentPort.on("error", cb)inside a worker still fires but now deliversundefinedwhere it previously delivered the thrown value. Not blocking — Node never fireserroronparentPortin the first place, so the pre-PR behavior was accidental Bun-only leakage through a TODO'd emulation — but worth noting since the code comment's assumption is incomplete.Extended reasoning...
What the bug is
functionForEventType("error", listener)was changed fromwrapped(errorEventHandler, listener)(which extractsevent.error) to the defaultwrapped(customEventHandler, listener)(which extractsevent.detail), justified by the comment "Everything else (including "error", which MessagePort never fires natively) rides CustomEvent.detail". That justification is correct for realMessagePortinstances but overlooks the fakeparentPort, which inherits the injected.on()fromMessagePort.prototypewhile binding its ownaddEventListenertoself.addEventListener. Uncaught worker exceptions dispatch anErrorEventonself, and anErrorEventhas.errorpopulated but no.detail.Code path
In
fakeParentPort()(worker_threads.ts:614-680):fake = Object.create(MessagePort.prototype)— inherits the injected NodeEventTarget methods, includingon().fake.addEventListeneris defined as an own property equal toself.addEventListener.bind(self)(line ~669).- No own
onis defined onfake, soparentPort.on(...)reaches the injectedon()on the intermediate prototype, which callsregister(this, event, listener, functionForEventType(event, listener), undefined)→this.addEventListener(event, wrapper)→self.addEventListener("error", wrapper).
In
WebWorker__dispatchError(src/jsc/bindings/webcore/Worker.cpp:727-737), on an uncaught worker exception:ErrorEvent::Init init; init.message = messageStr.isolatedCopy(); init.error = error; ... globalObject->globalEventScope->dispatchEvent(ErrorEvent::create(eventNames().errorEvent, init, ...));globalObject->globalEventScopeis the worker'sself, so the wrapper registered above fires with anErrorEventwhose.erroris the thrown value and whose.detailisundefined(ErrorEvent has nodetailfield).Step-by-step proof
Inside a worker:
const { parentPort } = require("worker_threads"); parentPort.on("error", e => console.log("got", e)); setImmediate(() => { throw new Error("boom"); });
parentPort.on("error", cb)→ injectedon()(fake has no ownon) →functionForEventType("error", cb). Before this PR that returnedwrapped(errorEventHandler, cb)(readsevent.error); after it returnswrapped(customEventHandler, cb)(readsevent.detail).register(...)callsthis.addEventListener("error", wrapper)→self.addEventListener("error", wrapper)(own-property override onfake).throw new Error("boom")reaches the uncaught-exception path →WebWorker__dispatchErrordispatches anErrorEventonselfwithinit.error = Error("boom").- The wrapper runs:
cb(customEventHandler(event))=cb(event.detail)=cb(undefined). Before this PR it wascb(event.error)=cb(Error("boom")).
So the callback still fires but now receives
undefinedinstead of the thrown value — a user-observable regression introduced by this PR.Why existing code doesn't prevent it
The new comment's premise ("MessagePort never fires
errornatively") is what the change relies on, and it holds for every path except the fake parentPort's own-propertyaddEventListenerbinding toself. Nothing infakeParentPort()shadows.on(), so it inherits the extractor table this PR changed. The PR's new tests only exercise realMessagePortinstances (new MessageChannel().port1), whereerrorevents indeed only arrive viaemit()asCustomEvent.Impact / severity
Low. The pre-PR behavior was accidental and Bun-specific: in Node,
parentPortis a realMessagePortthat never fireserror, soparentPort.on("error", cb)never fires at all — the fact that it fired in Bun with the thrown value was leakage ofself's events through the fake port's binding, andfakeParentPort()is explicitly marked "TODO: parent port emulation is not complete". The listener still fires, only its argument regresses;parentPort.addListener("error", cb)(own property on the fake, unwrapped) still delivers the rawErrorEventunchanged. Not a Node-compat break and not worth blocking on.How to fix
Either restore an
errorEventHandlerbranch infunctionForEventTypethat readsevent.error ?? event.detail(so both nativeErrorEvents andemit()-constructedCustomEvents round-trip), or shadow.on/.onceon the fakeparentPortso itserrorwrapper readsevent.error. The former is a two-line change and keeps the extractors the single source of truth; the latter keeps the extractor comment accurate for real MessagePorts. Given the fake-parentPort emulation is already TODO'd for replacement, a note in the comment ("except the fake parentPort routesself's ErrorEvent") plus the?? event.detailfallback is probably the pragmatic choice.
fakeParentPort binds addEventListener to self, so parentPort.on('error')
inside a worker receives the native ErrorEvent from an uncaught
exception. Read .error ?? .detail so both that path and emit()'s
CustomEvent deliver the value.
|
Good catch on the fake parentPort path. |
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:134-140— Collateral change:parentPort.on('error', cb)inside a worker now receivesundefinedinstead of the thrown value when the worker global dispatches a nativeErrorEventfor an uncaught exception.fakeParentPortbindsaddEventListenertoselfbut inherits thison(), so its'error'listener now unwraps viaevent.detail(thecustomEventHandlerdefault) rather than the removedevent.errorextractor. This was already a Bun-only, non-Node quirk on an admittedly incompleteparentPortemulation, so not worth blocking on — just flagging in case it was unintentional.Extended reasoning...
What changed
functionForEventType()previously routed'error'througherrorEventHandler(which returnedevent.error). This PR removes that arm, so'error'now falls through to thedefaultcase →customEventHandler(which returnsevent.detail). That's correct for theemit()round-trip this PR targets —emit('error', x)now builds aCustomEventwith{ detail: x }, and MessagePort itself never fires a native'error'event.But
injectFakeEmitteris also inherited byfakeParentPort, which is a different beast.The code path
fakeParentPort()(further down in the same file) createsfake = Object.create(MessagePort.prototype)and then defines an ownaddEventListenerproperty asself.addEventListener.bind(self)— but does not define an ownon. SoparentPort.on('error', cb)resolves to the injectedon()on the prototype, which calls:register(this, 'error', cb, functionForEventType('error', cb), undefined) → this.addEventListener('error', wrapper, undefined) // own property → self.addEventListener → self.addEventListener('error', wrapped(customEventHandler, cb))
Meanwhile,
WebWorker__dispatchError(src/jsc/bindings/webcore/Worker.cpp:737) unconditionally dispatches a nativeErrorEventonglobalObject->globalEventScope— i.e.self— for uncaught worker exceptions, before the Web/Node kind switch. ThatErrorEventhas.errorset to the thrown value and no.detailproperty.Step-by-step proof
Inside a
node:worker_threadsworker:- User calls
parentPort.on('error', cb). onis inherited →register(fake, 'error', cb, wrapped(customEventHandler, cb))→fake.addEventListener(...)→ own property →self.addEventListener('error', wrapper).- Worker code throws uncaught →
WebWorker__dispatchErrorfiresErrorEvent { error: <thrown>, message: ... }onself's event scope. wrapper(event)runs →cb(customEventHandler(event))→cb(event.detail)→cb(undefined).
Before this PR, step 2 produced
wrapped(errorEventHandler, cb), so step 4 wascb(event.error)→cb(<thrown value>).Why existing code doesn't prevent it
The PR's reasoning — "MessagePort never fires
'error'natively, so'error'can go through the CustomEvent default" — holds for realMessagePortinstances but not forfakeParentPort, whoseaddEventListeneris redirected to the worker global scope. Nothing infunctionForEventType()distinguishes the two receivers.Impact
Narrow and Bun-specific. Node's
parentPortnever emits'error'for uncaught exceptions (those go toprocess.on('uncaughtException')in the worker andworker.on('error')in the parent), so this listener pattern is not portable to begin with. The pre-PR behavior was already an undocumented Bun quirk on an emulation the file itself marks// TODO: parent port emulation is not complete, no test covers it, andgrepfinds noparentPort.on('error'anywhere in the repo. That said, going from "delivers the error" to "deliversundefined" is silently worse for anyone who was relying on the old Bun behavior.How to fix (if desired)
Either keep an
'error'arm infunctionForEventType()that readsevent.error ?? event.detail(covers both nativeErrorEventand the newCustomEventfromemit()), or — better long-term — stopfakeParentPortfrom proxying arbitrary events toselfat all, which is the real divergence from Node. Given the obscurity, deferring to the parentPort rewrite is reasonable. - User calls
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/worker_threads.ts`:
- Around line 200-212: Update emit in src/js/node/worker_threads.ts at lines
200-212 to include addEventListener() registrations when computing its boolean
return, while preserving the existing .on()/.once() listener count and dispatch
behavior. Add an addEventListener()-only test in
test/js/node/worker_threads/worker_threads.test.ts at lines 1379-1385 that
asserts emit() returns true.
🪄 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: 776440ce-3f8c-44b3-a118-5a454d5db51f
📒 Files selected for processing (2)
src/js/node/worker_threads.tstest/js/node/worker_threads/worker_threads.test.ts
There was a problem hiding this comment.
LGTM — the follow-up commits since my earlier note look good.
What was reviewed:
emit()now wraps the arg as{data:}/{detail:}so it round-trips through the.on()extractors; verified themessageerrorextractor switch to.datamatches native MessagePort dispatch.- f241783 restores the
errorextractor asevent.error ?? event.detail— checked that this covers bothfakeParentPort's native ErrorEvent path andemit('error', ...)'s CustomEvent, including falsy payloads. - The unresolved CodeRabbit note on
addEventListener-onlyemit()return is the same pre-existinglistenerCount()gap I flagged earlier and the author acknowledged for follow-up; it doesn't block this fix.
Extended reasoning...
Overview
This PR fixes injectFakeEmitter's emit() in src/js/node/worker_threads.ts (the NodeEventTarget shim applied to MessagePort.prototype). Previously it passed the user's payload directly as the event init dictionary (new MessageEvent('message', payload)), so .on() listeners — which read event.data/event.error — received the constructor defaults (null/undefined). It also returned this instead of a boolean. The fix constructs MessageEvent with {data: arg} for message/messageerror, CustomEvent with {detail: arg} for everything else, remaps the messageerror extractor to .data (matching native dispatch and Node's MessagePort[kCreateEvent]), keeps an error extractor reading event.error ?? event.detail for the fakeParentPort path, drops the dead EventClass helper, and returns listenerCount(type) > 0. A new test in worker_threads.test.ts covers all four event kinds for both .on() and addEventListener, plus the boolean return.
Security risks
None. This is a JS-side Node-compat shim adjusting how event payloads are wrapped/unwrapped for MessagePort listeners. No untrusted input parsing, no privilege boundaries, no native code.
Level of scrutiny
Moderate — it touches a builtin module hot path (src/js/), but the change is small (~30 lines net), mechanically clear, and the extractor/constructor pairing is now internally consistent (each emit() branch produces exactly the shape its functionForEventType extractor reads). I traced the fakeParentPort interaction: it overrides addEventListener to target self but inherits the injected .on(), so parentPort.on('error', cb) receives native ErrorEvents — the restored event.error ?? event.detail extractor handles both that and the CustomEvent produced by emit('error', ...). The ?? correctly falls through for CustomEvent (whose .error is undefined) even for falsy payloads like 0/false/null.
Other factors
- I previously flagged that
emit()returnsfalseforaddEventListener-only listeners becauselistenerCount()only reads the JS-side registry. The author acknowledged this as a pre-existinglistenerCount()gap and left it for follow-up; the return-value change is still a strict improvement overreturn this. CodeRabbit's later unresolved note is a duplicate of that same finding and doesn't need separate action. - The comment-cop bot's paragraph-length warnings were addressed across a1dbd7f/f999fcaa; the final comments are single-line.
- The signature change
emit(event, ...args)→emit(event, arg)matches Node'sNodeEventTarget.prototype.emit(single-arg) and the old code only ever usedargs[0]anyway. - Test coverage is thorough: identity assertions (
.toBe) for the.on()payload, event-class + property checks foraddEventListener, and the no-listenerfalsecase.
…Listeners .on() listeners are wrappers registered via addEventListener, so events.getEventListeners(port, type) already sees both styles on a real MessagePort. Fall back to the .on() registry for fakeParentPort (no EventTarget internal slot). Fixes emit() returning false when the only listener was registered via addEventListener.
events.getEventListeners first checks emitter?.listeners via a prototype walk, so a user-set MessagePort.prototype.listeners would silently hijack the count. Call jsFunctionNodeEventsGetEventListeners directly instead, matching the tamper-resistance of the pre-existing SafeMap path.
…ning event.error ?? event.detail reads a property absent from the event's own prototype in both directions (CustomEvent has no .error, ErrorEvent has no .detail), so either read walks to Object.prototype. Branching on the constructor keeps both reads on real prototype accessors.
There was a problem hiding this comment.
LGTM — the emit() payload/return fix and the follow-up hardening rounds all look correct.
What was reviewed:
emit()now wraps the arg in a null-proto init dict and returnslistenerCount(type) > 0before dispatch, matching Node'sNodeEventTarget;messageerrorextractor now reads.data(nativemessageerroris aMessageEvent).listenerCount()calls the samejsFunctionNodeEventsGetEventListenersbindingevents.tsuses; verified the C++ throwsERR_INVALID_ARG_TYPEon a non-JSEventTarget, so thefakeParentPortcatch-fallback engages.- Prior nits (
__proto__: nullinit dicts, captured_ErrorEvent,.listenerstamper bypass) all applied; the deferredeventNames()/Symbol.hasInstanceitems are pre-existing/out-of-scope and don't regress anything.
Extended reasoning...
Overview
Fixes injectFakeEmitter's emit() on MessagePort in src/js/node/worker_threads.ts: it was passing the user's payload as the WebIDL init dict (new MessageEvent('message', payload)) so .on() callbacks received null, and it returned this instead of a boolean. The fix wraps the arg in { __proto__: null, data: arg } / { __proto__: null, detail: arg }, moves messageerror to the .data extractor (native messageerror is a MessageEvent), reworks errorEventHandler to branch on a captured _ErrorEvent so fakeParentPort's native ErrorEvent and emit('error')'s CustomEvent both round-trip, and makes listenerCount() read the native EventTarget map via the jsFunctionNodeEventsGetEventListeners binding so emit()'s boolean return counts addEventListener-only listeners. EventClass() is deleted (dead). ~25 source lines, ~110 test lines.
Security risks
None. This is a Node-compat behavioural fix on MessagePort's emitter façade; no auth/crypto/permissions surface. The tamper-resistance angles (prototype pollution on init dicts, .listeners duck-typing, live ErrorEvent global) were raised in earlier rounds and addressed with __proto__: null literals, a direct native binding call, and a module-eval-time captured const. The remaining Symbol.hasInstance vector on _ErrorEvent is the same tamper class as the file's other instanceof uses and is not a privilege boundary.
Level of scrutiny
Medium — built-in JS module in the hostile-environment / hot-path category, but the change is small, mechanical (init-dict shape + return type), and mirrors Node's NodeEventTarget.prototype.emit. I verified the native binding path: JSEventTargetNode.cpp throws on non-JSEventTarget, so the fakeParentPort (an Object.create(MessagePort.prototype) without internal slot) hits the catch and falls back to the SafeMap registry. The $newCppFunction reference is byte-identical to the one already in events.ts.
Other factors
This PR has already been through four review rounds from me plus one from CodeRabbit; every thread is resolved. The new test covers all four event types × both listener styles × the boolean return × the addEventListener-only case × a .listeners tamper case, and the PR description shows fails-on-main / passes-with-fix evidence. The two acknowledged deferrals — eventNames()/removeAllListeners() still reading only the .on() registry (needs a new native binding), and Symbol.hasInstance on the captured ErrorEvent — are strictly-no-worse-than-main and reasonably scoped out.
There was a problem hiding this comment.
LGTM — the two nits from my last pass are addressed (null-proto init dicts, ErrorEvent captured at module eval). The remaining Symbol.hasInstance angle and the eventNames()/removeAllListeners() inconsistency were reasonably scoped out; neither regresses anything.
What was reviewed:
emit()payload delivery + boolean return round-trips through the.on()extractors for message/messageerror/error/customlistenerCount()now reads the native EventTarget map via the same$newCppFunctionbindingevents.tsuses; try/catch fallback coversfakeParentPortmessageerrorextractor reads.data(nativemessageerroris a MessageEvent) — verified against theJSMessagePort.cppevent-name wiring- Tamper-resistance: init dicts are null-proto;
_ErrorEventcaptured insideinjectFakeEmitter, which runs at module eval
Extended reasoning...
Overview
Fixes MessagePort.prototype.emit() in node:worker_threads: the injected fake-emitter previously passed the user's payload as the WebIDL init dictionary (new MessageEvent("message", payload)), so .on() listeners received the default null/undefined and emit() returned this. The fix wraps the payload correctly ({ __proto__: null, data: arg } / { __proto__: null, detail: arg }), returns listenerCount(type) > 0, moves messageerror to the .data extractor (native messageerror is a MessageEvent), and upgrades listenerCount() to read the native EventTarget listener map so addEventListener-only listeners count. Two files touched: src/js/node/worker_threads.ts (~50 lines net) and a 110-line test block in worker_threads.test.ts.
Security risks
None. This is Node-compat event plumbing; no auth/crypto/permissions/IO surface. The tamper-resistance rounds (null-proto init dicts, captured ErrorEvent, direct native binding for listenerCount) are hardening, not new exposure.
Level of scrutiny
Moderate. It's a built-in src/js/ module (hot-path, tamper-sensitive per REVIEW.md), but the change is localized to injectFakeEmitter and is a straightforward correctness fix with strong test coverage — seven sub-cases exercising both listener styles, all four event kinds, the boolean return with/without listeners, and a .listeners tamper test.
Other factors
This PR has been through four review rounds; all inline threads are resolved. Since my last pass the author applied both requested changes in 2feb39f. The two consciously deferred items — Symbol.hasInstance on the captured ErrorEvent and the eventNames()/removeAllListeners() store mismatch — are pre-existing-class nits I already marked non-blocking; the author's scoping rationale (same tamper class as the file's other bare instanceof URL/Map/Set checks; a proper eventNames() fix needs a native enumeration binding that doesn't exist yet) is sound. The $newCppFunction("JSEventTargetNode.cpp", "jsFunctionNodeEventsGetEventListeners", 1) call is byte-identical to the one already shipping in src/js/node/events.ts. The bug-hunting system found nothing this run.
There was a problem hiding this comment.
LGTM — the fixes from the last round (__proto__: null init dicts, module-local _ErrorEvent capture) landed in 2feb39f, and the two commits since are empty CI retriggers. The remaining Symbol.hasInstance and eventNames() points were reasonably deferred as out of scope.
What was reviewed:
emit()now wraps the payload in the correct init-dict shape and returnslistenerCount > 0; extractors round-trip for message/messageerror/error/custom.listenerCount()callsjsFunctionNodeEventsGetEventListenersdirectly (verified present inJSEventTargetNode.cpp), with a registry fallback forfakeParentPort; the.listenerstamper test guards it.- Checked that
fakeParentPort.on('error')still receivesevent.errorfrom native ErrorEvents via theinstanceof _ErrorEventbranch.
Extended reasoning...
Overview
Fixes MessagePort.emit() in the injected NodeEventTarget shim (src/js/node/worker_threads.ts): the payload was previously passed as the event's init dictionary rather than wrapped in {data: arg} / {detail: arg}, so .on() listeners received null/undefined, and emit() returned this instead of a boolean. Also fixes the messageerror extractor (native dispatches a MessageEvent, not an ErrorEvent) and upgrades listenerCount() to read the native EventTarget listener map so addEventListener-only listeners are counted, matching Node's NodeEventTarget. ~30 lines of production change plus a 110-line test with seven sub-cases.
Security risks
None material. This is a Node-compat shim over an in-process EventTarget; no auth, crypto, or I/O boundaries. The tamper-resistance concerns raised in earlier rounds (routing listenerCount through a user-overridable .listeners duck-type; init-dict prototype-chain reads; live ErrorEvent global lookup) have all been addressed — listenerCount now calls the C++ binding directly, both init dicts are { __proto__: null, … }, and ErrorEvent is captured into a module-local const. The residual Symbol.hasInstance vector is the same class as the file's pre-existing instanceof URL/Map/Set checks and requires deliberate defineProperty on a builtin; not a regression.
Level of scrutiny
Medium. Built-in JS in src/js/ is hot-path and tamper-sensitive per REVIEW.md, and this file has an explicit tamper-resistance invariant — hence the three prior review rounds. But the change itself is a small, well-understood Node-compat correction with clear before/after semantics, thorough test coverage (identity checks for .on(), event-class + payload-slot checks for addEventListener, boolean return with/without listeners, an addEventListener-only case, and a .listeners tamper case), and passing full-file / node-compat suites per the PR evidence.
Other factors
Since my last review the only substantive commit is 2feb39f, which applied both remaining nits; e34c22d and 6f947af are empty CI retriggers. All inline threads are resolved. The two acknowledged deferrals — eventNames()/removeAllListeners() not seeing addEventListener-only listeners (pre-existing, needs a new native binding) and the Symbol.hasInstance tamper vector (same class as other instanceof uses in the file) — are reasonable scope cuts that don't regress anything. No outstanding reviewer asks.
|
CI on build #81881 has two unrelated failures:
Neither touches |
2 participants
Repro
.on("error", cb)and.on("messageerror", cb)similarly receivenull/undefinedinstead of the emitted value, andemit()returnsthisinstead of a boolean.Cause
injectFakeEmitter'semit()insrc/js/node/worker_threads.tsbuilt the event asnew MessageEvent("message", payload)/new ErrorEvent("error", err), passing the user's value as the init dictionary instead of wrapping it ({data: payload}/{error: err}). The.on()wrapper then readevent.data/event.error, which were their defaults (null). The function also ended withreturn this.The
messageerrorextractor readevent.error, but both native MessagePortmessageerrordispatch and node'sMessagePort[kCreateEvent]produce aMessageEventwhose payload is.data, so.on("messageerror", cb)would miss the value even for real deserialization failures.Fix
Construct events whose shape matches node's
NodeEventTarget/MessagePort[kCreateEvent]and round-trips through the.on()extractors:"message"/"messageerror"→new MessageEvent(type, { data: arg })"error", which MessagePort never fires natively) →new CustomEvent(type, { detail: arg })Update the
messageerrorextractor to read.data, drop the now-unusederrorEventHandler/EventClass, and returnlistenerCount(type) > 0before dispatch (node'sNodeEventTarget.prototype.emitsemantics).Verification
New test in
test/js/node/worker_threads/worker_threads.test.tscoversmessage/error/messageerror/custom events for both.on()(identity) andaddEventListener(event class +.data/.detail), plus the boolean return for the listener / no-listener cases. Fails onmainwithonArg: null, aelData: null, ret: MessagePort, passes with the fix.The full
worker_threads.test.ts(92 tests),test/js/web/workers/message-channel.test.ts, and the node-compattest-worker-message-port.js/test-event-target.js/test-messagechannel.jsall pass.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 1 rejected · iteration 2
evidence per changed file