Fix stale-structure abort when a lazy Bun property builder throws mid-lookup - #37001
Fix stale-structure abort when a lazy Bun property builder throws mid-lookup#37001robobun wants to merge 5 commits into
Conversation
WalkthroughChangesThe WebKit pin now uses a PR 390 preview build. Bun SQL constructors no longer report exceptions through Bun SQL exception handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Those three are earlier passes at the same crash family, each working around it on the Bun side only: #33211 wraps the lazy builders so a throw is cleared and reified as undefined, #30245 rewrites the SQL builders (plus an unrelated Bun.inspect fix) to avoid the null JSCell deref, and #33418 covers the worker.terminate() interruption variant. This PR fixes the engine-level gap those were steering around. JSObject::getPropertySlot kept using the structure it read before getOwnNonIndexPropertySlot, and a static-table reification that transitions the object and then misses (the builder threw) leaves that pointer stale, which is the assert the fuzzer keeps hitting. That fix is oven-sh/WebKit#390, pinned here as its preview autobuild. With it in place a throwing builder is a legal state: the exception propagates to the caller like any other failed property read, so no swallowing is needed. The only textual overlap with #33211 is the two deleted BUN_DEBUG lines in the SQL builders; the rest is disjoint. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Good catch on the undrained stderr pipe, fixed in 2564ffa: the test now drains both pipes and asserts the combined { stdout, stderr, exitCode } object, so a regression surfaces the assertion text instead of just a wrong exit code. On CI: the red builds so far are all the vendor fetch step 404ing on the pinned WebKit preview tag, which publishes when oven-sh/WebKit#390's autobuild finishes. The next push or retry after that lands should build normally. |
|
Updated 6:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit 379f322 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 37001That installs a local version of the PR into your bun-37001 --bun |
There was a problem hiding this comment.
Thanks for addressing the stderr-drain nit — the test now asserts the combined { stdout, stderr, exitCode } object. I didn't find any further issues in this revision, but this needs a human reviewer: the load-bearing fix is a WebKit engine change (oven-sh/WebKit#390) pinned here via a preview autobuild tag, and there are three overlapping open PRs (#33211, #30245, #33418) targeting the same crash family that need reconciling.
What was reviewed
defaultBunSQLObject/constructBunSQLObject: removing theBUN_DEBUG-onlyreportUncaughtExceptionAtEventLoopis correct — it consumed the pending exception soRETURN_IF_EXCEPTIONfell through to a null-JSValue.getObject().prebuiltDestDir/prebuiltUrlinwebkit.tshandle theautobuild-prefixed tag correctly (no double-prefix, cache dir keyed on the full tag).- New test: subprocess pipes drained,
phase: 1assertion guards that the trigger path is still exercised.
Extended reasoning...
Overview
This PR fixes a debug-build assertion abort (ASSERT(object->structure() == this) in Structure::storedPrototype) that the fuzzer hits when a lazy Bun.* static-table property builder transitions the Bun object and then throws mid-getPropertySlot. Three files touched:
scripts/build/deps/webkit.ts: bumpsWEBKIT_VERSIONfrom a commit sha toautobuild-preview-pr-390-c3a156ea, a preview build of oven-sh/WebKit#390. That WebKit PR is the actual engine fix — it reloads the object's structure before the prototype step inJSObject::getPropertySlot.src/jsc/bindings/BunObject.cpp: deletes two#if BUN_DEBUGblocks indefaultBunSQLObject/constructBunSQLObjectthat calledreportUncaughtExceptionAtEventLoopafterrequireId. That call consumed the pending exception, soRETURN_IF_EXCEPTIONfell through andsqlValue.getObject()ran on an empty JSValue.test/js/bun/util/BunObject.test.ts: adds a spawned-subprocess regression test using a Proxy onglobalThis.Errorto make theBun.sqlbuilder re-enter the Bun object (viaBun.semver) and throw.
Security risks
None identified. The BunObject.cpp change removes debug-only diagnostic code; release builds are unchanged there. The test is hermetic (subprocess with bunEnv, no network). The WebKit change itself is out of scope for this diff.
Level of scrutiny
High, and beyond what I can sign off on:
- The core fix is a WebKit engine change in a separate repository. This diff only pins a preview autobuild tag; the actual
JSObject::getPropertySlotmodification isn't reviewable here. A JSC change to the property-lookup fast path affects every property read in the runtime and warrants maintainer review of the upstream PR. - The pin is to a preview build of an unmerged WebKit PR, not a merged commit. A human should confirm this is the intended workflow (vs. waiting for oven-sh/WebKit#390 to merge and pinning the resulting sha).
- Three open PRs (#33211, #30245, #33418) attack the same crash family with Bun-side workarounds. The author's response explains why this engine-level fix supersedes them, but a maintainer needs to decide which to close.
Other factors
- My prior review (the undrained-stderr nit) was addressed in 2564ffa; the test now drains both pipes and asserts the combined object.
- I checked
prebuiltUrl/prebuiltDestDirinwebkit.tsfor the new tag format:prebuiltUrlalready handles theautobuild-prefix (no double-prefixing), andprebuiltDestDiruses the full tag when it starts withautobuild-, so the cache-dir key is correct. - The BunObject.cpp deletion is straightforward and matches the release-build code path exactly — no behavioral change outside
BUN_DEBUG. - The PR description explicitly notes the test passes under
USE_SYSTEM_BUN=1by design (release builds never fired the assert). That's an acknowledged exception to the usual rule since the guarded failure is a debug-only assert. - CI was still building at the time of this review (initial runs 404'd on the not-yet-published preview tag).
|
On the preview-pin question: the plan is the usual two-step. This PR pins the preview tag so CI exercises the exact JSC change; once oven-sh/WebKit#390 merges, the pin moves to the merge commit sha (either here before landing or as the routine WebKit upgrade that follows). The preview tag stays downloadable either way. CI at 0a3a39f: 187 of 196 jobs passed so far. The only failures are the two debian x64-asan test lanes, both from tests unrelated to this diff: worker-transfer-terminate-stress.test.ts (a known intermittent terminate-during-transfer abort, documented in that test's own header and the subject of #33418) and AsyncLocalStorage-tracking.test.ts (pre-existing RSA keygen leak, also failing on main). Both are reported for main-side triage. |
|
Fuzzing hit the same assert through a different trigger, minimized to: globalThis.Symbol = undefined;
try { Bun.sql } catch {}Clobbering Verified against this branch with the pinned WebKit preview: the one-liner exits 0 with the TypeError caught. Unlike the |
0a3a39f to
88d2ba7
Compare
There was a problem hiding this comment.
I reviewed this PR and found no bugs; the earlier stderr-drain nit was addressed. Because it pins the entire runtime to an unmerged WebKit preview build (oven-sh/WebKit#390) and overlaps with three earlier PRs on the same crash family, a human should sign off on the landing sequence.
What was reviewed:
BunObject.cpp: verified removingreportUncaughtExceptionAtEventLoopleavesRETURN_IF_EXCEPTIONas the exception path — no other callers ofscopebetweenrequireIdand the check.webkit.ts: theautobuild-prefix handling inprebuiltUrl/prebuiltDestDiralready accommodates the new tag format.- Test: subprocess drains all pipes, asserts combined object, and
phase: 1guards the trigger staying live.
Extended reasoning...
Overview
Three files: scripts/build/deps/webkit.ts swaps the WebKit pin from commit sha ddea7131… to preview tag autobuild-preview-pr-390-01c983d8; src/jsc/bindings/BunObject.cpp deletes two #if BUN_DEBUG blocks that called reportUncaughtExceptionAtEventLoop (which consumed the pending exception, letting RETURN_IF_EXCEPTION fall through to a null-deref on sqlValue.getObject()); test/js/bun/util/BunObject.test.ts adds a spawned regression test using an Error.prototype proxy trap to transition the Bun object mid-lookup and throw.
Security risks
None identified. The C++ change deletes debug-only code and makes the debug exception path match release. The test is a spawned subprocess with fixed input. The WebKit pin points to a build produced by oven-sh's own release workflow.
Level of scrutiny
High. The load-bearing fix is in a separate repository (oven-sh/WebKit#390, a change to JSObject::getPropertySlot — one of the hottest paths in the engine) and cannot be reviewed from this diff. Pinning WEBKIT_VERSION to a preview of an unmerged PR is a dependency-bump decision the repo's landing-prs guidance calls out for maintainer attention, and the author explicitly describes a two-step plan (preview pin now, move to merge sha later) that a human should confirm. Per REVIEW.md, the test also passes under USE_SYSTEM_BUN=1 by the author's own account — the justification (guards debug/ASAN lanes only, since release never fired the assert) is reasonable but is a maintainer call.
Other factors
Three open PRs (#33211, #30245, #33418) target the same crash family with Bun-side workarounds; this PR supersedes them at the engine layer, so someone should decide which lands and close the rest. The BunObject.cpp deletion is small and clearly correct (reportUncaughtExceptionAtEventLoop clears the exception, so the following RETURN_IF_EXCEPTION becomes a no-op and control reaches .getObject() on an empty JSValue). The regression test now follows harness conventions after the earlier feedback. CI at 0a3a39f passed 187/196 with the two failures documented as pre-existing on main.
88d2ba7 to
80b355b
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs (the earlier stderr-drain nit was addressed in b8619cc). Because the load-bearing fix is a WebKit version bump to an unmerged preview build of oven-sh/WebKit#390 — an engine-level change to JSObject::getPropertySlot that isn't visible in this diff — a maintainer sign-off on the JSC change and the preview-pin landing plan is still warranted.
What was reviewed:
BunObject.cpp: the deletedBUN_DEBUGblocks — confirmedreportUncaughtExceptionAtEventLoopconsumes the pending exception, so removal restores correctRETURN_IF_EXCEPTIONpropagation and matches release-build behavior.- Regression test: subprocess drains both pipes and asserts the combined
{ stdout, stderr, exitCode };phase: 1guard makes the test self-invalidating if the sql module stops readingError.prototype. webkit.ts: theautobuild-tag prefix is already handled byprebuiltUrl/prebuiltDestDir, so the pin format is compatible with the existing download/cache-key logic.
Extended reasoning...
Overview
Three files touched: scripts/build/deps/webkit.ts bumps WEBKIT_VERSION from a merged commit sha to an unmerged preview tag autobuild-preview-pr-390-1394aa19; src/jsc/bindings/BunObject.cpp deletes two #if BUN_DEBUG blocks in defaultBunSQLObject/constructBunSQLObject that called reportUncaughtExceptionAtEventLoop before RETURN_IF_EXCEPTION; test/js/bun/util/BunObject.test.ts adds a spawned-subprocess regression test. The substantive fix — reloading the structure before the prototype step in JSObject::getPropertySlot — lives entirely in oven-sh/WebKit#390 and is not part of this diff.
Security risks
None identified. The deleted code was debug-only diagnostic reporting; the WebKit change is a correctness fix on the property-lookup slow path that affects debug-assert behavior for mono-proto objects. No auth, crypto, or untrusted-input handling is touched.
Level of scrutiny
High, because the real change is a JavaScriptCore engine modification to one of its hottest inline paths (JSObject::getPropertySlot). That change is only referenced here as a version pin — reviewing this PR alone cannot validate the JSC diff, its perf impact, or whether it interacts correctly with other prototype-chain callers. Per the repo's own guidance, dependency/vendoring bumps and WebKit upgrades warrant maintainer review. The Bun-side changes (6-line deletion, new test) are simple and would be approvable in isolation.
Other factors
- The pin targets a preview autobuild of an unmerged WebKit PR. The author's stated plan is the usual two-step (land on preview, then re-pin to the merge sha), but that is a process decision a maintainer should confirm before this hits main.
- Three overlapping open PRs (#33211, #30245, #33418) work around the same crash family on the Bun side; landing this one implies closing/rebasing those, which a maintainer should coordinate.
- The bug-hunting system found no issues this run; my only prior comment (undrained stderr pipe) was fixed and the thread is resolved.
- CI at 0a3a39f was mostly green with two ASAN-lane failures the author attributes to pre-existing flakes on main; the latest commit 80b355b is a CI retrigger for the now-published preview build.
80b355b to
9da6a46
Compare
![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 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 6-11: Update WEBKIT_VERSION to the commit SHA produced when WebKit
PR `#390` merges, replacing the preview tag so local and prebuilt builds resolve
the same revision; do not add a process.versions assertion.
🪄 Autofix
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: 38d53589-06a1-401a-a579-55a9821c4fbb
📒 Files selected for processing (1)
scripts/build/deps/webkit.ts
| // Preview of oven-sh/WebKit#390 (rebased on 447082ab, the previous pin here): | ||
| // JSObject::getPropertySlot reloads the structure before the prototype step, so | ||
| // a static-table lazy property builder that transitions the object and then | ||
| // throws no longer trips ASSERT(object->structure() == this) in | ||
| // Structure::storedPrototype. | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-390-0c51423a"; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- dependency file ---'
cat -n scripts/build/deps/webkit.ts
printf '%s\n' '--- WebKit version usage ---'
rg -n -C 3 'WEBKIT_VERSION|process\.versions' --glob '*.{ts,tsx,js,jsx}' .
printf '%s\n' '--- repository guidance ---'
if [ -f REVIEW.md ]; then
cat -n REVIEW.md
fiRepository: oven-sh/bun
Length of output: 50368
🌐 Web query:
GitHub oven-sh/WebKit pull request 390 current status merged commit
💡 Result:
There is no publicly available information indicating that pull request #390 exists in the oven-sh/WebKit repository as of August 10, 2026. Review of the repository's activity, merged pull requests, and associated release tags shows recent pull requests such as #370 and others, but no PR #390 [1][2][3]. Results related to "PR 390" in other contexts refer to unrelated items, such as automotive engine parts, consumer products, or pull requests in different repositories [4][5][6][7][8][9][10][11].
Citations:
- 1: Upgrade WebKit to 2e37adcc23b7 #36267
- 2: https://github.com/oven-sh/WebKit/releases/tag/autobuild-3167a44fb92c268c83f09b232b38a9f3e7f9655a
- 3: https://github.com/oven-sh/WebKit/releases/tag/autobuild-5488984d20e0dbfe4be2c3ba8fb18eb81a5e0e8b
- 4: https://catalog.elginind.com/engine/part/82f92c95-b74b-418b-ad15-9516f098ec52/
- 5: https://www.ruten.com.tw/item/22523739521258/
- 6: https://www.stelladelmobile.com/index.cfm/en/products/tables-1/pr390/
- 7: https://www.landtop.com.tw/products/sharp-wei-dian-nao-wei-bo-lu-25l
- 8: http://fotobus.msk.ru/vehicle/1244576/
- 9: Add WebProcess initial implementation of background fetch WebKit/WebKit#9913
- 10: Fix some assertions when using a canvas without a layer with webgpu WebKit/WebKit#25636
- 11: Fix layout tests in simulator WebKit/WebKit#23018
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- dependency-version assertion ---'
sed -n '45,75p' scripts/build/depVersionsHeader.ts
sed -n '565,600p' test/js/node/process/process.test.js
printf '%s\n' '--- preview-tag handling ---'
sed -n '275,300p' scripts/build/download.ts
sed -n '1,50p' scripts/sync-webkit-source.ts
printf '%s\n' '--- upstream PR status ---'
curl -fsSL -H 'Accept: application/vnd.github+json' \
https://api.github.com/repos/oven-sh/WebKit/pulls/390 |
jq '{number,state,merged_at,merge_commit_sha,head:.head.sha,base:.base.ref,html_url}'
printf '%s\n' '--- preview release and tag ---'
curl -sS -o /tmp/webkit-release.json -w 'release_http=%{http_code}\n' \
https://api.github.com/repos/oven-sh/WebKit/releases/tags/autobuild-preview-pr-390-0c51423a
if [ -s /tmp/webkit-release.json ]; then
jq '{tag_name,name,draft,prerelease,published_at,target_commitish,html_url}' /tmp/webkit-release.json
fi
curl -sS -o /tmp/webkit-ref.json -w 'ref_http=%{http_code}\n' \
https://api.github.com/repos/oven-sh/WebKit/git/ref/tags/autobuild-preview-pr-390-0c51423a
if [ -s /tmp/webkit-ref.json ]; then
jq '{ref,object}' /tmp/webkit-ref.json
fiRepository: oven-sh/bun
Length of output: 6565
🏁 Script executed:
#!/usr/bin/env bash
set -u
repo=https://github.com/oven-sh/WebKit.git
tag=autobuild-preview-pr-390-0c51423a
printf '%s\n' '--- remote preview tag ---'
git ls-remote "$repo" "refs/tags/$tag" "refs/tags/$tag^{}" 2>&1
printf 'tag_status=%s\n' "$?"
printf '%s\n' '--- remote PR refs ---'
git ls-remote "$repo" \
"refs/pull/390/head" \
"refs/pull/390/merge" 2>&1
printf 'pr_ref_status=%s\n' "$?"
printf '%s\n' '--- WebKit-specific version assertions and macros ---'
rg -n -i -C 3 'webkit|WEBKIT' \
test/js/node/process/process.test.js \
src \
scripts/build/depVersionsHeader.ts \
scripts/build/deps/webkit.ts \
scripts/build/bun.ts \
2>/dev/null | head -n 240Repository: oven-sh/bun
Length of output: 17974
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- pull request page markers ---'
curl -L -sS -A 'Mozilla/5.0' https://github.com/oven-sh/WebKit/pull/390 |
tr '\n' ' ' |
grep -oE '.{0,100}(Open|Closed|Merged|pull request|PR `#390`).{0,160}' |
head -n 20
printf 'page_status=%s\n' "$?"
printf '%s\n' '--- preview release asset status ---'
for asset in \
bun-webkit-linux-amd64.tar.gz \
bun-webkit-linux-amd64-debug.tar.gz \
bun-webkit-macos-amd64.tar.gz; do
url="https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-390-0c51423a/$asset"
status="$(curl -L -sS -o /dev/null -w '%{http_code}' "$url")"
printf '%s %s\n' "$status" "$asset"
done
printf '%s\n' '--- immutable commit existence ---'
for sha in 447082ab6897278727b44e1ba3c326ae6e1504c3 0c51423a042b9845a221bce1a6fb3a67a6b2afac eff835bf690f7a114669a4321c8a9a1323e5bbd2; do
git ls-remote https://github.com/oven-sh/WebKit.git "$sha^{commit}" 2>&1
doneRepository: oven-sh/bun
Length of output: 4872
Replace the preview pin before merging. The preview tag resolves to the previous commit 447082ab..., while prebuilt assets use PR head 0c51423a...; local and prebuilt modes therefore use different revisions. After PR #390 merges, set WEBKIT_VERSION to its merged commit SHA. No matching hard-coded process.versions assertion exists.
🤖 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 6 - 11, Update WEBKIT_VERSION to
the commit SHA produced when WebKit PR `#390` merges, replacing the preview tag so
local and prebuilt builds resolve the same revision; do not add a
process.versions assertion.
Sources: Coding guidelines, Learnings, MCP tools
|
On the webkit.ts line comment: the preview tag is deliberate while oven-sh/WebKit#390 is still open, there is no merge sha to pin yet. Once that PR lands, the pin moves to the resulting commit on WebKit main, as described in the landing plan in the PR description. In the meantime the tag resolves identically in both modes: it is a release tag on the PR head (0c51423a, a rebase onto the current pin 447082ab), which is the exact revision the prebuilt archives were built from. |
| // Preview of oven-sh/WebKit#390 (rebased on 447082ab, the previous pin here): | ||
| // JSObject::getPropertySlot reloads the structure before the prototype step, so | ||
| // a static-table lazy property builder that transitions the object and then | ||
| // throws no longer trips ASSERT(object->structure() == this) in | ||
| // Structure::storedPrototype. | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-390-0c51423a"; |
There was a problem hiding this comment.
🔴 WEBKIT_VERSION is pinned to autobuild-preview-pr-390-0c51423a, an ephemeral preview tag for an unmerged oven-sh/WebKit PR. Per the repo's Dependencies & vendoring rule (.claude/docs/landing-prs.md:47, "Never merge a pin to an ephemeral artifact … swap to the merged upstream SHA … before merge"), and per scripts/build/download.ts's own error handling ("GitHub deletes the preview release when the PR merges or closes, so every build 404s at once"), this must be swapped to the merged oven-sh/WebKit#390 commit SHA before this PR lands — deferring to "the routine WebKit upgrade that follows" leaves a range of main commits that fail vendor-fetch on any cold cache or git bisect once the preview release is pruned.
Extended reasoning...
What the issue is. scripts/build/deps/webkit.ts:11 sets WEBKIT_VERSION = "autobuild-preview-pr-390-0c51423a". This is an autobuild-preview-pr-* tag — a preview build for an as-yet-unmerged oven-sh/WebKit pull request — not a merged upstream commit SHA. prebuiltUrl() in the same file (line ~85) constructs the download URL directly from this constant: https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-390-0c51423a/bun-webkit-<os>-<arch><suffix>.tar.gz, and prebuiltDestDir() keys the extraction cache on it.
Why this is a problem. The repo's own review guidance is explicit. .claude/docs/landing-prs.md:47 (Dependencies & vendoring), which REVIEW.md designates as the situational rule for dependency bumps, states verbatim:
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 tag / unmerged-PR build" the rule prohibits. The codebase itself documents the concrete failure mode — scripts/build/download.ts:277-295 has a dedicated error handler for exactly this case:
A missing prebuilt tarball is a bad pin, not a network blip. The
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 accompanying hint tells the user to "Set WEBKIT_VERSION in scripts/build/deps/webkit.ts to the merged main sha".
Step-by-step failure path.
- This PR merges to main with
WEBKIT_VERSION = "autobuild-preview-pr-390-0c51423a". - JSObject::getPropertySlot: reload the structure before the prototype step WebKit#390 later merges (or is closed / force-pushed). Per the download.ts comment, GitHub deletes the
autobuild-preview-pr-390-0c51423arelease. - A developer does a fresh clone, or a CI runner starts with a cold cache, or someone runs
git bisectand lands on any main commit in the range [this PR's merge, the follow-up WebKit bump). - The build's vendor-fetch step calls
prebuiltUrl(cfg)→ GET.../releases/download/autobuild-preview-pr-390-0c51423a/bun-webkit-linux-amd64-debug.tar.gz→ HTTP 404. prebuiltDownloadError()throwsBuildError: WebKit preview release is gone. The build fails before compilation begins. There is no fallback — the only remedy is manually editingWEBKIT_VERSIONon that historical commit, which defeats bisect.
Why the timeline comment doesn't resolve this. The author's comment on 2026-08-06 says the pin will move to the merge commit SHA "either here before landing or as the routine WebKit upgrade that follows; the preview tag stays downloadable either way". The repo rule does not permit the second option: it requires the swap before merge, and the download.ts comment directly contradicts "stays downloadable either way" — preview releases are deleted when the PR merges or closes. Pinning the preview during CI iteration is fine; landing it on main is what the rule forbids.
Fix. Wait for oven-sh/WebKit#390 to merge, then before landing this PR update WEBKIT_VERSION to the merged 40-hex commit SHA on oven-sh/WebKit main (the durable autobuild-<sha> release), verify the prebuilt tarball exists for every platform × flavor combination prebuiltSuffix() produces, and drop the now-obsolete "Preview of oven-sh/WebKit#390" comment above the constant.
|
The review comment on the webkit.ts pin is right, and it corrects my earlier reply: the preview tag does not stay downloadable after the WebKit PR merges. scripts/build/download.ts documents that GitHub deletes autobuild-preview-pr-* releases when the PR merges or closes, and the vendoring rule requires the swap to the merged sha to happen before this PR lands, not in a follow-up bump. I updated the landing sequence in the PR description to match: land oven-sh/WebKit#390 first, swap WEBKIT_VERSION here to the merged sha once its prebuilt assets exist for every platform and flavor, then merge this PR. Until that merge happens there is no durable sha to pin, so the preview tag stays only for CI on this branch. |
|
Pushed 54a79e0, which adds a second regression test. Recent fuzzer samples hit the same assert without touching any global: they recurse until the stack overflows and then first-touch Bun.sql from the frame just above the overflow, so the builder's module evaluation throws a RangeError after the Bun object has already transitioned. The minimal form (now in the PR description) aborts an unfixed debug build every run and exits 0 printing RangeError with the fix, in about 0.6s under debug ASAN. It does not depend on the sql module reading Error.prototype, so it stays valid if that ever changes. Checked both tests fail on an unfixed debug binary (exit 134 with this assertion) and pass with the pinned preview. |
|
On the stack overflow test comment: the second half of it was right, and checking it turned up something worth recording. I traced the script on an unfixed debug binary with breakpoints on structure transitions. The Bun object never transitions in that scenario. What aborts is the debug-only report that the builder used to make while the RangeError was still pending: Bun__handleUncaughtException reads process._fatalException, the static table reifies it (transition), setUpStaticFunctionSlot then reports a miss because of the pending exception, and the prototype step asserts on the process object's stale structure. So that test covers the BunObject.cpp change (it is the same lines that also consumed the exception), while the Error proxy test is the one that transitions the Bun object inside the builder and needs the engine reload on its own. f1ce747 rewrites the test comment to say exactly that, and the PR description and the WebKit PR now describe both routes. On the exact stdout assertion, I kept it. The frame that runs the lookup is the one whose own call to f() just failed the stack check, and the builder cannot complete without entering JS functions that perform the same check, so it throws there on every configuration; the test passed on all 190 jobs of build 91796 (glibc, musl, darwin, windows, release and ASAN), and 8 of 8 local debug and 5 of 5 release runs print exactly RangeError. Accepting "no throw" would make the test pass in precisely the case where it stopped exercising anything, which is the failure mode the exact string is there to catch. The process-stdio precedent is different in kind: its getter is native and can legitimately finish in the remaining headroom. |
f1ce747 to
ed22778
Compare
ed22778 to
3a8de41
Compare
…-lookup Fuzzilli kept hitting ASSERT(object->structure() == this) in JSC::Structure::storedPrototype on debug builds (fingerprint StructureInlinesLight.h(56)). The inlined JSObject::getPropertySlot loop caches the structure, and reifying a static-table lazy property can transition the object inside getOwnNonIndexPropertySlot and still return false when the PropertyCallback builder throws, so the prototype step ran on the pre-transition structure. Trigger: a first read of Bun.sql whose module evaluation touches another lazy Bun property and then throws. oven-sh/WebKit#390 reloads the structure before the prototype step, the same reload the megamorphic slow paths in JITOperations.cpp already do. This bumps the WebKit pin to its preview build. Fixing that surfaced a second bug in the same path: defaultBunSQLObject and constructBunSQLObject had a BUN_DEBUG-only reportUncaughtExceptionAtEventLoop call that consumed the pending exception, so RETURN_IF_EXCEPTION fell through and getObject() ran on an empty JSValue (null deref), and an empty builder result with no pending exception would trip RELEASE_ASSERT_NOT_REACHED in setUpStaticFunctionSlot. Drop the debug reporting; the exception now propagates to the caller like in release builds.
3a8de41 to
e6e9d7b
Compare
e6e9d7b to
379f322
Compare
1 participant
What
Fuzzilli kept hitting a flaky abort on debug builds (fingerprint
StructureInlinesLight.h(56)):The fuzzer sample tripped it while reading every property on the
Bunobject, which first-touches all of its lazy static-table properties.Root cause
The inlined prototype-chain loop in
JSObject::getPropertySlot(JSObject.h) reads the object'sStructure*once, callsgetOwnNonIndexPropertySlot, and on a miss uses that same pointer forstructure->storedPrototype(object). A miss is not side-effect free for static-table objects: reifying a lazyPropertyCallbackproperty transitions the object, and with our fork's throwing builders,setUpStaticFunctionSlotcan return false after the transition happened (the builder threw, so the slot is reported as not found and the caller observes the exception). The prototype step then runs on the stale pre-transition structure. Release builds read the prototype off the old structure, which happens to be harmless for mono-proto objects, so only asserts builds crash.Deterministic trigger: a first read of
Bun.sqlevaluates the sql internal modules, whoseclass SQLError extends ErrorreadsError.prototypeat module scope. Hooking that with a proxy that first touches another lazyBunproperty (transitioning the Bun object) and then throws reproduces the abort every time:Later fuzzer samples reach the same assert with no hooks, by first-touching
Bun.sqlfrom the frame just above a stack overflow so the builder throws a RangeError. Tracing that variant in a debugger shows it takes a different route to the same line: the Bun object does not transition; instead the builder's debug-only exception report (fix 2 below) ran with the exception still pending and readprocess._fatalException, which reified the property but was reported as a miss because of the pending exception, so the prototype step asserted on the process object's stale structure. Plain code like this aborts an unfixed debug build every time:Fixes
WebKit (JSObject::getPropertySlot: reload the structure before the prototype step WebKit#390, pinned here as its preview autobuild): reload the structure before the prototype step in
JSObject::getPropertySlot. The megamorphic slow paths in JITOperations.cpp already do exactly this reload, with the comment "Reload it again since static-class-table can cause transition", andgetNonIndexPropertySlotusesgetPrototypeDirect()which re-reads it. This was the one caller left reusing the stale pointer.BunObject.cpp: fixing the assert surfaced a second bug in the same path.
defaultBunSQLObjectandconstructBunSQLObjecthad aBUN_DEBUG-onlyreportUncaughtExceptionAtEventLoopcall afterrequireId. Reporting consumes the pending exception, soRETURN_IF_EXCEPTIONfell through andsqlValue.getObject()ran on an empty JSValue (null deref, caught by UBSan), and a builder returning empty with no pending exception would hitRELEASE_ASSERT_NOT_REACHEDinsetUpStaticFunctionSlot. Dropped the debug reporting so debug builds propagate the exception the same way release builds do.Verification
caught: boom, exit 0) with both fixes.BUN_JSC_collectContinuously=1.test/js/bun/util/BunObject.test.ts. Both fail on an unfixed debug build (the spawned process exits 134 with this assertion) and pass with the fix. The Error proxy test transitions the Bun object inside the builder (phase: 1), so it needs the engine reload regardless of fix 2. The stack overflow test does not transition the Bun object; it covers fix 2 (its abort on main goes through the debug-only report, see above), takes about 0.6s under debug ASAN, and passed on every job of build 91796. The intermediate configuration is covered too: on a build with the patched WebKit but the BunObject.cpp blocks still present, both tests fail with UBSan's null member call atJSCJSValueCell.h:92(the consumed-exception bug from fix 2), so neither fix alone makes them pass. Release builds never fired the assert, so the tests pass underUSE_SYSTEM_BUN=1by design; they guard the debug and ASAN lanes that the fuzzer runs.test/js/sql/adapter-env-var-precedence.test.ts),test/js/bun/util/BunObject.test.ts,test/js/bun/globals.test.js, andtest/js/bun/namespace-prototype-pollution.test.tspass against a local build of the patched WebKit.f0f60fd2pin): no lane reported either regression test failing. The one red lane is debian 13 x64-asan, where four tests this change does not touch failed (three leak tests timing out and an elysia request cancellation test); the three that can be run locally pass on a release build of this head, and all four are reported separately as main breaks. Eight Windows test shards and the Windows baseline check never got agents (Azure throttling); everything else passed.Landing sequence
The
autobuild-preview-pr-390-*pin exists so CI on this branch can fetch the patched WebKit. It must not reach main: GitHub deletes the preview release once oven-sh/WebKit#390 merges or closes (scripts/build/download.tsdocuments exactly this failure mode), which would leave main commits that 404 on vendor fetch. So the order is: land oven-sh/WebKit#390 first, then swapWEBKIT_VERSIONhere to the merged sha (the durableautobuild-<sha>release, after its prebuilt assets exist for every platform and flavor), then merge this PR.[decide:webkit] gate passed · iteration 14 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 8 passed · 2 rejected · iteration 14
evidence per changed file