Fix crash running Node-API finalizers during VM destruction

src/bun.js/bindings/ZigGlobalObject.cpp

@@ -4646,8 +4646,8 @@ GlobalObject::PromiseFunctions GlobalObject::promiseHandlerID(Zig::FFIFunction h
 
 napi_env GlobalObject::makeNapiEnv(const napi_module& mod)
 {
-    m_napiEnvs.append(std::make_unique<napi_env__>(this, mod));
-    return m_napiEnvs.last().get();
+    m_napiEnvs.append(napi_env__::create(this, mod));
+    return &m_napiEnvs.last().get();
 }
 
 napi_env GlobalObject::makeNapiEnvForFFI()

src/bun.js/bindings/ZigGlobalObject.h

@@ -651,7 +651,7 @@ class GlobalObject : public Bun::GlobalScope {
     // De-optimization once `require("module").runMain` is written to
     bool hasOverriddenModuleRunMain = false;
 
-    WTF::Vector<std::unique_ptr<napi_env__>> m_napiEnvs;
+    Vector<Ref<napi_env__>> m_napiEnvs;
     napi_env makeNapiEnv(const napi_module&);
     napi_env makeNapiEnvForFFI();
     bool hasNapiFinalizers() const;

src/bun.js/bindings/napi.cpp

@@ -1237,9 +1237,9 @@ extern "C" napi_status napi_add_finalizer(napi_env env, napi_value js_object,
         *result = toNapi(ref);
     } else {
         // Otherwise, it's cheaper to just call .addFinalizer.
-        vm.heap.addFinalizer(object, [env, finalize_cb, native_object, finalize_hint](JSCell* cell) -> void {
+        vm.heap.addFinalizer(object, [weak_env = ThreadSafeWeakPtr(*env), finalize_cb, native_object, finalize_hint](JSCell* cell) -> void {
             NAPI_LOG("finalizer %p", finalize_hint);
-            env->doFinalizer(finalize_cb, native_object, finalize_hint);
+            napi_env__::doFinalizer(weak_env.get(), finalize_cb, native_object, finalize_hint);
         });
     }
 
@@ -2024,9 +2024,9 @@ extern "C" napi_status napi_create_external_buffer(napi_env env, size_t length,
 
     Zig::GlobalObject* globalObject = toJS(env);
 
-    auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast<const uint8_t*>(data), length }, createSharedTask<void(void*)>([env, finalize_hint, finalize_cb](void* p) {
+    auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast<const uint8_t*>(data), length }, createSharedTask<void(void*)>([weak_env = ThreadSafeWeakPtr(*env), finalize_hint, finalize_cb](void* p) {
         NAPI_LOG("external buffer finalizer");
-        env->doFinalizer(finalize_cb, p, finalize_hint);
+        napi_env__::doFinalizer(weak_env.get(), finalize_cb, p, finalize_hint);
     }));
     auto* subclassStructure = globalObject->JSBufferSubclassStructure();
 
@@ -2045,9 +2045,9 @@ extern "C" napi_status napi_create_external_arraybuffer(napi_env env, void* exte
     Zig::GlobalObject* globalObject = toJS(env);
     JSC::VM& vm = JSC::getVM(globalObject);
 
-    auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast<const uint8_t*>(external_data), byte_length }, createSharedTask<void(void*)>([env, finalize_hint, finalize_cb](void* p) {
+    auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast<const uint8_t*>(external_data), byte_length }, createSharedTask<void(void*)>([weak_env = ThreadSafeWeakPtr(*env), finalize_hint, finalize_cb](void* p) {
         NAPI_LOG("external ArrayBuffer finalizer");
-        env->doFinalizer(finalize_cb, p, finalize_hint);
+        napi_env__::doFinalizer(weak_env.get(), finalize_cb, p, finalize_hint);
     }));
 
     auto* buffer = JSC::JSArrayBuffer::create(vm, globalObject->arrayBufferStructure(ArrayBufferSharingMode::Default), WTFMove(arrayBuffer));

src/bun.js/bindings/napi.h

@@ -61,13 +61,11 @@ struct napi_async_cleanup_hook_handle__ {
     } while (0)
 
 // Named this way so we can manipulate napi_env values directly (since napi_env is defined as a pointer to struct napi_env__)
-struct napi_env__ {
+struct napi_env__ : public WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<napi_env__> {
 public:
-    napi_env__(Zig::GlobalObject* globalObject, const napi_module& napiModule)
-        : m_globalObject(globalObject)
-        , m_napiModule(napiModule)
+    static Ref<napi_env__> create(Zig::GlobalObject* globalObject, const napi_module& napiModule)
     {
-        napi_internal_register_cleanup_zig(this);
+        return adoptRef(*new napi_env__(globalObject, napiModule));
     }
 
     ~napi_env__()
@@ -192,20 +190,28 @@ struct napi_env__ {
         return JSC::getVM(m_globalObject).hasTerminationRequest();
     }
 
-    void doFinalizer(napi_finalize finalize_cb, void* data, void* finalize_hint)
+    static void doFinalizer(RefPtr<napi_env__>&& env, napi_finalize finalize_cb, void* data, void* finalize_hint)
     {
         if (!finalize_cb) {
             return;
         }
 
-        if (mustDeferFinalizers() && inGC()) {
-            napi_internal_enqueue_finalizer(this, finalize_cb, data, finalize_hint);
+        if (env) {
+            if (env->mustDeferFinalizers() && env->inGC()) {
+                napi_internal_enqueue_finalizer(env.get(), finalize_cb, data, finalize_hint);
+            } else {
+                finalize_cb(env.get(), data, finalize_hint);
+            }
         } else {
-            finalize_cb(this, data, finalize_hint);
+            NAPI_LOG("skipping finalizer as env is already freed");
         }
     }
 
-    inline Zig::GlobalObject* globalObject() const { return m_globalObject; }
+    inline Zig::GlobalObject*
+    globalObject() const
+    {
+        return m_globalObject;
+    }
     inline const napi_module& napiModule() const { return m_napiModule; }
 
     // Returns true if finalizers from this module need to be scheduled for the next tick after garbage collection, instead of running during garbage collection
@@ -291,6 +297,13 @@ struct napi_env__ {
     };
 
 private:
+    napi_env__(Zig::GlobalObject* globalObject, const napi_module& napiModule)
+        : m_globalObject(globalObject)
+        , m_napiModule(napiModule)
+    {
+        napi_internal_register_cleanup_zig(this);
+    }
+
     Zig::GlobalObject* m_globalObject = nullptr;
     napi_module m_napiModule;
     // TODO(@heimskr): Use WTF::HashSet

test/napi/napi-app/js_test_helpers.cpp

@@ -320,6 +320,23 @@ static napi_value create_weird_bigints(const Napi::CallbackInfo &info) {
   return array;
 }
 
+static napi_value add_finalizer_to_object(const Napi::CallbackInfo &info) {
+  napi_env env = info.Env();
+  napi_value js_object = info[0];
+  int *native_object = new int{123};
+  NODE_API_CALL(env,
+                napi_add_finalizer(
+                    env, js_object, static_cast<void *>(native_object),
+                    [](napi_env, void *data, void *) {
+                      auto casted = static_cast<int *>(data);
+                      printf("add_finalizer_to_object finalizer data = %d\n",
+                             *casted);
+                      delete casted;
+                    },
+                    nullptr, nullptr));
+  return ok(env);
+}
+
 void register_js_test_helpers(Napi::Env env, Napi::Object exports) {
   REGISTER_FUNCTION(env, exports, create_ref_with_finalizer);
   REGISTER_FUNCTION(env, exports, was_finalize_called);
@@ -333,6 +350,7 @@ void register_js_test_helpers(Napi::Env env, Napi::Object exports) {
   REGISTER_FUNCTION(env, exports, try_add_tag);
   REGISTER_FUNCTION(env, exports, check_tag);
   REGISTER_FUNCTION(env, exports, create_weird_bigints);
+  REGISTER_FUNCTION(env, exports, add_finalizer_to_object);
 }
 
 } // namespace napitests

test/napi/napi-app/module.js

@@ -2,6 +2,7 @@ const assert = require("node:assert");
 const nativeTests = require("./build/Debug/napitests.node");
 const secondAddon = require("./build/Debug/second_addon.node");
 const asyncFinalizeAddon = require("./build/Debug/async_finalize_addon.node");
+const { Worker } = require("node:worker_threads");
 
 async function gcUntil(fn) {
   const MAX = 100;
@@ -618,4 +619,12 @@ nativeTests.test_get_value_string = () => {
   }
 };
 
+// Should be run with
+// BUN_DESTRUCT_VM_ON_EXIT=1 -- makes us tear down the JSC::VM while exiting, so that finalizers run
+// BUN_JSC_useGC=0 -- ensures the object's finalizer will be called at exit not during normal GC
+nativeTests.test_finalizer_called_during_destruction = () => {
+  let object = {};
+  nativeTests.add_finalizer_to_object(object);
+};
+
 module.exports = nativeTests;

test/napi/napi.test.ts

@@ -1,6 +1,6 @@
 import { spawnSync } from "bun";
 import { beforeAll, describe, expect, it } from "bun:test";
-import { bunEnv, bunExe, tempDirWithFiles } from "harness";
+import { bunEnv, bunExe, isBroken, tempDirWithFiles } from "harness";
 import { join } from "path";
 
 describe("napi", () => {
@@ -453,18 +453,27 @@ describe("napi", () => {
   it("works when the module register function throws", () => {
     expect(() => require("./napi-app/build/Debug/throw_addon.node")).toThrow(new Error("oops!"));
   });
+
+  describe("napi_add_finalizer", () => {
+    it.todoIf(isBroken)("does not crash if the finalizer is called during VM shutdown", () => {
+      checkSameOutput("test_finalizer_called_during_destruction", [], {
+        BUN_DESTRUCT_VM_ON_EXIT: "1",
+        BUN_JSC_useGC: "0",
+      });
+    });
+  });
 });
 
-function checkSameOutput(test: string, args: any[] | string) {
-  const nodeResult = runOn("node", test, args).trim();
-  let bunResult = runOn(bunExe(), test, args);
+function checkSameOutput(test: string, args: any[] | string, env: object = {}) {
+  const nodeResult = runOn("node", test, args, env).trim();
+  let bunResult = runOn(bunExe(), test, args, env);
   // remove all debug logs
   bunResult = bunResult.replaceAll(/^\[\w+\].+$/gm, "").trim();
   expect(bunResult).toEqual(nodeResult);
   return nodeResult;
 }
 
-function runOn(executable: string, test: string, args: any[] | string) {
+function runOn(executable: string, test: string, args: any[] | string, env: object) {
   // when the inspector runs (can be due to VSCode extension), there is
   // a bug that in debug modes the console logs extra stuff
   const { BUN_INSPECT_CONNECT_TO: _, ...rest } = bunEnv;
@@ -476,7 +485,8 @@ function runOn(executable: string, test: string, args: any[] | string) {
       test,
       typeof args == "string" ? args : JSON.stringify(args),
     ],
-    env: rest,
+    env: { ...rest, ...env },
+    cwd: join(__dirname, "napi-app"),
   });
   const errs = exec.stderr.toString();
   if (errs !== "") {