jsc: treat DFG Plan::m_mustHandleValues as weak so queued compiles don't root user objects - #34640
jsc: treat DFG Plan::m_mustHandleValues as weak so queued compiles don't root user objects#34640robobun wants to merge 6 commits into
Conversation
…ot user objects The retaining root behind the test-gc-http-client N-1/N stall (and the earlier once()-nulling workarounds in node:events / internal/shared) is not the conservative stack scan: a GCDebugging heap snapshot taken when the test is stuck shows the surviving IncomingMessage/ClientRequest rooted with RootMarkReason::JITWorkList. DFG::Plan::m_mustHandleValues captures whatever locals were live in the frame that triggered loop OSR and marks them as roots for the life of the concurrent compile. oven-sh/WebKit#308 makes that snapshot weak: entries that nothing else marks are dropped in Plan::finalizeInGC, and every compiler phase that reads m_mustHandleValues already skips nullopt. The fixture drives enough independent functions to DFG at once via the http client/server path that several plans are queued when the first gc() runs, then asserts zero ClientRequest/IncomingMessage are JITWorkList-rooted in the debugging heap snapshot.
|
Updated 11:21 AM PT - Jul 18th, 2026
❌ @robobun, your commit 20a81ca has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34640That installs a local version of the PR into your bun-34640 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Neither of those is this.
This change removes a bounded, short-lived extra root. It makes |
… m_mustHandleValues change
Also:
- generateHeapSnapshotForDebugging: RELEASE_AND_RETURN around JSONParse
so validateExceptionChecks does not assert on the simulated throw the
fixture exercises on the x64-asan lane.
- fixture: skip the heap snapshot when alive=0 (it is expensive under
debug+ASAN and the count is trivially zero).
- test: run the child with a single DFG/FTL compiler thread so plans
queue rather than drain in parallel, and allow 30s on debug/ASAN
builds (the 32 http requests alone take ~5s there).
WalkthroughThe PR updates the WebKit preview version, releases the JSC throw scope when returning a heap snapshot, and adds a DFG garbage-collection regression fixture and test. ChangesWebKit build update
JSC heap snapshot GC regression
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/deps/webkit.ts`:
- Line 10: Update WEBKIT_VERSION to use the stable merged autobuild-<sha> tag
instead of the PR-specific autobuild-preview-pr-308-06ecce07 tag, ensuring
WebKit downloads remain available after the upstream PR lifecycle ends.
In `@test/js/bun/jsc/dfg-plan-gc-fixture.js`:
- Around line 26-46: The fixture must signal that the intended queued
DFG/worklist state was reached before calling Bun.gc(true), and the parent test
must assert that signal alongside jitworklist-rooted=0. Update the check
function in test/js/bun/jsc/dfg-plan-gc-fixture.js at lines 26-46 to emit the
observable precondition signal, then update test/js/bun/jsc/bun-jsc.test.ts at
lines 590-592 to require both outputs so a drained-plan run cannot pass as a
false positive.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 32a45784-e1b0-4db9-9ec5-7f158debfbc8
📒 Files selected for processing (4)
scripts/build/deps/webkit.tssrc/jsc/modules/BunJSCModule.htest/js/bun/jsc/bun-jsc.test.tstest/js/bun/jsc/dfg-plan-gc-fixture.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Matches the combined {stdout, exitCode} toEqual pattern the neighboring
deserialize-* tests use; stderr is drained but not asserted so a benign
ASAN/debug warning cannot mask the jitworklist-rooted=0 check.
…LI flags Bun only reads JSC options from BUN_JSC_* env vars (ZigGlobalObject.cpp); unknown --long flags are silently skipped, so the fixture was running with the default 2 DFG threads and the 'one compiler thread so plans queue' precondition was never applied. With the env vars actually taking effect fail-before is 30/30 on the current release build.
…) and trim comments oven-sh/WebKit#308 is now based on 4895f45d (the currently pinned commit) so the preview artifact contains only the DFGPlan change, and finalizeInGC skips jsEmpty() entries (a TDZ-sentinel or LLInt-zeroed temp at the OSR point satisfies isCell() but asCell() is null).
There was a problem hiding this comment.
All three earlier findings (stderr-empty assertion, --jsc-* CLI flags, comment length) are addressed and the Bun-side diff now looks correct, but I'm leaving this for a human: the substantive change is the JSC GC-semantics patch in oven-sh/WebKit#308 (making m_mustHandleValues weak during concurrent compilation), which is outside this diff and needs maintainer sign-off — and WEBKIT_VERSION is still pinned to the preview tag pending that PR's merge.
What was reviewed:
RELEASE_AND_RETURNaroundJSONParseinfunctionGenerateHeapSnapshotForDebugging— correct throw-scope hygiene, matches the pattern used elsewhere in the file.- Test now uses
BUN_JSC_*env vars, drains stderr without asserting empty, and matches the neighboring{stdout, exitCode}assertion shape. - Fixture's snapshot-skip when
alive == 0is sound (empty worklist ⇒ trivially zero JITWorkList roots);http.geterror-handler concern was examined and ruled out.
Extended reasoning...
Overview
Four files: WEBKIT_VERSION bump to autobuild-preview-pr-308-40ff52aa, a one-line RELEASE_AND_RETURN wrap in BunJSCModule.h, a new subprocess test in bun-jsc.test.ts, and its fixture dfg-plan-gc-fixture.js. The actual behavioral fix — stopping DFG::Plan::checkLivenessAndVisitChildren from marking m_mustHandleValues and instead weak-clearing dead cells in Plan::finalizeInGC — lives entirely in oven-sh/WebKit#308 and is pulled in via the version bump.
Security risks
None from the Bun-side diff. The WebKit-side change alters which JSValues are treated as GC roots while a concurrent DFG/FTL compile is queued; getting that wrong is a use-after-free / type-confusion risk in the JIT (a cell freed mid-compile whose value is later consulted for prediction seeding). The PR description argues every consumer already tolerates nullopt and that clearing happens with compiler threads suspended, but that reasoning belongs to the WebKit PR review, not this one.
Level of scrutiny
High — even though the visible diff here is small and mechanical, what it ships is a change to JSC's GC-root set during concurrent compilation. That is exactly the class of change (GC marking, weak references, cross-thread lifetime) REVIEW.md flags as most-blocked. A maintainer who can read the WebKit-side diff should approve this.
Other factors
- All three findings from my earlier passes were fixed (a9a337a, c9a03e2, 20a81ca) and verified in the current diff.
WEBKIT_VERSIONis still a preview tag; the PR description says it will be swapped for the mergedautobuild-<sha>once WebKit#308 lands, so this is effectively blocked on that anyway.- The regression test's assertion is monotone (as the author argued in-thread), so pass-after should be stable; fail-before is 30/30 against the release build per the author's measurement.
|
CI on 20a81ca (build #75495): Remaining red is unrelated: Ready for review. Once oven-sh/WebKit#308 merges I'll swap |
1 participant
What
The N-1/N stall in
test-gc-http-client*(and the motivation for theonce()closure-nulling in #34500 and 9abee13) was attributed to JSC's conservative stack scan. It isn't: aGCDebuggingheap snapshot taken while the test is at N-1/N shows the survivingIncomingMessage/ClientRequestrooted directly withRootMarkReason::JITWorkList, and the same run withBUN_JSC_useConcurrentJIT=0orBUN_JSC_useDFGJIT=0collects everything in one gc.The retainer is
DFG::Plan::m_mustHandleValues. When a hot loop triggers Baseline->DFG (or DFG->FTL) OSR, the tier-up path copies every live argument and local of the triggering frame into the compilation plan so the compiler can seed OSR-entry type predictions.Plan::checkLivenessAndVisitChildrenmarked those JSValues as roots for the life of the concurrent compile, so whatever happened to be in scope at the tier-up point (anIncomingMessage, the nextTick drain loop'stockwithargs:[req], etc.) stayed alive until the plan was finalized.Fix
oven-sh/WebKit#308 stops visiting
m_mustHandleValuesduring marking and, inPlan::finalizeInGC(post-mark, compiler threads suspended, before sweep), drops any entry whose cell was not otherwise marked. Every compiler phase that readsm_mustHandleValues(PredictionInjection,CFA::injectOSR,TypeCheckHoisting) already treatsnulloptas "unknown", so compilation continues; the OSR-entry prediction for that local simply widens and OSR-entry validation handles the rest. No plan cancellation, no blocking on the compile thread.This PR:
WEBKIT_VERSIONtoautobuild-preview-pr-308-06ecce07(to be replaced with the merged sha once DFG: stop rooting m_mustHandleValues from the concurrent compiler plan WebKit#308 lands)generateHeapSnapshotForDebugging:RELEASE_AND_RETURNaroundJSONParsesovalidateExceptionChecksdoes not assert on the simulated throw the fixture exercisesVerification
test/js/bun/jsc/dfg-plan-gc-fixture.jsmakes 32http.getrequests withBUN_JSC_numberOfDFGCompilerThreads=1/BUN_JSC_numberOfFTLCompilerThreads=1, waits for all responses, callsBun.gc(true)once, and (if anything survived) takes agenerateHeapSnapshotForDebugging()snapshot and countsClientRequest/IncomingMessagenodes whose root reason isJITWorkList.jitworklist-rootedafter oneBun.gc(true)ClientRequestalive after one gcAll four
test-gc-http-client*andtest-net-connect-memleakpass cleanly on the patched build with no N-1/N stall.bun bd test test/js/bun/jsc/bun-jsc.test.tsis 37/37.The
once()closure-nulling in 9abee13 (part of #34519) is unnecessary once this lands; thenode:eventswrapper can match Node's shape.Note for the gate
The fix is a
WEBKIT_VERSIONbump inscripts/build/deps/webkit.ts, whichgit stash push -- src/ packages/does not revert, so the gate's fail-before build picks up the new WebKit and the test passes both ways. The 30/30 fail rate against the current release build (table above) is the honest fail-before evidence.no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/jsc/bun-jsc.test.ts