inspector: bump WebKit so Runtime.evaluate survives validateExceptionChecks - #35333
inspector: bump WebKit so Runtime.evaluate survives validateExceptionChecks#35333robobun wants to merge 4 commits into
Conversation
…Checks The injected-script prototype host functions in JavaScriptCore declare a ThrowScope for their type check and then tail-call into an impl that declares its own, without releasing the outer scope. Under BUN_JSC_validateExceptionChecks=1 the first Runtime.evaluate over an inspector connection aborts with 'Unchecked JS exception' at jsInjectedScriptHostPrototypeFunctionEvaluateWithScopeExtension. The fix is oven-sh/WebKit#328 (wrap every such return in RELEASE_AND_RETURN). This bumps WEBKIT_VERSION to its preview build and adds a debug-only regression test that spawns an inspectee with validateExceptionChecks=1, sends Runtime.evaluate, and asserts the reply arrives (it never does on the unfixed build: the child SIGABRTs and the WebSocket closes 1006).
WalkthroughThe PR updates the default WebKit preview build identifier and adds a debug-only inspector test that evaluates ChangesWebKit build selection
Inspector Runtime.evaluate validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:25 PM PT - Jul 23rd, 2026
❌ @robobun, your commit afe1ec4 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35333That installs a local version of the PR into your bun-35333 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Preview of oven-sh/WebKit#328: RELEASE_AND_RETURN on the inspector | ||
| // injected-script prototype host functions so Runtime.evaluate survives | ||
| // validateExceptionChecks=1. | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-328-31913c3e"; |
There was a problem hiding this comment.
🔴 WEBKIT_VERSION is pinned to autobuild-preview-pr-328-31913c3e, an ephemeral preview tag that GitHub deletes as soon as oven-sh/WebKit#328 merges or closes — at which point every fresh build of bun main 404s downloading WebKit. Before this lands, swap the pin to the merged 40-hex oven-sh/WebKit main sha (and drop the "Preview of…" comment).
Extended reasoning...
What the bug is
scripts/build/deps/webkit.ts:16 sets WEBKIT_VERSION = "autobuild-preview-pr-328-31913c3e". This is an autobuild-preview-pr-* tag — a temporary GitHub release that oven-sh/WebKit's CI publishes while a WebKit PR is open, and deletes when that PR merges or closes. The previous pin was a permanent 40-hex commit sha (a40d462206e1caf8388062120acde61e37a4ae7d).
The code path that triggers it
For a prebuilt WebKit (the default, cfg.webkit === "prebuilt"), webkit.source() calls prebuiltUrl(), which turns cfg.webkitVersion into a release-download URL. autobuild-* prefixes are handled (const tag = version.startsWith("autobuild-") ? version : \autobuild-${version}`), so the pin *works today* — the URL resolves to https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-328-31913c3e/bun-webkit--.tar.gz. At build time, ninja invokes fetch-cli.ts→download.ts::downloadWithRetry()to fetch that tarball intocfg.cacheDir`.
Why existing code doesn't prevent it
The build system explicitly supports preview pins so a WebKit PR can be tested inside a bun PR — that's why prebuiltUrl(), prebuiltDestDir(), and depVersionsHeader.ts all special-case the autobuild- prefix. But the same codebase also documents that preview pins must not persist. scripts/build/download.ts:276-295 has a dedicated prebuiltDownloadError() branch for exactly this failure:
autobuild-preview-pr-*WebKit tags are the sharp edge: GitHub deletes the preview release when the PR merges or closes, so every build 404s at once.
and the remediation it prints is:
Set WEBKIT_VERSION in scripts/build/deps/webkit.ts to the merged main sha
So the repo's own error-handling treats a preview pin on main as a bad pin, not a supported state. Nothing at configure time rejects the preview tag — the build only fails once the release is gone, which is precisely when it's most disruptive (post-merge, on other people's machines).
Impact
If this PR merges to main as-is and oven-sh/WebKit#328 subsequently merges (which is the intended outcome — this bun PR exists to ship that WebKit fix), the preview release is deleted. From that moment:
- Every fresh clone of bun
mainfailsbun bd/bun run buildat the WebKit download step with HTTP 404. - Every CI lane without a warm
$BUN_INSTALL/build-cache/webkit-preview-pr-328-31913c3e*extraction fails the same way. - Developers with a cached extraction keep working, masking the break for whoever lands it.
The PR description already acknowledges this is a preview build ("the first CI run here will fail … until the preview build … finishes publishing"), and the added code comment says "Preview of oven-sh/WebKit#328" — so the author is aware. This finding is the merge-gate reminder, not an oversight report.
Step-by-step proof
- This PR merges to
mainwithWEBKIT_VERSION = "autobuild-preview-pr-328-31913c3e". - inspector: release throw scope before tail-calling impl in injected-script prototype host functions WebKit#328 merges. oven-sh/WebKit's release automation deletes the
autobuild-preview-pr-328-31913c3erelease (per the doc comment atdownload.ts:278-279). - A contributor clones bun
mainfresh and runsbun bd. - Configure emits a
dep_fetchedge for WebKit; ninja runsfetch-cli.ts dep WebKit … https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-328-31913c3e/bun-webkit-linux-amd64-debug.tar.gz. - GitHub returns HTTP 404.
downloadWithRetry()throws;prebuiltDownloadError()matchesname === "WebKit" && missing && url.includes("/autobuild-preview-pr-")and raisesBuildError: WebKit preview release is gone: … HTTP 404. - The build fails. Every fresh build of
mainis broken until someone lands a follow-up commit changingWEBKIT_VERSION.
How to fix
Before merging this PR: merge oven-sh/WebKit#328 first, then update WEBKIT_VERSION here to the resulting 40-hex commit sha on oven-sh/WebKit main (and replace the "Preview of oven-sh/WebKit#328" comment with the permanent description, matching the style of the surrounding lines). The PR description's own workflow — "re-run once autobuild-preview-pr-328-31913c3e appears" — is fine for iterating on the branch, but the final merged commit must carry the permanent sha.
There was a problem hiding this comment.
Intentional for now: the preview pin lets CI exercise the fix before oven-sh/WebKit#328 merges. I will swap this to the merged 40-hex main sha (and drop the preview comment) before this PR is marked ready to merge. Noted in the PR body under Notes as well.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/deps/webkit.ts`:
- Around line 13-16: Update WEBKIT_VERSION to reference the merged WebKit
commit’s stable release or commit-based artifact instead of the temporary
autobuild-preview-pr-328-31913c3e tag, ensuring default downloads remain
available after the upstream PR lifecycle ends.
In `@test/cli/inspect/inspect.test.ts`:
- Line 308: Update the independent subprocess test named “Runtime.evaluate does
not trip exception-check validation” to use the concurrent test variant while
preserving its existing skipIf condition and test body.
- Around line 347-350: Update the post-open promise in the message/close race
around the WebSocket event listeners to accept a reject callback and add an
error listener that rejects with the socket error. Preserve the existing message
and close resolution behavior while ensuring post-open errors cannot leave the
promise pending.
🪄 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: 905760cf-1bde-4796-a26f-65d854f89ec1
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/cli/inspect/inspect.test.ts
| // Preview of oven-sh/WebKit#328: RELEASE_AND_RETURN on the inspector | ||
| // injected-script prototype host functions so Runtime.evaluate survives | ||
| // validateExceptionChecks=1. | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-328-31913c3e"; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== scripts/build/deps/webkit.ts ==\n'
cat -n scripts/build/deps/webkit.ts | sed -n '1,120p'
printf '\n== references to WEBKIT_VERSION ==\n'
rg -n "WEBKIT_VERSION|autobuild-preview-pr-328-31913c3e|webkit" scripts packages -g '!**/node_modules/**' | sed -n '1,200p'
printf '\n== dependency docs or notes ==\n'
rg -n "preview-pr|WebKit#328|validateExceptionChecks|autobuild-preview" . -g '!**/node_modules/**' | sed -n '1,200p'Repository: oven-sh/bun
Length of output: 21931
🌐 Web query:
WebKit autobuild-preview-pr-328-31913c3e release preview pr 328 validateExceptionChecks=1
💡 Result:
The term "validateExceptionChecks" refers to a specific diagnostic option in WebKit's JavaScriptCore (JSC) engine [1][2]. When set to true (or 1), this option enables verification that necessary exception checks are performed within the engine [2], which helps identify and debug missing exception handling in the code [3][4]. The identifier "autobuild-preview-pr-328-31913c3e" appears to be a specific build artifact or container image tag, likely generated by a CI/CD pipeline for a pull request (PR). While "pr-328" may refer to a specific pull request in a repository, the exact combination provided does not correspond to a singular, globally recognized WebKit release or public document. The structure is consistent with automated PR preview environments where unique tags (like 31913c3e) are used to track specific builds [5][6][7]. In summary: - validateExceptionChecks: A JSC engine configuration option used for validating exception handling [1][2]. - autobuild-preview-pr-328-31913c3e: An internal identifier for a transient build environment, typical of automated CI systems [5][6][7].
Citations:
- 1: https://github.com/WebKit/webkit/blob/master/Tools/Scripts/run-jsc-stress-tests
- 2: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/OptionsList.h
- 3: https://trac.webkit.org/timeline?from=2019-05-01T13%3A44%3A36-07%3A00&precision=second
- 4: https://trac.webkit.org/timeline?from=2021-05-09T11%3A39%3A30-07%3A00&precision=second
- 5: https://github.com/zimmertr/bluebird/blob/20a276b7bd50e896ceb5a77c2fefb8b00e57889d/.github/workflows/pr-preview.yml
- 6: https://github.com/aerolalit/homespun/blob/4b15702ce26bc40720fa3247230193966389fb96/.github/workflows/pr-preview.yml
- 7: https://github.com/cobaltcore-dev/aurora-dashboard/blob/a215e0db713503d0497520879e56b8e9712c28c9/.github/workflows/build-push-aurora-pr-preview.yaml
Replace the preview WebKit pin before release. scripts/build/deps/webkit.ts:16 points WEBKIT_VERSION at an autobuild-preview-pr-* tag, and GitHub can remove those releases when the PR merges or closes. Pin the merged WebKit commit instead so default downloads keep working.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build/deps/webkit.ts` around lines 13 - 16, Update WEBKIT_VERSION to
reference the merged WebKit commit’s stable release or commit-based artifact
instead of the temporary autobuild-preview-pr-328-31913c3e tag, ensuring default
downloads remain available after the upstream PR lifecycle ends.
There was a problem hiding this comment.
Acknowledged: the pin stays on the preview tag while iterating on this branch, and afe1ec4 adds a source-lint test that fails on any autobuild-preview-* value so CI itself blocks the merge until the pin is swapped to the merged main sha. Leaving this thread open as the merge-gate reminder alongside the earlier one.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== referenced commit =='
git show --no-ext-diff --format=fuller --stat afe1ec4
printf '%s\n' '== referenced commit patch =='
git show --no-ext-diff --format= --find-renames afe1ec4 -- \
scripts/build/deps/webkit.ts test scripts | sed -n '1,280p'
printf '%s\n' '== preview-pin guard candidates on PR head =='
rg -n -C 4 'autobuild-preview|WEBKIT_VERSION' \
scripts test .github -g '!**/node_modules/**' -g '!**/.git/**' | sed -n '1,280p'Length of output: 19755
@robobun, verified. afe1ec4 adds test/internal/source-lints/webkit-prebuilt-url.test.ts, whose new assertion rejects WEBKIT_VERSION values beginning with autobuild-preview-. Since the current pin still uses that prefix, CI will intentionally fail until it is swapped for the merged WebKit SHA.
That fully addresses the risk during iteration while preserving the required pre-merge action. Keeping this thread open as the merge-gate reminder is appropriate.
✏️ Learnings added
Learnt from: robobun
Repo: oven-sh/bun PR: 35333
File: scripts/build/deps/webkit.ts:13-16
Timestamp: 2026-07-24T00:19:05.827Z
Learning: For oven-sh/bun PR `#35333`, `test/internal/source-lints/webkit-prebuilt-url.test.ts` intentionally rejects `WEBKIT_VERSION` values starting with `autobuild-preview-`. A preview WebKit pin may be used while iterating on the branch, but it must be replaced with the merged WebKit main SHA before merge; the source-lint test is the CI merge gate.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: robobun
Repo: oven-sh/bun PR: 32761
File: scripts/build/deps/ghostty-vt.ts:1-18
Timestamp: 2026-06-26T13:57:16.037Z
Learning: For dependency definition files in scripts/build/deps/*.ts, keep any top-of-file header comment limited to dependency-specific context (e.g., why a particular commit/version is pinned and how related package hashes or hashes-to-pins are connected). Follow the existing convention used by the other dependency files. For build flags in these dep files that have a non-obvious invariant or safety rationale (e.g., ODR concerns, sanitizer/runtime requirements, or other correctness constraints), prefer a short comment placed next to that specific flag rather than a long file-level “playbook” comment.
There was a problem hiding this comment.
The two test-diagnostic notes from the last pass are addressed in b29be6c — urlPromise now rejects on early inspectee exit, and the SIGABRT branch surfaces the captured stderr in the failure message. Deferring to a human for the WebKit bump itself: WEBKIT_VERSION is still on the ephemeral autobuild-preview-pr-328-* tag (acknowledged as pending the upstream merge), and CI is red across all build lanes on the tarball fetch.
Checked this pass:
- The reworked drain/assert ordering —
drainedalways settles (rejectUrl on an already-resolved promise is a no-op), soPromise.all([child.exited, drained])can't hang on the happy path. await using child+ explicitchild.kill()infinally— redundant but harmless; no leaked subprocess on any exit path.prebuiltUrl()/prebuiltDestDir()handling of theautobuild-prefix — the preview tag resolves correctly today; the concern is only post-upstream-merge deletion.
Extended reasoning...
Overview
Two files: scripts/build/deps/webkit.ts swaps WEBKIT_VERSION from a 40-hex sha to an autobuild-preview-pr-328-31913c3e preview tag, and test/cli/inspect/inspect.test.ts gains a debug-only regression test that spawns an inspectee under BUN_JSC_validateExceptionChecks=1, connects over WebSocket, sends Runtime.evaluate, and asserts the reply arrives (rather than the process SIGABRTing on the unchecked-exception assertion in JSInjectedScriptHostPrototype.cpp). The actual fix is in oven-sh/WebKit#328; this PR just consumes it.
Security risks
None. The test spawns a local subprocess with --inspect-wait=127.0.0.1:0 and connects over loopback; no external network, no auth/crypto/permissions surface. The WebKit-side change (RELEASE_AND_RETURN wrapping in inspector prototype host functions) is a debug-assertion hygiene fix with no release-build behavior change.
Level of scrutiny
High — not because the diff here is complex, but because WEBKIT_VERSION is the single most load-bearing build pin in the repo. Any bump pulls in whatever else landed on oven-sh/WebKit main between the old and new pins (the PR body notes at least one other change, the Date.parse Unicode-whitespace fold from #324), and preview tags are deleted when the upstream PR closes. A human maintainer needs to sign off on the WebKit rev and confirm the pin is swapped to the permanent sha before merge.
Other factors
- CI (Build #79026) is failing on every
build-bun/build-cpplane, consistent with the PR body's note that the preview tarball hadn't finished publishing. This needs to go green before the PR is meaningful to review further. - The author already acknowledged the preview-pin comment as intentional-for-now with a commitment to swap before merge; that thread is the merge gate.
- The b29be6c rework of the test correctly addresses both prior 🟡 findings:
replyis hoisted, the assertion runs afterdrainedcompletes, and the SIGABRT branch throws with both the reply shape and full stderr. TherejectUrlcall after the drain loop is safe on the happy path becausewithResolversignores settle-after-settle. - The test is
skipIf(!isDebug)which is correct (validateExceptionChecks compiles out of release), so it adds zero runtime to release CI lanes.
… too test/expectations.txt quarantined test/cli/inspect/inspect.test.ts on ASAN as a TIMEOUT ever since ASAN CI was enabled (d8a69d6). That timeout is this exact bug: the describe("websocket") tests send Runtime.evaluate with bunEnv (which inherits the runner's BUN_JSC_validateExceptionChecks=1 on ASAN), the inspectee SIGABRTs before replying, and the unawaited .resolves assertion then hangs. With the WebKit fix those tests pass, so remove the quarantine. ENABLE_EXCEPTION_SCOPE_VERIFICATION is (ASSERT_ENABLED || ASAN_ENABLED), and the build forces assertions on for ASAN, so the explicit regression test should run on release-asan as well as debug; gate on (!isDebug && !isASAN) instead of just !isDebug. Also wire the post-open WebSocket error event to rejection, and add a source-lint test that fails when WEBKIT_VERSION is an autobuild-preview-* tag so a preview pin cannot be merged to main by accident (it is the merge gate for this PR until oven-sh/WebKit#328 lands and the pin is swapped to the merged sha).
|
Status: blocked on the oven-sh/WebKit#328 preview release publishing. 35 of 36 WebKit CI jobs passed on the first run; the one Next action: once the preview release is published, push any commit to this branch (or comment here) to re-run; I will then build against the preview and verify Two intentional red tests once the build succeeds:
|
2 participants
What does this PR do?
Any
Runtime.evaluate(orDebugger.evaluateOnCallFrame) over an inspector connection aborts the inspected process underBUN_JSC_validateExceptionChecks=1:This was surfaced by #31823, which currently exempts its
inspector.open()fixture from exception-check validation on ASAN to work around it. It is also whytest/cli/inspect/inspect.test.tshas been quarantined on ASAN since ASAN CI was enabled: itsdescribe("websocket")tests spawn inspectees withbunEnv(which inherits the runner'sBUN_JSC_validateExceptionChecks=1) and sendRuntime.evaluate, the inspectee SIGABRTs before replying, and the file times out.Cause
Every host function in
JSInjectedScriptHostPrototype.cppandJSJavaScriptCallFramePrototype.cppdeclares aThrowScopefor thedynamicDowncasttype error and then tail-calls into aJSInjectedScriptHost/JSJavaScriptCallFramemember. Thirteen of those members declare their ownThrowScope(evaluateWithScopeExtension,getInternalProperties,iteratorEntries,queryInstances,queryHolders,weakMapEntries,weakSetEntries,getOwnPrivatePropertySymbols,getOwnPrivatePropertyMethods,isPromiseRejectedWithNativeGetterTypeError, andJSJavaScriptCallFrame'sevaluateWithScopeExtension/scopeDescriptions/scopeChain). The outer scope is never released before the inner one is created, so exception-scope validation trips on the first call. The same code is present in upstream WebKit.Fix
oven-sh/WebKit#328 wraps all 31
return JSValue::encode(castedThis->...)sites in both files inRELEASE_AND_RETURN(scope, ...). This PR:WEBKIT_VERSIONto that change's preview build;[ ASAN ] test/cli/inspect/inspect.test.ts [ TIMEOUT ]fromtest/expectations.txt, so the existing websocket tests exerciseRuntime.evaluateunder the ambientvalidateExceptionChecksflag on the ASAN lane again;test/cli/inspect/inspect.test.tsthat spawns an inspectee withBUN_JSC_validateExceptionChecks=1, sendsRuntime.evaluate, and asserts the reply arrives (gated onisDebug || isASAN, sinceENABLE_EXCEPTION_SCOPE_VERIFICATIONisASSERT_ENABLED || ASAN_ENABLED);test/internal/source-lints/webkit-prebuilt-url.test.tsthat fails whenWEBKIT_VERSIONis anautobuild-preview-*tag, so a preview pin cannot be merged tomainby accident.Verification
With the current
WEBKIT_VERSION(a40d462206…, debug build) the new test fails because the inspectee SIGABRTs before replying and the WebSocket closes 1006:fail-before (debug build, current WebKit)
Merge gate
WEBKIT_VERSIONis currently pinned toautobuild-preview-pr-328-31913c3eso CI can exercise the fix before oven-sh/WebKit#328 merges. Preview releases are deleted when the upstream PR closes, so before merging this PR, oven-sh/WebKit#328 must be merged first andWEBKIT_VERSIONhere swapped to the resulting 40-hexmainsha. The newwebkit-prebuilt-url.test.tslint test fails until that swap is made, so CI enforces this.The preview build also includes
81ea8578e5(oven-sh/WebKit#324, theDate.parseUnicode-whitespace fold), which sits between the current pin and oven-sh/WebKitmain.The fix lives in
scripts/build/deps/webkit.ts(notsrc/), so asrc/-stashing fail-before check sees the new WebKit in both arms; the fail-before evidence above is against a build with the current pin.