From 2b2a366a3faea16f92f934f219c70fa076e08fd0 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 19 Aug 2026 09:37:10 +0000 Subject: [PATCH 1/2] JSModuleLoader: a removed registry entry's in-flight load must not re-cache its record Bun's removeEntry() (mock.module(), delete require.cache[key], plugin virtual modules) drops a registry entry, and its m_loadedModules entry, while a load of that entry can still be in flight. When that load completed, finishLoadingImportedModule() put its record back into m_loadedModules. The next top-level import of the key then took the [[LoadedModules]] fast path in hostLoadImportedModule(), looked up the registry entry that now exists for the key (a fresh one created by provideFetch(), or none), and returned its loadPromise(), which is null. JSModuleLoader::loadModule() dereferenced it in performPromiseThenWithInternalMicrotask(). finishLoadingImportedModule() now only caches a record in the realm-level map while the registry still holds that record through a loaded entry. The in-flight load still continues its payload, and a module's own [[LoadedModules]] is still filled in. hostLoadImportedModule() checks the same condition on a realm-level cache hit, and otherwise forgets the record and loads what the registry holds. --- .../JavaScriptCore/runtime/JSModuleLoader.cpp | 44 ++++++++++++++++++- .../JavaScriptCore/runtime/JSModuleLoader.h | 13 ++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/Source/JavaScriptCore/runtime/JSModuleLoader.cpp b/Source/JavaScriptCore/runtime/JSModuleLoader.cpp index 8f36cd3c96f5..0309735121de 100644 --- a/Source/JavaScriptCore/runtime/JSModuleLoader.cpp +++ b/Source/JavaScriptCore/runtime/JSModuleLoader.cpp @@ -638,7 +638,21 @@ JSPromise* JSModuleLoader::hostLoadImportedModule(JSGlobalObject* globalObject, // Consult it now so we can skip the host resolve() hook for repeat imports. { auto& loadedModules = record ? record->loadedModules() : m_loadedModules; - if (auto iter = loadedModules.find(moduleMapKey); iter != loadedModules.end()) { + auto iter = loadedModules.find(moduleMapKey); +#if USE(BUN_JSC_ADDITIONS) + // removeEntry() edits the registry underneath the realm-level cache, so the + // hit below cannot take what it asserts for granted there: the registry may + // by now hold a new entry for the key (whose loadPromise may still be null) + // or none at all. Forget such a record and load what the registry holds. + // A module's own [[LoadedModules]] is not affected: innerModuleLoading + // consults it before calling here. + if (!record && iter != loadedModules.end() && !isCacheableLoadedModule(iter->value.m_module.get(), type)) { + Locker locker { cellLock() }; + loadedModules.remove(iter); + iter = loadedModules.end(); + } +#endif + if (iter != loadedModules.end()) { AbstractModuleRecord* loaded = iter->value.m_module.get(); ModuleRegistryEntry* loadedEntry = getRegisteredMayBeNull(loaded->moduleKey(), type); ASSERT(loadedEntry); @@ -958,8 +972,27 @@ void JSModuleLoader::finishLoadingImportedModule(JSGlobalObject* globalObject, c VM& vm = globalObject->vm(); auto scope = DECLARE_THROW_SCOPE(vm); +#if USE(BUN_JSC_ADDITIONS) + // A top-level load whose registry entry removeEntry() dropped while it was in + // flight still completes here (its ModuleLoadingContext holds the detached + // entry) and must still continue its payload, but its record has to stay out + // of the realm-level cache: the next import of the specifier has to load the + // key again, and a replacement load may already have cached its own record + // under the specifier, which step 1.a would assert against. A module's own + // [[LoadedModules]] is always filled in: it links against the record it got. + bool cacheRecord = true; + if (referrer.isRealm()) { + if (auto* resultRecord = std::get_if(&result)) + cacheRecord = isCacheableLoadedModule(*resultRecord, moduleRequest.type()); + } +#endif + // 1. If result is a normal completion, then +#if USE(BUN_JSC_ADDITIONS) + if (auto* resultRecord = cacheRecord ? std::get_if(&result) : nullptr) { +#else if (auto* resultRecord = std::get_if(&result)) { +#endif JSCell* owner = nullptr; auto& loadedModules = [&] -> ModuleMap & { @@ -1125,6 +1158,15 @@ ModuleRegistryEntry* JSModuleLoader::getRegisteredMayBeNull(const Identifier& ke } #if USE(BUN_JSC_ADDITIONS) +bool JSModuleLoader::isCacheableLoadedModule(AbstractModuleRecord* record, ScriptFetchParameters::Type type) +{ + // Mirrors what hostLoadImportedModule's cache hit asserts and returns: the + // entry's loadedPromise(), which exists for an entry that went through + // hostLoadImportedModule and for one the embedder markLoaded(). + ModuleRegistryEntry* entry = getRegisteredMayBeNull(record->moduleKey(), type); + return entry && entry->record() == record && (entry->loadPromise() || entry->isLoaded()); +} + int64_t JSModuleLoader::asyncEvaluationOrderForKey(const Identifier& key) { // For the deadlock-avoidance skip at innerModuleEvaluation step 12.b.v. diff --git a/Source/JavaScriptCore/runtime/JSModuleLoader.h b/Source/JavaScriptCore/runtime/JSModuleLoader.h index a422b279dd0f..9ae95973d4b7 100644 --- a/Source/JavaScriptCore/runtime/JSModuleLoader.h +++ b/Source/JavaScriptCore/runtime/JSModuleLoader.h @@ -197,6 +197,10 @@ class JSModuleLoader final : public JSCell { { // Bun's registry is conceptually flat (one entry per specifier), so // delete every (specifier, type) variant — text/json/HostDefined etc. + // + // A load of the removed entry may still be in flight; it completes + // against the detached entry. isCacheableLoadedModule() keeps its + // record out of m_loadedModules from then on. auto* impl = key.impl(); m_loadedModules.removeIf([&](auto& entry) { return entry.key.first == impl; }); m_resolutionFailures.removeIf([&](auto& entry) { return entry.key.first == impl || entry.key.second == impl; }); @@ -221,6 +225,15 @@ class JSModuleLoader final : public JSCell { JSModuleLoader(VM&, Structure*); void finishCreation(JSGlobalObject*, VM&); +#if USE(BUN_JSC_ADDITIONS) + // hostLoadImportedModule() answers a hit in m_loadedModules with the registry + // entry of the cached record, so the cache may only hold records that the + // registry still holds, through an entry that has been loaded. removeEntry() + // can drop or replace an entry while a load of it is still in flight; both + // the hit and the insertion (finishLoadingImportedModule()) check this. + bool isCacheableLoadedModule(AbstractModuleRecord*, ScriptFetchParameters::Type); +#endif + void addResolutionFailure(VM&, const ResolutionMapKey&, JSValue error); // Corresponds to RealmRecord.[[LoadedModules]]. From eaacac2867b9ee5a07b85f0e3fce794c11e72a93 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 19 Aug 2026 18:16:27 +0000 Subject: [PATCH 2/2] JSModuleLoader: say how a cache line outlives its entry, and that removeEntry() callers hold the cell lock --- Source/JavaScriptCore/runtime/JSModuleLoader.cpp | 12 +++++++----- Source/JavaScriptCore/runtime/JSModuleLoader.h | 6 +++--- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/Source/JavaScriptCore/runtime/JSModuleLoader.cpp b/Source/JavaScriptCore/runtime/JSModuleLoader.cpp index 0309735121de..94aa4729a482 100644 --- a/Source/JavaScriptCore/runtime/JSModuleLoader.cpp +++ b/Source/JavaScriptCore/runtime/JSModuleLoader.cpp @@ -641,11 +641,13 @@ JSPromise* JSModuleLoader::hostLoadImportedModule(JSGlobalObject* globalObject, auto iter = loadedModules.find(moduleMapKey); #if USE(BUN_JSC_ADDITIONS) // removeEntry() edits the registry underneath the realm-level cache, so the - // hit below cannot take what it asserts for granted there: the registry may - // by now hold a new entry for the key (whose loadPromise may still be null) - // or none at all. Forget such a record and load what the registry holds. - // A module's own [[LoadedModules]] is not affected: innerModuleLoading - // consults it before calling here. + // hit below cannot take what it asserts for granted there. removeEntry() + // takes a key and this cache is keyed by the request specifier, so when the + // host resolves the specifier to a different key the line outlives its entry: + // the registry may by now hold a new entry for the record's key (whose + // loadPromise may still be null) or none at all. Forget such a record and + // load what the registry holds. A module's own [[LoadedModules]] is not + // affected: innerModuleLoading consults it before calling here. if (!record && iter != loadedModules.end() && !isCacheableLoadedModule(iter->value.m_module.get(), type)) { Locker locker { cellLock() }; loadedModules.remove(iter); diff --git a/Source/JavaScriptCore/runtime/JSModuleLoader.h b/Source/JavaScriptCore/runtime/JSModuleLoader.h index 9ae95973d4b7..bb3aa3778c91 100644 --- a/Source/JavaScriptCore/runtime/JSModuleLoader.h +++ b/Source/JavaScriptCore/runtime/JSModuleLoader.h @@ -198,9 +198,9 @@ class JSModuleLoader final : public JSCell { // Bun's registry is conceptually flat (one entry per specifier), so // delete every (specifier, type) variant — text/json/HostDefined etc. // - // A load of the removed entry may still be in flight; it completes - // against the detached entry. isCacheableLoadedModule() keeps its - // record out of m_loadedModules from then on. + // A load of the removed entry may still be in flight. Its steps keep + // the detached entry, and isCacheableLoadedModule() keeps the record + // they produce out of m_loadedModules. Callers hold cellLock(). auto* impl = key.impl(); m_loadedModules.removeIf([&](auto& entry) { return entry.key.first == impl; }); m_resolutionFailures.removeIf([&](auto& entry) { return entry.key.first == impl || entry.key.second == impl; });