A few fixes related to CommonJS module loading (#9540)

* Ensure we always deref the source code * Move more work to concurrent transpiler * Update NodeModuleModule.h * Update string.zig * Make `Bun.gc()` more effective * Update text-loader-fixture-dynamic-import-stress.ts * Update ZigSourceProvider.cpp * Fixes #6946 * Update ModuleLoader.cpp * Update ModuleLoader.cpp * Fixes #8965 * Fixups * Update ModuleLoader.cpp * Update ModuleLoader.cpp * Load it * Update module_loader.zig * Update module_loader.zig --------- Co-authored-by: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com>
2026-02-09 18:38:55 +00:00 · 2024-03-21 10:42:57 -07:00
parent 0948727243
commit e124f08caf
27 changed files with 428 additions and 189 deletions
--- a/src/bun.js/bindings/ModuleLoader.cpp
+++ b/src/bun.js/bindings/ModuleLoader.cpp
@@ -1,8 +1,8 @@
 #include "root.h"
 #include "headers-handwritten.h"
-
+#include "JavaScriptCore/JSGlobalObject.h"
 #include "ModuleLoader.h"
-
+#include "JavaScriptCore/Identifier.h"
 #include "ZigGlobalObject.h"
 #include <JavaScriptCore/JSCInlines.h>
 #include <JavaScriptCore/JSNativeStdFunction.h>
@@ -290,8 +290,15 @@ static JSValue handleVirtualModuleResult(
    auto onLoadResult = handleOnLoadResult(globalObject, virtualModuleResult, specifier, wasModuleMock);
    JSC::VM& vm = globalObject->vm();
    auto scope = DECLARE_THROW_SCOPE(vm);
+    WTF::String sourceCodeStringForDeref;
+    const auto getSourceCodeStringForDeref = [&]() {
+        if (res->success && res->result.value.needsDeref && res->result.value.source_code.tag == BunStringTag::WTFStringImpl) {
+            res->result.value.needsDeref = false;
+            sourceCodeStringForDeref = String(res->result.value.source_code.impl.wtf);
+        }
+    };

-    auto reject = [&](JSC::JSValue exception) -> JSValue {
+    const auto reject = [&](JSC::JSValue exception) -> JSValue {
        if constexpr (allowPromise) {
            return rejectedInternalPromise(globalObject, exception);
        } else {
@@ -300,7 +307,7 @@ static JSValue handleVirtualModuleResult(
        }
    };

-    auto resolve = [&](JSValue code) -> JSValue {
+    const auto resolve = [&](JSValue code) -> JSValue {
        res->success = true;
        if constexpr (allowPromise) {
            scope.release();
@@ -310,7 +317,7 @@ static JSValue handleVirtualModuleResult(
        }
    };

-    auto rejectOrResolve = [&](JSValue code) -> JSValue {
+    const auto rejectOrResolve = [&](JSValue code) -> JSValue {
        if (auto* exception = scope.exception()) {
            if constexpr (allowPromise) {
                scope.clearException();
@@ -336,6 +343,7 @@ static JSValue handleVirtualModuleResult(
        if (!res->success) {
            return reject(JSValue::decode(reinterpret_cast<EncodedJSValue>(res->result.err.ptr)));
        }
+        getSourceCodeStringForDeref();

        auto provider = Zig::SourceProvider::create(globalObject, res->result.value);
        return resolve(JSC::JSSourceCode::create(vm, JSC::SourceCode(provider)));
@@ -383,38 +391,62 @@ static JSValue handleVirtualModuleResult(
 }

 extern "C" void Bun__onFulfillAsyncModule(
-    JSC::EncodedJSValue promiseValue,
+    Zig::GlobalObject* globalObject,
+    JSC::EncodedJSValue encodedPromiseValue,
    ErrorableResolvedSource* res,
    BunString* specifier,
    BunString* referrer)
 {
-    JSC::JSValue value = JSValue::decode(promiseValue);
-    JSC::JSInternalPromise* promise = jsCast<JSC::JSInternalPromise*>(value);
-    auto* globalObject = promise->globalObject();
+    WTF::String sourceCodeStringForDeref;
+    const auto getSourceCodeStringForDeref = [&]() {
+        if (res->result.value.needsDeref && res->result.value.source_code.tag == BunStringTag::WTFStringImpl) {
+            res->result.value.needsDeref = false;
+            sourceCodeStringForDeref = String(res->result.value.source_code.impl.wtf);
+        }
+    };
    auto& vm = globalObject->vm();
    auto scope = DECLARE_THROW_SCOPE(vm);
+    JSC::JSInternalPromise* promise = jsCast<JSC::JSInternalPromise*>(JSC::JSValue::decode(encodedPromiseValue));

    if (!res->success) {
        throwException(scope, res->result.err, globalObject);
        auto* exception = scope.exception();
        scope.clearException();
-        return promise->reject(promise->globalObject(), exception);
+        return promise->reject(globalObject, exception);
    }

-    if (res->result.value.commonJSExportsLen) {
-        auto created = Bun::createCommonJSModule(jsCast<Zig::GlobalObject*>(globalObject), res->result.value);
+    getSourceCodeStringForDeref();
+    auto specifierValue = Bun::toJS(globalObject, *specifier);

-        if (created.has_value()) {
-            return promise->resolve(promise->globalObject(), JSSourceCode::create(vm, WTFMove(created.value())));
+    if (auto entry = globalObject->esmRegistryMap()->get(globalObject, specifierValue)) {
+        if (res->result.value.commonJSExportsLen) {
+            if (entry.isObject()) {
+                if (auto isEvaluated = entry.getObject()->getIfPropertyExists(globalObject, Bun::builtinNames(vm).evaluatedPublicName())) {
+                    if (isEvaluated.isTrue()) {
+                        // it's a race! we lost.
+                        // https://github.com/oven-sh/bun/issues/6946
+                        return;
+                    }
+                }
+            }
+
+            auto created = Bun::createCommonJSModule(jsCast<Zig::GlobalObject*>(globalObject), specifierValue, res->result.value);
+            if (created.has_value()) {
+                JSSourceCode* code = JSSourceCode::create(vm, WTFMove(created.value()));
+                promise->resolve(globalObject, code);
+            } else {
+                auto* exception = scope.exception();
+                scope.clearException();
+                promise->reject(globalObject, exception);
+            }
        } else {
-            auto* exception = scope.exception();
-            scope.clearException();
-            return promise->reject(promise->globalObject(), exception);
+            auto&& provider = Zig::SourceProvider::create(jsDynamicCast<Zig::GlobalObject*>(globalObject), res->result.value);
+            promise->resolve(globalObject, JSC::JSSourceCode::create(vm, JSC::SourceCode(provider)));
        }
+    } else {
+        // the module has since been deleted from the registry.
+        // let's not keep it forever for no reason.
    }
-
-    auto provider = Zig::SourceProvider::create(jsDynamicCast<Zig::GlobalObject*>(globalObject), res->result.value);
-    promise->resolve(promise->globalObject(), JSC::JSSourceCode::create(vm, JSC::SourceCode(provider)));
 }

 extern "C" bool isBunTest;
@@ -432,7 +464,13 @@ JSValue fetchCommonJSModule(
    auto scope = DECLARE_THROW_SCOPE(vm);
    ErrorableResolvedSource resValue;
    ErrorableResolvedSource* res = &resValue;
-
+    WTF::String sourceCodeStringForDeref;
+    const auto getSourceCodeStringForDeref = [&]() {
+        if (res->success && res->result.value.needsDeref && res->result.value.source_code.tag == BunStringTag::WTFStringImpl) {
+            res->result.value.needsDeref = false;
+            sourceCodeStringForDeref = String(res->result.value.source_code.impl.wtf);
+        }
+    };
    auto& builtinNames = WebCore::clientData(vm)->builtinNames();

    bool wasModuleMock = false;
@@ -543,7 +581,7 @@ JSValue fetchCommonJSModule(

    JSMap* registry = globalObject->esmRegistryMap();

-    auto hasAlreadyLoadedESMVersionSoWeShouldntTranspileItTwice = [&]() -> bool {
+    const auto hasAlreadyLoadedESMVersionSoWeShouldntTranspileItTwice = [&]() -> bool {
        JSValue entry = registry->get(globalObject, specifierValue);

        if (!entry || !entry.isObject()) {
@@ -559,6 +597,7 @@ JSValue fetchCommonJSModule(
    }

    Bun__transpileFile(bunVM, globalObject, specifier, referrer, typeAttribute, res, false);
+    getSourceCodeStringForDeref();

    if (res->success && res->result.value.commonJSExportsLen) {
        target->evaluate(globalObject, specifier->toWTFString(BunString::ZeroCopy), res->result.value);
@@ -605,6 +644,7 @@ extern "C" bool isBunTest;
 template<bool allowPromise>
 static JSValue fetchESMSourceCode(
    Zig::GlobalObject* globalObject,
+    JSC::JSValue specifierJS,
    ErrorableResolvedSource* res,
    BunString* specifier,
    BunString* referrer,
@@ -614,16 +654,16 @@ static JSValue fetchESMSourceCode(
    auto& vm = globalObject->vm();
    auto scope = DECLARE_THROW_SCOPE(vm);

-    auto reject = [&](JSC::JSValue exception) -> JSValue {
+    const auto reject = [&](JSC::JSValue exception) -> JSValue {
        if constexpr (allowPromise) {
            return rejectedInternalPromise(globalObject, exception);
        } else {
            throwException(globalObject, scope, exception);
-            return JSC::jsUndefined();
+            return {};
        }
    };

-    auto resolve = [&](JSValue code) -> JSValue {
+    const auto resolve = [&](JSValue code) -> JSValue {
        if constexpr (allowPromise) {
            auto* ret = resolvedInternalPromise(globalObject, code);
            scope.release();
@@ -633,7 +673,7 @@ static JSValue fetchESMSourceCode(
        }
    };

-    auto rejectOrResolve = [&](JSValue code) -> JSValue {
+    const auto rejectOrResolve = [&](JSValue code) -> JSValue {
        if (auto* exception = scope.exception()) {
            if constexpr (!allowPromise) {
                scope.release();
@@ -709,17 +749,27 @@ static JSValue fetchESMSourceCode(
        }
    }

+    WTF::String sourceCodeStringForDeref;
+    const auto getSourceCodeStringForDeref = [&]() {
+        if (res->success && res->result.value.needsDeref && res->result.value.source_code.tag == BunStringTag::WTFStringImpl) {
+            res->result.value.needsDeref = false;
+            sourceCodeStringForDeref = String(res->result.value.source_code.impl.wtf);
+        }
+    };
+
    if constexpr (allowPromise) {
-        void* pendingCtx = Bun__transpileFile(bunVM, globalObject, specifier, referrer, typeAttribute, res, true);
+        auto* pendingCtx = Bun__transpileFile(bunVM, globalObject, specifier, referrer, typeAttribute, res, true);
+        getSourceCodeStringForDeref();
        if (pendingCtx) {
-            return reinterpret_cast<JSC::JSInternalPromise*>(pendingCtx);
+            return pendingCtx;
        }
    } else {
        Bun__transpileFile(bunVM, globalObject, specifier, referrer, typeAttribute, res, false);
+        getSourceCodeStringForDeref();
    }

    if (res->success && res->result.value.commonJSExportsLen) {
-        auto created = Bun::createCommonJSModule(globalObject, res->result.value);
+        auto created = Bun::createCommonJSModule(globalObject, specifierJS, res->result.value);

        if (created.has_value()) {
            return rejectOrResolve(JSSourceCode::create(vm, WTFMove(created.value())));
@@ -730,7 +780,7 @@ static JSValue fetchESMSourceCode(
            scope.clearException();
            return rejectedInternalPromise(globalObject, exception);
        } else {
-            return JSC::jsUndefined();
+            return {};
        }
    }

@@ -769,8 +819,8 @@ static JSValue fetchESMSourceCode(
        return rejectOrResolve(JSSourceCode::create(globalObject->vm(), WTFMove(source)));
    }

-    auto&& provider = Zig::SourceProvider::create(globalObject, res->result.value);
-    return rejectOrResolve(JSC::JSSourceCode::create(vm, JSC::SourceCode(provider)));
+    return rejectOrResolve(JSC::JSSourceCode::create(vm,
+        JSC::SourceCode(Zig::SourceProvider::create(globalObject, res->result.value))));
 }

 extern "C" JSC::EncodedJSValue jsFunctionOnLoadObjectResultResolve(JSC::JSGlobalObject* globalObject, JSC::CallFrame* callFrame)
@@ -828,21 +878,23 @@ extern "C" JSC::EncodedJSValue jsFunctionOnLoadObjectResultReject(JSC::JSGlobalO

 JSValue fetchESMSourceCodeSync(
    Zig::GlobalObject* globalObject,
+    JSC::JSValue specifierJS,
    ErrorableResolvedSource* res,
    BunString* specifier,
    BunString* referrer,
    BunString* typeAttribute)
 {
-    return fetchESMSourceCode<false>(globalObject, res, specifier, referrer, typeAttribute);
+    return fetchESMSourceCode<false>(globalObject, specifierJS, res, specifier, referrer, typeAttribute);
 }

 JSValue fetchESMSourceCodeAsync(
    Zig::GlobalObject* globalObject,
+    JSC::JSValue specifierJS,
    ErrorableResolvedSource* res,
    BunString* specifier,
    BunString* referrer,
    BunString* typeAttribute)
 {
-    return fetchESMSourceCode<true>(globalObject, res, specifier, referrer, typeAttribute);
+    return fetchESMSourceCode<true>(globalObject, specifierJS, res, specifier, referrer, typeAttribute);
 }
 }