[JSC] A failed loadModule() stores its error in the registry entry it loaded

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;

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

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::span<const J
         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 USE(BUN_JSC_ADDITIONS)
+        ModuleRegistryEntry* entry = context->entry();
+#else
         ModuleRegistryEntry* entry = context->loader()->getRegisteredMayBeNull(specifier, type);
+#endif
         if (!entry)
             return;
         if (auto* error = dynamicDowncast<ErrorInstance>(errorValue)) {

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

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

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