Bump WebKit: assert the VM's API lock is held on HandleSet mutation - #36958
Bump WebKit: assert the VM's API lock is held on HandleSet mutation#36958robobun wants to merge 3 commits into
Conversation
Brings in oven-sh/WebKit#387: HandleSet::allocate/deallocate/writeBarrier now ASSERT (debug builds only) that the current thread holds the owning VM's API lock, matching the existing assertion in Heap::protect/unprotect. This gives the #30185 bug class (a Strong captured by value in a cross-thread lambda, destroyed on another thread, corrupting the owner VM's strong-handle list) a deterministic detector in every debug build. The probabilistic stress guard for that class stopped detecting after the GC scheduling change in #35356: 0 detections in 18,000 iterations with the bug deliberately reintroduced (measured in #36952). With this assertion the same reintroduced bug aborts on the first cross-VM round-trip, naming the invariant and the call site.
|
Status (robobun)
|
WalkthroughThe change adds VM API-lock assertions for JSC strong handles, an assertion-only cross-thread mutation helper, debug regression tests, and a new WebKit release identifier. ChangesStrong handle thread guards
WebKit release update
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found in the diff itself, but deferring to a human: this is a WebKit engine bump pinned to a preview tag for an unmerged upstream PR (oven-sh/WebKit#387), with an explicit plan to flip the pin once that lands. A maintainer should confirm CI is green across all lanes and decide whether to merge on the preview tag or wait for the final sha.
What was reviewed:
prebuiltUrl()andprebuiltDestDir()both already special-case theautobuild-prefix, so the non-sha version string resolves to the correct release tag and a distinct cache dir.- The change is debug-only on the WebKit side (release builds unchanged per the description); no bun-side code paths touched.
Extended reasoning...
Overview
Single-line change to scripts/build/deps/webkit.ts: WEBKIT_VERSION moves from commit sha e6e37cda... to autobuild-preview-pr-387-f6049b84, a preview build tag for oven-sh/WebKit#387 (adds a debug-only assertion that the VM API lock is held during HandleSet mutation).
Security risks
None. This is a build-config version pin; no runtime code, no auth/crypto/permissions surface. The upstream change is a debug-only assertion.
Level of scrutiny
High, despite the tiny diff. WebKit is the JavaScript engine — the single most load-bearing dependency in the project — and the repo's review guidance explicitly calls out dependency/vendor bumps as needing situational care. More importantly, this pins to a preview artifact of an unmerged upstream PR. The PR description itself frames this as step one of a two-step process: land the preview to run full CI, then flip to the merge sha once oven-sh/WebKit#387 merges. That workflow decision (merge on preview tag vs. wait for the final sha, and confirming all 38 CI variants are actually green) is a maintainer call, not something to auto-approve.
Other factors
- Mechanical correctness checks out:
prebuiltUrl()guards against double-prefixing (version.startsWith("autobuild-")), andprebuiltDestDir()strips the prefix to key the cache dir onpreview-pr-387-f6049b84, so there's no cache collision or malformed URL. - No new test is expected here — the assertion is a detector that only fires when bun's C++ is wrong, and the description documents verification against reintroduced #30185 plus a clean run of the Strong-heavy suites.
- No prior reviews from me; no outstanding human reviewer comments to address.
|
Right: the preview pin is for CI validation only, and the plan is to not merge it. Once CI here is green and oven-sh/WebKit#387 lands, this PR's pin flips to the final merge commit sha (after the main-branch autobuild publishes its artifacts), so what merges into bun points at a commit on the WebKit fork's main. |
…ctors Review of the WebKit bump surfaced two gaps: Bun__StrongRef__new/set/delete mutate the StrongRootBlock list that the Srb marking constraint scans, with the same API-lock requirement as JSC's HandleSet but no assertion. Add the mirror asserts, checking the owner VM (the block's), so a thread holding some other VM's lock still trips them. Strong.rs's existing TLS check covers only the delete path and only proves the thread has a VM, not the owner VM's lock; the C++ Bun::StrongRef deleter bypasses it entirely. Neither detector had a liveness test, which is how the previous probabilistic guard for #30185 died silently (#35356, measured in #36952). jscInternals gains a debug-only crossThreadStrongHandleMutation hook that violates the contract from a spawned thread, and the new test asserts the child aborts with each guard's message. If a future WebKit bump or binding refactor drops an assertion, the test fails instead of the coverage silently disappearing.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs in the bun-side changes, but this shouldn't merge without a maintainer: the WebKit pin currently points at a preview PR artifact (autobuild-preview-pr-387-f6049b84), and the author's own status notes it's blocked on oven-sh/WebKit#387 landing and the pin flipping to the merge sha.
What was reviewed:
Bun__StrongRef__setnow asserts on the block's owner VM (block->vm()) rather than the passedglobalObject's VM — correct, since a wrong-VM caller is exactly what this guards.- All three new asserts and the test hook are gated on
ASSERT_ENABLED, so release builds are unchanged. - The liveness test drains stdout/stderr/exited concurrently, is
test.if(isDebug)-gated, and disables crash reporting for the intentional aborts.
Extended reasoning...
Overview
This PR bumps the WebKit pin to a preview build of oven-sh/WebKit#387 (which adds a debug-only currentThreadIsHoldingAPILock() assertion to HandleSet::allocate/deallocate/writeBarrier), mirrors the same debug assertion onto Bun's own StrongRootBlock mutation path in StrongRef.cpp (Bun__StrongRef__new/set/delete), and adds a debug-only bun:internal-for-testing hook plus a test that spawns a child, deliberately mutates a strong handle from a thread without the API lock, and asserts the child aborts with the guard's message. The goal is a deterministic detector for the #30185 bug class (cross-thread strong-handle mutation racing the GC's Sh/Srb marking constraints).
Security risks
None. All added code is debug-only (ASSERT_ENABLED gate on both the assertion macro and the test hook registration), and the hook is exposed only through bun:internal-for-testing. Release builds are byte-for-byte unchanged in the affected functions except for the mechanical decompose of Bun__StrongRef__set into two statements, which the optimizer collapses.
Level of scrutiny
High — this is a WebKit dependency bump coupled with GC-adjacent binding code. Even though the bun-side diff is small and debug-only, the WebKit change itself touches HandleSet, which sits directly under every JSC::Strong in the process, and I can't review that half here. Per the repo's own guidance, dependency/vendor bumps warrant maintainer review.
Other factors
The decisive factor is that the PR is self-declared as not ready to merge in its current form: WEBKIT_VERSION is pinned to autobuild-preview-pr-387-f6049b84, a preview artifact for an unmerged WebKit PR. The author's status comment says "Blocked on: oven-sh/WebKit#387 merge (maintainer), then the pin flip here", and a follow-up confirms "the preview pin is for CI validation only, and the plan is to not merge it." Merging as-is would pin bun to an artifact that may be garbage-collected once the upstream PR closes. A maintainer needs to land the WebKit PR, wait for the main-branch autobuild, and flip the pin — none of which I can do.
The bun-side code itself looks correct: the assertion in Bun__StrongRef__set checks block->vm() (the owner) rather than getVM(globalObject) (the caller's), which is the right choice for catching cross-VM misuse; the test hook joins the spawned thread before returning so no stack state outlives it (moot anyway since the process aborts first); the test follows harness conventions (bunEnv spread, concurrent pipe drain, test.if(isDebug), crash reporting disabled). CI on two rounds shows all build lanes green with no spurious assertion failures. But given the pending pin flip and the nature of WebKit bumps, this needs human sign-off.
2 participants
What does this PR do?
Gives the #30185 bug class (a strong handle owned by one VM, mutated from a thread that does not hold that VM's API lock) a deterministic, self-verifying detector. Three pieces:
HandleSet::allocate,deallocate, andwriteBarriernow assert (debug builds only) that the current thread holds the owning VM's API lock, matching the existing assertion inHeap::protect/unprotect. The pin currently points at that PR's preview build (autobuild-preview-pr-387-f6049b84) and flips to the merge commit once it lands.bun_jsc::Strongroutes throughStrongRootBlockslots instead of JSC's HandleSet, and the "Srb" marking constraint scans that list under the same mutator-only assumption, with no guard (Strong.rschecks only that the dropping thread has a VM, not the owner VM's lock, and the C++Bun::StrongRefdeleter bypasses even that).Bun__StrongRef__new/set/deletenow carry the same debug-only assertion, checked against the owner VM (the block's), so holding some other VM's lock does not pass.jscInternals.crossThreadStrongHandleMutation(kind)(debug builds only) deliberately violates the contract from a spawned thread, andtest/js/bun/jsc/strong-handle-thread-guard.test.tsasserts the child aborts with each guard's message. This is the piece that keeps the detectors from dying the way the old guard did: if a future WebKit bump or binding refactor drops an assertion, this test fails in the debug lanes instead of the coverage silently disappearing.Why
These mutation paths write lists that GC marking constraints scan ("Sh" for HandleSet, "Srb" for StrongRootBlock); the API lock is what orders mutations with the scans. A
JSC::Strongcaptured by value in a cross-thread lambda and destroyed on the other thread was exactly #30185:panic: Segmentation fault at address 0x10in the Sh constraint, or a livelock on the tornSentinelLinkedList.The class currently has no effective guard. The stress workload in
worker_heap_snapshot_gc.test.tswas sized (#35200) to catch a reintroduced #30185 at roughly 60% per process, but after #35356 replaced per-tick GC collections with an idle timer, the same reintroduced bug produced 0 detections in 18,000 iterations (measured in #36952, which resizes the now-ineffective workload and defers a deterministic detector to this PR). Detection no longer depends on a GC scan landing in a few-instruction window: the unlocked mutation itself aborts, naming the invariant and the call site.Verification
Reintroduced the canonical worker: fix cross-thread HandleSet race in getHeapSnapshot #30185 bug (by-value
Strong<JSPromise>capture ingetHeapSnapshot's cross-thread lambda) on top of the bump:bun bdaborts on the firstworker.getHeapSnapshot()round-trip, every run:The new liveness test passes with this diff (both children die with the right message) and fails without the src/ changes (the hook is gone, the child survives), on linux x64 debug+ASAN.
No spurious assertion failures across the Strong-heavy suites locally (debug+ASAN): worker_threads, web workers, fetch (11,232 tests), streams, Bun.spawn, dns, timers, sqlite + Bun.password, node:vm, bun shell, plugin, bun:jsc, node:http2, mock functions. The only local failures are machine artifacts that reproduce identically without this change (stress-test timeouts on a throttled container, plus two already-tracked pre-existing bugs found along the way). First CI round on the bump alone: all 26 build/link lanes green, zero assertion failures anywhere.
Release builds are unchanged: both assertions compile out (
ASSERT_ENABLED=0), and the testing hook is not registered.Upstream note: WebKit/WebKit@ff64aee116 replaces
HandleSetwithStrongSet/StrongBlock, so the fork's next upstream merge will hit a delete/modify conflict on these files and needs to port the assertion (tracked on oven-sh/WebKit#387). The liveness test is what enforces that port: if it gets dropped, the debug lanes fail.[decide:webkit] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file