DFG: LiveCatchVariablePreservationPhase flushed the wrong handler's locals when one try range leads directly into another - #417
Conversation
|
Warning Review limit reached
Next review available in: 6 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes a DFG compiler phase that governs which locals stay available across exception exits, a human look from someone familiar with JSC's OSR-exit invariants would still be worthwhile.
What was reviewed:
- The reordering:
liveAtCatchHeadnow stays pinned to the handler being left until afterflushEverything, and is only recomputed for the newly entered handler afterwards — the block-end flush and the SetLocal/SetArgument path still see liveness forcurrentExceptionHandler. cachedCatchOriginis only read viacomputeLiveAtCatchHead, which runs only whencatchHandlerjust returned non-null on a cache miss (a cache hit always yieldsnewHandler == currentExceptionHandler), so it is never consumed stale or uninitialized.- Both new stress tests exercise the described handler-to-handler transition (for-of finally / user try-catch enclosing the synthesized dispose-call handler).
Extended reasoning...
Overview
The PR fixes an ordering bug in DFGLiveCatchVariablePreservationPhase::handleBlockForTryCatch. Previously, the catchHandler lookup lambda both resolved the handler for a node's origin and eagerly refilled liveAtCatchHead with that handler's liveness. When the loop then detected a handler transition and called flushEverything for the handler being left, liveAtCatchHead had already been overwritten with the liveness of the handler being entered, so the wrong locals were flushed. The fix splits the lookup from the liveness computation: catchHandler now only records the target CodeOrigin in cachedCatchOrigin, and a new computeLiveAtCatchHead lambda is invoked after the flush and after currentExceptionHandler is updated. Two stress tests are added covering the FTL validation assertion and the observable SuppressedError miscompile.
Security risks
None in the traditional sense (no auth, parsing, or untrusted-input surface). The risk profile is JIT correctness: a mistake here could leave locals unavailable at an OSR exit and produce wrong values after deopt. The change strictly increases the set of locals flushed at handler-to-handler transitions (it now flushes the inner handler's live set, which is what the exit actually needs) and does not remove any existing flush, so it should be monotonically safer than before.
Level of scrutiny
High. This is a DFG optimization phase that directly affects OSR-exit availability across all JIT-compiled code with nested exception handlers. Although the diff is small (~20 lines net) and the reasoning in the PR description is thorough and matches my reading of the code, JIT phase invariants are subtle enough that a reviewer with JSC/DFG expertise should confirm the sequencing is sound in all handler-transition shapes (including inlined frames, since cachedCatchOrigin carries the inlineCallFrame through).
Other factors
- I traced the cache-hit path: a hit on
cachedCodeOriginreturns the same handler as the immediately preceding call, sonewHandler == currentExceptionHandlerandcomputeLiveAtCatchHeadis not invoked; thereforecachedCatchOriginis never read stale. - The block-end
flushEverythingstill sees the correctliveAtCatchHeadbecause it is only refilled whencurrentExceptionHandleris reassigned. - The author verified the fix against a debug-asan build (both the fuzzer input and the two new tests pass; unmodified build fails all three), and notes the file is identical to upstream WebKit so the fix is upstreamable.
- No prior human or bot review comments to consider beyond a CodeRabbit rate-limit notice.
|
Bun side, pinned to this PR's preview build (autobuild-preview-pr-417-fbb76610): oven-sh/bun#37941. It adds the same two stress tests as jsc-stress fixtures; with the preview tarball both pass and the full jsc-stress suite is 117/117 on a debug build, while both fail on the engine without this change. |
Preview Builds
|
fbb7661 to
bc08a6e
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The reordering of liveAtCatchHead population relative to flushEverything looks correct and the analysis is thorough, but since this changes OSR-exit liveness handling in a DFG compiler phase — where a mistake is a silent miscompile — a human look from someone familiar with this phase would still be worthwhile.
What was reviewed:
- Traced the handler-transition loop before/after:
flushEverythingnow reads the old handler'sliveAtCatchHead, andcomputeLiveAtCatchHeadruns only aftercurrentExceptionHandleris reassigned. - Checked the
cachedCodeOriginfast-path: a cache hit impliesnewHandler == currentExceptionHandler, so a stalecachedCatchOriginis never consumed. - Confirmed the
SetLocal/SetArgumentpath and the block-endflushEverythingstill see aliveAtCatchHeadthat matchescurrentExceptionHandler.
Extended reasoning...
Overview
The PR fixes an ordering bug in DFGLiveCatchVariablePreservationPhase::handleBlockForTryCatch. Previously the catchHandler lookup lambda refilled liveAtCatchHead as a side effect before the caller decided whether to flush for the outgoing handler, so a direct handler→handler transition flushed the incoming handler's live set instead of the outgoing one. The fix splits the lookup from the liveness computation: catchHandler now only records the handler and its catch CodeOrigin, and a new computeLiveAtCatchHead lambda is invoked after the flush, once currentExceptionHandler has been updated. Two stress tests cover the fuzzer-found FTL validation crash and the observable SuppressedError miscompile.
Security risks
None in the classic sense (no auth/crypto/input parsing). The risk profile here is JIT-correctness: a wrong flush set can leave a bytecode local unavailable at an exception exit, which in release builds is a silent wrong-result miscompile rather than a crash. That is exactly the class of bug being fixed, and it is also the class of bug a regression here would introduce.
Level of scrutiny
High. This is a DFG compiler phase that directly governs which locals are kept alive for OSR exit into catch handlers. The diff is small (~20 lines of logic movement) and I traced every consumer of liveAtCatchHead (flushEverything, the SetLocal/SetArgument branch, and the block-end flush) to confirm each still sees a bitmap that corresponds to the current currentExceptionHandler. I also checked the origin == cachedCodeOrigin early-return: on a cache hit the returned handler equals the one from the previous iteration, so the transition branch is not entered and cachedCatchOrigin is not read. The fix is strictly more conservative than before (it never flushes fewer locals than the old code on any path), so the failure mode of a mistake would be over-flushing rather than under-flushing.
Other factors
The PR description is unusually detailed, includes before/after DFG graph dumps, and the author verified against a relinked debug-asan build plus the full jsc-stress suite (117/117) via the linked Bun PR. The file is noted as identical to upstream WebKit, so this is upstreamable. All of that raises confidence, but DFG/FTL exception-handling liveness is subtle enough that I'm deferring rather than auto-approving — a JSC reviewer should sanity-check the interaction with inlined frames and the block-boundary reasoning around emitUsingBodyScope.
…ized code DFG's LiveCatchVariablePreservationPhase flushed the locals of the handler being entered instead of the handler being left when one try range led directly into another. The dispose call emitted for a `using` block ends its synthesized catch range right at the call, inside the enclosing handler, so the "body threw" flag that only the synthesized catch reads was never flushed. FTL compiles of such code failed OSR availability validation, and optimized code restored the flag as undefined when a dispose method threw after the body threw, dropping the body's error instead of reporting a SuppressedError. Pins oven-sh/WebKit#417's preview build and adds both shapes as jsc-stress fixtures.
|
Rebased onto main (0cbb4a1, which is what Bun currently pins) with no changes to the diff; preview for this head is autobuild-preview-pr-417-bc08a6e1. Checked both stress tests against the jsc shells in the debug-asan prebuilts: they assert on 0cbb4a1 and pass on this preview. oven-sh/bun#37941 now pins this preview on top of current Bun main. |
|
A second fuzzer input hit the same assertion with the (trivial) dispose method inlined into the dispose call and the function made hot through recursion; same phase and same local, and this branch's preview engine compiles it fine. A deterministic fixture for that shape was added on the Bun side (oven-sh/bun#37941), nothing to change here. |
bc08a6e to
9984a38
Compare
| if (newHandler != currentExceptionHandler) { | ||
| // liveAtCatchHead still describes the handler we are leaving. Flush for it before | ||
| // switching over to the liveness of the handler we are entering, otherwise a | ||
| // transition straight from one handler into another (e.g. leaving a try range that | ||
| // is nested inside another one) flushes the outer handler's locals instead of the | ||
| // inner handler's. | ||
| if (currentExceptionHandler) | ||
| flushEverything(node->origin, nodeIndex); | ||
| currentExceptionHandler = newHandler; | ||
| if (newHandler) | ||
| computeLiveAtCatchHead(); | ||
| } |
There was a problem hiding this comment.
🔴 The transition is now keyed on the HandlerInfo* pointer alone, so computeLiveAtCatchHead() is skipped when the same handler entry is reached via a different inlineCallFrame — recursive inlining puts the same baseline CodeBlock at two depths, so the same try range returns the identical HandlerInfo* for both, but the catch head's CodeOrigin (and hence its operand liveness) differs. The pre-PR code refreshed liveAtCatchHead on every cache-miss lookup so it stayed in sync for this case; keying the transition on cachedCatchOrigin (or the (HandlerInfo*, InlineCallFrame*) pair) instead of the pointer alone restores that and also makes the transition flush fire, which is what you want anyway.
Extended reasoning...
What changed. Before this PR, catchHandler() refilled liveAtCatchHead inline on every cache-miss lookup that found a handler. After this PR the refill is factored into computeLiveAtCatchHead() and gated on newHandler != currentExceptionHandler — a HandlerInfo* pointer comparison. cachedCatchOrigin is still updated inside catchHandler() on every cache miss, but liveAtCatchHead is only recomputed when the caller sees the pointer change.
Why the pointer can stay the same while the catch origin changes. handlerForBytecodeIndex() returns a pointer into the baseline CodeBlock's m_rareData->m_exceptionHandlers. With recursive inlining (maximumInliningRecursion defaults to 2, and functions containing op_catch are inlinable — DFGByteCodeParser handles inlined op_catch at ~line 9774 rather than refusing), the outer and inner inline frames share the same baseline CodeBlock*, so the same try range yields the identical HandlerInfo* at both depths. But the catch head's CodeOrigin differs by InlineCallFrame*, and forAllLocalsAndTmpsLiveInBytecode() remaps each local through inlineCallFrame->stackOffset and walks the caller chain, so {handler->target, ICF_outer} and {handler->target, ICF_inner} produce different operand bitmaps.
Step-by-step trace. Take function f() { try { f(); } catch {} } with the recursive call inlined once. inlineCall() does not allocate a new block at callee entry (parseCodeBlock() reuses m_currentBlock on its first iteration), and a try-range start is not a jump target, so one DFG block can contain, in order:
- Outer-frame try-body nodes at
{bc#X, ICF=null}.catchHandlerfindsHin the root baseline block;cachedCatchOrigin = {H->target, null}.newHandler != currentExceptionHandler(null → H), socomputeLiveAtCatchHead()runs andliveAtCatchHeaddescribes the outer frame's locals. - Inlined prologue /
op_enterat{bc#0, innerICF}.bc#0is outside every try range, socatchHandlerwalks up to the direct caller and again findsHwithcachedCatchOrigin = {H->target, null}. Same pointer, no recompute — still correct. - Inner-frame try-body nodes at
{bc#Y, innerICF}. Cache miss;catchHandlerfindsHdirectly in the inner frame's baseline block (sameCodeBlock*as the root) and setscachedCatchOrigin = {H->target, innerICF}. But it returns the sameH, sonewHandler == currentExceptionHandlerandcomputeLiveAtCatchHead()is skipped.liveAtCatchHeadis still keyed to{H->target, null}; the inner frame's locals (mapped to higher machine-local indices viainnerICF->stackOffset) are allfalsein it.
From step 3 onward, the SetLocal check at line 210 and the block-end flushEverything at line 228 both consult the stale outer-frame bitmap, so inner-frame locals live at the inner catch head get no Flush. An exception from the inner try body OSR-exits to the inner catch with those locals unavailable — exactly the failure class this PR is fixing (DFGOSRAvailabilityAnalysisPhase.cpp(198) assertion in debug, wrong restored value in release).
Regression vs. pre-PR. The old code recomputed liveAtCatchHead inside the lambda at step 3 regardless of whether the returned pointer changed, so the SetLocal path and the block-end flush saw the correct inner-frame bitmap. The pre-PR code already failed to flushEverything at a same-pointer transition — that gap is pre-existing and not what this comment is about — but it did keep liveAtCatchHead in sync for everything after the transition; the new code no longer does.
Fix. Key the transition on the catch CodeOrigin rather than the HandlerInfo* alone: e.g. remember cachedCatchOrigin before calling catchHandler() and compare the before/after values (treating the null-handler case as an invalid origin), or compare the (HandlerInfo*, InlineCallFrame*) pair. That also makes the mid-block flushEverything fire at the outer→inner boundary, which is the desired behaviour since the inner catch's live set is what the exception exit needs from that point on.
…ized code DFG's LiveCatchVariablePreservationPhase flushed the locals of the handler being entered instead of the handler being left when one try range led directly into another. The dispose call emitted for a `using` block ends its synthesized catch range right at the call, inside the enclosing handler, so the "body threw" flag that only the synthesized catch reads was never flushed. FTL compiles of such code failed OSR availability validation, and optimized code restored the flag as undefined when a dispose method threw after the body threw, dropping the body's error instead of reporting a SuppressedError. Pins oven-sh/WebKit#417's preview build and adds both shapes as jsc-stress fixtures.
|
Rebased onto c6cfe90 (Bun main's current pin) so the preview stays usable from oven-sh/bun#37941; diff unchanged. Preview for this head: autobuild-preview-pr-417-9984a386. |
|
A third fuzzer input hit the same assertion, this time the eval shape from the description again ( |
…the handler being entered LiveCatchVariablePreservationPhase walks each block and, whenever a node's origin is covered by a different exception handler than the previous node, inserts Flushes for the locals live at the head of the handler it is leaving. The handler lookup lambda also refilled liveAtCatchHead with the liveness of the handler it found, so on a transition straight from one handler into another the flushes were computed from the handler being entered instead of the one being left. Transitions into no handler at all were unaffected, which is why ordinary try/catch (whose jump over the catch block sits inside the try range) did not hit this. The disposal code emitted for `using` ends the try range of its synthesized catch right after the dispose call, so the block containing that call ends with a synthesized Jump whose origin already belongs to the enclosing handler. The "body threw" flag, which only the synthesized catch reads, was therefore never flushed or given a Phi and was unavailable at the dispose call's exception exit: FTL compiles of such code fail OSR availability validation, and optimized code restores the flag as undefined when a dispose method throws after the body threw, so the body's error is dropped instead of being reported through a SuppressedError. Keep the lookup free of side effects, flush for the old handler first, and only then compute the liveness of the new one.
9984a38 to
ece09c1
Compare
|
Rebased onto eeab040 (Bun main's current pin, after #455 and #463); the patch is unchanged and DFGLiveCatchVariablePreservationPhase.cpp did not change in the upstream merge. The eeab040 debug-asan jsc shell still asserts on both stress tests here, so this is still needed after the upgrade. Preview for this head will be autobuild-preview-pr-417-ece09c13, for oven-sh/bun#37941. |
| return null; | ||
| } | ||
|
|
||
| for (let i = 0; i < 20000; ++i) { |
There was a problem hiding this comment.
🟡 Both new stress tests hardcode their warmup iteration counts (20000 here and 2000000 at for-using-dispose-call-live-catch-locals-ftl-validation.js:15) instead of using testLoopCount. JSTests/README.md requires new tests to use testLoopCount so they tier up where that matters and stay under the 200ms budget in no-JIT configurations; it is a global on the jsc shell's GlobalObject, so it is visible inside indirect eval (or can be interpolated via ${testLoopCount} since the eval string is already a template literal).
Extended reasoning...
What the issue is
JSTests/README.md (pulled into JSTests/CLAUDE.md via @README.md) states, under "Adding Tests":
New tests are required to adhere to the following rules:
- Tests must run in under 200ms in all configurations. …
- Use
testLoopCountorwasmTestLoopCountto control how many iterations a test runs. ThejscCLI sets these based on the configuration of the test, so tests iterate enough to tier up where that matters and exit early where it doesn't.
Both new stress tests in this PR hardcode their tier-up iteration counts instead:
JSTests/stress/using-dispose-throw-after-body-throw-in-jit.js:44—for (let i = 0; i < 20000; ++i)JSTests/stress/for-using-dispose-call-live-catch-locals-ftl-validation.js:15—for (let i = 0; i < 2000000; ++i) { }inside the eval string
Why testLoopCount applies here
testLoopCount is set by the jsc shell as a direct property on the global object (jsc.cpp:674, via putDirect) and is scaled per configuration (jsc.cpp:662-666): it defaults to max(10000, thresholdForFTLOptimizeAfterWarmUp() * 3) when the FTL is on, drops to ~1000 with --useDFGJIT=0, and to ~100 with --useBaselineJIT=0. Both tests already pass --useConcurrentJIT=0, so testLoopCount iterations are sufficient to reach the FTL in the configurations where that is possible.
For the eval test specifically: the loop lives inside (0, eval)(...), but indirect eval executes in the global scope, so the testLoopCount global is visible there. The eval string is also already a template literal, so ${testLoopCount} interpolation works if you prefer to bake the number in.
Step-by-step: why the hardcoded counts are a problem
Take for-using-dispose-call-live-catch-locals-ftl-validation.js under the no-jit variant that run-jsc-stress-tests runs for every stress test:
run-jsc-stress-testslaunches the test with--useBaselineJIT=0(among other flags). The jsc shell computestestLoopCount = clampLoopCount(100, thresholdForJITAfterWarmUp() * 3)— on the order of100.- The test ignores that and runs
for (let i = 0; i < 2000000; ++i) { }in the LLInt. Two million empty iterations in the interpreter is well over the 200ms budget from rule maybe upgrade #1, and none of it is useful because there is no FTL to compile the eval code in this configuration. - Multiply across the several no-JIT / no-LLInt variants and this adds measurable dead time to every stress run.
using-dispose-throw-after-body-throw-in-jit.js is less extreme (20k iterations of a real function call), but the same reasoning applies: in the no-jit variant those 20k warmup calls never tier run() up, so the loop is doing nothing the test needs while still counting against the 200ms budget.
Why nothing else prevents this
Neither file has a //@ skip if … or //@ runDefault directive that would exclude the no-JIT variants, so run-jsc-stress-tests runs them in every configuration. The --useConcurrentJIT=0 / --validateGraph=1 options passed via //@ requireOptions do not change which variants run.
Impact
This is a test-hygiene / repository-convention issue, not a correctness bug in the DFG fix. The tests still reproduce the bug and still pass with the fix; they just spend more wall-clock than necessary in configurations where the JIT is disabled and diverge from the ~2200 existing stress tests that use testLoopCount.
Fix
// using-dispose-throw-after-body-throw-in-jit.js:44
for (let i = 0; i < testLoopCount; ++i) {
// for-using-dispose-call-live-catch-locals-ftl-validation.js:15
for (let i = 0; i < ${testLoopCount}; ++i) { }
// or, since indirect eval sees globals:
for (let i = 0; i < testLoopCount; ++i) { }…ized code DFG's LiveCatchVariablePreservationPhase flushed the locals of the handler being entered instead of the handler being left when one try range led directly into another. The dispose call emitted for a `using` block ends its synthesized catch range right at the call, inside the enclosing handler, so the "body threw" flag that only the synthesized catch reads was never flushed. FTL compiles of such code failed OSR availability validation, and optimized code restored the flag as undefined when a dispose method threw after the body threw, dropping the body's error instead of reporting a SuppressedError. Pins oven-sh/WebKit#417's preview build and adds both shapes as jsc-stress fixtures.
1 participant
Found by fuzzing Bun. Fingerprint:
DFGOSRAvailabilityAnalysisPhase.cpp(198).Symptom
Debug/ASAN builds abort in the FTL compile of code whose
usingblock's dispose call is not inlined. The fuzzer hit it in eval code; a plain function does the same once it reaches the FTL. Reduced repro (for (using r of [resource]) { try { r(); } catch {} }followed by a hot loop, evaluated with indirect eval):Release builds miscompile instead.
loc21here is thehasError("body threw") flag of the disposal code emitted byBytecodeGenerator::emitUsingBodyScope. Once the enclosing function is optimized (the DFG tier is enough), a dispose method that throws after the body also threw yields the dispose method's plainErrorinstead of aSuppressedErrorcarrying both errors, so the body's exception is silently dropped. For the scenario inJSTests/stress/using-dispose-throw-after-body-throw-in-jit.jsthe interpreter returns aSuppressedErrorand optimized code on current main returnsError("dispose 0").Cause
LiveCatchVariablePreservationPhase::handleBlockForTryCatchwalks the nodes of a block, looks up the handler covering each node's origin and, when that handler changes, callsflushEverythingto flush the locals live at the head of the handler being left. The lookup lambda (catchHandler) also refilledliveAtCatchHeadwith the liveness of whatever handler it found, and it ran beforeflushEverything. So on a transition from one handler straight into another, the flushes were computed from the handler being entered. Only transitions into "no handler" behaved, because the lambda leavesliveAtCatchHeadalone in that case.Ordinary
try/catchdoes not expose this:TryNodeemits the jump over the catch block before it pops the try range, so a block ending at the range boundary ends on a node that is still inside the range and gets the block-end flush for the right handler. Theusingdisposal code is laid out differently:emitUsingBodyScopeemits the dispose call, ends the synthesized catch's range (trySlotEndis an emitted label, hence a jump target and a DFG block boundary) and only then emits the jump. The block holding the dispose call therefore ends with a synthesizedJumpwhose origin is the first bytecode after the range, which belongs to the enclosing handler (the for-of's synthesized finally in the fuzzer case, the user'stry/catchin the function case).hasErroris read only by the synthesized catch, so nothing else in the graph keeps it alive; without the flush it gets no Phi at the merge after the "seed pendingError with the body's exception" branch and is unavailable at the dispose call's exception exit.Graph after parsing for the reduced repro, block with the dispose call. Before:
After:
Change
catchHandlernow only records which handler covers an origin, plus that handler's catchCodeOrigin. When the handler changes, the block is flushed for the old handler first and the liveness of the new one is computed afterwards. The file is identical to upstreammainat this point, so the same fix applies upstream.Two stress tests:
for-using-dispose-call-live-catch-locals-ftl-validation.jsis the fuzzer shape and runs with--validateGraph=1, so it fails on release builds as well;using-dispose-throw-after-body-throw-in-jit.jschecks theSuppressedErrorbehaviour after the function is optimized. On current main the first one hits the assertion above and the second one hits the same assertion on a debug build (wrong result on a release build); both pass with the fix.Verification
Compiled the modified
UnifiedSource-dfg-9bundle with the flags recorded in the447082abdebug-asan prebuilt'scompile_commands.json, replaced that member in the prebuilt'slibJavaScriptCore.aand relinked Bun's debug-asan binary against it. With that binary the original fuzzer input and both stress tests run to completion (the eval now goes through its regular FTL compile instead of aborting), and theSuppressedErrorscenario matches the interpreter at both the DFG and FTL tiers. The unmodified binary still fails all three. Bun's test suite will run against this PR's preview tarballs from the Bun PR that pins them; I will link it here.