From 478371077d4b16c92dd79f27234c1e94bd4c5d06 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 25 Aug 2026 15:00:23 +0000 Subject: [PATCH] JSModuleLoader: hand each synchronous load step to the registry entry, and register a fetch before its hook runs Bun's require(esm) drives the module loader synchronously (loadModuleSync). In hostLoadImportedModule it meets entries that an async graph already started and advances them inline: it re-issues the embedder's fetch() for a pending fetch, or runs makeModule() for a fetched source, instead of waiting for the ModuleRegistryFetchSettled / ModuleSettled reactions. Both calls run embedder code. fetch() runs a plugin's load hook or a module mock's factory; makeModule() runs a synthetic module's generator, which reads a user object's getters. That code can load the same key again before the call returns. The nested load reaches the same entry, takes the same branch and settles the step first. The inline code then completed the entry a second time from the state it had read before the call: fetchComplete() on a Fetched entry, fulfillPromise() on a settled promise (both debug assertions), and for makeModule() a second record for one key. The registry held the outer record, every importer held the inner one (Bun: oven-sh/bun#40170, Sentry BUN-4QBM). The same hazard sat in the New path: fetch() ran before the entry was Fetching, so a nested load found a New entry (ASSERT(status != New)), fetched a second time, and the outer return path reset a Fetched entry to Fetching. Make the steps belong to ModuleRegistryEntry. settleFetch() and settleModule() take the promise a hook call produced and apply the step only while the entry is still at it; they return whether they did. hostLoadImportedModule no longer decides anything from state read before a hook call. The reactions already have this shape (moduleRegistryFetchSettled / moduleRegistryModuleSettled), so the first completion now wins on every path. Register a fetch before its hook runs: create the fetch promise and move the entry to Fetching first, pipe the hook's promise in afterwards only if nothing settled the fetch promise meanwhile, and fail the entry if the hook throws. A nested load during the hook now sees a Fetching entry with a pending fetch promise, which is the state the synchronous path knows how to drive. --- .../JavaScriptCore/runtime/JSModuleLoader.cpp | 51 ++++++++++++------ .../runtime/ModuleRegistryEntry.cpp | 54 +++++++++++++++++++ .../runtime/ModuleRegistryEntry.h | 14 +++++ 3 files changed, 104 insertions(+), 15 deletions(-) diff --git a/Source/JavaScriptCore/runtime/JSModuleLoader.cpp b/Source/JavaScriptCore/runtime/JSModuleLoader.cpp index 8f36cd3c96f5..054a56bb53da 100644 --- a/Source/JavaScriptCore/runtime/JSModuleLoader.cpp +++ b/Source/JavaScriptCore/runtime/JSModuleLoader.cpp @@ -731,23 +731,26 @@ JSPromise* JSModuleLoader::hostLoadImportedModule(JSGlobalObject* globalObject, // synchronously so this graph can complete without yielding; the // outer async path will see the entry as Fetched when it eventually // drains and short-circuit. + // + // fetch() and makeModule() run embedder code (a plugin's load hook, + // a synthetic module's generator reading user getters), and that + // code can load this same key again before the call returns. The + // nested load reaches this block for the same entry, takes the same + // branch, and settles the step first. So the value a call returns is + // handed to the entry, which applies the step only if the entry is + // still at it (ModuleRegistryEntry::settleFetch / settleModule). + // Nothing here decides from state read before the call. JSPromise* fetchPromise = mapEntry->ensureFetchPromise(globalObject); JSPromise* modulePromise = mapEntry->ensureModulePromise(globalObject); if (modulePromise->status() == JSPromise::Status::Pending) { if (fetchPromise->status() == JSPromise::Status::Pending) { // Transpilation still in flight — re-issue through the - // embedder's synchronous fetch. fetchPromise was already - // pipeFrom()'d by the async path which set - // isFirstResolvingFunctionCalledFlag, so use the unguarded - // fulfill/reject. The ModuleRegistryFetchSettled reaction on - // fetchPromise lands on the sync queue and drives the rest of - // the chain (including loadPromise). + // embedder's synchronous fetch. The ModuleRegistryFetchSettled + // reaction on fetchPromise lands on the sync queue and drives + // the rest of the chain (including loadPromise). JSPromise* promise = fetch(globalObject, identifierToJSValue(vm, resolved), moduleReferrer(referrerKey), nullptr, scriptFetcher.copyRef()); RETURN_IF_EXCEPTION(scope, nullptr); - if (promise->status() == JSPromise::Status::Fulfilled) - fetchPromise->fulfillPromise(vm, promise->result()); - else if (promise->status() == JSPromise::Status::Rejected) - fetchPromise->rejectPromise(vm, promise->result()); + mapEntry->settleFetch(globalObject, promise); } else if (fetchPromise->status() == JSPromise::Status::Fulfilled) { // fetchPromise already settled but its // ModuleRegistryFetchSettled reaction is sitting on the @@ -757,11 +760,7 @@ JSPromise* JSModuleLoader::hostLoadImportedModule(JSGlobalObject* globalObject, // settled and bail in moduleRegistryFetchSettled's handler. JSPromise* makePromise = makeModule(globalObject, resolved, uncheckedDowncast(fetchPromise->result())); RETURN_IF_EXCEPTION(scope, nullptr); - if (makePromise->status() == JSPromise::Status::Fulfilled) { - mapEntry->fetchComplete(globalObject, uncheckedDowncast(makePromise->result())); - modulePromise->fulfillPromise(vm, makePromise->result()); - } else if (makePromise->status() == JSPromise::Status::Rejected) - modulePromise->rejectPromise(vm, makePromise->result()); + mapEntry->settleModule(globalObject, makePromise); } // The reactions above were diverted to the sync queue but // haven't *run* yet — they'll run when the caller's @@ -800,11 +799,33 @@ JSPromise* JSModuleLoader::hostLoadImportedModule(JSGlobalObject* globalObject, if (mapEntry->status() == ModuleRegistryEntry::Status::New) { // Per "fetch the descendants of a module script", the referrer is the referring module's base URL. +#if USE(BUN_JSC_ADDITIONS) + // Register the fetch before the embedder hook runs. The hook can run + // user code that loads this key again. That load then finds a Fetching + // entry with a pending fetch promise to join (or, in a synchronous load, + // to drive through the block above) instead of a New entry it would + // fetch a second time and whose status this path would then reset. + JSPromise* fetchPromise = mapEntry->ensureFetchPromise(globalObject); + mapEntry->setStatus(ModuleRegistryEntry::Status::Fetching); + JSPromise* promise = fetch(globalObject, identifierToJSValue(vm, resolved), moduleReferrer(referrerKey), moduleRequest.m_attributes, scriptFetcher); + if (Exception* exception = scope.exception()) [[unlikely]] { + // A fetch that threw has nothing else to settle its promise, unless + // a nested load during the hook already did (failFetch checks). A + // termination exception ends the run; leave the entry as it is for it. + if (!vm.isTerminationException(exception)) + mapEntry->failFetch(globalObject, exception->value()); + return nullptr; + } + // A nested load may have settled the fetch promise while the hook ran. + if (fetchPromise->status() == JSPromise::Status::Pending) + fetchPromise->pipeFrom(vm, promise); +#else JSPromise* promise = fetch(globalObject, identifierToJSValue(vm, resolved), moduleReferrer(referrerKey), moduleRequest.m_attributes, scriptFetcher); RETURN_IF_EXCEPTION(scope, nullptr); mapEntry->setStatus(ModuleRegistryEntry::Status::Fetching); mapEntry->ensureFetchPromise(globalObject)->pipeFrom(vm, promise); +#endif } JSPromise* modulePromise = mapEntry->ensureModulePromise(globalObject); RETURN_IF_EXCEPTION(scope, nullptr); diff --git a/Source/JavaScriptCore/runtime/ModuleRegistryEntry.cpp b/Source/JavaScriptCore/runtime/ModuleRegistryEntry.cpp index 4577a58f42b6..2ffa69785b56 100644 --- a/Source/JavaScriptCore/runtime/ModuleRegistryEntry.cpp +++ b/Source/JavaScriptCore/runtime/ModuleRegistryEntry.cpp @@ -290,4 +290,58 @@ void ModuleRegistryEntry::fetchComplete(JSGlobalObject* globalObject, AbstractMo m_status = Status::Fetched; } +#if USE(BUN_JSC_ADDITIONS) +bool ModuleRegistryEntry::settleFetch(JSGlobalObject* globalObject, JSPromise* fetched) +{ + if (m_status != Status::Fetching || !m_fetchPromise || m_fetchPromise->status() != JSPromise::Status::Pending) + return false; + // Settled directly rather than through fulfill()/reject(): once + // hostLoadImportedModule has pipeFrom()'d this promise, + // isFirstResolvingFunctionCalled is set and the guarded forms do nothing. + VM& vm = globalObject->vm(); + switch (fetched->status()) { + case JSPromise::Status::Fulfilled: + m_fetchPromise->fulfillPromise(vm, fetched->result()); + return true; + case JSPromise::Status::Rejected: + m_fetchPromise->rejectPromise(vm, fetched->result()); + return true; + case JSPromise::Status::Pending: + return false; + } + RELEASE_ASSERT_NOT_REACHED(); +} + +bool ModuleRegistryEntry::failFetch(JSGlobalObject* globalObject, JSValue error) +{ + if (m_status != Status::Fetching || !m_fetchPromise || m_fetchPromise->status() != JSPromise::Status::Pending) + return false; + setFetchError(globalObject, error); + m_fetchPromise->rejectPromise(globalObject->vm(), error); + return true; +} + +bool ModuleRegistryEntry::settleModule(JSGlobalObject* globalObject, JSPromise* made) +{ + if (!m_modulePromise || m_modulePromise->status() != JSPromise::Status::Pending) + return false; + VM& vm = globalObject->vm(); + switch (made->status()) { + case JSPromise::Status::Fulfilled: { + auto* record = uncheckedDowncast(made->result()); + fetchComplete(globalObject, record); + m_modulePromise->fulfill(vm, record); + return true; + } + case JSPromise::Status::Rejected: + setEvaluationError(globalObject, made->result()); + m_modulePromise->reject(vm, made->result()); + return true; + case JSPromise::Status::Pending: + return false; + } + RELEASE_ASSERT_NOT_REACHED(); +} +#endif + } // namespace JSC diff --git a/Source/JavaScriptCore/runtime/ModuleRegistryEntry.h b/Source/JavaScriptCore/runtime/ModuleRegistryEntry.h index 79db0ce32115..0808d07d9add 100644 --- a/Source/JavaScriptCore/runtime/ModuleRegistryEntry.h +++ b/Source/JavaScriptCore/runtime/ModuleRegistryEntry.h @@ -101,6 +101,20 @@ class ModuleRegistryEntry final : public JSCell { JSPromise* loadedPromise(JSGlobalObject*); #endif +#if USE(BUN_JSC_ADDITIONS) + // A synchronous load (Bun's require(esm)) drives an entry through its fetch + // and module steps inline instead of through the microtask reactions. The + // embedder hook that produces each value can run user code, and that code + // can load this same entry again before the hook returns. So the entry may + // already be past the step by the time the caller holds the value. Each of + // these applies its step only while the entry is still at it, and returns + // whether it did. The reactions check the same condition on their path + // (moduleRegistryFetchSettled, moduleRegistryModuleSettled). + bool settleFetch(JSGlobalObject*, JSPromise* fetched); + bool failFetch(JSGlobalObject*, JSValue error); + bool settleModule(JSGlobalObject*, JSPromise* made); +#endif + private: ModuleRegistryEntry(VM&, Structure*, Identifier key, ScriptFetchParameters::Type, RefPtr);