module-loader: narrow TLA re-entrancy skip to dynamic-import initiator (#30651)

scripts/build/deps/webkit.ts

@@ -3,11 +3,12 @@
  * for local mode. Override via `--webkit-version=<hash>` 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.

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);
     }

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);
     }

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": `