Upgrade WebKit to 39862040be27 - #30096
Conversation
|
Updated 8:07 AM PT - May 2nd, 2026
❌ @Jarred-Sumner, your commit aeac651 has 2 failures in
DetailsDetails🧪 To try this PR locally: bunx bun-pr 30096That installs a local version of the PR into your bun-30096 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR updates the exported ChangesWebKit Version Bump
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 2/5 reviews remaining, refill in 25 minutes and 28 seconds. Comment |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward WEBKIT_VERSION hash bump; CI will validate the actual build.
Extended reasoning...
Overview
This PR changes a single line in scripts/build/deps/webkit.ts, updating the WEBKIT_VERSION constant from 6ef83cb6... to 181881a5.... The substantive changes (226 upstream WebKit commits, BBQCallee keepalive fix, etc.) live in oven-sh/WebKit#210; this repo's diff is purely the version pin.
Security risks
None from the diff itself — it's a constant string update that selects which prebuilt tarball / git ref to pull from oven-sh's own WebKit fork. No new code paths, no auth/crypto/permissions changes.
Level of scrutiny
Low for the bun-side change. WebKit version bumps are a routine, well-established procedure here (the most recent commit on main is literally the previous one of these). The real review surface is the WebKit PR and the CI build/test matrix, not this one-line pin. The PR description is thorough — author confirmed JSType.h is unchanged (so no JSType.zig sync needed) and flagged the header-reshuffle risk with a remediation pointer if CI hits include errors.
Other factors
The only finding is a pre-existing stale doc path in .claude/commands/upgrade-webkit.md referencing the old SetupWebKit.cmake location — not introduced here and purely a drive-by nit. No outstanding reviewer comments. Buildkite is running; any actual breakage from the new WebKit will surface there.
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "6ef83cb658722ff1f33f4a4c9335fb094bed4c6b"; | ||
| export const WEBKIT_VERSION = "181881a5aadc46dafb8ae44d25cf21e2c321a4f0"; |
There was a problem hiding this comment.
🟣 Pre-existing nit, not introduced here: the /upgrade-webkit slash command at .claude/commands/upgrade-webkit.md:18 still says to update WEBKIT_VERSION in cmake/tools/SetupWebKit.cmake, which no longer exists — it now lives in scripts/build/deps/webkit.ts (which this PR correctly updated). Worth a drive-by fix since that file documents the exact procedure this PR performs.
Extended reasoning...
What's stale
.claude/commands/upgrade-webkit.md is the slash-command playbook for bumping WebKit. Line 18 instructs:
Update
WEBKIT_VERSIONincmake/tools/SetupWebKit.cmaketo the commit SHA of your new commit
That file no longer exists in the repo. WEBKIT_VERSION was moved to scripts/build/deps/webkit.ts:6 when the build system migrated from CMake helpers to the TS-based dep scripts — and indeed, that's exactly the file this PR edits.
Why it matters (mildly)
This is the documented procedure for the very operation this PR performs. The next person (or agent) who runs /upgrade-webkit will be told to edit a nonexistent file, then has to grep for WEBKIT_VERSION to find the real location. It's a minor speed bump rather than a correctness issue, but it's directly in scope for a WebKit-bump PR.
Step-by-step proof
.claude/commands/upgrade-webkit.md:18referencescmake/tools/SetupWebKit.cmake.ls cmake/tools/SetupWebKit.cmake→ no such file; glob for**/SetupWebKit.cmakereturns nothing.grep -r WEBKIT_VERSIONresolves toscripts/build/deps/webkit.ts:6— the line this PR changes.- Therefore the slash command points at a dead path while the live definition is in
webkit.ts.
Why nothing else catches it
It's a markdown doc under .claude/commands/; nothing in the build or CI validates that the referenced path exists. The PR author clearly already knows the right location (they edited webkit.ts), so the staleness only bites whoever follows the written instructions next time.
Fix
One-line change in .claude/commands/upgrade-webkit.md:18:
- Update `WEBKIT_VERSION` in `cmake/tools/SetupWebKit.cmake` to the commit SHA of your new commit
+ Update `WEBKIT_VERSION` in `scripts/build/deps/webkit.ts` to the commit SHA of your new commitSeverity
pre_existing — not introduced by this PR, purely a stale internal doc. Flagging only because it's the documented workflow for exactly this change and the fix is trivial.
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "6ef83cb658722ff1f33f4a4c9335fb094bed4c6b"; | ||
| export const WEBKIT_VERSION = "c5293afddb249ee72b2e9b88f6856a8a374514f7"; |
There was a problem hiding this comment.
🟣 Pre-existing, but it directly affects the testing this PR receives: .buildkite/ci.mjs:670 (hasWebKitChanges()) still looks for SetupWebKit.cmake in the changed-files list — that file no longer exists, so the --jit-stress flag at line 683 and the 30-min timeout at line 719 are never applied on WebKit bumps. The predicate should check scripts/build/deps/webkit.ts instead. (Distinct from the .claude/commands/upgrade-webkit.md doc-staleness comment above — this one silently drops CI coverage.)
Extended reasoning...
What the bug is
.buildkite/ci.mjs:668-671 defines:
function hasWebKitChanges(options) {
const { changedFiles = [] } = options;
return changedFiles.some(file => file.includes("SetupWebKit.cmake"));
}SetupWebKit.cmake no longer exists anywhere in the repo — WEBKIT_VERSION was migrated to scripts/build/deps/webkit.ts when the build moved to TS-based dep scripts (the very file this PR edits). So hasWebKitChanges() is now permanently false on every PR.
The code path
The function has exactly two consumers, both in getVerifyBaselineStep():
- Line 683 —
const jitStressFlag = hasWebKitChanges(options) ? " --jit-stress" : "";— appended tobun scripts/verify-baseline.tsat line 723. - Line 719 —
timeout_in_minutes: hasWebKitChanges(options) ? 30 : 10— gives the JIT-stress run room to finish.
When the predicate is false, the verify-baseline step runs without --jit-stress and with a 10-minute timeout. Nothing errors; the extra coverage is just silently omitted.
Why nothing else catches it
There's no alternate check — grep for webkit.ts in .buildkite/ returns nothing. changedFiles is the Buildkite-computed list of paths touched by the PR, and "scripts/build/deps/webkit.ts".includes("SetupWebKit.cmake") is false. The step still appears in the pipeline and passes, so there's no signal that the JIT-stress mode was skipped.
Step-by-step proof on this PR
- This PR's only changed file is
scripts/build/deps/webkit.ts. options.changedFiles = ["scripts/build/deps/webkit.ts"]..some(f => f.includes("SetupWebKit.cmake"))→false.- Line 683:
jitStressFlag = ""; line 719:timeout_in_minutes = 10. - Line 723 runs
bun scripts/verify-baseline.ts --binary … --emulator …without--jit-stress. - The JIT stress coverage that exists specifically to catch JSC regressions on WebKit upgrades does not run on this WebKit upgrade.
Impact
WebKit version bumps are precisely the PR class this check was added for — they pull in DFG/FTL/B3 changes (this one alone touches DFG/FTL string equality, B3 bit-rotate, IntegerRangeOptimizations, etc.). The --jit-stress baseline verification is the guardrail meant to shake those out under SDE emulation before merge. With the predicate dead, every WebKit bump since the CMake → TS migration has merged without that coverage.
Fix
function hasWebKitChanges(options) {
const { changedFiles = [] } = options;
- return changedFiles.some(file => file.includes("SetupWebKit.cmake"));
+ return changedFiles.some(file => file.includes("scripts/build/deps/webkit.ts"));
}(Or check both paths if you want backward-compat with branches that pre-date the migration.)
Relationship to the earlier comment
This is not a duplicate of the .claude/commands/upgrade-webkit.md doc-staleness comment already on the PR. That one is a stale instruction in a markdown playbook; this one is a dead predicate in CI with concrete behavioral effect (lost test coverage + wrong timeout) on exactly this PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build/deps/webkit.ts`:
- Line 6: The WEBKIT_VERSION constant in scripts/build/deps/webkit.ts points to
a commit tag ("a10fd987b87b7101a465d4f3faad0539d486adde") whose release has no
prebuilt assets and causes a 404; update the exported WEBKIT_VERSION value to a
valid WebKit release tag that actually publishes the
bun-webkit-linux-amd64.tar.gz asset (match the repo's release naming, e.g. the
"autobuild-<tag>" format), replace the string in WEBKIT_VERSION, and verify the
computed URL
https://github.com/oven-sh/WebKit/releases/download/autobuild-<NEW_VALUE>/bun-webkit-linux-amd64.tar.gz
returns HTTP 200 before committing.
🪄 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: 3f345d52-8dd7-4595-b575-30fe5c49d18e
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
|
|
…e + #211 ICU filter)
2996fe0 to
aeac651
Compare
Jarred-Sumner
changed the title
|
✅ |
…lated residual WebKit JIT crash) WebKit hits ERROR_INVALID_ADDRESS (Windows error 487) in VirtualAlloc(MEM_COMMIT) during IPInt→BBQ tier-up specifically on ipint-bbq-osr-with-try3.js on Windows x64. Related BBQCallee UAF was fixed via WebKit 39862040be27 (#30096 — already in this PR's canary) but a residual access-violation in the IPInt→BBQ path remains on this one fixture. The other ipint-bbq-osr-with-tryN.js fixtures pass. Mirrors the test.todoIf pattern PR #30012 removed for JSPI Wasm fixtures after their upstream fix landed. Scope-narrowed to exactly one fixture on exactly one platform; the other 82 Wasm stress fixtures continue to run normally on Windows x64. Unrelated to embedded-native-module extraction (#29585) — the VirtualAlloc failure is inside WebKit's wasm JIT tier-up path, which this PR doesn't touch.
…lated residual WebKit JIT crash) WebKit hits ERROR_INVALID_ADDRESS (Windows error 487) in VirtualAlloc(MEM_COMMIT) during IPInt→BBQ tier-up specifically on ipint-bbq-osr-with-try3.js on Windows x64. Related BBQCallee UAF was fixed via WebKit 39862040be27 (#30096 — already in this PR's canary) but a residual access-violation in the IPInt→BBQ path remains on this one fixture. The other ipint-bbq-osr-with-tryN.js fixtures pass. Mirrors the test.todoIf pattern PR #30012 removed for JSPI Wasm fixtures after their upstream fix landed. Scope-narrowed to exactly one fixture on exactly one platform; the other 82 Wasm stress fixtures continue to run normally on Windows x64. Unrelated to embedded-native-module extraction (#29585) — the VirtualAlloc failure is inside WebKit's wasm JIT tier-up path, which this PR doesn't touch.
2 participants
Bumps
WEBKIT_VERSIONto oven-sh/WebKit@181881a5aadc — see oven-sh/WebKit#210.Syncs with upstream WebKit
6cc6aff95e13(226 commits, 48 in JSC/WTF/bmalloc) and includes the BBQCallee keepalive fix from oven-sh/WebKit#209.Highlights:
pas_process_is_shutting_down()TLS guardNo
JSType.hchanges —JSType.zigunchanged.Header reshuffling (
5cdc5c93d15b) moved several inline definitions into*InlinesLight.hheaders and addedJSDOMBindingFacade.h. Local build of Bun against this WebKit compiled clean; if CI surfaces missing-include errors insrc/bun.js/bindings/webcore/, compare againstSource/WebCore/bindings/scripts/test/JS/*.cppfor the new include set.Supersedes oven-sh/WebKit#209.