DFG: LiveCatchVariablePreservationPhase flushed the wrong handler's locals when one try range leads directly into another - #417
Conversation
|
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 (3)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughThe DFG phase now preserves catch-local liveness across nested handler transitions. New stress tests cover explicit resource disposal, FTL compilation, JIT execution, and combined body and disposal errors. ChangesCatch-local preservation for explicit resource disposal
Merge Risk: ⚪ Minimal · up to The change corrects handler-local flushing so optimized using/disposal code preserves SuppressedError behavior. The supplied validation passes for the reproduced assertion and incorrect result, and no actionable merge-blocking risk remains beyond normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and on topic. It explains the bug, cause, change, tests, and verification. It does not include the Bugzilla link, review line, or explicit changed-file list from the template, but these omissions do not make the otherwise complete description largely incomplete. 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 |
![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 ( |
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.
ece09c1 to
ee07b02
Compare
…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 0f966e8 (Bun main's current pin); patch unchanged, and the files it touches did not change between eeab040 and 0f966e8. The 0f966e8 debug-asan jsc shell still asserts on both stress tests. Preview for this head: autobuild-preview-pr-417-ee07b022 (it needed several reruns because of the image setup failures that #473 addresses). oven-sh/bun#37941 pins it. |
ee07b02 to
59fa7da
Compare
…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 b7f217b (Bun main's current pin); patch unchanged, the only commit in between is the FFI lowering fix and it does not touch these files. The b7f217b debug-asan jsc shell still asserts on both stress tests. Preview for this head: autobuild-preview-pr-417-59fa7dae, pinned by oven-sh/bun#37941. |
|
Fourth fuzzer hit on this assertion, again the for-using eval shape that for-using-dispose-call-live-catch-locals-ftl-validation.js covers: asserts on the b7f217b debug-asan shell, compiles on this PR's 59fa7dae preview. No change needed here. |
59fa7da to
191bbb7
Compare
…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.
|
Fifth fuzzer hit on this assertion, again the for-using eval shape that for-using-dispose-call-live-catch-locals-ftl-validation.js covers: asserts on the aea1f01 debug-asan shell, compiles on this PR's 191bbb7d preview. No change needed here. |
|
Rebased onto 7688227 (Bun main's current pin); patch unchanged, the commits since 1cb96a7 only touch the bytecode cache and the sampling profiler. The 7688227 debug-asan jsc shell still asserts on both stress tests. Preview for this head: autobuild-preview-pr-417-56ce8b5b, pinned by oven-sh/bun#37941. |
|
Seventh fuzzer hit on this assertion, the for-using eval shape again with the hot loop inside the using body this time: asserts on the 7688227 debug-asan shell, compiles and runs to completion on this PR's 56ce8b5b preview. No change needed here. |
56ce8b5 to
5acb58a
Compare
…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 2da33d5 (Bun main's current pin); patch unchanged, the one commit since 7688227 only touches the bytecode cache. The 2da33d5 debug-asan jsc shell still asserts on both stress tests. Preview for this head: autobuild-preview-pr-417-5acb58a1, pinned by oven-sh/bun#37941. |
5acb58a to
ceaecd1
Compare
|
Stacked a copy of the merged #523 commit (9877148, the Windows arm64 Scoop fix) on top of the fix so this branch's preview can build again while staying on 7259739, the commit Bun main pins. The engine change is still only the DFG phase and the two stress tests; the extra commit is CI-only and will be dropped at the next rebase, once Bun's pin moves past #523. |
…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.
512638c to
79dfb46
Compare
…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 0bb01ed (Bun main's current pin, which includes #523), back to the single fix commit; patch unchanged, the module loader commits in between do not touch this phase. The 0bb01ed debug-asan jsc shell still asserts on both stress tests. Preview for this head: autobuild-preview-pr-417-79dfb46e, pinned by oven-sh/bun#37941. |
79dfb46 to
6654f96
Compare
…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 1817c3c (Bun main's current pin, after the upstream upgrade in #528); patch unchanged and applies cleanly, the upstream range changes the FTL OSR exit encoding and a few DFG phases but not the catch handling, the using disposal emission or this phase. The 1817c3c debug-asan jsc shell still asserts on both stress tests. Preview for this head: autobuild-preview-pr-417-6654f96e, pinned by oven-sh/bun#37941. |
6654f96 to
adefab2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…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 ceb9f90 (Bun main's current pin); patch unchanged, the only DFG file touched in between is DFGIntegerRangeOptimizationPhase. The ceb9f90 debug-asan jsc shell still asserts on both stress tests. Preview for this head: autobuild-preview-pr-417-adefab25, pinned by oven-sh/bun#37941. |
…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.
adefab2 to
f5d2f3a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased onto d71031a (bun's current pin, the Heap initial-budget change from #533); the patch is unchanged (head f5d2f3a, An eighth fuzzer input landed on the same fingerprint, marked flaky. Its crash output has the same graph shape (hasError temporary, loc18 there, unavailable at the dispose call's exception exit after the merge block). The reduced script in that report does not reach the |
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 2 findings from earlier reviews are still open above.
Still open from earlier reviews (2):
- 🔴
Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp:205—The transition is now keyed on the HandlerInfo* pointer alone, so computeLiveAtCatchHead() is skipped when the same han… - Also unresolved: 1 minor or pre-existing.
…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.