Repository navigation
Conversation
…ng to occupy A microtask checkpoint is where a turn of the embedder's event loop resumes an async function: a timer or an I/O callback settles a promise, and the queue is drained. Every turn does that from the same stack depth, so the frames of the checkpoint (MicrotaskQueue::drain*(), runInternalMicrotask(), the job's, the entry to JS) are at the same addresses in every turn. They stay for as long as the turn's JS runs, and they lie over whatever ran at that depth since the last checkpoint: that checkpoint's own frames, or the embedder's. What they do not write is in reach of the conservative scan of every collection made from under them, and sanitizeStackForVM(), which such a collection calls, clears only what is below the frame that calls it. #673 took the words that ASan poisons (redzones, locals out of scope) out of the scan. This is about the words it cannot take out: spill slots and locals of a live frame that this job's path through the function does not write. Builds without ASan have only those, and ASan builds have them too. runInternalMicrotask() is one switch over every kind of job, with callMicrotask() inlined into several arms, and a job uses one arm. Seen as: what a finished async function held is not collected by collections made from later turns. async function makeGarbage() { /* two objects in a list, one await per object */ } await makeGarbage(); for (;;) { await turn(); fullGC(); } // turn(): a promise that a timer resolves jsc shell of c281568 (linux amd64, lto): 32 turns until the objects are collected (the loop tiers up and the frames change), 60 of 60 turns not collected with --useJIT=0. Bun at that WebKit (release, x86_64): not collected in 8 of 8 turns when the timer is Bun.sleep(1); Bun's debug ASan build, which has #673: not collected in 8 of 8 turns for the module shape with any timer. A heap snapshot has the objects held by `list`, held by the lexical environment of the finished function's JSAsyncFunctionGenerator, which has no incoming edge and no root. In a debugger, the one word of the scanned span that holds the generator's address is at rbp-64 of runInternalMicrotask()'s frame (272 bytes), under drainWithUseCallOnEachMicrotask() (624 bytes). The job that runs in the later turns (AsyncModuleExecutionResume for the module, AsyncFunctionResume when the loop is in an async function of its own) does not write that slot. Which shape shows it moves with the compiler: the same script with setTimeout() kept the objects with clang 21 and does not with clang 23. Earlier sightings of this frame in Bun's leak tests: oven-sh/bun#37853 (the same generator word), oven-sh/bun#41607 (darwin arm64, release). So a checkpoint that has jobs to run first clears MicrotaskQueue::stackBytesClearedForCheckpoint bytes of the stack below performMicrotaskCheckpoint(), before the first of those frames exists: 2 KB in a release build (from there to the JS frame is 1.5 KB for x86_64), 32 KB with assertions or ASan (12 KB with both). It does that with a function that is never inlined and whose frame is an array of that size, which it zeroes with memset(): that frame lies exactly where the caller's next callee has its own, on every ABI, and the compiler probes the stack for it where a platform needs that. The array's address goes through an empty asm statement. Without it the stores are dead to the compiler (zeroBytes() and secureZeroBytes() both compile to a bare `ret` here: the memory clobber of secureZeroSpan() does not keep stores to a local whose address does not escape). callMicrotask() asserts that it runs within seven eighths of the window below the checkpoint's MicrotaskCallCache, which is a local of drainImpl(). Not sanitizeStackForVM() at the checkpoint: it clears from where it was last called, and nothing need have called it from below the checkpoint since. The jsc shell does not change with it (JSLock::didAcquireLock() resets VM::m_lastStackTop for every task). Not sanitizeStackForVMImpl() with m_lastStackTop lowered to the bottom of the window, which was the first version: its loop for x86_64 stores 8 bytes per iteration, and a checkpoint with one job went from 106 ns to 195 ns. Cost, Xeon 8375C, clang 23: `promise.then(noop); drainMicrotasks()` 5 M times in the jsc shell, 101 ns per iteration without the clear and 122 ns with it. Bun, 1 M turns of `await new Promise(r => setImmediate(r))`: 2531 ms without and 2547 ms with (medians of 11, same binary, the option). 20 M awaits in one checkpoint: 602 ms and 602 ms. An empty checkpoint clears nothing. Once per checkpoint and not once per job, which would cost every job that much: what one job leaves is still there for the later jobs of the same checkpoint, and is gone at the next one. Not when the window would reach below the soft stack limit. An embedder that holds the API lock for the life of its thread can get much of this from a sanitizeStackForVM() per event loop turn instead (oven-sh/bun#37853): in Bun that collects the same six cases at the same cost. It depends on something having sanitized from below the stale word since it was written, clears per turn and not per checkpoint, and covers the embedder's own frames as well. The two do not exclude each other. Options::clearStackForMicrotaskCheckpoint (default on) is for comparisons. Tests: JSTests/modules/microtask-checkpoint-clears-its-stack.js (an async function, then the module) and JSTests/stress/microtask-checkpoint-clears-its-stack.js (a promise reaction, then an async generator, each followed by an async function). Both fail in the shell of c281568 and with --clearStackForMicrotaskCheckpoint=0, and pass in the 28 configurations that run-javascriptcore-tests runs them in (release, x86_64).
|
Preview build of 9832655: |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked two things and ruled them out: the GNU-style __asm__ volatile in clearStackBelowCaller builds on the Windows lanes, since Dockerfile.windows compiles with clang-cl (JSLock.cpp and JSObject.cpp already use the same form), and the 2 KB release frame is under the 4 KB page so it needs no __chkstk probe there (the 32 KB assert/ASan frame gets one from the compiler). The softStackLimit() guard in clearStackForCheckpoint keeps the window out of the reserved zone, so the clear cannot itself trip the stack guard.
Extended reasoning...
A finding is posted inline, so this body only records what else was examined. The inline-asm portability question mattered because the Windows lanes are cross-compiled and MSVC proper would reject __asm__; Dockerfile.windows shows clang-cl is the compiler and the same construct already exists in runtime/JSLock.cpp and runtime/JSObject.cpp. The soft-stack-limit check in MicrotaskQueue.cpp bounds the window before the frame is created, so a checkpoint near the end of the stack skips the clear rather than faulting. This is informational only and not a guarantee of correctness; a human should still weigh the unvalidated release window size raised inline.
| #if ASSERT_ENABLED || ASAN_ENABLED | ||
| static constexpr size_t stackBytesClearedForCheckpoint = 32 * KB; | ||
| #else | ||
| static constexpr size_t stackBytesClearedForCheckpoint = 2 * KB; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Windows x64 release users may keep getting the leak this change fixes on Linux, if their frame chain exceeds the unchecked 2 KB window. The release window at MicrotaskQueue.h:275 is validated by nothing: the assert at JSMicrotask.cpp:102 exists only in builds that use the 32 KB window. The chain measured 1520 of 2048 bytes on Linux x64; the MS ABI adds a 32-byte home area per call, stack-passed arguments past four, and xmm6-15 saves in the entry thunk, and the Windows lane is not tested. Fix: verify the release window on every shipped ABI or size it with margin (e.g. 4 KB), and let an option run the depth check in release so the Windows lane can confirm it. [also at: Source/JavaScriptCore/runtime/MicrotaskQueueInlines.h:144 - Bun users who poll WeakRef or FinalizationRegistry after a GC still get objects that survive every turn when the stale word sits deeper than the window, and the PR presents the leak as fixed.]
A small fix can ride a push you are already making; otherwise a short reply is enough.
Extended reasoning...
The PR measured the frames from performMicrotaskCheckpoint() to the JS frame on Linux x86_64 with clang 23: 624 + 272 + 64 + 160 + 224 + 176 = 1520 bytes, a 528-byte margin under stackBytesClearedForCheckpoint = 2 * KB (MicrotaskQueue.h:275). No release build checks that figure: assertJobOfCheckpointRunsInClearedStack (JSMicrotask.cpp:97-103) is compiled only under ASSERT_ENABLED, and in every such build the window is 32 KB (MicrotaskQueue.h:272-273), so the assertion never says anything about the 2 KB constant that ships. On Windows x64 (clang-cl, MS ABI, lanes.mjs:150-173) each non-leaf frame carries a 32-byte home area, runInternalMicrotask() (7 register-sized parameters) and callMicrotask() (7 to 11) pass arguments beyond the fourth on the stack, and the entry thunk saves rsi, rdi and xmm6-xmm15 (160 bytes more than the 176-byte Linux thunk). That plausibly pushes the same chain past 2048. When it does, clearStackBelowCaller<2048>()…
Verification: pre-existing (nit): on a platform/compiler whose frames from performMicrotaskCheckpoint() to the JS entry exceed 2 KB, the lower frames stay uncleared and the base's stale-word leak persists there; nothing regresses against the base. Mechanism verified: Source/JavaScriptCore/runtime/MicrotaskQueue.h:272-276 sets the window to 32 KB under ASSERT_ENABLED || ASAN_ENABLED and 2 KB otherwise, and…
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughChangesMicrotask checkpoint stack clearing
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No merge-blocking issue was identified from the available evidence. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed and relevant explanation of the problem, fix, performance impact, testing, and changed behavior. It does not include the required Bugzilla bug title and link, the "Reviewed by NOBODY (OOPS!)." line, or the template-style file and function change list.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
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.
|
Closing this PR.
The branch stays, in case a clear inside JSC is wanted later. The repro, the two artifacts and the measurements are in the description above. |
Problem
jscshell ofc28156899ekeeps what the async function of the repro in the notes held for 32 turns, and for 60 of 60 turns with--useJIT=0.runInternalMicrotask()(one switch over all job kinds) are at the same addresses each time. A job writes only some of their slots. The conservative scan reads all of them.rbp-64of the frame ofrunInternalMicrotask(). It holds the generator of the finished function. [JSC] The conservative scan does not read stack words ASan has poisoned #673 does not cover it: the word is a normal slot, not an ASan redzone.Fix
performMicrotaskCheckpoint()zeroes the stack below itself before it drains a queue that has jobs: 2 KB in a release build, 32 KB with assertions or ASan.Options::clearStackForMicrotaskCheckpointturns the clear off.run-javascriptcore-tests.Background
awaitcontinues there.sanitizeStackForVM()zeroes the stack below its caller. It cannot reach a live frame.Notes
Scope. The object that stays is the last one that went through the slot. In Bun 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,
FinalizationRegistryandWeakRefusers that poll), not about unbounded growth.Repro (
jsc -m, or Bun withBun.gc(true)):jscofautobuild-c28156899e5f(linux amd64, lto)collections: 32, alive: 0, with--useJIT=0:collections: 60, alive: 2collections: 1, alive: 0--clearStackForMicrotaskCheckpoint=0collections: 32, alive: 0Bun at
c28156899e(it has #673), six cases: the collections are made by the module or by an async function, and a turn issetTimeout,setImmediateorBun.sleep(1). Eight turns each.Bun.sleepcases keep both objects for 8 of 8 turns.BUN_JSC_clearStackForMicrotaskCheckpoint=0: all six keep both objects.setTimeoutcases kept the objects with clang 21 and do not with clang 23.The two artifacts (Bun release build
c6b7fcb5b,BUN_GC_TIMER_DISABLE=1, the module case withBun.sleep(1)):generateHeapSnapshotForDebugging()after five collections: the objects are held bylist, held by aJSLexicalEnvironment, held by theAsyncFunctionGeneratorofmakeGarbage(). 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()(624 bytes) andVM::drainMicrotasks(). The job that runs isAsyncModuleExecutionResume. With clang 21 the same word showed with the loop in an async function (AsyncFunctionResume).Other sightings of this frame in Bun's leak tests: oven-sh/bun#37853 (the same generator word), oven-sh/bun#41607 (darwin arm64 release build, a
ReadableStreamDefaultController). oven-sh/bun#42877 and #669 saw ASan redzone words of it, which #673 now covers.How the window is cleared.
clearStackBelowCaller<size>()inMicrotaskQueue.cppisNEVER_INLINE, its only local is astd::array<uint8_t, size>, and it callszeroBytes()on it. Its frame lies right below its caller's frame, which is where the caller's next callee has its own. The compiler emits the stack probe for a large frame where a platform needs one.asmstatement. Without it clang removes thememset()and the function is a bareret.secureZeroBytes()does not help here: the memory clobber insecureZeroSpan()does not keep stores to a local whose address does not escape.push rbx; sub rsp, 0x800; memset(rsp, 0, 0x800); add rsp, 0x800; pop rbx; ret.callMicrotask()asserts (ASSERT_ENABLED) that it runs within seven eighths of the window below the checkpoint'sMicrotaskCallCache, which is a local ofdrainImpl().Sizes. From
performMicrotaskCheckpoint()(inlined intoVM::drainMicrotasks()) to the JS frame, Bun release build for x86_64, clang 23:drainWithUseCallOnEachMicrotask()624,runInternalMicrotask()272,asyncModuleExecutionResume()64,JSModuleRecord::evaluate()160,Interpreter::executeModuleProgram()224, the entry thunk 176: 1520 in all. Debug ASan build: 12 KB, of whichrunInternalMicrotask()is 6320.Why not
sanitizeStackForVM()at the checkpoint.sanitizeStackForVM()at the checkpoint the repro does not change in thejscshell, whereJSLock::didAcquireLock()resetsVM::m_lastStackTopfor every task.VM::m_lastStackTopto the bottom of the window and calledsanitizeStackForVMImpl(). Its loop for x86_64 stores 8 bytes per iteration: a checkpoint with one job went from 106 ns to 195 ns.The alternative in the embedder. oven-sh/bun#37853 calls
sanitizeStackForVM()once per event loop turn. Bun holds the API lock for the life of the thread, som_lastStackTopis not reset there. I tried it on the same binary (option off, onesanitizeStackForVM()at the top ofus_loop_run_bun_tick): it collects the same six cases at turn 1, and costs the same (2522 ms against 2525 ms per 1 M turns, 2504 ms with neither). Differences: it needs something to have sanitized from below the stale word since the word was written, it clears per turn and not per checkpoint, and it also covers the embedder's own frames and entries that are not checkpoints. The two do not exclude each other.Once per checkpoint. What one job leaves is still there for the later jobs of the same checkpoint, and is gone at the next checkpoint. A clear per job would cost every job the 21 ns.
Cost. Xeon 8375C, release build, clang 23.
jsc:promise.then(noop); drainMicrotasks();5 M times: 101 ns per iteration without, 122 ns with (5 runs each, same binary, the option).await new Promise(r => setImmediate(r)): 2531 ms without, 2547 ms with (medians of 11). 20 M awaits in one checkpoint: 602 ms and 602 ms.Tests.
JSTests/modules/microtask-checkpoint-clears-its-stack.js: an async function makes the garbage, the module collects from later turns.JSTests/stress/microtask-checkpoint-clears-its-stack.js: a promise reaction that returns the object, then an async generator that yields it. An async function collects from later turns.c28156899e: both fail (exit 3). This branch: both pass in all 28 configurations, and fail with--clearStackForMicrotaskCheckpoint=0.run-javascriptcore-tests --filter="microtask|async-func|async-gen|top-level-await|tla": 894 runs, 0 failures.Bun PR that pins the preview build of this branch and adds the Bun test: oven-sh/bun#42975. With the preview build
bun bd test(debug ASan) and a release build pass its 6 cases; the same binaries fail 3 and 2 of them withBUN_JSC_clearStackForMicrotaskCheckpoint=0.