Bump WebKit (oven-sh/WebKit#681 preview): a microtask checkpoint clears the stack its frames use - #42975
Bump WebKit (oven-sh/WebKit#681 preview): a microtask checkpoint clears the stack its frames use#42975robobun wants to merge 2 commits into
Conversation
…vent loop turn Every event loop turn that resumes an async function runs a microtask checkpoint at the same stack depth. A job writes only some slots of the frames of the drain and of runInternalMicrotask(), so what a job of an earlier turn left there was a root of every Bun.gc(true) made from a later turn. The fixture makes garbage in an async function, then collects from later turns, in the module's continuation and in an async function, with turns via setTimeout, setImmediate and Bun.sleep.
A microtask checkpoint that has jobs to run first clears the stack that the frames of the drain, of runInternalMicrotask() and of the job are going to occupy. What a job of an earlier event loop turn left in those frames is then no longer a root of a collection made from a later turn. The pin is autobuild-preview-pr-681-98326556: c28156899e plus the one commit of that PR.
|
Status Closed without merge. The reasons and the evidence are in the closing comment below.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe change pins WebKit to a preview build containing the microtask checkpoint fix. It adds a Bun regression test that checks stale stack roots do not retain objects across event-loop turns. ChangesMicrotask checkpoint garbage collection
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The WebKit pin and its regression coverage have no actionable current-head issue identified, so the change is ready to merge after normal checks. 🚥 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.
Beyond the inline note on the preview-tag pin, I also checked the new test's plumbing: BUN_GC_TIMER_DISABLE is consumed by src/jsc/GarbageCollectionController.rs, each case runs in a fresh subprocess with pipes drained concurrently, and the cleanup is registered via using before the assertions — no issues there. The engine change itself (oven-sh/WebKit#681) is outside this repo, so a human should confirm it has merged and swap the pin to the merged SHA before landing.
Extended reasoning...
The only substantive Bun-side content is a version pin and a new regression test; the behavioral fix lives in an external WebKit PR that cannot be inspected from this checkout. The inline finding already covers the ephemeral pin. Separately I verified the test follows harness conventions (spread bunEnv, tempDir, describe.concurrent, Promise.all over stdout/stderr/exited, output asserted before exit code) and that the env knob it sets is actually read by the GC controller, so the test is not vacuous on that axis. The remaining risk is the upstream engine change and its merge status, which a human needs to check.
|
On the review of the pin: agreed. This PR must not merge while |
|
Updated 2:08 PM PT - Sep 16th, 2026
❌ @robobun, your commit 008752a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 42975That installs a local version of the PR into your bun-42975 --bun |
|
Closing this PR. Three reasons:
The analysis stays here and in oven-sh/WebKit#681 for reference. I closed that PR too. |
1 participant
Problem
Bun.gc(true)made from later event loop turns. On main the new test fails 2 of 6 cases (release build) and 3 of 6 (debug ASAN build).runInternalMicrotask()is at the same address each time. A job writes only some of its slots. The stale word holds the generator of a finished async function.Fix
WEBKIT_VERSIONto the preview build of its head,autobuild-preview-pr-681-98326556. No change undersrc/.autobuild-<merge sha>release.test/js/bun/gc/microtask-checkpoint-stale-stack.test.tspasses 6 of 6 withbun bd test, and fails 3 of 6 withBUN_JSC_clearStackForMicrotaskCheckpoint=0. Self-reviewed: 5 concerns raised, 5 addressed.Background
awaitcontinues there.sanitizeStackForVM()zeroes the stack below its caller. It cannot reach a live frame.Notes
The pin is
c28156899e, the current pin, plus the one commit of oven-sh/WebKit#681.Scope. The object that stays is the last one that went through the slot. With default settings it goes away when other work overwrites the word, for example the collector's 1 s timer. So this is about collections that are deterministic (leak tests, code that polls a
WeakRefor aFinalizationRegistry), not about unbounded growth.The test. A fixture makes two objects in an async function that awaits one event loop turn per object. Then it calls
Bun.gc(true)from later turns until theWeakRefs are empty, for at most 8 turns. Six cases: the collections are made by the module (top-level await) or by an async function, and a turn issetTimeout,setImmediateorBun.sleep(1). Each case is a fresh process withBUN_GC_TIMER_DISABLE=1.c6b7fcb5b, release (USE_SYSTEM_BUN=1)Bun.sleepcases,{"alive":2,"collections":8}bun bd(debug ASAN, WebKitc28156899ewith #673)bun bdbun bd,BUN_JSC_clearStackForMicrotaskCheckpoint=0--profile=release, the lto prebuilt)BUN_JSC_clearStackForMicrotaskCheckpoint=0a8e4e9042, Windows x64Bun.sleepcasesWhich case shows it moves with the compiler: with clang 21 the
setTimeoutcases failed on the release build too.The two artifacts (release build
c6b7fcb5b,BUN_GC_TIMER_DISABLE=1, the module case withBun.sleep(1)):generateHeapSnapshotForDebugging()after five collections: the objects are held by the function'slist, held by aJSLexicalEnvironment, held by theAsyncFunctionGeneratorof the finished function. The generator has no incoming edge, and norootsentry reaches any of them.ConservativeRoots::genericAddSpan<CompositeMarkHook>during the next collection, search of the scanned span (10,432 bytes) for the five cell addresses: one hit, the generator. I walked the frame-pointer chain by hand. The word is atrbp-64of the 272-byte frame ofrunInternalMicrotask(), underdrainWithUseCallOnEachMicrotask()andVM::drainMicrotasks(). The job that runs isAsyncModuleExecutionResume, which does not write that slot.Earlier sightings of this frame: #37853 (the same generator word), #41607 (darwin arm64 release build). #42877 saw ASAN redzone words of it, which oven-sh/WebKit#673 now covers.
The alternative in Bun. #37853 calls
sanitizeStackForVM()once per event loop tick. I tried that on a build of this branch with the engine option off: it collects the same six cases at turn 1, and costs the same (2522 ms against 2525 ms per 1 MsetImmediateturns, 2504 ms with neither). Differences: it needs something to have sanitized from below the stale word since the word was written, it clears per tick and not per checkpoint, and it also covers Bun's own frames and entries that are not checkpoints. The two do not exclude each other. The test in this PR passes with either.Cost (Xeon 8375C, release build, clang 23): a checkpoint with one job goes from 101 ns to 122 ns in the
jscshell. In Bun, 1 M turns ofawait new Promise(r => setImmediate(r))take 2531 ms without the clear and 2547 ms with it (medians of 11, same binary). 20 M awaits in one checkpoint: 602 ms and 602 ms.Also run with
bun bdon this branch:test/js/web/timers/microtask.test.js,test/js/bun/net/socket-retention.test.ts,test/js/web/fetch/fetch-stream-cancel-leak.test.ts,test/js/bun/http/serve-pending-promise-abort-leak.test.ts,test/js/web/streams/streams.test.js: all pass, and the new engine assertion (a job of a checkpoint runs inside the cleared window) does not fire.Self-review of the engine change. It asked for five things, and oven-sh/WebKit#681 has them: a rebase onto oven-sh/WebKit#673, the scope moved from the ASAN lane to normal slots, the code comment no longer names redzones, an assertion that a job runs inside the cleared window, and the comparison with #37853 above.
The fail-before proof for this PR is the stock canary and the engine option, not a checkout of
src/from main. The fix is in the WebKit prebuilt thatscripts/build/deps/webkit.tsnames.[policy-decision:webkit] gate passed · iteration 0 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file