napi: run napi_add_finalizer and napi_create_external finalizers at env teardown

src/jsc/bindings/napi.cpp

@@ -1166,26 +1166,23 @@ extern "C" napi_status napi_add_finalizer(napi_env env, napi_value js_object,
     NAPI_CHECK_ENV_NOT_IN_GC(env);
     NAPI_CHECK_ARG(env, js_object);
     NAPI_CHECK_ARG(env, finalize_cb);
-    Zig::GlobalObject* globalObject = toJS(env);
-    JSC::VM& vm = JSC::getVM(globalObject);
 
     JSC::JSValue objectValue = toJS(js_object);
     JSC::JSObject* object = objectValue.getObject();
     NAPI_RETURN_EARLY_IF_FALSE(env, object, napi_object_expected);
 
+    // Mirror napi_wrap: create a NapiRef and register it with the env so the
+    // finalizer also runs at env teardown for objects still alive at exit.
+    auto* ref = new NapiRef(*env, 0, Bun::NapiFinalizer { finalize_cb, finalize_hint });
+    const auto& bound_cleanup = env->addFinalizer(wrap_cleanup, native_object, ref);
+    ref->boundCleanup = &bound_cleanup;
+    ref->nativeObject = native_object;
+
     if (result) {
-        // If they're expecting a Ref, use the ref.
-        auto* ref = new NapiRef(*env, 0, Bun::NapiFinalizer { finalize_cb, finalize_hint });
-        // TODO(@heimskr): consider detecting whether the value can't be weak, as we do in napi_create_reference.
-        ref->setValueInitial(object, true);
-        ref->nativeObject = native_object;
+        ref->weakValueRef.set(objectValue, Napi::NapiRefWeakHandleOwner::weakValueHandleOwner(), ref);
         *result = toNapi(ref);
     } else {
-        // Otherwise, it's cheaper to just call .addFinalizer.
-        vm.heap.addFinalizer(object, [env = WTF::Ref<NapiEnv>(*env), finalize_cb, native_object, finalize_hint](JSCell* cell) -> void {
-            NAPI_LOG("finalizer %p", finalize_hint);
-            env->doFinalizer(finalize_cb, native_object, finalize_hint);
-        });
+        ref->weakValueRef.set(objectValue, Napi::NapiRefSelfDeletingWeakHandleOwner::weakValueHandleOwner(), ref);
     }
 
     NAPI_RETURN_SUCCESS(env);
@@ -2452,6 +2449,18 @@ extern "C" napi_status napi_create_object(napi_env env, napi_value* result)
     NAPI_RETURN_SUCCESS(env);
 }
 
+static void external_cleanup(napi_env env, void* data, void* hint)
+{
+    auto* external = reinterpret_cast<Bun::NapiExternal*>(data);
+    ASSERT(external->m_boundCleanup != nullptr);
+    external->m_boundCleanup->deactivate(*env);
+    external->m_boundCleanup = nullptr;
+    // The user callback may do anything, so detach all state from the external first.
+    Bun::NapiFinalizer finalizer = external->m_finalizer;
+    external->m_finalizer.clear();
+    finalizer.call(env, external->m_value, true);
+}
+
 extern "C" napi_status napi_create_external(napi_env env, void* data,
     napi_finalize finalize_cb,
     void* finalize_hint,
@@ -2464,9 +2473,16 @@ extern "C" napi_status napi_create_external(napi_env env, void* data,
     JSC::VM& vm = JSC::getVM(globalObject);
 
     auto* structure = globalObject->NapiExternalStructure();
-    JSValue value = Bun::NapiExternal::create(vm, structure, data, finalize_hint, finalize_cb, env);
-    JSC::EnsureStillAliveScope ensureStillAlive(value);
-    *result = toNapi(value, globalObject);
+    Bun::NapiExternal* external = Bun::NapiExternal::create(vm, structure, data, finalize_hint, finalize_cb, env);
+    JSC::EnsureStillAliveScope ensureStillAlive(external);
+    if (finalize_cb) {
+        // Register with the env so the finalizer also runs at env teardown for
+        // externals still alive at exit. The external's destructor deactivates
+        // this entry if GC runs first.
+        const auto& bound_cleanup = env->addFinalizer(external_cleanup, nullptr, external);
+        external->m_boundCleanup = &bound_cleanup;
+    }
+    *result = toNapi(JSValue(external), globalObject);
     NAPI_RETURN_SUCCESS(env);
 }
 

src/jsc/bindings/napi.h

@@ -222,18 +222,35 @@ struct NapiEnv : public WTF::RefCounted<NapiEnv> {
         // Reverse insertion order so children are torn down before parents (Node.js LIFO).
         // ListHashSet iteration is safe against concurrent inserts, and m_isFinishingFinalizers
         // routes all removals to active=false, so the only unsafe op (erase-current) can't occur.
-        for (auto it = m_finalizers.rbegin(); it != m_finalizers.rend(); ++it) {
-            Bun::NapiHandleScope handle_scope(m_globalObject);
-            it->call(this);
-            // Each finalizer starts from a clean exception state: Node.js
-            // never propagates one finalizer's throw into the next (there
-            // is no JS frame to catch in between). Leaving a pending
-            // exception also breaks later finalizers in subtle ways --
-            // napi_is_exception_pending skips the VM check during cleanup
-            // for safety, so user code thinks there is no exception, but
-            // the next napi call with a throw scope sees it. See #30286.
-            clearExceptionsBetweenFinalizers();
-        }
+        //
+        // A finalizer may register new finalizers, appended past where this pass already is.
+        // Repeat until a pass runs nothing new: clear() after one pass would free entries a
+        // still-live NapiRef or NapiExternal points at through boundCleanup.
+        bool ranAny;
+        do {
+            ranAny = false;
+            for (auto it = m_finalizers.rbegin(); it != m_finalizers.rend(); ++it) {
+                if (!it->active) {
+                    continue;
+                }
+                // Deactivate before calling so an entry can never run on a later pass;
+                // the callback's own deactivate() is then an idempotent no-op.
+                it->active = false;
+                ranAny = true;
+                Bun::NapiHandleScope handle_scope(m_globalObject);
+                if (it->callback) {
+                    it->callback(this, it->data, it->hint);
+                }
+                // Each finalizer starts from a clean exception state: Node.js
+                // never propagates one finalizer's throw into the next (there
+                // is no JS frame to catch in between). Leaving a pending
+                // exception also breaks later finalizers in subtle ways --
+                // napi_is_exception_pending skips the VM check during cleanup
+                // for safety, so user code thinks there is no exception, but
+                // the next napi call with a throw scope sees it. See #30286.
+                clearExceptionsBetweenFinalizers();
+            }
+        } while (ranAny);
         m_finalizers.clear();
         m_isFinishingFinalizers = false;
 
@@ -256,7 +273,12 @@ struct NapiEnv : public WTF::RefCounted<NapiEnv> {
 
     const auto& addFinalizer(napi_finalize callback, void* hint, void* data)
     {
-        return *m_finalizers.add({ callback, hint, data }).iterator;
+        // add() dedups on (callback, hint, data), so during cleanup() a new registration
+        // whose freed-and-reused address matches a tombstoned entry gets that entry back.
+        // Reactivate it so the drain runs it before clear() frees it under its new owner.
+        const auto& bound = *m_finalizers.add({ callback, hint, data }).iterator;
+        bound.active = true;
+        return bound;
     }
 
     bool hasFinalizers() const
@@ -479,13 +501,6 @@ struct NapiEnv : public WTF::RefCounted<NapiEnv> {
         {
         }
 
-        void call(napi_env env) const
-        {
-            if (callback && active) {
-                callback(env, data, hint);
-            }
-        }
-
         void deactivate(NapiEnv& env) const
         {
             if (env.isFinishingFinalizers()) {

src/jsc/bindings/napi_external.cpp

@@ -6,6 +6,10 @@ namespace Bun {
 NapiExternal::~NapiExternal()
 {
     auto* env = m_env.get();
+    if (m_boundCleanup) {
+        m_boundCleanup->deactivate(*env);
+        m_boundCleanup = nullptr;
+    }
     m_finalizer.call(env, m_value, env && !env->mustDeferFinalizers());
 }
 

src/jsc/bindings/napi_external.h

@@ -96,6 +96,7 @@ class NapiExternal : public JSC::JSDestructibleObject {
     void* m_value;
     NapiFinalizer m_finalizer;
     WTF::RefPtr<NapiEnv> m_env;
+    const NapiEnv::BoundFinalizer* m_boundCleanup = nullptr;
 
 #if ASSERT_ENABLED
     String sourceOriginURL = String();

test/napi/napi-app/binding.gyp

@@ -275,5 +275,16 @@
                 "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1",
             ],
         },
+        {
+            "target_name": "test_teardown_finalizers",
+            "sources": ["test_teardown_finalizers.c"],
+            "include_dirs": ["<!@(node -p \"require('node-addon-api').include\")"],
+            "libraries": [],
+            "dependencies": ["<!(node -p \"require('node-addon-api').gyp\")"],
+            "defines": [
+                "NAPI_DISABLE_CPP_EXCEPTIONS",
+                "NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT=1",
+            ],
+        },
     ]
 }

test/napi/napi-app/test_teardown_finalizers.c

@@ -0,0 +1,172 @@
+// Every finalizer registration API must run its finalizer at env teardown,
+// exactly once, including finalizers registered by another teardown finalizer.
+// Each prints "finalize: <name>" to stdout (a child's stderr is unreliable on CI).
+
+#include <node_api.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+static int finalize_count = 0;
+
+static void finalize(napi_env env, void* data, void* hint) {
+    (void)env;
+    (void)data;
+    printf("finalize: %s\n", (const char*)hint);
+    fflush(stdout);
+    free(hint);
+    finalize_count++;
+}
+
+// Copies the string in `arg` into a malloc'd buffer that finalize() frees.
+static char* dup_name_arg(napi_env env, napi_value arg) {
+    size_t len = 0;
+    napi_get_value_string_utf8(env, arg, NULL, 0, &len);
+    char* name = (char*)malloc(len + 1);
+    napi_get_value_string_utf8(env, arg, name, len + 1, &len);
+    return name;
+}
+
+// wrap(obj, name)
+static napi_value wrap(napi_env env, napi_callback_info info) {
+    size_t argc = 2;
+    napi_value argv[2];
+    napi_get_cb_info(env, info, &argc, argv, NULL, NULL);
+    napi_wrap(env, argv[0], NULL, finalize, dup_name_arg(env, argv[1]), NULL);
+    return argv[0];
+}
+
+// addFinalizer(obj, name, wantRef): wantRef=true exercises the napi_ref-returning overload.
+static napi_value add_finalizer(napi_env env, napi_callback_info info) {
+    size_t argc = 3;
+    napi_value argv[3];
+    napi_get_cb_info(env, info, &argc, argv, NULL, NULL);
+    bool want_ref = false;
+    napi_get_value_bool(env, argv[2], &want_ref);
+    // Leaked on purpose: this test never releases the ref, matching addons
+    // that hold a weak ref for the object's whole lifetime.
+    napi_ref* ref = want_ref ? (napi_ref*)malloc(sizeof(napi_ref)) : NULL;
+    napi_add_finalizer(env, argv[0], NULL, finalize, dup_name_arg(env, argv[1]), ref);
+    return argv[0];
+}
+
+// createExternal(name) -> external value
+static napi_value create_external(napi_env env, napi_callback_info info) {
+    size_t argc = 1;
+    napi_value argv[1];
+    napi_get_cb_info(env, info, &argc, argv, NULL, NULL);
+    napi_value external;
+    napi_create_external(env, NULL, finalize, dup_name_arg(env, argv[0]), &external);
+    return external;
+}
+
+// finalizeCount() -> number of finalizers that have already run (and flushed).
+static napi_value get_finalize_count(napi_env env, napi_callback_info info) {
+    (void)info;
+    napi_value out;
+    napi_create_int32(env, finalize_count, &out);
+    return out;
+}
+
+typedef struct {
+    char* outer_name;
+    char* nested_external_name;
+    char* nested_add_finalizer_name;
+} NestingContext;
+
+// Runs as a teardown finalizer and registers two new finalizers while the env
+// is already draining its finalizer list. Bun accepts these calls here (Node
+// rejects them with napi_pending_exception), so it must drain them safely.
+static void nesting_finalize(napi_env env, void* data, void* hint) {
+    (void)hint;
+    NestingContext* ctx = (NestingContext*)data;
+    printf("finalize: %s\n", ctx->outer_name);
+    fflush(stdout);
+    finalize_count++;
+    napi_value external;
+    napi_create_external(env, NULL, finalize, ctx->nested_external_name, &external);
+    napi_add_finalizer(env, external, NULL, finalize, ctx->nested_add_finalizer_name, NULL);
+    free(ctx->outer_name);
+    free(ctx);
+}
+
+// wrapNesting(obj, outerName, nestedExternalName, nestedAddFinalizerName)
+static napi_value wrap_nesting(napi_env env, napi_callback_info info) {
+    size_t argc = 4;
+    napi_value argv[4];
+    napi_get_cb_info(env, info, &argc, argv, NULL, NULL);
+    NestingContext* ctx = (NestingContext*)malloc(sizeof(NestingContext));
+    ctx->outer_name = dup_name_arg(env, argv[1]);
+    ctx->nested_external_name = dup_name_arg(env, argv[2]);
+    ctx->nested_add_finalizer_name = dup_name_arg(env, argv[3]);
+    napi_wrap(env, argv[0], ctx, nesting_finalize, NULL, NULL);
+    return argv[0];
+}
+
+static napi_ref saved_ref = NULL;
+
+// addFinalizerSaveRef(obj, name): napi_add_finalizer keeping the returned ref
+// so a later teardown finalizer can napi_delete_reference it.
+static napi_value add_finalizer_save_ref(napi_env env, napi_callback_info info) {
+    size_t argc = 2;
+    napi_value argv[2];
+    napi_get_cb_info(env, info, &argc, argv, NULL, NULL);
+    napi_add_finalizer(env, argv[0], NULL, finalize, dup_name_arg(env, argv[1]), &saved_ref);
+    return argv[0];
+}
+
+typedef struct {
+    char* outer_name;
+    char* recycled_name;
+} RecycleContext;
+
+// Runs as a teardown finalizer: deletes the ref from addFinalizerSaveRef, then
+// registers a new finalizer. The allocator commonly hands back the just-freed
+// NapiRef address; the new finalizer must still run.
+static void recycle_finalize(napi_env env, void* data, void* hint) {
+    (void)hint;
+    RecycleContext* ctx = (RecycleContext*)data;
+    printf("finalize: %s\n", ctx->outer_name);
+    fflush(stdout);
+    finalize_count++;
+    void* old_ref = (void*)saved_ref;
+    napi_delete_reference(env, saved_ref);
+    napi_value obj;
+    napi_create_object(env, &obj);
+    napi_ref new_ref = NULL;
+    napi_add_finalizer(env, obj, NULL, finalize, ctx->recycled_name, &new_ref);
+    // Diagnostic only (not asserted): says whether the address-reuse collision
+    // this exercise targets actually happened on this platform and allocator.
+    printf("recycled-address: %s\n", (void*)new_ref == old_ref ? "yes" : "no");
+    fflush(stdout);
+    free(ctx->outer_name);
+    free(ctx);
+}
+
+// wrapRecycling(obj, outerName, recycledName)
+static napi_value wrap_recycling(napi_env env, napi_callback_info info) {
+    size_t argc = 3;
+    napi_value argv[3];
+    napi_get_cb_info(env, info, &argc, argv, NULL, NULL);
+    RecycleContext* ctx = (RecycleContext*)malloc(sizeof(RecycleContext));
+    ctx->outer_name = dup_name_arg(env, argv[1]);
+    ctx->recycled_name = dup_name_arg(env, argv[2]);
+    napi_wrap(env, argv[0], ctx, recycle_finalize, NULL, NULL);
+    return argv[0];
+}
+
+static napi_value init(napi_env env, napi_value exports) {
+    napi_property_descriptor properties[] = {
+        { "wrap", 0, wrap, 0, 0, 0, napi_default, 0 },
+        { "addFinalizer", 0, add_finalizer, 0, 0, 0, napi_default, 0 },
+        { "createExternal", 0, create_external, 0, 0, 0, napi_default, 0 },
+        { "finalizeCount", 0, get_finalize_count, 0, 0, 0, napi_default, 0 },
+        { "wrapNesting", 0, wrap_nesting, 0, 0, 0, napi_default, 0 },
+        { "addFinalizerSaveRef", 0, add_finalizer_save_ref, 0, 0, 0, napi_default, 0 },
+        { "wrapRecycling", 0, wrap_recycling, 0, 0, 0, napi_default, 0 },
+    };
+    napi_define_properties(env, exports, sizeof(properties) / sizeof(properties[0]), properties);
+    return exports;
+}
+
+NAPI_MODULE(NODE_GYP_MODULE_NAME, init)