diff --git a/scripts/build/deps/webkit.ts b/scripts/build/deps/webkit.ts index ed55d05b4c5..01ba1b32e65 100644 --- a/scripts/build/deps/webkit.ts +++ b/scripts/build/deps/webkit.ts @@ -3,11 +3,12 @@ * for local mode. Override via `--webkit-version=` to test a branch. * From https://github.com/oven-sh/WebKit releases. */ -// oven-sh/WebKit main: macOS + Windows artifacts cross-compiled on Linux, -// -lto variants built with ThinLTO (per-module summaries for cross-language -// importing), and the Windows ICU data table filtered + per-item zstd -// compressed (lazily decompressed via bun_icu_decompress.cpp). -export const WEBKIT_VERSION = "09f04cd5a489b7c0b44aed255bfafce2a316eada"; +// oven-sh/WebKit#230 preview (rebased onto main 9cb85a07, the upstream +// WebKit 24362e675175 bump): narrows the TLA re-entrancy skip to the +// dynamic-import initiator for #30651. macOS + Windows artifacts +// cross-compiled on Linux, -lto variants ThinLTO, Windows ICU data +// filtered + per-item zstd compressed. +export const WEBKIT_VERSION = "autobuild-preview-pr-230-7dea873b"; /** * WebKit (JavaScriptCore) — the JS engine. diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 7ec2a769837..c022b0166c1 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -3477,6 +3477,37 @@ JSC::JSPromise* GlobalObject::moduleLoaderImportModule(JSGlobalObject* jsGlobalO } } + // Registry key of the module whose body is initiating this dynamic + // import, for the #30651 dep == initiator discriminator. Inverts + // Zig::toSourceOrigin — `node:fs` is stored under `node:fs` but its + // sourceOrigin URL is `builtin://node/fs`, so a raw `substring(10)` + // would yield `node/fs` and miss the registry. Query-string-loaded + // file modules (`import("./x.mjs?v=1")`) are still not round-trip- + // recoverable from sourceOrigin alone — those fall back to the + // coarse VM::hasPendingDynamicImport() gate on the WebKit side. + auto referrerKeyFromSourceOrigin = [&]() -> JSC::Identifier { + const auto& url = sourceOrigin.url(); + if (url.isEmpty()) + return {}; + String keyString; + if (url.protocolIsFile()) { + keyString = url.fileSystemPath(); + } else if (url.protocol() == "builtin"_s && url.string().startsWith("builtin://"_s)) { + auto rest = url.string().substring(10); + if (rest.startsWith("node/"_s)) + keyString = makeString("node:"_s, rest.substring(5)); + else if (rest.startsWith("bun/"_s)) + keyString = makeString("bun:"_s, rest.substring(4)); + else + keyString = WTF::move(rest); + } else { + keyString = url.string(); + } + if (keyString.isEmpty()) + return {}; + return JSC::Identifier::fromString(vm, keyString); + }; + JSC::Identifier resolvedIdentifier; auto moduleName = moduleNameValue->value(globalObject); @@ -3485,7 +3516,7 @@ JSC::JSPromise* GlobalObject::moduleLoaderImportModule(JSGlobalObject* jsGlobalO if (auto resolution = globalObject->onLoadPlugins.resolveVirtualModule(moduleName, sourceOrigin.url().protocolIsFile() ? sourceOrigin.url().fileSystemPath() : String())) { resolvedIdentifier = JSC::Identifier::fromString(vm, resolution.value()); - auto result = JSC::importModule(globalObject, resolvedIdentifier, JSC::Identifier(), parameters, nullptr); + auto result = JSC::importModule(globalObject, resolvedIdentifier, referrerKeyFromSourceOrigin(), parameters, nullptr); if (scope.exception()) [[unlikely]] { return JSC::JSPromise::rejectedPromiseWithCaughtException(globalObject, scope); } @@ -3559,7 +3590,7 @@ JSC::JSPromise* GlobalObject::moduleLoaderImportModule(JSGlobalObject* jsGlobalO // ScriptFetchParameters before calling this hook, so `parameters` is // already the parsed RefPtr (or null). Just forward it. auto result = JSC::importModule(globalObject, resolvedIdentifier, - JSC::Identifier(), WTF::move(parameters), nullptr); + referrerKeyFromSourceOrigin(), WTF::move(parameters), nullptr); if (scope.exception()) [[unlikely]] { return JSC::JSPromise::rejectedPromiseWithCaughtException(globalObject, scope); } diff --git a/src/runtime/bake/BakeGlobalObject.cpp b/src/runtime/bake/BakeGlobalObject.cpp index 43919747bd8..55421c128be 100644 --- a/src/runtime/bake/BakeGlobalObject.cpp +++ b/src/runtime/bake/BakeGlobalObject.cpp @@ -25,6 +25,17 @@ bakeModuleLoaderImportModule(JSC::JSGlobalObject* global, WTF::String keyString = moduleNameValue->getString(global); if (keyString.startsWith("bake:/"_s)) { auto& vm = JSC::getVM(global); + // Pass an empty referrer here rather than plumbing the source + // origin through for the #30651 dep == initiator discriminator. + // bakeModuleLoaderResolve's behavior depends on the referrer: + // with a `bake:/...` referrer, BakeProdResolve is fed the + // specifier as a referrer-relative path and rejects `bake:/...` + // inputs as "Non-relative import … in production assets". An + // empty referrer takes the `keyView.startsWith("bake:/")` branch + // (see bakeModuleLoaderResolve below) which calls BakeProdResolve + // with `"bake:/"` as the base instead. Bake dynamic imports + // therefore fall back to the coarse VM::hasPendingDynamicImport() + // gate on the WebKit side, matching pre-#30651 behavior. return JSC::importModule(global, JSC::Identifier::fromString(vm, keyString), JSC::Identifier(), WTF::move(parameters), nullptr); } @@ -45,6 +56,9 @@ bakeModuleLoaderImportModule(JSC::JSGlobalObject* global, BunString result = BakeProdResolve(global, Bun::toString(refererString), Bun::toString(keyString)); RETURN_IF_EXCEPTION(scope, nullptr); + // Same reasoning as the bake:/ specifier branch above — passing + // `refererString` here would route through BakeProdResolve again + // via bakeModuleLoaderResolve and throw "Non-relative import". return JSC::importModule(global, JSC::Identifier::fromString(vm, result.toWTFString()), JSC::Identifier(), WTF::move(parameters), nullptr); } diff --git a/test/js/bun/resolve/dynamic-import-tla-cycle.test.ts b/test/js/bun/resolve/dynamic-import-tla-cycle.test.ts index 503b185f0c7..b0fe01eb970 100644 --- a/test/js/bun/resolve/dynamic-import-tla-cycle.test.ts +++ b/test/js/bun/resolve/dynamic-import-tla-cycle.test.ts @@ -162,9 +162,66 @@ test("static sibling import waits for a TLA dep that suspended earlier in the sa expect(exitCode).toBe(0); }); -// Same as above but the TLA dep is reached indirectly through different parents -// (so neither parent is on the DFS stack when the second one visits it). Guards -// against discriminating by "is an asyncParentModule on the stack". +// #30651: same TDZ hole as #30259 but reached through two *separate* dynamic +// imports — the #30262 fix keys on `asyncEvaluationOrder < asyncOrderWatermark`, +// which fires whenever the TLA dep first suspended in a prior Evaluate() pass. +// For a parallel dynamic import that just walks into the suspended dep (not +// the Nitro self-deadlock the skip was written for) we must still take the +// spec wait. Discriminator: the dynamic-import initiator the DFS was launched +// from — skip only when dep == initiator. +// +// Timing note: `setTimeout` is the race condition this PR fixes. The first +// dynamic import must fully complete its DFS (tla.mjs transitions to +// EvaluatingAsync) before the second dynamic import's Evaluate() runs. File +// fetching in the module loader is async I/O, so purely promise-chained +// coordination inside JS can't sequence it deterministically — we need a +// timer to yield to the loop iteration that drains the I/O completions +// between the two imports. +test("parallel dynamic imports of the same TLA dep wait instead of running against TDZ bindings", async () => { + using dir = tempDir("parallel-dynamic-tla", { + "driver.mjs": ` + const p1 = import("./entry1.mjs"); + // 10ms is enough for p1's fetch+link+Evaluate() to complete and + // leave tla.mjs parked in EvaluatingAsync (its \`await\` is on a + // 100ms timer that outlives this delay). + await new Promise(r => setTimeout(r, 10)); + const p2 = import("./entry2.mjs"); + await Promise.all([p1, p2]); + `, + "entry1.mjs": ` + import { foo } from "./tla.mjs"; + console.log("entry1 foo:", foo); + `, + "entry2.mjs": ` + import { foo } from "./tla.mjs"; + console.log("entry2 foo:", foo); + `, + "tla.mjs": ` + await new Promise(r => setTimeout(r, 100)); + export const foo = 123; + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "driver.mjs"], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(stdout.trim().split("\n").sort()).toEqual(["entry1 foo: 123", "entry2 foo: 123"]); + expect(exitCode).toBe(0); +}); + +// Variant of the "static sibling import waits for a TLA dep that suspended +// earlier in the same Evaluate()" test above — the TLA dep is reached +// indirectly through different parents (so neither parent is on the DFS +// stack when the second one visits it). Guards against discriminating by +// "is an asyncParentModule on the stack". test("static sibling import waits for an indirectly-shared TLA dep in the same Evaluate()", async () => { using dir = tempDir("static-sibling-tla-indirect", { "root.ts": `