Merge branch 'main' into kai/napi-fixes-2

src/bun.js/bindings/NapiRef.cpp

@@ -41,6 +41,70 @@ void NapiRef::clear()
     globalObject.clear();
     weakValueRef.clear();
     strongRef.clear();
+    if (boundCleanup) {
+        env->removeFinalizer(*boundCleanup);
+    }
+}
+
+NapiRef::NapiRef(napi_env env, uint32_t count, Bun::NapiFinalizer finalizer, bool deleteSelf)
+    : env(env)
+    , globalObject(JSC::Weak<JSC::JSGlobalObject>(env->globalObject()))
+    , finalizer(WTFMove(finalizer))
+    , refCount(count)
+    , m_deleteSelf(deleteSelf)
+{
+}
+
+void NapiRef::setValueInitial(JSC::JSValue value, bool can_be_weak)
+{
+    if (refCount > 0) {
+        strongRef.set(globalObject->vm(), value);
+    }
+
+    // In NAPI non-experimental, types other than object, function and symbol can't be used as values for references.
+    // In NAPI experimental, they can be, but we must not store weak references to them.
+    if (can_be_weak) {
+        weakValueRef.set(value, Napi::NapiRefWeakHandleOwner::weakValueHandleOwner(), this);
+    }
+
+    if (value.isSymbol()) {
+        auto* symbol = jsDynamicCast<JSC::Symbol*>(value);
+        ASSERT(symbol != nullptr);
+        if (symbol->uid().isRegistered()) {
+            // Global symbols must always be retrievable,
+            // even if garbage collection happens while the ref count is 0.
+            m_isEternal = true;
+            if (refCount == 0) {
+                strongRef.set(globalObject->vm(), symbol);
+            }
+        }
+    }
+}
+
+void NapiRef::callFinalizer()
+{
+    // Calling the finalizer may delete `this`, so we have to do state changes on `this` before
+    // calling the finalizer
+    Bun::NapiFinalizer saved_finalizer = this->finalizer;
+    this->finalizer.clear();
+    saved_finalizer.call(env, nativeObject, !env->mustDeferFinalizers() || !env->inGC());
+
+    (void)m_deleteSelf;
+}
+
+NapiRef::~NapiRef()
+{
+    NAPI_LOG("destruct napi ref %p", this);
+    if (boundCleanup) {
+        boundCleanup->deactivate(env);
+        boundCleanup = nullptr;
+    }
+
+    if (!m_isEternal) {
+        strongRef.clear();
+    }
+
+    weakValueRef.clear();
 }
 
 }

src/bun.js/bindings/napi.h

@@ -505,13 +505,7 @@ public:
     void unref();
     void clear();
 
-    NapiRef(napi_env env, uint32_t count, Bun::NapiFinalizer finalizer)
-        : env(env)
-        , globalObject(JSC::Weak<JSC::JSGlobalObject>(env->globalObject()))
-        , finalizer(WTFMove(finalizer))
-        , refCount(count)
-    {
-    }
+    NapiRef(napi_env env, uint32_t count, Bun::NapiFinalizer finalizer, bool deleteSelf = false);
 
     JSC::JSValue value() const
     {
@@ -522,57 +516,11 @@ public:
         return strongRef.get();
     }
 
-    void setValueInitial(JSC::JSValue value, bool can_be_weak)
-    {
-        if (refCount > 0) {
-            strongRef.set(globalObject->vm(), value);
-        }
+    void setValueInitial(JSC::JSValue value, bool can_be_weak);
 
-        // In NAPI non-experimental, types other than object, function and symbol can't be used as values for references.
-        // In NAPI experimental, they can be, but we must not store weak references to them.
-        if (can_be_weak) {
-            weakValueRef.set(value, Napi::NapiRefWeakHandleOwner::weakValueHandleOwner(), this);
-        }
+    void callFinalizer();
 
-        if (value.isSymbol()) {
-            auto* symbol = jsDynamicCast<JSC::Symbol*>(value);
-            ASSERT(symbol != nullptr);
-            if (symbol->uid().isRegistered()) {
-                // Global symbols must always be retrievable,
-                // even if garbage collection happens while the ref count is 0.
-                m_isEternal = true;
-                if (refCount == 0) {
-                    strongRef.set(globalObject->vm(), symbol);
-                }
-            }
-        }
-    }
-
-    void callFinalizer()
-    {
-        // Calling the finalizer may delete `this`, so we have to do state changes on `this` before
-        // calling the finalizer
-        Bun::NapiFinalizer saved_finalizer = this->finalizer;
-        this->finalizer.clear();
-        saved_finalizer.call(env, nativeObject, !env->mustDeferFinalizers() || !env->inGC());
-    }
-
-    ~NapiRef()
-    {
-        NAPI_LOG("destruct napi ref %p", this);
-        if (boundCleanup) {
-            boundCleanup->deactivate(env);
-            boundCleanup = nullptr;
-        }
-
-        if (!m_isEternal) {
-            strongRef.clear();
-        }
-
-        // The weak ref can lead to calling the destructor
-        // so we must first clear the weak ref before we call the finalizer
-        weakValueRef.clear();
-    }
+    ~NapiRef();
 
     napi_env env = nullptr;
     JSC::Weak<JSC::JSGlobalObject> globalObject;
@@ -586,6 +534,7 @@ public:
 
 private:
     bool m_isEternal = false;
+    bool m_deleteSelf = false;
 };
 
 static inline napi_ref toNapi(NapiRef* val)

test/napi/napi.test.ts

@@ -530,6 +530,12 @@ describe("napi", () => {
   it("behaves as expected when performing operations with an exception pending", async () => {
     await checkSameOutput("test_deferred_exceptions", []);
   });
+
+  describe("napi_ref deletion", () => {
+    it("works", async () => {
+      await checkSameOutput("test_ref_deletion", []);
+    });
+  });
 });
 
 async function checkSameOutput(test: string, args: any[] | string, envArgs: Record<string, string> = {}) {