Fix memory leak in JSBundlerPlugin and remove a couple JSC::Strong (#22488)

### What does this PR do? Since `JSBundlerPlugin` did not inherit from `JSDestructibleObject`, it did not call the destructor. This means it never called the destructor on `BundlerPlugin`, which means it leaked the WTF::Vector of RegExp and strings. This adds a small `WriteBarrierList` abstraction that is a `WriteBarrier` guarded by the owning `JSCell`'s `cellLock()` that has a `visitChildren` function. This also removes two usages of `JSC::Strong` on the `Zig::GlboalObject` and replaces them with the `WriteBarrierList`. ### How did you verify your code works? Added a test. The test did not previously fail. But it's still good to have a test that checks the onLoad callbacks are finalized.
2026-02-02 15:08:46 +00:00 · 2025-09-08 14:11:38 -07:00
parent 7a199276fb
commit 18e4da1903
6 changed files with 236 additions and 88 deletions
--- a/src/bun.js/bindings/JSBundlerPlugin.cpp
+++ b/src/bun.js/bindings/JSBundlerPlugin.cpp
@@ -29,7 +29,7 @@
 #include <JavaScriptCore/YarrMatchingContextHolder.h>
 #include "ErrorCode.h"
 #include "napi_external.h"
-#include <JavaScriptCore/Strong.h>
+
 #include <JavaScriptCore/JSPromise.h>

 #if OS(WINDOWS)
@@ -117,9 +117,9 @@ static const HashTableValue JSBundlerPluginHashTable[] = {
    { "generateDeferPromise"_s, static_cast<unsigned>(JSC::PropertyAttribute::Function | JSC::PropertyAttribute::ReadOnly | JSC::PropertyAttribute::DontDelete), NoIntrinsic, { HashTableValue::NativeFunctionType, jsBundlerPluginFunction_generateDeferPromise, 0 } },
 };

-class JSBundlerPlugin final : public JSC::JSNonFinalObject {
+class JSBundlerPlugin final : public JSC::JSDestructibleObject {
 public:
-    using Base = JSC::JSNonFinalObject;
+    using Base = JSC::JSDestructibleObject;
    static JSBundlerPlugin* create(JSC::VM& vm,
        JSC::JSGlobalObject* globalObject,
        JSC::Structure* structure,
@@ -156,6 +156,9 @@ public:
    }

    DECLARE_VISIT_CHILDREN;
+    DECLARE_VISIT_OUTPUT_CONSTRAINTS;
+
+    template<typename Visitor> void visitAdditionalChildren(Visitor&);

    Bun::BundlerPlugin plugin;
    /// These are defined in BundlerPlugin.ts
@@ -165,30 +168,53 @@ public:

    JSC::JSGlobalObject* m_globalObject;

+    static void destroy(JSC::JSCell* cell)
+    {
+        JSBundlerPlugin* thisObject = static_cast<JSBundlerPlugin*>(cell);
+        thisObject->~JSBundlerPlugin();
+    }
+
 private:
    JSBundlerPlugin(JSC::VM& vm, JSC::JSGlobalObject* global, JSC::Structure* structure, void* config, BunPluginTarget target,
        JSBundlerPluginAddErrorCallback addError, JSBundlerPluginOnLoadAsyncCallback onLoadAsync, JSBundlerPluginOnResolveAsyncCallback onResolveAsync)
-        : JSC::JSNonFinalObject(vm, structure)
+        : Base(vm, structure)
        , plugin(BundlerPlugin(config, target, addError, onLoadAsync, onResolveAsync))
        , m_globalObject(global)
    {
    }

+    ~JSBundlerPlugin() = default;
    void finishCreation(JSC::VM&);
 };

+template<typename Visitor>
+void JSBundlerPlugin::visitAdditionalChildren(Visitor& visitor)
+{
+    this->onLoadFunction.visit(visitor);
+    this->onResolveFunction.visit(visitor);
+    this->setupFunction.visit(visitor);
+    this->plugin.deferredPromises.visit(this, visitor);
+}
+
 template<typename Visitor>
 void JSBundlerPlugin::visitChildrenImpl(JSCell* cell, Visitor& visitor)
 {
    JSBundlerPlugin* thisObject = jsCast<JSBundlerPlugin*>(cell);
    ASSERT_GC_OBJECT_INHERITS(thisObject, info());
    Base::visitChildren(thisObject, visitor);
-    thisObject->onLoadFunction.visit(visitor);
-    thisObject->onResolveFunction.visit(visitor);
-    thisObject->setupFunction.visit(visitor);
+    thisObject->visitAdditionalChildren(visitor);
 }
 DEFINE_VISIT_CHILDREN(JSBundlerPlugin);

+template<typename Visitor>
+void JSBundlerPlugin::visitOutputConstraintsImpl(JSCell* cell, Visitor& visitor)
+{
+    JSBundlerPlugin* thisObject = jsCast<JSBundlerPlugin*>(cell);
+    ASSERT_GC_OBJECT_INHERITS(thisObject, info());
+    thisObject->visitAdditionalChildren(visitor);
+}
+DEFINE_VISIT_OUTPUT_CONSTRAINTS(JSBundlerPlugin);
+
 const JSC::ClassInfo JSBundlerPlugin::s_info = { "BundlerPlugin"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSBundlerPlugin) };

 /// `BundlerPlugin.prototype.addFilter(filter: RegExp, namespace: string, isOnLoad: 0 | 1): void`
@@ -425,10 +451,11 @@ JSC_DEFINE_HOST_FUNCTION(jsBundlerPluginFunction_onResolveAsync, (JSC::JSGlobalO

 extern "C" JSC::EncodedJSValue JSBundlerPlugin__appendDeferPromise(Bun::JSBundlerPlugin* pluginObject)
 {
-    JSC::JSGlobalObject* globalObject = pluginObject->globalObject();
-    Strong<JSPromise> strong_promise = JSC::Strong<JSPromise>(globalObject->vm(), JSPromise::create(globalObject->vm(), globalObject->promiseStructure()));
-    JSPromise* ret = strong_promise.get();
-    pluginObject->plugin.deferredPromises.append(strong_promise);
+    auto* vm = &pluginObject->vm();
+    auto* globalObject = pluginObject->globalObject();
+
+    JSPromise* ret = JSPromise::create(*vm, globalObject->promiseStructure());
+    pluginObject->plugin.deferredPromises.append(*vm, pluginObject, ret);

    return JSC::JSValue::encode(ret);
 }
@@ -638,15 +665,22 @@ extern "C" void JSBundlerPlugin__setConfig(Bun::JSBundlerPlugin* plugin, void* c

 extern "C" void JSBundlerPlugin__drainDeferred(Bun::JSBundlerPlugin* pluginObject, bool rejected)
 {
-    auto deferredPromises = std::exchange(pluginObject->plugin.deferredPromises, {});
-    for (auto& promise : deferredPromises) {
+    auto* globalObject = pluginObject->globalObject();
+    MarkedArgumentBuffer arguments;
+    pluginObject->plugin.deferredPromises.moveTo(pluginObject, arguments);
+    ASSERT(!arguments.hasOverflowed());
+
+    auto scope = DECLARE_THROW_SCOPE(pluginObject->vm());
+    for (auto promiseValue : arguments) {
+        JSPromise* promise = jsCast<JSPromise*>(JSValue::decode(promiseValue));
        if (rejected) {
-            promise->reject(pluginObject->globalObject(), JSC::jsUndefined());
+            promise->reject(globalObject, JSC::jsUndefined());
        } else {
-            promise->resolve(pluginObject->globalObject(), JSC::jsUndefined());
+            promise->resolve(globalObject, JSC::jsUndefined());
        }
-        promise.clear();
+        RETURN_IF_EXCEPTION(scope, );
    }
+    RETURN_IF_EXCEPTION(scope, );
 }

 extern "C" void JSBundlerPlugin__tombstone(Bun::JSBundlerPlugin* plugin)
--- a/src/bun.js/bindings/JSBundlerPlugin.h
+++ b/src/bun.js/bindings/JSBundlerPlugin.h
@@ -7,6 +7,7 @@
 #include <JavaScriptCore/RegularExpression.h>
 #include "napi_external.h"
 #include <JavaScriptCore/Yarr.h>
+#include "WriteBarrierList.h"

 typedef void (*JSBundlerPluginAddErrorCallback)(void*, void*, JSC::EncodedJSValue, JSC::EncodedJSValue);
 typedef void (*JSBundlerPluginOnLoadAsyncCallback)(void*, void*, JSC::EncodedJSValue, JSC::EncodedJSValue);
@@ -134,7 +135,7 @@ public:
    NativePluginList onBeforeParse = {};
    BunPluginTarget target { BunPluginTargetBrowser };

-    Vector<Strong<JSPromise>> deferredPromises = {};
+    WriteBarrierList<JSC::JSPromise> deferredPromises = {};

    JSBundlerPluginAddErrorCallback addError;
    JSBundlerPluginOnLoadAsyncCallback onLoadAsync;
--- a/src/bun.js/bindings/WriteBarrierList.h
+++ b/src/bun.js/bindings/WriteBarrierList.h
@@ -0,0 +1,92 @@
+#pragma once
+
+#include <type_traits>
+#include <wtf/Vector.h>
+#include <JavaScriptCore/WriteBarrier.h>
+
+namespace Bun {
+
+/**
+ * A variable-length list of JSValue objects with garbage collection support.
+ *
+ * This class provides a thread-safe container for WriteBarrier<T> objects that can
+ * dynamically grow and shrink. It includes helper methods for visiting contained
+ * objects during garbage collection traversal.
+ *
+ * Use this class when:
+ * - The number of items may change at runtime (append/remove operations)
+ * - You need thread-safe access to the list
+ * - You need automatic garbage collection support for contained JSValues
+ *
+ * For better performance when the length is known and fixed, prefer
+ * FixedVector<WriteBarrier<T>> instead.
+ *
+ * @tparam T The type of JSC objects to store (must inherit from JSC::JSCell)
+ */
+template<typename T>
+class WriteBarrierList {
+public:
+    WriteBarrierList()
+    {
+    }
+
+    void append(JSC::VM& vm, JSC::JSCell* owner, T* value)
+    {
+        WTF::Locker locker { owner->cellLock() };
+        m_list.append(JSC::WriteBarrier<T>(vm, owner, value));
+    }
+
+    std::span<JSC::WriteBarrier<T>> list()
+    {
+        return m_list.mutableSpan();
+    }
+
+    void moveTo(JSC::JSCell* owner, JSC::MarkedArgumentBuffer& arguments)
+    {
+        WTF::Locker locker { owner->cellLock() };
+        for (JSC::WriteBarrier<T>& value : m_list) {
+            if (auto* cell = value.get()) {
+                arguments.append(cell);
+                value.clear();
+            }
+        }
+    }
+
+    template<typename Visitor>
+    void visit(JSC::JSCell* owner, Visitor& visitor)
+    {
+        WTF::Locker locker { owner->cellLock() };
+        for (auto& value : m_list) {
+            visitor.append(value);
+        }
+    }
+
+    bool isEmpty() const
+    {
+        return m_list.isEmpty();
+    }
+
+    T* takeFirst(JSC::JSCell* owner)
+    {
+        WTF::Locker locker { owner->cellLock() };
+        if (m_list.isEmpty()) {
+            return nullptr;
+        }
+
+        T* value = m_list.first().get();
+        m_list.removeAt(0);
+        return value;
+    }
+
+    template<typename MatchFunction>
+    bool removeFirstMatching(JSC::JSCell* owner, const MatchFunction& matches)
+    {
+        WTF::Locker locker { owner->cellLock() };
+        return m_list.removeFirstMatching(matches);
+    }
+
+private:
+    WTF::Vector<JSC::WriteBarrier<T>> m_list;
+};
+
+}
--- a/src/bun.js/bindings/ZigGlobalObject.cpp
+++ b/src/bun.js/bindings/ZigGlobalObject.cpp
@@ -1354,10 +1354,10 @@ void GlobalObject::promiseRejectionTracker(JSGlobalObject* obj, JSC::JSPromise*
    auto* globalObj = static_cast<GlobalObject*>(obj);
    switch (operation) {
    case JSPromiseRejectionOperation::Reject:
-        globalObj->m_aboutToBeNotifiedRejectedPromises.append(JSC::Strong<JSPromise>(obj->vm(), promise));
+        globalObj->m_aboutToBeNotifiedRejectedPromises.append(obj->vm(), globalObj, promise);
        break;
    case JSPromiseRejectionOperation::Handle:
-        bool removed = globalObj->m_aboutToBeNotifiedRejectedPromises.removeFirstMatching([&](Strong<JSPromise>& unhandledPromise) {
+        bool removed = globalObj->m_aboutToBeNotifiedRejectedPromises.removeFirstMatching(globalObj, [&](JSC::WriteBarrier<JSC::JSPromise>& unhandledPromise) {
            return unhandledPromise.get() == promise;
        });
        if (removed) break;
@@ -4048,16 +4048,13 @@ void GlobalObject::handleRejectedPromises()
 {
    JSC::VM& virtual_machine = vm();
    auto scope = DECLARE_CATCH_SCOPE(virtual_machine);
-    do {
-        auto unhandledRejections = WTFMove(m_aboutToBeNotifiedRejectedPromises);
-        for (auto& promise : unhandledRejections) {
-            if (promise->isHandled(virtual_machine))
-                continue;
+    while (auto* promise = m_aboutToBeNotifiedRejectedPromises.takeFirst(this)) {
+        if (promise->isHandled(virtual_machine))
+            continue;

-            Bun__handleRejectedPromise(this, promise.get());
-            if (auto ex = scope.exception()) this->reportUncaughtExceptionAtEventLoop(this, ex);
-        }
-    } while (!m_aboutToBeNotifiedRejectedPromises.isEmpty());
+        Bun__handleRejectedPromise(this, promise);
+        if (auto ex = scope.exception()) this->reportUncaughtExceptionAtEventLoop(this, ex);
+    }
 }

 DEFINE_VISIT_CHILDREN(GlobalObject);
@@ -4070,6 +4067,9 @@ void GlobalObject::visitAdditionalChildren(Visitor& visitor)

    thisObject->globalEventScope->visitJSEventListeners(visitor);

+    thisObject->m_aboutToBeNotifiedRejectedPromises.visit(thisObject, visitor);
+    thisObject->m_ffiFunctions.visit(thisObject, visitor);
+
    ScriptExecutionContext* context = thisObject->scriptExecutionContext();
    visitor.addOpaqueRoot(context);
 }
@@ -4625,18 +4625,13 @@ void GlobalObject::setNodeWorkerEnvironmentData(JSMap* data) { m_nodeWorkerEnvir

 void GlobalObject::trackFFIFunction(JSC::JSFunction* function)
 {
-    this->m_ffiFunctions.append(JSC::Strong<JSC::JSFunction> { vm(), function });
+    this->m_ffiFunctions.append(vm(), this, function);
 }
 bool GlobalObject::untrackFFIFunction(JSC::JSFunction* function)
 {
-    for (size_t i = 0; i < this->m_ffiFunctions.size(); ++i) {
-        if (this->m_ffiFunctions[i].get() == function) {
-            this->m_ffiFunctions[i].clear();
-            this->m_ffiFunctions.removeAt(i);
-            return true;
-        }
-    }
-    return false;
+    return this->m_ffiFunctions.removeFirstMatching(this, [&](JSC::WriteBarrier<JSC::JSFunction>& untrackedFunction) -> bool {
+        return untrackedFunction.get() == function;
+    });
 }

 extern "C" void Zig__GlobalObject__destructOnExit(Zig::GlobalObject* globalObject)
--- a/src/bun.js/bindings/ZigGlobalObject.h
+++ b/src/bun.js/bindings/ZigGlobalObject.h
@@ -57,6 +57,7 @@ class GlobalInternals;
 #include "BunGlobalScope.h"
 #include <js_native_api.h>
 #include <node_api.h>
+#include "WriteBarrierList.h"

 namespace Bun {
 class JSCommonJSExtensions;
@@ -735,8 +736,8 @@ private:
    DOMGuardedObjectSet m_guardedObjects WTF_GUARDED_BY_LOCK(m_gcLock);
    WebCore::SubtleCrypto* m_subtleCrypto = nullptr;

-    WTF::Vector<JSC::Strong<JSC::JSPromise>> m_aboutToBeNotifiedRejectedPromises;
-    WTF::Vector<JSC::Strong<JSC::JSFunction>> m_ffiFunctions;
+    Bun::WriteBarrierList<JSC::JSPromise> m_aboutToBeNotifiedRejectedPromises;
+    Bun::WriteBarrierList<JSC::JSFunction> m_ffiFunctions;
 };

 class EvalGlobalObject : public GlobalObject {
--- a/test/bundler/bundler_defer.test.ts
+++ b/test/bundler/bundler_defer.test.ts
@@ -536,57 +536,79 @@ warn: (msg: string) => console.warn(\`[WARN] \${msg}\`)

    const outdir = path.join(folder, "dist");

-    const result = await Bun.build({
-      entrypoints: [entrypoint],
-      outdir,
-      plugins: [
-        {
-          name: "xXx123_import_checker_321xXx",
-          setup(build) {
-            type Import = {
-              imported: string[];
-              dep: string;
-            };
-            type Export = {
-              ident: string;
-            };
-            let imports_and_exports: Record<string, { imports: Array<Import>; exports: Array<Export> }> = {};
-
-            build.onLoad({ filter: /\.ts/ }, async ({ path }) => {
-              const contents = await Bun.$`cat ${path}`.quiet().text();
-
-              const import_regex = /import\s+(?:([\s\S]*?)\s+from\s+)?['"]([^'"]+)['"];/g;
-              const imports: Array<Import> = [...contents.toString().matchAll(import_regex)].map(m => ({
-                imported: m
-                  .slice(1, m.length - 1)
-                  .map(match => (match[0] === "{" ? match.slice(2, match.length - 2) : match)),
-                dep: m[m.length - 1],
-              }));
-
-              const export_regex =
-                /export\s+(?:default\s+|const\s+|let\s+|var\s+|function\s+|class\s+|enum\s+|type\s+|interface\s+)?([\w$]+)?(?:\s*=\s*|(?:\s*{[^}]*})?)?[^;]*;/g;
-              const exports: Array<Export> = [...contents.matchAll(export_regex)].map(m => ({
-                ident: m[1],
-              }));
-
-              imports_and_exports[path.replaceAll("\\", "/").split("/").pop()!] = { imports, exports };
-              return undefined;
-            });
-
-            build.onLoad({ filter: /module_data\.json/ }, async ({ defer }) => {
-              await defer();
-              const contents = JSON.stringify(imports_and_exports);
-
-              return {
-                contents,
-                loader: "json",
-              };
-            });
-          },
-        },
-      ],
+    let onFinalizeCallCount = 0;
+    let onFinalizeCalledThrice = Promise.withResolvers();
+    let onFinalizeCallRegistry = new FinalizationRegistry(() => {
+      onFinalizeCallCount++;
+      if (onFinalizeCallCount === 3) {
+        onFinalizeCalledThrice.resolve();
+      }
    });

+    const result = await (async function () {
+      return await Bun.build({
+        entrypoints: [entrypoint],
+        outdir,
+        plugins: [
+          (() => {
+            const plugin = {
+              name: "xXx123_import_checker_321xXx",
+              setup(build) {
+                type Import = {
+                  imported: string[];
+                  dep: string;
+                };
+                type Export = {
+                  ident: string;
+                };
+                let imports_and_exports: Record<string, { imports: Array<Import>; exports: Array<Export> }> = {};
+
+                const onLoadTS = async ({ path }) => {
+                  const contents = await Bun.$`cat ${path}`.quiet().text();
+
+                  const import_regex = /import\s+(?:([\s\S]*?)\s+from\s+)?['"]([^'"]+)['"];/g;
+                  const imports: Array<Import> = [...contents.toString().matchAll(import_regex)].map(m => ({
+                    imported: m
+                      .slice(1, m.length - 1)
+                      .map(match => (match[0] === "{" ? match.slice(2, match.length - 2) : match)),
+                    dep: m[m.length - 1],
+                  }));
+
+                  const export_regex =
+                    /export\s+(?:default\s+|const\s+|let\s+|var\s+|function\s+|class\s+|enum\s+|type\s+|interface\s+)?([\w$]+)?(?:\s*=\s*|(?:\s*{[^}]*})?)?[^;]*;/g;
+                  const exports: Array<Export> = [...contents.matchAll(export_regex)].map(m => ({
+                    ident: m[1],
+                  }));
+
+                  imports_and_exports[path.replaceAll("\\", "/").split("/").pop()!] = { imports, exports };
+                  return undefined;
+                };
+
+                const onLoadModuleData = async ({ defer }) => {
+                  await defer();
+                  const contents = JSON.stringify(imports_and_exports);
+
+                  return {
+                    contents,
+                    loader: "json",
+                  };
+                };
+
+                build.onLoad({ filter: /\.ts/ }, onLoadTS);
+
+                build.onLoad({ filter: /module_data\.json/ }, onLoadModuleData);
+
+                onFinalizeCallRegistry.register(onLoadTS, undefined);
+                onFinalizeCallRegistry.register(onLoadModuleData, undefined);
+              },
+            };
+            onFinalizeCallRegistry.register(plugin.setup, undefined);
+            return plugin;
+          })(),
+        ],
+      });
+    })();
+
    expect(result.success).toBeTrue();
    await Bun.$`${bunExe()} run ${result.outputs[0].path}`;
    const output = await Bun.$`cat ${path.join(folder, "dist", "output.json")}`.json();
@@ -619,5 +641,8 @@ warn: (msg: string) => console.warn(\`[WARN] \${msg}\`)
        "exports": [{ "ident": "logger" }],
      },
    });
+    Bun.gc(true);
+    await onFinalizeCalledThrice.promise;
+    expect(onFinalizeCallCount).toBe(3);
  });
 });