From 7c721e52437cfa1586655dc4cf70c67eb8ae1ce9 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 1 Oct 2026 13:36:56 +0000 Subject: [PATCH 1/2] [JSC] A failed loadModule() stores its error in the registry entry it loaded moduleLoadTopRejected and moduleLoadStoreError looked the entry up by key when a load by name failed. removeEntry() and clearAll() can leave another load's entry under the key while the first load is in flight, and a second load of the key can register one at any time, so the error went to a module that had loaded. Every later load of the key was then answered with that error. The two loadModule() contexts now record the entry the load has once it has one: the entry the key holds after provideFetch() in moduleLoadTopSettled, and after hostLoadImportedModule() in the loadModule() that takes a referrer. The two error stores write to that entry. A load that failed before it had an entry, which is a load whose fetch failed, stores nothing: an entry under its key at that point was registered by another load. Inside USE(BUN_JSC_ADDITIONS), with upstream's lookups kept in #else. Upstream never rebinds a key, and its lookup by key is the same entry unless two loads of one key are in flight at once. Co-authored-by: Dylan Conway --- Source/JavaScriptCore/runtime/JSMicrotask.cpp | 11 +++++++++++ Source/JavaScriptCore/runtime/JSModuleLoader.cpp | 3 +++ .../JavaScriptCore/runtime/ModuleLoadingContext.cpp | 7 +++++++ Source/JavaScriptCore/runtime/ModuleLoadingContext.h | 5 +++++ 4 files changed, 26 insertions(+) diff --git a/Source/JavaScriptCore/runtime/JSMicrotask.cpp b/Source/JavaScriptCore/runtime/JSMicrotask.cpp index 25281addf0bd5..c7616186bea04 100644 --- a/Source/JavaScriptCore/runtime/JSMicrotask.cpp +++ b/Source/JavaScriptCore/runtime/JSMicrotask.cpp @@ -1192,6 +1192,7 @@ static void moduleLoadTopSettled(JSGlobalObject* globalObject, VM& vm, ThrowScop } #if USE(BUN_JSC_ADDITIONS) } + context->setEntry(vm, context->loader()->getRegisteredMayBeNull(specifier, type)); #endif JSPromise* statePromise = JSPromise::create(vm, globalObject->promiseStructure()); @@ -1274,11 +1275,17 @@ static void moduleLoadTopRejected(JSGlobalObject* globalObject, VM& vm, std::spa if (status == JSPromise::Status::Fulfilled) resultPromise->fulfill(vm, arguments[1]); else { +#if USE(BUN_JSC_ADDITIONS) + // The load's own entry, if it got as far as having one. A load whose fetch failed has none and stores nothing: + // the entry the key holds is then another load's. + if (ModuleRegistryEntry* entry = context->entry()) +#else const Identifier& specifier = context->moduleRequest().m_specifier; auto type = context->moduleRequest().type(); // https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-single-module-script step 13.1 // Only set an error if the entry already exists. if (ModuleRegistryEntry* entry = context->loader()->getRegisteredMayBeNull(specifier, type)) +#endif entry->setEvaluationError(globalObject, arguments[1]); resultPromise->reject(vm, arguments[1]); } @@ -1433,7 +1440,11 @@ static void moduleLoadStoreError(JSGlobalObject* globalObject, std::spanmoduleRequest().type(); // https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-single-module-script step 13.1 // Only set an error if the entry already exists. +#if USE(BUN_JSC_ADDITIONS) + ModuleRegistryEntry* entry = context->entry(); +#else ModuleRegistryEntry* entry = context->loader()->getRegisteredMayBeNull(specifier, type); +#endif if (!entry) return; if (auto* error = dynamicDowncast(errorValue)) { diff --git a/Source/JavaScriptCore/runtime/JSModuleLoader.cpp b/Source/JavaScriptCore/runtime/JSModuleLoader.cpp index ef4f008c26d6a..4bd96b54749d1 100644 --- a/Source/JavaScriptCore/runtime/JSModuleLoader.cpp +++ b/Source/JavaScriptCore/runtime/JSModuleLoader.cpp @@ -926,6 +926,9 @@ JSPromise* JSModuleLoader::loadModule(JSGlobalObject* globalObject, const Module RETURN_IF_EXCEPTION(scope, nullptr); auto* context = ModuleLoadingContext::create(vm, this, moduleRequest, WTF::move(scriptFetcher), flags); +#if USE(BUN_JSC_ADDITIONS) + context->setEntry(vm, getRegisteredMayBeNull(moduleRequest.m_specifier, moduleRequest.type())); +#endif JSPromise* resultPromise = JSPromise::create(vm, globalObject->promiseStructure()); resultPromise->markAsHandled(); diff --git a/Source/JavaScriptCore/runtime/ModuleLoadingContext.cpp b/Source/JavaScriptCore/runtime/ModuleLoadingContext.cpp index 7415905a43c75..a6200fdac20af 100644 --- a/Source/JavaScriptCore/runtime/ModuleLoadingContext.cpp +++ b/Source/JavaScriptCore/runtime/ModuleLoadingContext.cpp @@ -86,6 +86,13 @@ ModuleLoadingContext* ModuleLoadingContext::create(VM& vm, JSModuleLoader* loade return context; } +#if USE(BUN_JSC_ADDITIONS) +void ModuleLoadingContext::setEntry(VM& vm, ModuleRegistryEntry* entry) +{ + m_entry.setMayBeNull(vm, this, entry); +} +#endif + JSModuleLoader::ModuleReferrer ModuleLoadingContext::referrer() const { JSValue ref = m_referrer.get(); diff --git a/Source/JavaScriptCore/runtime/ModuleLoadingContext.h b/Source/JavaScriptCore/runtime/ModuleLoadingContext.h index 24c5a1ef94a7e..6f20118ca5685 100644 --- a/Source/JavaScriptCore/runtime/ModuleLoadingContext.h +++ b/Source/JavaScriptCore/runtime/ModuleLoadingContext.h @@ -70,6 +70,11 @@ class ModuleLoadingContext final : public JSCell { const AbstractModuleRecord::ModuleRequest& moduleRequest() const { return m_moduleRequest; } JSCell* payload() const { return m_payload.get(); } ModuleRegistryEntry* entry() const { return m_entry.get(); } +#if USE(BUN_JSC_ADDITIONS) + // A loadModule() context has none until the load has one. What the load then stores, it stores there: removeEntry() + // and clearAll() can leave another load's entry, or none, under the key while this one is in flight. + void setEntry(VM&, ModuleRegistryEntry*); +#endif ScriptFetcher* scriptFetcher() const { return m_scriptFetcher.get(); } AbstractModuleRecord* module() const { return m_module.get(); } void module(VM& vm, AbstractModuleRecord* mod) { m_module.set(vm, this, mod); } From 08bf09746739fceb5b7e7e313d2f163bdc05545c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 2 Oct 2026 12:19:06 +0000 Subject: [PATCH 2/2] [JSC] A load whose fetch failed leaves another load's entry alone, and a stress test moduleLoadTopSettled registers the key and stores the error when a fetch rejects with an error the loader tagged as not being a fetch error of this key. The load has no entry at that point, so an entry that holds the key is another load's. The store now leaves it alone, like the two stores of the previous commit. With nothing under the key the key is registered as before. JSTests/stress/module-load-failed-fetch-keeps-concurrent-load.js covers the route that needs no removeEntry(): two import()s of one key overlap and the first fails at its fetch. The later import() of the key rejects on main and gets the module here. --- ...load-failed-fetch-keeps-concurrent-load.js | 38 +++++++++++++++++++ JSTests/stress/resources/.gitignore | 2 + Source/JavaScriptCore/runtime/JSMicrotask.cpp | 5 +++ 3 files changed, 45 insertions(+) create mode 100644 JSTests/stress/module-load-failed-fetch-keeps-concurrent-load.js create mode 100644 JSTests/stress/resources/.gitignore diff --git a/JSTests/stress/module-load-failed-fetch-keeps-concurrent-load.js b/JSTests/stress/module-load-failed-fetch-keeps-concurrent-load.js new file mode 100644 index 0000000000000..ef6907eb8c593 --- /dev/null +++ b/JSTests/stress/module-load-failed-fetch-keeps-concurrent-load.js @@ -0,0 +1,38 @@ +// The bytecode-cache mode runs a test twice and the second time takes all code from the cache of the first. The module +// this test writes has a new name on every run, so it is never in that cache. +//@ $skipModes << "bytecode-cache".to_sym + +// Bun: this fork's test. Two loads by name of one key overlap, and the first fails at its fetch because the file is not +// there yet. The first load has no registry entry, so its error must not be stored into the entry the second load +// registered: a later import() of the key gets the module. +// +// The test writes the module it imports into resources/, under a name of its own for every run. + +function shouldBe(actual, expected) +{ + if (actual !== expected) + throw new Error(`expected ${expected} but got ${actual}`); +} + +// writeFile() takes a path as the shell was given this file's; import() resolves against this file. +const directory = /@(.*?)[^\/\\]*:\d+:\d+$/m.exec(new Error().stack)[1]; +const name = `resources/module-load-failed-fetch-${Date.now()}-${Math.floor(Math.random() * 1e9)}.generated.js`; + +const settled = promise => promise.then(module => module.value, error => "rejected"); + +let outcome = "did not finish"; +(async function () { + const first = settled(import(`./${name}`)); + writeFile(directory + name, "export const value = 42;"); + const second = import(`./${name}`); + + shouldBe(await first, "rejected"); + // Whether the second load shares the first one's fetch is not what this tests. + await second.catch(() => { }); + shouldBe(await settled(import(`./${name}`)), 42); + outcome = "passed"; +}()).catch(error => { outcome = error; }); + +drainMicrotasks(); +if (outcome !== "passed") + throw outcome; diff --git a/JSTests/stress/resources/.gitignore b/JSTests/stress/resources/.gitignore new file mode 100644 index 0000000000000..60110308205a9 --- /dev/null +++ b/JSTests/stress/resources/.gitignore @@ -0,0 +1,2 @@ +# Written by ../module-load-failed-fetch-keeps-concurrent-load.js, one file per run. +module-load-failed-fetch-*.generated.js diff --git a/Source/JavaScriptCore/runtime/JSMicrotask.cpp b/Source/JavaScriptCore/runtime/JSMicrotask.cpp index c7616186bea04..a4e0e9b76a517 100644 --- a/Source/JavaScriptCore/runtime/JSMicrotask.cpp +++ b/Source/JavaScriptCore/runtime/JSMicrotask.cpp @@ -1250,7 +1250,12 @@ static void moduleLoadTopSettled(JSGlobalObject* globalObject, VM& vm, ThrowScop auto failure = JSModuleLoader::getErrorInfo(globalObject, error); // https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-single-module-script step 13.1 // Don't register the module unless it's an evaluation error. +#if USE(BUN_JSC_ADDITIONS) + // This load has no entry. One that holds its key is another load's, and is not where this error goes. + if (failure.isEvaluationError(specifier, type) && !context->loader()->getRegisteredMayBeNull(specifier, type)) { +#else if (failure.isEvaluationError(specifier, type)) { +#endif ModuleRegistryEntry* entry = context->loader()->ensureRegistered(globalObject, specifier, type); if (scope.exception()) { intermediatePromise->rejectWithCaughtException(vm, scope);