From a96ddb6d630469cdddb3170ecf3f4bbacf3aca0e Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 5 Aug 2026 11:49:45 +0000 Subject: [PATCH 1/3] Bump WebKit: assert the VM's API lock is held on HandleSet mutation 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. --- scripts/build/deps/webkit.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/build/deps/webkit.ts b/scripts/build/deps/webkit.ts index 87776fa6480d..a8407b517d30 100644 --- a/scripts/build/deps/webkit.ts +++ b/scripts/build/deps/webkit.ts @@ -3,7 +3,7 @@ * for local mode. Override via `--webkit-version=` to test a branch. * From https://github.com/oven-sh/WebKit releases. */ -export const WEBKIT_VERSION = "e6e37cda216c0292ae68c30c84a9dc8601d0fba5"; +export const WEBKIT_VERSION = "autobuild-preview-pr-387-f6049b84"; /** * WebKit (JavaScriptCore) — the JS engine. From 9886d89bfc674cf6c378402a8d9c0ae9c1485d64 Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 5 Aug 2026 15:55:00 +0000 Subject: [PATCH 2/3] Guard StrongRootBlock mutations and add a liveness test for both detectors 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. --- src/jsc/bindings/JSCTestingHelpers.cpp | 53 +++++++++++++++++++ src/jsc/bindings/StrongRef.cpp | 15 +++++- .../jsc/strong-handle-thread-guard-fixture.js | 7 +++ .../jsc/strong-handle-thread-guard.test.ts | 39 ++++++++++++++ 4 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 test/js/bun/jsc/strong-handle-thread-guard-fixture.js create mode 100644 test/js/bun/jsc/strong-handle-thread-guard.test.ts diff --git a/src/jsc/bindings/JSCTestingHelpers.cpp b/src/jsc/bindings/JSCTestingHelpers.cpp index caca7766c736..3b64d930f704 100644 --- a/src/jsc/bindings/JSCTestingHelpers.cpp +++ b/src/jsc/bindings/JSCTestingHelpers.cpp @@ -6,6 +6,13 @@ #include #include "ZigGlobalObject.h" +#if ASSERT_ENABLED +#include "StrongRef.h" +#include +#include +#include +#endif + #if OS(WINDOWS) #include #include @@ -64,6 +71,45 @@ JSC_DEFINE_HOST_FUNCTION(jsFunctionStartOfFixedExecutableMemoryPool, } #endif +#if ASSERT_ENABLED +// Deliberately mutates a strong handle owned by this VM from a spawned thread +// that does not hold the API lock. The debug-only thread-affinity assertions +// (JSC::HandleSet::assertMayMutate for JSC::Strong, the Bun__StrongRef__* +// asserts for StrongRootBlock slots) must abort the process before the +// mutation lands; the test asserting on that crash is what keeps the +// detectors from rotting silently. Only compiled when the assertions are. +JSC_DEFINE_HOST_FUNCTION(jsFunctionCrossThreadStrongHandleMutation, + (JSGlobalObject * globalObject, CallFrame* callframe)) +{ + auto& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + WTF::String kind = callframe->argument(0).toWTFString(globalObject); + RETURN_IF_EXCEPTION(scope, {}); + + if (kind == "strong"_s) { + // The #30185 shape: a by-value Strong capture destroyed off-thread. + JSC::Strong strong(vm, JSC::constructEmptyObject(globalObject)); + Ref thread = Thread::create("StrongHandleGuardTest"_s, [strong]() mutable { + strong.clear(); + }); + thread->waitForCompletion(); + return JSValue::encode(jsUndefined()); + } + + if (kind == "strongRef"_s) { + auto* ref = Bun__StrongRef__new(globalObject, JSValue::encode(JSC::constructEmptyObject(globalObject))); + Ref thread = Thread::create("StrongHandleGuardTest"_s, [ref]() { + Bun__StrongRef__delete(ref); + }); + thread->waitForCompletion(); + return JSValue::encode(jsUndefined()); + } + + throwTypeError(globalObject, scope, "Expected \"strong\" or \"strongRef\""_s); + return {}; +} +#endif + JSC::JSValue createJSCTestingHelpers(Zig::GlobalObject* globalObject) { auto& vm = JSC::getVM(globalObject); @@ -87,6 +133,13 @@ JSC::JSValue createJSCTestingHelpers(Zig::GlobalObject* globalObject) JSC::PropertyAttribute::DontDelete | 0); #endif +#if ASSERT_ENABLED + object->putDirectNativeFunction( + vm, globalObject, JSC::Identifier::fromString(vm, "crossThreadStrongHandleMutation"_s), 1, + jsFunctionCrossThreadStrongHandleMutation, ImplementationVisibility::Public, NoIntrinsic, + JSC::PropertyAttribute::DontDelete | 0); +#endif + return object; } diff --git a/src/jsc/bindings/StrongRef.cpp b/src/jsc/bindings/StrongRef.cpp index 5744b2593472..c5b31ebe98b2 100644 --- a/src/jsc/bindings/StrongRef.cpp +++ b/src/jsc/bindings/StrongRef.cpp @@ -48,9 +48,19 @@ static ALWAYS_INLINE StrongRootBlock* decodeStrongRefBlock(StrongRefImpl* ref) return reinterpret_cast(slot - static_cast(decodeStrongRefIndex(ref)) * sizeof(StrongRootBlock::Slot) - StrongRootBlock::slotsOffset()); } +// The StrongRootBlock list hangs off JSVMClientData with no synchronization: +// the "Srb" marking constraint (BunClientData.cpp) scans it assuming only the +// mutator, holding the VM's API lock, ever writes it. These asserts are the +// StrongRootBlock mirror of JSC::HandleSet::assertMayMutate, and check the +// OWNER VM (the block's), so a thread that has some other VM's lock still +// trips them. +#define ASSERT_STRONG_REF_MUTATION_ALLOWED(vm) \ + ASSERT_WITH_MESSAGE((vm).currentThreadIsHoldingAPILock(), "Bun::StrongRef handles may only be created, written, or destroyed while holding their VM's API lock") + extern "C" StrongRefImpl* Bun__StrongRef__new(JSC::JSGlobalObject* globalObject, JSC::EncodedJSValue encodedValue) { auto& vm = JSC::getVM(globalObject); + ASSERT_STRONG_REF_MUTATION_ALLOWED(vm); unsigned index; auto* block = StrongRootBlock::acquire(clientDataFast(vm), vm, index); block->set(vm, index, JSC::JSValue::decode(encodedValue)); @@ -59,7 +69,9 @@ extern "C" StrongRefImpl* Bun__StrongRef__new(JSC::JSGlobalObject* globalObject, extern "C" void Bun__StrongRef__set(StrongRefImpl* _Nonnull ref, JSC::JSGlobalObject* globalObject, JSC::EncodedJSValue encodedValue) { - decodeStrongRefBlock(ref)->write(JSC::getVM(globalObject), decodeStrongRefIndex(ref), JSC::JSValue::decode(encodedValue)); + auto* block = decodeStrongRefBlock(ref); + ASSERT_STRONG_REF_MUTATION_ALLOWED(block->vm()); + block->write(JSC::getVM(globalObject), decodeStrongRefIndex(ref), JSC::JSValue::decode(encodedValue)); } // The Rust caller (Strong.rs Impl::destroy) skips this call once @@ -71,6 +83,7 @@ extern "C" void Bun__StrongRef__delete(StrongRefImpl* _Nonnull ref) { auto* block = decodeStrongRefBlock(ref); auto& vm = block->vm(); + ASSERT_STRONG_REF_MUTATION_ALLOWED(vm); auto* clientData = clientDataFast(vm); // This block just freed a slot, so the next acquire() should try it first // (covers the FIFO pattern where the oldest-armed block gets room while diff --git a/test/js/bun/jsc/strong-handle-thread-guard-fixture.js b/test/js/bun/jsc/strong-handle-thread-guard-fixture.js new file mode 100644 index 000000000000..fd52385268bb --- /dev/null +++ b/test/js/bun/jsc/strong-handle-thread-guard-fixture.js @@ -0,0 +1,7 @@ +import { jscInternals } from "bun:internal-for-testing"; + +// Violates the strong-handle thread-affinity contract on purpose; in a debug +// build the assertion must abort the process inside this call. +jscInternals.crossThreadStrongHandleMutation(process.argv[2]); + +console.log("survived"); diff --git a/test/js/bun/jsc/strong-handle-thread-guard.test.ts b/test/js/bun/jsc/strong-handle-thread-guard.test.ts new file mode 100644 index 000000000000..a683862c91b5 --- /dev/null +++ b/test/js/bun/jsc/strong-handle-thread-guard.test.ts @@ -0,0 +1,39 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, isDebug } from "harness"; +import path from "path"; + +// Each child deliberately mutates a strong handle owned by the main VM from a +// spawned thread that does not hold the VM's API lock. Debug builds carry +// assertions guarding exactly that (JSC::HandleSet::assertMayMutate for +// JSC::Strong, the Bun__StrongRef__* asserts for StrongRootBlock slots), so +// the child must die with the assertion's message before the mutation lands. +// +// This is the liveness check for those detectors: the previous guard for the +// #30185 cross-thread HandleSet race was a probabilistic crash workload that +// silently stopped detecting when GC scheduling changed (#35356, measured in +// #36952). If a WebKit bump or binding refactor drops the assertions, the +// child survives and this test fails instead of the coverage disappearing +// unnoticed. +// +// These crashes are intentional; keep them out of crash reporting so CI does +// not pin them on unrelated tests. +const noReportEnv = { ...bunEnv, BUN_CRASH_REPORT_URL: "", BUN_ENABLE_CRASH_REPORTING: "0" }; + +for (const [kind, message] of [ + ["strong", "Strong handles may only be created, written, or destroyed while holding their VM's API lock"], + ["strongRef", "Bun::StrongRef handles may only be created, written, or destroyed while holding their VM's API lock"], +] as const) { + test.if(isDebug)(`unlocked off-thread ${kind} mutation aborts with the guard's message`, async () => { + await using proc = Bun.spawn({ + cmd: [bunExe(), path.join(import.meta.dir, "strong-handle-thread-guard-fixture.js"), kind], + env: noReportEnv, + stdio: ["ignore", "pipe", "pipe"], + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toContain("ASSERTION FAILED"); + expect(stderr).toContain(message); + expect(stdout).not.toContain("survived"); + expect(exitCode).not.toBe(0); + }); +} From cce71d34a1f603beafaaf5de281f9d81731638c6 Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 5 Aug 2026 15:56:38 +0000 Subject: [PATCH 3/3] Tighten assertion comments --- src/jsc/bindings/JSCTestingHelpers.cpp | 10 ++++------ src/jsc/bindings/StrongRef.cpp | 9 +++------ 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/jsc/bindings/JSCTestingHelpers.cpp b/src/jsc/bindings/JSCTestingHelpers.cpp index 3b64d930f704..c1bbecbc9f2d 100644 --- a/src/jsc/bindings/JSCTestingHelpers.cpp +++ b/src/jsc/bindings/JSCTestingHelpers.cpp @@ -72,12 +72,10 @@ JSC_DEFINE_HOST_FUNCTION(jsFunctionStartOfFixedExecutableMemoryPool, #endif #if ASSERT_ENABLED -// Deliberately mutates a strong handle owned by this VM from a spawned thread -// that does not hold the API lock. The debug-only thread-affinity assertions -// (JSC::HandleSet::assertMayMutate for JSC::Strong, the Bun__StrongRef__* -// asserts for StrongRootBlock slots) must abort the process before the -// mutation lands; the test asserting on that crash is what keeps the -// detectors from rotting silently. Only compiled when the assertions are. +// Test hook: mutates a strong handle owned by this VM from a spawned thread +// without the API lock. The debug assertions in JSC::HandleSet and +// Bun__StrongRef__* must abort before the mutation lands; +// strong-handle-thread-guard.test.ts asserts on that crash. JSC_DEFINE_HOST_FUNCTION(jsFunctionCrossThreadStrongHandleMutation, (JSGlobalObject * globalObject, CallFrame* callframe)) { diff --git a/src/jsc/bindings/StrongRef.cpp b/src/jsc/bindings/StrongRef.cpp index c5b31ebe98b2..f83d3b65c66d 100644 --- a/src/jsc/bindings/StrongRef.cpp +++ b/src/jsc/bindings/StrongRef.cpp @@ -48,12 +48,9 @@ static ALWAYS_INLINE StrongRootBlock* decodeStrongRefBlock(StrongRefImpl* ref) return reinterpret_cast(slot - static_cast(decodeStrongRefIndex(ref)) * sizeof(StrongRootBlock::Slot) - StrongRootBlock::slotsOffset()); } -// The StrongRootBlock list hangs off JSVMClientData with no synchronization: -// the "Srb" marking constraint (BunClientData.cpp) scans it assuming only the -// mutator, holding the VM's API lock, ever writes it. These asserts are the -// StrongRootBlock mirror of JSC::HandleSet::assertMayMutate, and check the -// OWNER VM (the block's), so a thread that has some other VM's lock still -// trips them. +// The "Srb" marking constraint (BunClientData.cpp) scans the StrongRootBlock +// list without synchronization; holding the owner VM's API lock is what +// orders these mutations with that scan. Mirrors JSC::HandleSet's assertion. #define ASSERT_STRONG_REF_MUTATION_ALLOWED(vm) \ ASSERT_WITH_MESSAGE((vm).currentThreadIsHoldingAPILock(), "Bun::StrongRef handles may only be created, written, or destroyed while holding their VM's API lock")