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 25281addf0bd5..a4e0e9b76a517 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()); @@ -1249,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); @@ -1274,11 +1280,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 +1445,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); }