Conversation
…e word VM::drainMicrotasks() and MicrotaskQueue::performMicrotaskCheckpoint() each keep a std::optional<VMEntryScope> in their frame, and those frames stay on the stack for as long as the jobs they drain run. A std::optional keeps its flag in one byte, and nothing writes the seven bytes after it, so the flag's word keeps the rest of what an earlier frame left in that stack slot. Over a pointer to a cell that leaves a pointer into whichever cell lies at (the old address & ~0xff) + the flag. The conservative scan reads whole words and accepts a pointer into the middle of a cell, so it marks that cell at every collection made under the drain. The drain runs at the same stack depth in every event loop turn and only ever stores one byte into that word, so the cell stays marked until something else writes the slot. In the x86_64 release build VM::drainMicrotasks() writes the word at rbp-0x48 only with "movb $0x0" and "movb $0x1". Seen in Bun as one module graph of 50 that 100 full collections made from an async function did not collect: the word read <address of a function of that graph, which ends in 00> | 1. OptionalVMEntryScope is the same thing with a flag that is a whole word. This is the defect that MicrotaskCallCache already describes for the type byte of a MicrotaskCall.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Preview build of 2a67b82: |
|
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 (4)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughThe patch adds ChangesMicrotask entry scope
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No actionable behavior regression was identified in the reviewed change, so it is ready to merge subject to normal build validation. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides a detailed and relevant explanation of the bug, fix, evidence, and limitations, but it does not include the required Bugzilla title and link, review line, or structured changed-file and function list from the repository template.
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 |
1 participant
Problem
Bun.gc(true)calls from an async function leave one module graph of 50 alive. This happens in 8 of 80 runs.VM::drainMicrotasks()(VM.cpp:1702) andMicrotaskQueue::performMicrotaskCheckpoint()(MicrotaskQueueInlines.h:140) each keep astd::optional<VMEntryScope>on the stack while the jobs run. Its flag is one byte. Nothing writes the other seven bytes of that word: the release build stores to-0x48(%rbp)only withmovb.(old address & ~0xff) | 1. The collector marks that cell at every collection under the drain.Fix
OptionalVMEntryScopereplaces both optionals with the same interface. Its flag is auintptr_t. Astatic_assertrejects padding.movbstores to the two flags are gone, andmovqwrites them. The cause is verified in gdb on an unmodified Bun binary (Notes).Background
ConservativeRoots::genericAddPointeraccepts a pointer into the middle of a cell.VMEntryScoperecords that native code entered the VM. The drain makes one only when the queue has jobs.Notes
The intervention (gdb, unmodified
bun-profileof oven-sh/bun0d3492e353,BUN_GC_TIMER_DISABLE=1). The test calls a host function once, at round 5, only when a graph is still alive. A breakpoint there walks the frame-pointer chain to the frame that returns intoBun::jsFunctionDrainMicrotaskQueue, which is the frame ofVM::drainMicrotasks(), and readsrbp-0x58,rbp-0x50andrbp-0x48. Arm A writes the flag byte back as a whole word, which zeroes the seven stale bytes. Arm B makes the same stop and writes nothing. The arms alternate by run. 154 runs gave 7 triggers.[rbp-0x48]0x2422f3628010x4cba749e7010x486324027010x3473b1727010x525f385de010x3c608d927010x257da7ee701Arm A: 4 of 4 collected at the next collection after the write. Arm B: 3 of 3 still alive at round 100. The sample is small. In every triggered run
[rbp-0x58]is the VM,[rbp-0x50]is the global object, and the inner optional's flag word atrbp-0x30is a clean0x1.The heap snapshot.
generateHeapSnapshotForDebugging()in three failing runs: the retained module (environment, record, namespace object, its two functions, its array) has no incoming edge from outside itself and norootsentry. The function has the lowest node id of the six, so it was visited first.The roots list. In 7 of 7 failing runs, the
ConservativeRootsinline buffer that the previous collection left in dead stack memory lists that function. Its position in the list is between the two stack words that surroundrbp-0x48of the drain frame.Run counts cannot verify this change. I first tried to prove the cause with a Bun build that zeroes the stack before the drain, switched by an environment variable. The control arm stopped reproducing: 0 of 120. The instrumentation had added
push %rbx; push %raxto the prologue ofjsFunctionDrainMicrotaskQueue, in both arms, so the drain frame sat 16 bytes lower over a different stale word. The unmodified binary reproduced in 8 of 80 runs at the same time. So a rebuild alone moves the word, and "0 failures after the change" would not tell a fixed word from a moved one. That is why the evidence here is the disassembly and the single-word write.The disassembly, local build.
UnifiedSource-runtime-49.cpp.o(it includesruntime/VM.cpp),-DPORT=JSCOnly -DCMAKE_BUILD_TYPE=Release, clang 23, no LTO, no frame pointer. Every byte-wide store inVM::drainMicrotasks():movb $0x0,0x10(%rsp)x4,movb $0x1,0x10(%rsp)x2,movb $0x0,0x28(%rsp)x4,movb $0x1,0x28(%rsp)x1, and three stores to fields of the VM and of the queue.movq $0x0,(%rsp),movq $0x1,(%rsp),movq $0x0,0x18(%rsp)andmovq $0x1,0x18(%rsp).The disassembly, shipping-style build. Bun
0d3492e353, release profile (LTO, frame pointers), linked againstautobuild-preview-pr-694-2a67b829, compared with the same Bun linked against000c489972. Every byte-wide store to memory inVM::drainMicrotasks():movb $0x0,-0x48(%rbp)x2,movb $0x1,-0x48(%rbp),movb $0x0,-0x30(%rbp)x3,movb $0x1,-0x30(%rbp). These are the two flags.movb $0x1,-0x40(%rbp),movb $0x2,-0x40(%rbp),sete -0x30(%rbp),sete -0x2f(%rbp), and three stores to fields of the VM and of the queue. The four frame stores are the inlinedgetCallData()for the promise rejection callback: the type byte and two bools of aCallData. That is the struct of [JSC] CallData has no padding after its type #660. This PR does not change it.-0x58(%rbp)and-0x40(%rbp), written withmovq $0x0andmovq $0x1. The caller,jsFunctionDrainMicrotaskQueue, has the same frame in both binaries.A read that did not discriminate. I read the outer flag word in the live drain frame at round 5 of every run, 10 runs per binary. Both binaries read
0x1in 10 of 10. I expected stale bytes in most runs of the old build, and that was wrong. On the old build the word is stale only in a minority of runs: 0 of 10 here, against 7 of 7 in the runs where a graph was still alive at round 5. This read does not show the fix at run time. The disassembly does: no code stores less than the whole word.The Bun side. The test is unchanged and is the check for this fix: "50 graphs alive at once, each with its own state, then all exit and are collected" in
test/js/bun/module-graph/module-graph.test.ts. I opened oven-sh/bun#43217, which moved the collections of that test out from under the drain, and closed it again: that edit routed the test around this defect.Not covered.
std::optional. Zeroing them needs a store that the compiler keeps for memory nobody reads, and I cannot guarantee that here.runInternalMicrotask().std::optional<VMEntryScope>toVM::drainMicrotasks(). It would need this class too.No JSTest. A test has to put a chosen pointer under the flag word. Spraying the stack does that, but the same spray also lands under the padding that #660 and #678 remove, and in slots that the frames never write. Such a test fails with and without this change.
Self-review. A second read of the diff changed three things:
reset()now destroys before it clears the flag, asstd::optionaldoes. The headers include<cstddef>,<memory>and<new>directly. The comment gives the general form of the bad pointer and not only the case of an address that ends in00.