Fire the close event on MessagePort - #32565
Closed
robobun wants to merge 9 commits into
Closed
Claude / Claude Code Review
completed
Jun 21, 2026 in 18m 35s
Code review found 2 important issues
Found 5 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 0 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/jsc/bindings/webcore/MessagePort.cpp:336-340 |
hasPendingActivity() leak: close listener added after close() pins wrapper forever |
| 🔴 Important | src/jsc/bindings/webcore/MessagePort.cpp:409-411 |
Close event never fires on unstarted peer with only a close listener; wrapper leaks |
Annotations
Check failure on line 340 in src/jsc/bindings/webcore/MessagePort.cpp
claude / Claude Code Review
hasPendingActivity() leak: close listener added after close() pins wrapper forever
Adding a `close` listener to an already-closed port pins its JS wrapper for the lifetime of the context. `close()` with no listener leaves `m_closeEventDispatched=false`, then `addEventListener('close', ...)` (which has no `m_isDetached` guard) sets `m_hasCloseEventListener=true`; this branch now returns `true` forever because it runs before the `m_isDetached` check and nothing will ever clear the flag. Either set `m_closeEventDispatched=true` in `close()`'s `!scheduledClose` fallthrough (and ga
Check failure on line 411 in src/jsc/bindings/webcore/MessagePort.cpp
claude / Claude Code Review
Close event never fires on unstarted peer with only a close listener; wrapper leaks
Adding only a `close` listener (no `message` listener, no explicit `start()`) leaves the port un-`Attached` on the pipe, so the new peer-wake block in `MessagePortPipe::close()` skips it and the peer's `close` event never fires — diverging from Node, where `port2.on('close', cb)` alone fires when `port1.close()` is called. Worse, the new `hasPendingActivity()` branch then returns `true` forever (`m_hasCloseEventListener && !isOtherSideOpen`), pinning the wrapper for the lifetime of the context —
Loading