Repository navigation
Cherry-pick 4 WebCore memory safety fixes from upstream WebKit - #28494
Conversation
WalkthroughThis pull request strengthens memory and thread safety throughout WebCore bindings by converting raw pointers to weak references in broadcast channels, adding thread-safety assertions to event listener operations, improving garbage collection visitation for abort signals, preventing message posting to closed ports, and introducing comprehensive safety test coverage for these scenarios. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
b9fdd16 to
01af732
Compare
![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 the current code and only fix it if needed.
Inline comments:
In `@docs/webcore-sync-plan.md`:
- Around line 116-135: Update the Phase 1 checklist to reflect what actually
landed: mark the four fixes that have already been merged as checked/removed so
they are not chased again; for BroadcastChannel, change the target from
channelToContextIdentifier to the actual raw-pointer map symbol
allBroadcastChannels() in BroadcastChannel.cpp; for JSAbortController, rename
the checklist hook to the exact implemented hook used in JSAbortController.cpp
(replace the incorrect visitAdditionalChildrenInGCThread with the actual
implemented visit hook name used in the current code); and ensure the
EventListenerMap entry references the correct class/method names
(EventListenerMap::releaseAssertOrSetThreadUID) and only leave entries unchecked
that truly remain outstanding.
In `@src/bun.js/bindings/webcore/EventListenerMap.h`:
- Around line 75-89: The m_threadUID field is racy because
releaseAssertOrSetThreadUID() reads/writes it before acquiring m_lock in
mutators clear(), replace(), add(), remove(), and
removeFirstEventListenerCreatedFromMarkup(); fix by either making m_threadUID an
atomic<uint32_t> and use an atomic compare-and-swap to perform the "first-touch"
set safely inside releaseAssertOrSetThreadUID(), or move the initial
check-and-assign logic so it runs while holding Lock m_lock (i.e., acquire
m_lock before testing/setting m_threadUID) so publication is protected by the
existing mutex.
In `@test/js/web/abort/abort-controller-gc-reason.test.ts`:
- Line 56: The test uses string.repeat to generate the payload; update the abort
call so the data payload is generated with Buffer.alloc(count, fill).toString()
instead of "x".repeat(100) — i.e., in the c.abort({ index: i, data: ... })
invocation replace the "x".repeat(100) expression with Buffer.alloc(100,
'x').toString() to follow test conventions.
In `@test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts`:
- Around line 55-66: Replace the fixed setTimeout waits with a condition-based
barrier: create an awaitable promise (e.g., waitForMessages or waitForDispatch)
that resolves when the test-observable (the BroadcastChannel message handler or
a counter updated by mainChannel.onmessage / worker message handler) reaches the
expected state/count, then await that promise instead of sleeping; specifically,
after sending the initial messages and again after Bun.gc(true) + posting the
100 "after-gc" messages, await the corresponding waitForMessages(expectedCount)
tied to mainChannel.onmessage (or the worker-side handler) so the test proceeds
only when the messages have actually been observed rather than after a fixed
timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d15d8d0-f962-44d9-88fd-66e29300ffdd
📥 Commits
Reviewing files that changed from the base of the PR and between 639bc43 and b9fdd16b3221b0c807f60a972c13c52dd50de5de.
📒 Files selected for processing (9)
docs/webcore-sync-plan.mdsrc/bun.js/bindings/webcore/BroadcastChannel.cppsrc/bun.js/bindings/webcore/EventListenerMap.cppsrc/bun.js/bindings/webcore/EventListenerMap.hsrc/bun.js/bindings/webcore/JSAbortController.cppsrc/bun.js/bindings/webcore/MessagePortChannel.cpptest/js/web/abort/abort-controller-gc-reason.test.tstest/js/web/broadcastchannel/broadcast-channel-worker-gc.test.tstest/js/web/workers/message-port-closed-leak.test.ts
| - [ ] **`JSAbortController::visitAdditionalChildrenInGCThread`追加** | ||
| - ファイル: `src/bun.js/bindings/webcore/JSAbortController.cpp`（要生成 or 手動実装） | ||
| - 理由: `signal().reason()`のJSValueがGCでvisitされていない可能性。メモリ破壊リスク | ||
| - 参照: `vendor/WebKit/Source/WebCore/bindings/js/JSAbortControllerCustom.cpp` | ||
|
|
||
| - [ ] **`BroadcastChannel`の`ThreadSafeWeakPtr`化** | ||
| - ファイル: `src/bun.js/bindings/webcore/BroadcastChannel.cpp:channelToContextIdentifier` | ||
| - 理由: 現在raw pointerでマップ格納。Worker終了時のUAF可能性 | ||
| - 参照: `vendor/WebKit/Source/WebCore/dom/BroadcastChannel.cpp` | ||
|
|
||
| - [ ] **`MessagePortChannel`の閉じたポートチェック（メモリリーク修正）** | ||
| - ファイル: `src/bun.js/bindings/webcore/MessagePortChannel.cpp:postMessageToRemote` | ||
| - 理由: 閉じたMessageChannelへのbuffer転送がメモリリークを引き起こす（WebKit bug 281662） | ||
| - WebKit該当行: `if (m_isClosed[i]) return;` | ||
| - WebKitコミット: `ae1111af5882` "Transferring buffers to a closed MessageChannel causes memory leaks" | ||
|
|
||
| - [ ] **`EventListenerMap::releaseAssertOrSetThreadUID`追加** | ||
| - ファイル: `src/bun.js/bindings/webcore/EventListenerMap.{cpp,h}` | ||
| - 理由: スレッド違反の早期検出。デバッグビルドでの品質向上 | ||
|
|
There was a problem hiding this comment.
Sync the Phase 1 checklist with what this PR already landed.
These entries are already stale in the same PR: the four fixes remain unchecked, the BroadcastChannel row points at channelToContextIdentifier instead of the allBroadcastChannels() raw-pointer map, and the JSAbortController row names a different hook than the implementation now uses. Please update this section so the next sync pass does not chase the wrong follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/webcore-sync-plan.md` around lines 116 - 135, Update the Phase 1
checklist to reflect what actually landed: mark the four fixes that have already
been merged as checked/removed so they are not chased again; for
BroadcastChannel, change the target from channelToContextIdentifier to the
actual raw-pointer map symbol allBroadcastChannels() in BroadcastChannel.cpp;
for JSAbortController, rename the checklist hook to the exact implemented hook
used in JSAbortController.cpp (replace the incorrect
visitAdditionalChildrenInGCThread with the actual implemented visit hook name
used in the current code); and ensure the EventListenerMap entry references the
correct class/method names (EventListenerMap::releaseAssertOrSetThreadUID) and
only leave entries unchecked that truly remain outstanding.
| void releaseAssertOrSetThreadUID() | ||
| { | ||
| if (!m_threadUID) { | ||
| ASSERT(!Thread::mayBeGCThread()); | ||
| m_threadUID = Thread::currentSingleton().uid(); | ||
| return; | ||
| } | ||
| if (m_threadUID == Thread::currentSingleton().uid()) [[likely]] | ||
| return; | ||
| RELEASE_ASSERT(Thread::mayBeGCThread()); | ||
| } | ||
|
|
||
| Vector<std::pair<AtomString, EventListenerVector>, 0, CrashOnOverflow, 4> m_entries; | ||
| Lock m_lock; | ||
| uint32_t m_threadUID { 0 }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== EventListenerMap thread UID state ==\n'
rg -n -C2 'releaseAssertOrSetThreadUID|m_threadUID' src/bun.js/bindings/webcore/EventListenerMap.h
printf '\n== Mutators calling the helper before taking m_lock ==\n'
rg -n -C2 'releaseAssertOrSetThreadUID\(\);|Locker locker \{ m_lock \};' src/bun.js/bindings/webcore/EventListenerMap.cppRepository: oven-sh/bun
Length of output: 1934
Make m_threadUID atomic or publish it under m_lock.
releaseAssertOrSetThreadUID() reads and writes m_threadUID before Locker locker { m_lock }; is acquired in all five mutators (clear(), replace(), add(), remove(), removeFirstEventListenerCreatedFromMarkup()). Since m_threadUID is a plain uint32_t, this creates a data race: concurrent threads can race on the initial read and write of m_threadUID, allowing the wrong thread to observe or set 0 before acquiring the mutex. This defeats the guard's purpose and invokes undefined behavior. Use an atomic<uint32_t> with compare-and-swap for safe first-touch publication, or move the assignment inside the existing lock.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/webcore/EventListenerMap.h` around lines 75 - 89, The
m_threadUID field is racy because releaseAssertOrSetThreadUID() reads/writes it
before acquiring m_lock in mutators clear(), replace(), add(), remove(), and
removeFirstEventListenerCreatedFromMarkup(); fix by either making m_threadUID an
atomic<uint32_t> and use an atomic compare-and-swap to perform the "first-touch"
set safely inside releaseAssertOrSetThreadUID(), or move the initial
check-and-assign logic so it runs while holding Lock m_lock (i.e., acquire
m_lock before testing/setting m_threadUID) so publication is protected by the
existing mutex.
| const controllers = []; | ||
| for (let i = 0; i < 100; i++) { | ||
| const c = new AbortController(); | ||
| c.abort({ index: i, data: "x".repeat(100) }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify string repeat usage in this test file
rg -n --type=ts '"[^"]+"\.repeat\(' test/js/web/abort/abort-controller-gc-reason.test.tsRepository: oven-sh/bun
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Search for all .repeat() usages in the file (not just string literals)
rg -n 'repeat\(' test/js/web/abort/abort-controller-gc-reason.test.tsRepository: oven-sh/bun
Length of output: 115
Replace string .repeat() test payload generation with Buffer.alloc().
At line 56, use Buffer.alloc(count, fill).toString() instead of "x".repeat(100) to match test conventions.
🔧 Proposed fix
- c.abort({ index: i, data: "x".repeat(100) });
+ c.abort({ index: i, data: Buffer.alloc(100, "x").toString() });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| c.abort({ index: i, data: "x".repeat(100) }); | |
| c.abort({ index: i, data: Buffer.alloc(100, "x").toString() }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/web/abort/abort-controller-gc-reason.test.ts` at line 56, The test
uses string.repeat to generate the payload; update the abort call so the data
payload is generated with Buffer.alloc(count, fill).toString() instead of
"x".repeat(100) — i.e., in the c.abort({ index: i, data: ... }) invocation
replace the "x".repeat(100) expression with Buffer.alloc(100, 'x').toString() to
follow test conventions.
| // Give the event loop time to process any pending dispatches. | ||
| await new Promise(resolve => setTimeout(resolve, 100)); | ||
|
|
||
| // Force GC to clean up any lingering references. | ||
| Bun.gc(true); | ||
|
|
||
| // Post more messages after GC. | ||
| for (let i = 0; i < 100; i++) { | ||
| mainChannel.postMessage("after-gc " + i); | ||
| } | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, 50)); |
There was a problem hiding this comment.
Replace the fixed sleeps with a condition-based barrier.
The 100ms/50ms waits make this regression machine-speed dependent, so it can miss the teardown window on slow CI or fail spuriously under load. Please wait on an observable completion signal instead of sleeping.
As per coding guidelines, "Do not use setTimeout in tests; instead await the condition to be met - you are testing the CONDITION, not TIME PASSING".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts` around
lines 55 - 66, Replace the fixed setTimeout waits with a condition-based
barrier: create an awaitable promise (e.g., waitForMessages or waitForDispatch)
that resolves when the test-observable (the BroadcastChannel message handler or
a counter updated by mainChannel.onmessage / worker message handler) reaches the
expected state/count, then await that promise instead of sleeping; specifically,
after sending the initial messages and again after Bun.gc(true) + posting the
100 "after-gc" messages, await the corresponding waitForMessages(expectedCount)
tied to mainChannel.onmessage (or the worker-side handler) so the test proceeds
only when the messages have actually been observed rather than after a fixed
timeout.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/bun.js/bindings/webcore/EventListenerMap.cpp (1)
76-77:⚠️ Potential issue | 🔴 CriticalAcquire
m_lockbeforereleaseAssertOrSetThreadUID()to avoid a data race.At Line 76, Line 106, Line 120, Line 147, and Line 186, the helper is called before lock acquisition. Since the helper reads/writes
m_threadUID(non-atomic), this is UB under concurrent access.🔧 Minimal fix (serialize helper under existing lock)
void EventListenerMap::clear() { - releaseAssertOrSetThreadUID(); Locker locker { m_lock }; + releaseAssertOrSetThreadUID(); @@ void EventListenerMap::replace(const AtomString& eventType, EventListener& oldListener, Ref<EventListener>&& newListener, const RegisteredEventListener::Options& options) { - releaseAssertOrSetThreadUID(); Locker locker { m_lock }; + releaseAssertOrSetThreadUID(); @@ bool EventListenerMap::add(const AtomString& eventType, Ref<EventListener>&& listener, const RegisteredEventListener::Options& options) { - releaseAssertOrSetThreadUID(); Locker locker { m_lock }; + releaseAssertOrSetThreadUID(); @@ bool EventListenerMap::remove(const AtomString& eventType, EventListener& listener, bool useCapture) { - releaseAssertOrSetThreadUID(); Locker locker { m_lock }; + releaseAssertOrSetThreadUID(); @@ void EventListenerMap::removeFirstEventListenerCreatedFromMarkup(const AtomString& eventType) { - releaseAssertOrSetThreadUID(); Locker locker { m_lock }; + releaseAssertOrSetThreadUID();Verification (read-only):
#!/bin/bash set -euo pipefail echo "== Check helper vs lock order in mutators ==" rg -n -C2 'void EventListenerMap::(clear|replace|add|remove|removeFirstEventListenerCreatedFromMarkup)\(|releaseAssertOrSetThreadUID\(\);|Locker locker \{ m_lock \};' src/bun.js/bindings/webcore/EventListenerMap.cpp echo echo "== Check m_threadUID declaration ==" rg -n -C1 '\bm_threadUID\b' src/bun.js/bindings/webcore/EventListenerMap.hAlso applies to: 106-107, 120-121, 147-148, 186-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/webcore/EventListenerMap.cpp` around lines 76 - 77, The helper releaseAssertOrSetThreadUID() accesses non-atomic m_threadUID and must be called while holding the mutex; move the call to occur after acquiring the lock (i.e., after Locker locker { m_lock };) in all mutators of EventListenerMap (clear, replace, add, remove, removeFirstEventListenerCreatedFromMarkup) so the helper runs under the existing lock protecting m_threadUID and eliminate the data race.test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts (1)
55-66:⚠️ Potential issue | 🟠 MajorReplace the fixed sleeps with a condition-based barrier.
Line 56 and Line 66 make this regression timing-dependent; on slow or contended CI, the teardown window can shift enough that the test either misses the race or fails spuriously. Please gate progress on a test-observable condition instead of elapsed time.
As per coding guidelines, "Do not use
setTimeoutin tests; insteadawaitthe condition to be met - you are testing the CONDITION, not TIME PASSING".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts` around lines 55 - 66, Replace the fixed setTimeout waits with condition-based awaits that resolve when the test-observable message counts reach expected values: add/use the existing message handler on mainChannel (or worker) to collect received messages and expose a Promise that resolves when the pre-GC dispatches have been observed, then await that Promise instead of setTimeout before calling Bun.gc(true); after calling Bun.gc(true) and posting the 100 "after-gc" messages, await a second Promise that resolves when the post-GC message counter reaches 100 (or the total expected), ensuring the test proceeds only once the observable condition (message receipt) is met rather than a fixed timeout while keeping Bun.gc(true) and mainChannel.postMessage as-is.test/js/web/abort/abort-controller-gc-reason.test.ts (1)
54-57:⚠️ Potential issue | 🟡 MinorUse
Buffer.alloc()for the repeated payload.Line 56 uses
"x".repeat(100), which goes against the repo convention for repetitive test strings.♻️ Proposed fix
- c.abort({ index: i, data: "x".repeat(100) }); + c.abort({ index: i, data: Buffer.alloc(100, "x").toString() });As per coding guidelines, "Use
Buffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/web/abort/abort-controller-gc-reason.test.ts` around lines 54 - 57, Replace the repeated string construction in the test loop with the repo-standard Buffer.alloc approach: when creating the AbortController and calling c.abort({ index: i, data: "x".repeat(100) }) (inside the for loop that pushes into controllers), use Buffer.alloc(100, "x").toString() for the data payload instead of "x".repeat(100) so the test follows the project's repetitive-string convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/bun.js/bindings/webcore/EventListenerMap.cpp`:
- Around line 76-77: The helper releaseAssertOrSetThreadUID() accesses
non-atomic m_threadUID and must be called while holding the mutex; move the call
to occur after acquiring the lock (i.e., after Locker locker { m_lock };) in all
mutators of EventListenerMap (clear, replace, add, remove,
removeFirstEventListenerCreatedFromMarkup) so the helper runs under the existing
lock protecting m_threadUID and eliminate the data race.
In `@test/js/web/abort/abort-controller-gc-reason.test.ts`:
- Around line 54-57: Replace the repeated string construction in the test loop
with the repo-standard Buffer.alloc approach: when creating the AbortController
and calling c.abort({ index: i, data: "x".repeat(100) }) (inside the for loop
that pushes into controllers), use Buffer.alloc(100, "x").toString() for the
data payload instead of "x".repeat(100) so the test follows the project's
repetitive-string convention.
In `@test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts`:
- Around line 55-66: Replace the fixed setTimeout waits with condition-based
awaits that resolve when the test-observable message counts reach expected
values: add/use the existing message handler on mainChannel (or worker) to
collect received messages and expose a Promise that resolves when the pre-GC
dispatches have been observed, then await that Promise instead of setTimeout
before calling Bun.gc(true); after calling Bun.gc(true) and posting the 100
"after-gc" messages, await a second Promise that resolves when the post-GC
message counter reaches 100 (or the total expected), ensuring the test proceeds
only once the observable condition (message receipt) is met rather than a fixed
timeout while keeping Bun.gc(true) and mainChannel.postMessage as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4935520c-9027-439e-97b6-a3d78746927f
📥 Commits
Reviewing files that changed from the base of the PR and between b9fdd16b3221b0c807f60a972c13c52dd50de5de and 9bfffb1.
📒 Files selected for processing (8)
src/bun.js/bindings/webcore/BroadcastChannel.cppsrc/bun.js/bindings/webcore/EventListenerMap.cppsrc/bun.js/bindings/webcore/EventListenerMap.hsrc/bun.js/bindings/webcore/JSAbortController.cppsrc/bun.js/bindings/webcore/MessagePortChannel.cpptest/js/web/abort/abort-controller-gc-reason.test.tstest/js/web/broadcastchannel/broadcast-channel-worker-gc.test.tstest/js/web/workers/message-port-closed-leak.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fixes are clean surgical backports with good test coverage, but these changes touch critical C++ GC marking and threading paths (JSAbortController::visitChildrenImpl, ThreadSafeWeakPtr map, RELEASE_ASSERT in EventListenerMap) that warrant human eyes before landing.
Extended reasoning...
Overview
This PR cherry-picks 4 memory safety fixes from upstream WebKit: drop messages to closed MessagePort, visit signal.reason in GC for JSAbortController, use ThreadSafeWeakPtr in BroadcastChannel global map, and add per-instance thread affinity assertions to EventListenerMap. A docs file tracking future WebCore sync work is also added.
Security / Correctness Risks
All four changes address real memory safety bugs (UAF, GC premature collection, memory leaks), so they are net improvements. The main correctness risk is in the backport itself. The JSAbortController change calls thisObject->wrapped().signal().reason().visit(visitor) from the GC thread - reason() must be safe to access without JSLock; upstream added this line so it is presumed safe, but worth verifying against Bun's fork divergence. The EventListenerMap change adds RELEASE_ASSERT(Thread::mayBeGCThread()) which will crash in production if any non-GC, non-owning thread calls a mutation method - correct by design but any missed code path would be a hard crash.
Level of Scrutiny
These changes are minimal and directly mirror confirmed upstream WebKit fixes with bug IDs cited. However, GC marking changes and threading assertions in C++ are high-stakes: a wrong assumption causes either memory corruption or a process crash. A quick human verification that the backport context matches Bun's fork correctly is warranted.
Other Factors
The only automated bug found is a nit: the BroadcastChannel test uses setTimeout instead of condition-based synchronization. The report itself acknowledges this is unavoidable since worker.terminate() has no observable completion event from JS, and the test cannot produce false failures - only potentially reduced sensitivity on fast machines. This should not block the PR.
|
|
||
| // Give the event loop time to process any pending dispatches. | ||
| await new Promise(resolve => setTimeout(resolve, 100)); | ||
|
|
||
| // Force GC to clean up any lingering references. | ||
| Bun.gc(true); | ||
|
|
||
| // Post more messages after GC. | ||
| for (let i = 0; i < 100; i++) { | ||
| mainChannel.postMessage("after-gc " + i); | ||
| } | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, 50)); | ||
|
|
||
| mainChannel.close(); |
There was a problem hiding this comment.
🟡 The first test in broadcast-channel-worker-gc.test.ts uses setTimeout(resolve, 100) and setTimeout(resolve, 50) (lines 56 and 66) inside the subprocess script, violating the CLAUDE.md guideline: "never wait for time to pass in tests." While there is no existing synchronization primitive to replace these delays (since worker.terminate() is fire-and-forget), the test could be made more robust by increasing the message count rather than relying on time windows.
Extended reasoning...
The test/CLAUDE.md file explicitly states: "Do not write flaky tests. Unless explicitly asked, never wait for time to pass in tests. Always wait for the condition to be met instead of waiting for an arbitrary amount of time." The subprocess script in broadcast-channel-worker-gc.test.ts (the first test) uses await new Promise(resolve => setTimeout(resolve, 100)) after terminating workers and await new Promise(resolve => setTimeout(resolve, 50)) after posting more messages, which is a direct violation of this rule in newly-added test code.
One refutation argues that the setTimeout lives inside a -e script string passed to bunExe() rather than directly in the bun:test framework, and thus the CLAUDE.md rule doesn’t apply. However, this is still test code authored for this PR, and the spirit of the guideline — avoid time-based synchronization in test infrastructure — clearly applies to subprocess scripts used as test harnesses.
A stronger point from the refutation is that there is genuinely no synchronization primitive available to replace the delays. The Worker API provides no “terminated” event observable from JS, and worker.terminate() is fire-and-forget. This means the condition-based alternative recommended by CLAUDE.md literally does not exist for this particular race scenario.
The practical impact is also bounded: the test asserts exitCode == 0, stderr == "", and stdout.trim() == "OK". The console.log("OK") is reached unconditionally at the end of the script (assuming no crash). So the test cannot produce false failures from timing — the worst case is that the race window is not stressed adequately on very fast machines, reducing the test’s ability to detect regressions (false passes), not causing spurious failures.
Step-by-step proof of the violation: (1) Worker threads are terminated via worker.terminate() with no observable completion event. (2) The script then calls await new Promise(resolve => setTimeout(resolve, 100)) — a hardcoded 100 ms pause — hoping workers have torn down. (3) On an extremely fast machine or lightly loaded CI, the GC + race window may complete well within 100 ms, meaning the race is never exercised. Conversely the 100 ms is arbitrary and not tied to any condition. (4) The same pattern repeats with setTimeout(resolve, 50) after the post-GC messages. Both delays are arbitrary time budgets with no fallback condition.
A conformant fix would remove the time-based delays and instead rely on a higher message count (e.g., 500 post/terminate cycles) to increase the statistical probability of catching the race, combined with the existing Bun.gc(true) calls which are synchronous. Alternatively, a comment explaining why no condition-based wait is possible would at minimum document the deliberate exception to the rule.
Follow-up to #28494. Backports WebKit 276563 which removed the Strong/Weak split in favor of a single Weak-only JSCallbackData class, making it structurally impossible to introduce Strong callback handles (a recurring source of GC reference cycles). The prerequisite AbortSignal::m_algorithms restructuring that #28494 deferred this work for was already completed in #28491, so this is now a pure rename + dead code removal with no behavior change. Also deletes ErrorCallback/JSErrorCallback (FileSystemEntry API callback, never used in Bun).
…28539) ## What Follow-up to #28494. Backports WebKit [276563](https://bugs.webkit.org/show_bug.cgi?id=276563) ("Make all callbacks Weak handles"), which removed `JSCallbackDataStrong`/`JSCallbackDataWeak` in favor of a single Weak-only `JSCallbackData` class. ## Why `JSC::Strong` handles in callback wrappers have been a recurring source of GC reference cycles (see #28491 for the `pipeTo + signal` leak). Upstream WebKit solved this structurally in 2024-07 by deleting the Strong variant entirely, making such leaks impossible to introduce by construction. \#28494 deferred this backport because it "requires restructuring `AbortSignal::m_algorithms` to allow GC visitation." That restructuring was already done in #28491 (`m_abortAlgorithms` + `visitAbortAlgorithms()` + Lock), so the prerequisite is satisfied and this is now a pure rename + dead code removal. ## Changes ### `JSCallbackData.h` / `.cpp` - Merged `JSCallbackDataWeak` into the base `JSCallbackData` class - Deleted `JSCallbackDataStrong` - Replaced `Strong.h`/`StrongInlines.h` includes with `Weak.h`/`WeakInlines.h` - Kept Bun's existing `invokeCallback(VM&, ...)` signature and `visitJSFunction` method name (not upstream's `visitJSFunctionInGCThread`) to avoid touching base class overrides — this is a structural sync, not a full API sync ### `JSAbortAlgorithm`, `JSPerformanceObserverCallback` - Type rename only: `JSCallbackDataWeak*` → `JSCallbackData*` - No semantic change (both were already Weak) ### Deleted: `ErrorCallback.{h,cpp,idl}`, `JSErrorCallback.{h,cpp}` - FileSystemEntry API callback (`webkitGetAsEntry()` etc.), never implemented in Bun - Zero `create()` call sites, zero `#include` references outside itself - Was the only remaining `JSCallbackDataStrong` user - Upstream still has `ErrorCallback` under `Modules/entriesapi/`, but with `[GenerateIsReachable]` which generates a Weak callback — our copy predated that and was a stale Strong version ## Verification - `nm build/debug/bun-debug | grep -cE "JSCallbackData(Strong|Weak)"` → `0` - `test/js/web/abort/` — pass - `test/js/web/streams/pipeTo-signal-leak.test.ts` — pass (regression check for #28491) - `test/js/web/timers/performance-entries.test.ts` — pass - `BUN_JSC_validateExceptionChecks=1` — pass - GC stress: 100 AbortSignal callbacks + 50× `Bun.gc(true)` → all 100 fire (opaque-root reachability preserved) - GC stress: 50 PerformanceObserver callbacks + 30× `Bun.gc(true)` → all 50 fire --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
…sh#28494) ## What Cherry-picks 4 memory safety fixes from upstream WebKit into `src/bun.js/bindings/webcore/`. ## Why Bun's WebCore code was forked from WebKit ~2022-03 and has since missed several GC/memory safety fixes. These are minimal, surgical backports of confirmed bug fixes. ## Changes ### 1. `MessagePortChannel`: drop messages to closed ports **Upstream:** WebKit [281662](https://bugs.webkit.org/show_bug.cgi?id=281662) Messages sent to a closed `MessagePort` were queued indefinitely in `m_pendingMessages`, never to be delivered. **Measured:** 5000 × 64KB `postMessage` to closed port: RSS growth 332MB → 1.5MB ### 2. `JSAbortController`: visit `signal.reason` in GC **Upstream:** WebKit [293319](https://bugs.webkit.org/show_bug.cgi?id=293319) `controller.signal.reason` was not marked during GC when only the controller was retained, causing `reason` to become `undefined` after collection. ### 3. `BroadcastChannel`: use `ThreadSafeWeakPtr` in global map **Upstream:** WebKit [267883](https://bugs.webkit.org/show_bug.cgi?id=267883) The global channel map held raw `BroadcastChannel*` pointers. If a worker-owned channel was being destroyed while the main thread looked it up, the resulting `RefPtr` assignment could race. `ThreadSafeWeakPtr::get()` atomically checks liveness via the control block. ### 4. `EventListenerMap`: per-instance thread affinity assert **Upstream:** WebKit [292397](https://bugs.webkit.org/show_bug.cgi?id=292397) + [304961](https://bugs.webkit.org/show_bug.cgi?id=304961) Adds `releaseAssertOrSetThreadUID()` to mutation operations. Unlike a global `isMainThread()` check, this records the owning thread per-instance, so worker-owned `EventTarget`s work correctly. GC thread sweeps are exempted. ## Tests - `test/js/web/abort/abort-controller-gc-reason.test.ts` - `test/js/web/workers/message-port-closed-leak.test.ts` - `test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts` All 270 tests in `test/js/web/{abort,workers,broadcastchannel}/` + `test/js/deno/event/` pass. ## Not in this PR `JSCallbackDataStrong` removal (WebKit [276563](https://bugs.webkit.org/show_bug.cgi?id=276563)) requires restructuring `AbortSignal::m_algorithms` to allow GC visitation. Deferred to a follow-up. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…ven-sh#28539) ## What Follow-up to oven-sh#28494. Backports WebKit [276563](https://bugs.webkit.org/show_bug.cgi?id=276563) ("Make all callbacks Weak handles"), which removed `JSCallbackDataStrong`/`JSCallbackDataWeak` in favor of a single Weak-only `JSCallbackData` class. ## Why `JSC::Strong` handles in callback wrappers have been a recurring source of GC reference cycles (see oven-sh#28491 for the `pipeTo + signal` leak). Upstream WebKit solved this structurally in 2024-07 by deleting the Strong variant entirely, making such leaks impossible to introduce by construction. \oven-sh#28494 deferred this backport because it "requires restructuring `AbortSignal::m_algorithms` to allow GC visitation." That restructuring was already done in oven-sh#28491 (`m_abortAlgorithms` + `visitAbortAlgorithms()` + Lock), so the prerequisite is satisfied and this is now a pure rename + dead code removal. ## Changes ### `JSCallbackData.h` / `.cpp` - Merged `JSCallbackDataWeak` into the base `JSCallbackData` class - Deleted `JSCallbackDataStrong` - Replaced `Strong.h`/`StrongInlines.h` includes with `Weak.h`/`WeakInlines.h` - Kept Bun's existing `invokeCallback(VM&, ...)` signature and `visitJSFunction` method name (not upstream's `visitJSFunctionInGCThread`) to avoid touching base class overrides — this is a structural sync, not a full API sync ### `JSAbortAlgorithm`, `JSPerformanceObserverCallback` - Type rename only: `JSCallbackDataWeak*` → `JSCallbackData*` - No semantic change (both were already Weak) ### Deleted: `ErrorCallback.{h,cpp,idl}`, `JSErrorCallback.{h,cpp}` - FileSystemEntry API callback (`webkitGetAsEntry()` etc.), never implemented in Bun - Zero `create()` call sites, zero `#include` references outside itself - Was the only remaining `JSCallbackDataStrong` user - Upstream still has `ErrorCallback` under `Modules/entriesapi/`, but with `[GenerateIsReachable]` which generates a Weak callback — our copy predated that and was a stale Strong version ## Verification - `nm build/debug/bun-debug | grep -cE "JSCallbackData(Strong|Weak)"` → `0` - `test/js/web/abort/` — pass - `test/js/web/streams/pipeTo-signal-leak.test.ts` — pass (regression check for oven-sh#28491) - `test/js/web/timers/performance-entries.test.ts` — pass - `BUN_JSC_validateExceptionChecks=1` — pass - GC stress: 100 AbortSignal callbacks + 50× `Bun.gc(true)` → all 100 fire (opaque-root reachability preserved) - GC stress: 50 PerformanceObserver callbacks + 30× `Bun.gc(true)` → all 50 fire --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
…sh#28494) ## What Cherry-picks 4 memory safety fixes from upstream WebKit into `src/bun.js/bindings/webcore/`. ## Why Bun's WebCore code was forked from WebKit ~2022-03 and has since missed several GC/memory safety fixes. These are minimal, surgical backports of confirmed bug fixes. ## Changes ### 1. `MessagePortChannel`: drop messages to closed ports **Upstream:** WebKit [281662](https://bugs.webkit.org/show_bug.cgi?id=281662) Messages sent to a closed `MessagePort` were queued indefinitely in `m_pendingMessages`, never to be delivered. **Measured:** 5000 × 64KB `postMessage` to closed port: RSS growth 332MB → 1.5MB ### 2. `JSAbortController`: visit `signal.reason` in GC **Upstream:** WebKit [293319](https://bugs.webkit.org/show_bug.cgi?id=293319) `controller.signal.reason` was not marked during GC when only the controller was retained, causing `reason` to become `undefined` after collection. ### 3. `BroadcastChannel`: use `ThreadSafeWeakPtr` in global map **Upstream:** WebKit [267883](https://bugs.webkit.org/show_bug.cgi?id=267883) The global channel map held raw `BroadcastChannel*` pointers. If a worker-owned channel was being destroyed while the main thread looked it up, the resulting `RefPtr` assignment could race. `ThreadSafeWeakPtr::get()` atomically checks liveness via the control block. ### 4. `EventListenerMap`: per-instance thread affinity assert **Upstream:** WebKit [292397](https://bugs.webkit.org/show_bug.cgi?id=292397) + [304961](https://bugs.webkit.org/show_bug.cgi?id=304961) Adds `releaseAssertOrSetThreadUID()` to mutation operations. Unlike a global `isMainThread()` check, this records the owning thread per-instance, so worker-owned `EventTarget`s work correctly. GC thread sweeps are exempted. ## Tests - `test/js/web/abort/abort-controller-gc-reason.test.ts` - `test/js/web/workers/message-port-closed-leak.test.ts` - `test/js/web/broadcastchannel/broadcast-channel-worker-gc.test.ts` All 270 tests in `test/js/web/{abort,workers,broadcastchannel}/` + `test/js/deno/event/` pass. ## Not in this PR `JSCallbackDataStrong` removal (WebKit [276563](https://bugs.webkit.org/show_bug.cgi?id=276563)) requires restructuring `AbortSignal::m_algorithms` to allow GC visitation. Deferred to a follow-up. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…ven-sh#28539) ## What Follow-up to oven-sh#28494. Backports WebKit [276563](https://bugs.webkit.org/show_bug.cgi?id=276563) ("Make all callbacks Weak handles"), which removed `JSCallbackDataStrong`/`JSCallbackDataWeak` in favor of a single Weak-only `JSCallbackData` class. ## Why `JSC::Strong` handles in callback wrappers have been a recurring source of GC reference cycles (see oven-sh#28491 for the `pipeTo + signal` leak). Upstream WebKit solved this structurally in 2024-07 by deleting the Strong variant entirely, making such leaks impossible to introduce by construction. \oven-sh#28494 deferred this backport because it "requires restructuring `AbortSignal::m_algorithms` to allow GC visitation." That restructuring was already done in oven-sh#28491 (`m_abortAlgorithms` + `visitAbortAlgorithms()` + Lock), so the prerequisite is satisfied and this is now a pure rename + dead code removal. ## Changes ### `JSCallbackData.h` / `.cpp` - Merged `JSCallbackDataWeak` into the base `JSCallbackData` class - Deleted `JSCallbackDataStrong` - Replaced `Strong.h`/`StrongInlines.h` includes with `Weak.h`/`WeakInlines.h` - Kept Bun's existing `invokeCallback(VM&, ...)` signature and `visitJSFunction` method name (not upstream's `visitJSFunctionInGCThread`) to avoid touching base class overrides — this is a structural sync, not a full API sync ### `JSAbortAlgorithm`, `JSPerformanceObserverCallback` - Type rename only: `JSCallbackDataWeak*` → `JSCallbackData*` - No semantic change (both were already Weak) ### Deleted: `ErrorCallback.{h,cpp,idl}`, `JSErrorCallback.{h,cpp}` - FileSystemEntry API callback (`webkitGetAsEntry()` etc.), never implemented in Bun - Zero `create()` call sites, zero `#include` references outside itself - Was the only remaining `JSCallbackDataStrong` user - Upstream still has `ErrorCallback` under `Modules/entriesapi/`, but with `[GenerateIsReachable]` which generates a Weak callback — our copy predated that and was a stale Strong version ## Verification - `nm build/debug/bun-debug | grep -cE "JSCallbackData(Strong|Weak)"` → `0` - `test/js/web/abort/` — pass - `test/js/web/streams/pipeTo-signal-leak.test.ts` — pass (regression check for oven-sh#28491) - `test/js/web/timers/performance-entries.test.ts` — pass - `BUN_JSC_validateExceptionChecks=1` — pass - GC stress: 100 AbortSignal callbacks + 50× `Bun.gc(true)` → all 100 fire (opaque-root reachability preserved) - GC stress: 50 PerformanceObserver callbacks + 30× `Bun.gc(true)` → all 50 fire --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
What
Cherry-picks 4 memory safety fixes from upstream WebKit into
src/bun.js/bindings/webcore/.Why
Bun's WebCore code was forked from WebKit ~2022-03 and has since missed several GC/memory safety fixes. These are minimal, surgical backports of confirmed bug fixes.
Changes
1.
MessagePortChannel: drop messages to closed portsUpstream: WebKit 281662
Messages sent to a closed
MessagePortwere queued indefinitely inm_pendingMessages, never to be delivered.Measured: 5000 × 64KB
postMessageto closed port: RSS growth 332MB → 1.5MB2.
JSAbortController: visitsignal.reasonin GCUpstream: WebKit 293319
controller.signal.reasonwas not marked during GC when only the controller was retained, causingreasonto becomeundefinedafter collection.3.
BroadcastChannel: useThreadSafeWeakPtrin global mapUpstream: WebKit 267883
The global channel map held raw
BroadcastChannel*pointers. If a worker-owned channel was being destroyed while the main thread looked it up, the resultingRefPtrassignment could race.ThreadSafeWeakPtr::get()atomically checks liveness via the control block.4.
EventListenerMap: per-instance thread affinity assertUpstream: WebKit 292397 + 304961
Adds
releaseAssertOrSetThreadUID()to mutation operations. Unlike a globalisMainThread()check, this records the owning thread per-instance, so worker-ownedEventTargets work correctly. GC thread sweeps are exempted.Tests
test/js/web/abort/abort-controller-gc-reason.test.tstest/js/web/workers/message-port-closed-leak.test.tstest/js/web/broadcastchannel/broadcast-channel-worker-gc.test.tsAll 270 tests in
test/js/web/{abort,workers,broadcastchannel}/+test/js/deno/event/pass.Not in this PR
JSCallbackDataStrongremoval (WebKit 276563) requires restructuringAbortSignal::m_algorithmsto allow GC visitation. Deferred to a follow-up.