Bump WebKit: keep the locals of a using block's synthesized catch alive in optimized code - #37941
Bump WebKit: keep the locals of a using block's synthesized catch alive in optimized code#37941robobun wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 56 seconds 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 12:07 AM PT - Aug 18th, 2026
✅ @robobun, your commit db1e4fe60f79bb220a14d93e5112e1e19b8066a9 passed in 🧪 To try this PR locally: bunx bun-pr 37941That installs a local version of the PR into your bun-37941 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The Bun-side diff is small and the two new stress fixtures look correct (the requireOptions directive is parsed by the existing harness, and the SuppressedError assertions are exact). That said, this pins WebKit to a preview build of an unmerged PR and pulls in everything between the old pin and current WebKit main (including oven-sh/WebKit#410) — a maintainer should sign off on shipping against a preview tag and on the wider engine bump.
Extended reasoning...
Overview
This PR bumps WEBKIT_VERSION in scripts/build/deps/webkit.ts from a commit sha to autobuild-preview-pr-417-fbb76610, a preview build of oven-sh/WebKit#417 that fixes a DFG LiveCatchVariablePreservationPhase bug affecting using disposal in optimized code. It also adds two JSC stress fixtures (the fuzzer-shaped validation case and a behavioural SuppressedError check) and wires them into jsc-stress.test.ts.
Security risks
None identified. The change is a dependency version bump plus test fixtures; no auth, crypto, or input-handling code is touched in this repo.
Level of scrutiny
High. Although the local diff is tiny, the effective change is a new JavaScriptCore binary for every platform. The preview build is cut from current WebKit main, so it carries not only the targeted fix but everything between the previous pin (7b763944…) and main — the description calls out oven-sh/WebKit#410 explicitly. Pinning to a preview tag of an unmerged upstream PR (with the description noting the pin "should move to the final autobuild-<sha> once it lands") is a release-process decision that a maintainer should confirm rather than an automated reviewer.
Other factors
- The fixtures follow the existing
jsc-stressconventions:// @bunheader,//@ requireOptions(...)directives thatparseJSCFlagsalready handles, and self-checking viashouldBe/ non-zero exit on failure.--validateGraph=1on the FTL fixture makes it fail on release builds of the unfixed engine, and the behavioural fixture asserts the exactSuppressedErrorshape. - Verified that
prebuiltUrl/prebuiltDestDirinwebkit.tsalready handleautobuild--prefixed version strings, so the tag form of the pin resolves to the correct release URL and cache key without further changes. - The description's verification (fixtures fail under
USE_SYSTEM_BUN=1, pass on the preview build, related ERM/async_hooks suites pass) is thorough, but CI on the actual preview tarball is what proves the wider bump is safe across platforms — that's the part a human should look at before merging.
|
On the pin: the preview tag is only there so CI can exercise the fix before oven-sh/WebKit#417 lands; once it merges this should be re-pointed at the merged main sha, as with previous bumps of this kind. The engine content between the current pin (7b763944) and the preview is #417 itself plus oven-sh/WebKit#410 (InternalFieldTuple cast by JSType); the remaining commits in that range (#411 to #414) only change how the prebuilts are built (line tables only for the -lto variants, extra freebsd/android variants). Any bump that picks up #417 necessarily carries those too. What the preview tarball has shown so far: locally the debug-asan tarball builds and passes jsc-stress (117/117), the explicit resource management, mock-disposable, bun-jsc and async_hooks suites (the last one covers the #410 cast path). The one failure in build 93631 was the x64-musl build step getting a 404 for bun-webkit-linux-amd64-musl-lto.tar.gz: the preview release's last asset upload had stalled, so that lane started before the asset existed. The WebKit release job has been rerun and is re-uploading the assets; I will retrigger CI here once the release is complete. |
|
The preview release is complete now (all 42 tarballs uploaded, including the musl-lto one that 404'd in build 93631), so CI has been retriggered: https://buildkite.com/bun/bun/builds/93640. No code changes since the review above; the new commit is empty. |
|
Status: build 93640's only failure is the linux x64 build-bun step, and it failed fetching the lolhtml source tarball (github.com archive download, "fetch failed" after 5 retries), not anything in this change; the preview WebKit tarball itself downloaded and extracted fine on that agent, and the other platforms' lanes are still running. That step just needs a retry. I am not going to push another empty commit for it; whoever picks this up can retry the step in Buildkite, and I will follow up on anything it turns up. Unchanged from the description: with the preview tarball locally, both new fixtures pass, jsc-stress is 117/117, and the original fuzzer input no longer asserts; both fixtures fail on the engine without oven-sh/WebKit#417. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs in the diff itself. Because it bumps the WebKit/JavaScriptCore pin — and to a preview tag that the description says should be re-pointed at the merged autobuild-<sha> once oven-sh/WebKit#417 lands — a human should sign off on the engine bump and confirm the final pin before merge.
What was reviewed:
prebuiltUrl/prebuiltDestDirhandle theautobuild--prefixed version string correctly (no double prefix; cache key uses the full tag).- The two new fixtures use
//@ requireOptions(...), whichparseJSCFlagsalready handles, and are wired into thejsFixtureslist. - The behavioural fixture asserts
SuppressedErrorwith both.errorand.suppressedmessages after warming the JIT; the validation fixture relies on--validateGraph=1so it fails on release builds too.
Extended reasoning...
Overview
This PR changes the WEBKIT_VERSION constant in scripts/build/deps/webkit.ts from the SHA 7b763944… to a preview tag autobuild-preview-pr-417-fbb76610, and adds two JSC stress-test fixtures (plus registers them in jsc-stress.test.ts). The actual code fix lives in oven-sh/WebKit#417; this PR is the Bun-side pin bump and regression coverage.
Security risks
None introduced by the diff. The bump pulls a prebuilt tarball from oven-sh/WebKit's own GitHub releases, same mechanism as before. No new user-facing surface, parsing, or auth paths are touched.
Level of scrutiny
High. Although the textual diff is tiny, changing the JavaScriptCore pin swaps out the JS engine for every code path in Bun. The bump also carries oven-sh/WebKit#410 and build-only changes #411–#414 in addition to the targeted #417 fix. Engine bumps are exactly the kind of change where a maintainer should confirm CI is fully green across all lanes and decide whether to land on the preview tag or wait for the merged SHA.
Other factors
- The PR description and the follow-up comment both state the preview pin is temporary and should move to the final
autobuild-<sha>once #417 merges — that's a pending action a human needs to track. - The prior CI run (#93631) had a lane fail on a 404 for a not-yet-uploaded preview asset; the current build (#93640) was retriggered after re-uploading. A human should verify it went green.
- The test additions themselves look correct and follow the existing jsc-stress fixture conventions (
//@ requireOptions,shouldBehelper, megamorphic call-site trick to prevent inlining). No issues found there. - No prior review comments from me on this PR.
|
Nothing new to change from this review; the pin handling and the CI state are covered in the two comments above. For the record on the "verify it went green" point: in build 93640 the only failure is still the linux x64 build step (lolhtml source tarball download), which blocks the 41 x64 test lanes behind it; everything else is proceeding against the preview tarball, 113 jobs passed so far, including test lanes on the x64-asan (assert-enabled engine), alpine and windows builds. The handful of yellow entries so far are timing and GC sensitive tests that passed on retry or alone (test-fs-read-stream-pos, inspect-error-leak, sourcetextmodule-leak, napi GC condition, test-error-code-done-callback), none of them near exception handling or disposal. The x64 build step still just needs a retry in Buildkite. |
ae6e00a to
61a54bd
Compare
|
Rebased onto current main. The WebKit pin had moved five times underneath this branch, so the only conflict was the WEBKIT_VERSION line. oven-sh/WebKit#417 is now rebased onto 0cbb4a19 (main's current pin) and this branch pins its new preview, autobuild-preview-pr-417-bc08a6e1, so the engine delta against main is just that one change; the earlier note about the preview also carrying #410 no longer applies. The empty retrigger commit is gone, the fixtures are unchanged. Re-verified on the rebased branch: both fixtures still assert on the jsc shell from main's current 0cbb4a19 debug-asan prebuilt, and with the new preview |
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "0cbb4a194653231955187f9d8a2990d4b4a55266"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-417-bc08a6e1"; |
There was a problem hiding this comment.
🔴 WEBKIT_VERSION is pinned to autobuild-preview-pr-417-bc08a6e1, a preview tag for the unmerged oven-sh/WebKit#417. Per the repo's Dependencies & vendoring rules ("Never merge a pin to an ephemeral artifact (preview tags, unmerged-PR builds)"), this needs to be re-pointed at the merged main commit's autobuild-<sha> — with all 42 platform×flavor tarballs verified present — before this PR lands. The PR description already notes this; flagging so it isn't merged as-is.
Extended reasoning...
What the bug is
scripts/build/deps/webkit.ts:6 sets WEBKIT_VERSION = "autobuild-preview-pr-417-bc08a6e1". This is a preview-build tag published by oven-sh/WebKit's CI for the not-yet-merged PR oven-sh/WebKit#417, not a release tag for a commit on WebKit main.
The repo's own dependency rules in .claude/docs/landing-prs.md:47 (Dependencies & vendoring, referenced from REVIEW.md) state explicitly:
Version bumps are repo-wide, verified operations. Never merge a pin to an ephemeral artifact (preview tags, unmerged-PR builds) — swap to the merged upstream SHA and verify prebuilt artifacts exist for every platform × flavor before merge.
An autobuild-preview-pr-* tag is precisely the "preview tags, unmerged-PR builds" case that rule prohibits merging.
The code path that triggers it
prebuiltUrl() in the same file constructs the download URL from cfg.webkitVersion:
const tag = version.startsWith("autobuild-") ? version : `autobuild-${version}`;
return `https://github.com/oven-sh/WebKit/releases/download/${tag}/${name}.tar.gz`;So every fresh build with cfg.webkit === "prebuilt" (the default for CI and most local builds) fetches from https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-417-bc08a6e1/bun-webkit-<os>-<arch><suffix>.tar.gz. Preview releases are transient by convention — once oven-sh/WebKit#417 merges and gets its own autobuild-<merged-sha> release, the preview release can be (and typically is) cleaned up. At that point every fresh build 404s on the WebKit tarball, and bun bd / CI's build-bun step fails before compilation even starts.
Why nothing prevents it
There is no fallback in source.ts's prebuilt fetch — a 404 on the tarball is a hard build failure (the timeline already shows one lane 404ing on a not-yet-uploaded asset in build 93631). prebuiltDestDir() caches by version string, so machines that already have webkit-preview-pr-417-bc08a6e1<suffix> extracted keep working, which masks the problem locally but leaves every fresh checkout, new CI runner, and cache-cleared machine broken.
Step-by-step proof
- This PR merges with
WEBKIT_VERSION = "autobuild-preview-pr-417-bc08a6e1". - DFG: LiveCatchVariablePreservationPhase flushed the wrong handler's locals when one try range leads directly into another WebKit#417 merges to main; CI publishes
autobuild-<merged-sha>with the same 42 tarballs. - The now-redundant
autobuild-preview-pr-417-bc08a6e1GitHub release is deleted (standard practice for preview tags). - A developer clones bun fresh and runs
bun bd, or a CI runner starts with an empty cache. prebuiltUrl()returns.../autobuild-preview-pr-417-bc08a6e1/bun-webkit-linux-amd64-lto.tar.gz→ 404.- Build fails at the "fetch WebKit" step. Every platform × flavor is affected.
Impact
Merging as-is puts main one preview-release deletion away from being unbuildable from a clean checkout. The author has already acknowledged this in both the PR description ("the pin should move to the final autobuild-<sha> once it lands on WebKit main") and the timeline ("once it merges this should be re-pointed at the merged main sha"), so this is not a disagreement about the fix — it is the repo-mandated gate that must be satisfied before merge.
How to fix
Wait for oven-sh/WebKit#417 to land on main, then replace this line with the merged commit's 40-hex SHA (e.g. export const WEBKIT_VERSION = "<merged-sha>";) and confirm all 42 tarballs (os × arch × {musl,android,freebsd} × {debug,lto,asan}) exist under the corresponding autobuild-<sha> release before merging this PR.
|
Agreed, and that matches the plan in the description: the preview pin exists so CI can exercise the fix, it must not land. To make that explicit I have converted this PR to a draft. Once oven-sh/WebKit#417 merges I will re-point WEBKIT_VERSION at the merged commit's autobuild- release, check that all 42 tarballs are present in it (the same list the preview release has), and mark the PR ready. The version only lives in scripts/build/deps/webkit.ts on this branch (git grep finds no other copy of the current pin), so that swap is the one line. |
|
A second fuzzer input landed on the same fingerprint (same phase, same Pushed c507330 with a small deterministic version of that shape as a third fixture, |
c507330 to
298e421
Compare
|
Rebased again: main moved its pin to c6cfe90c (#39368), so the only conflict was the WEBKIT_VERSION line once more. oven-sh/WebKit#417 is now rebased onto c6cfe90c and this branch pins its new preview, autobuild-preview-pr-417-9984a386 (all 42 tarballs present), so the engine delta against main is still just that one change. With it, |
|
A third fuzzer input came in on the same fingerprint. It is the eval shape again: const v3 = { [Symbol.dispose]() {} };
for (using v5 of [v3]) {
try { SharedArrayBuffer.call(); } catch (e) {}
}
let v12 = new Int8Array(51)[44];
v12++;
while (v12) {} // never ends; this is what tiers the eval code up to the FTLThat is the shape CI for the rebased head (298e421) passed: build 99684, 179/179. Still a draft until oven-sh/WebKit#417 lands and the pin can move to the merged sha. |
…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.
…fixture A second fuzzer input hit the same assertion with the dispose method inlined into the dispose call (the method is trivial, so inlining does not split the block) and the function reached through recursion. The original input does quadratic work, so this is a small deterministic version of that shape; it fails OSR availability validation on the unfixed engine and passes with the fix.
|
Main moved its pin to eeab0404 (#39371, the upstream merge), so the WEBKIT_VERSION line conflicts again. The bug is still there in that engine: all three fixtures assert on the jsc shell from the eeab0404 debug-asan prebuilt, and the phase itself did not change in the upstream range (the file is byte-identical between c6cfe90c and eeab0404), so oven-sh/WebKit#417 is still needed. It is now rebased onto eeab0404 (same patch, head ece09c13) and its preview build is running. This branch is rebased locally onto current main with the pin pointing at that preview; I will push it once the preview release has all of its tarballs, so CI does not run against a half-uploaded release like it did in build 93631. |
298e421 to
db1e4fe
Compare
|
Pushed the rebase (db1e4fe). The preview for the rebased oven-sh/WebKit#417 is autobuild-preview-pr-417-ece09c13, built on eeab0404, and all 42 tarballs are in the release (one of its runner-side builds and then the last asset upload needed a rerun, which is why this took a while). This branch is still the same two commits, only the pin line changed in the conflict resolution. With that preview, |
Found by fuzzing. Fingerprint:
DFGOSRAvailabilityAnalysisPhase.cpp(198).Problem
Reduced fuzzer input, run through indirect eval (that is how the REPRL harness evaluates scripts; a plain function with a
usingblock hits the same thing once it reaches the FTL, see the second fixture):Debug/ASAN builds abort when the FTL compiles it:
loc21is thehasError("the body threw") flag of the disposal code the bytecode generator emits forusing. Release builds do not abort, they miscompile: once a function containing ausingblock is optimized (the DFG tier is enough; the dispose call just has to remain a real call instead of being inlined), a dispose method that throws after the body also threw comes out as the dispose method's plainErrorinstead of aSuppressedErrorholding both errors, so the body's exception is silently dropped. On a release build of the unfixed engine (the phase in question is identical from the build I used through main's current pin and WebKit main):Cause
DFG's
LiveCatchVariablePreservationPhasekeeps the locals a catch handler reads alive by insertingFlushes: in front ofSetLocals inside a try range, and whenever a block's nodes cross from one handler's range into a different one (plus at the end of the block). The handler lookup it uses to detect "the handler changed" also overwrote its cached live-at-catch set with the liveness of the handler it had just found, before the flush for the handler being left was emitted, so a transition straight from one handler into another flushed the wrong handler's locals. Plaintry/catchnever crosses handlers inside a block becauseTryNodeemits its jump over the catch block inside the try range. Theusingdisposal code ends the synthesized catch's range right after the dispose call, at a label, so the block ends there as well and its terminal carries the origin of the next bytecode, which belongs to the enclosing handler (the for-of's synthesized finally here, the user'stry/catchin the function case).hasErroris only read by that synthesized catch, so with the flush missing nothing keeps it alive across the merge where it is eitherfalseortrue; it is unavailable at the dispose call's exception exit, the exit restores it asundefined, and the catch treats that as "no pending error".Fix
oven-sh/WebKit#417: flush for the handler being left first, then compute the liveness of the handler being entered. That PR is rebased onto WebKit
eeab0404, which is exactly what main pins today (the upstream merge in #39371 did not touch this phase), so its preview build (autobuild-preview-pr-417-ece09c13, the pin in this PR) is main's engine plus that one change. The pin should move to the finalautobuild-<sha>once #417 lands.Fixtures in
test/js/bun/jsc-stress:for-using-dispose-call-live-catch-locals-ftl-validation.jsis the first fuzzer shape (eval, dispose call not inlined) andusing-inlined-dispose-live-catch-locals-ftl-validation.jsis a small deterministic version of a second fuzzer input that hit the same assertion with the dispose method inlined into the call and the function reached through recursion; both run with--validateGraph=1, so they fail on release builds of the unfixed engine too.using-dispose-throw-after-body-throw-in-jit.jschecks theSuppressedErrorbehaviour once the function is optimized. The first and third of these are also in the WebKit PR as stress tests.Verification
jscshell shipped in the0cbb4a19debug-asan prebuilt (main pinned that when the evidence was gathered) and on the shell from today's pineeab0404(loc12in the function shapes,loc21in the eval shape), the two validation fixtures also fail through the harness withUSE_SYSTEM_BUN=1, and the behaviour fixture returns the wrong result on a release build of the unfixed engine. The second fuzzer input itself asserts on the unfixed engine within its first FTL compile.bun bdagainst the preview tarball, on this branch rebased onto current main: all three fixtures pass,jsc-stress.test.tsis 117/117 before the third fixture was added, and both fuzzer inputs no longer assert (the second one completes the FTL compile that used to fail; the input itself then runs for minutes even on a release build because it recurses to stack overflow from every level of another recursion). The preview's ownjscshell passes the fixtures as well. (Before the rebase, the earlier preview also passedtest/js/web/explicit-resource-management.test.ts,test/js/bun/test/mock-disposable.test.ts,test/js/bun/resolve/lower-using-bun-target.test.ts,test/js/bun/jsc/bun-jsc.test.tsandtest/js/node/async_hooks.)SuppressedErrorscenario matches the interpreter at both the DFG and FTL tiers with the fix.Rebased onto main three times as the WebKit pin moved underneath, most recently for the upstream merge in #39371; each time the only conflict was the
WEBKIT_VERSIONline, resolved to the preview built on top of the new pin. Latest: previewece09c13oneeab0404, jsc-stress 118/118 with it, and the three fixtures still fail on theeeab0404shell.[decide:webkit] gate passed · iteration 1 · 5 files touched
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 1
evidence per changed file