fix(napi): Make napi_wrap work on regular objects

src/bun.js/bindings/napi.cpp

@@ -1025,6 +1025,15 @@ extern "C" void napi_module_register(napi_module* mod)
     globalObject->m_pendingNapiModuleAndExports[1].set(vm, globalObject, object);
 }
 
+static inline bool hasNapiWrap(VM& vm, JSGlobalObject* globalObject, JSObject* object)
+{
+    if (auto* napi_instance = jsDynamicCast<NapiPrototype*>(object)) {
+        return napi_instance->napiRef != nullptr;
+    } else {
+        return !object->getDirect(vm, WebCore::builtinNames(vm).napiWrappedContentsPrivateName()).isEmpty();
+    }
+}
+
 extern "C" napi_status napi_wrap(napi_env env,
     napi_value js_object,
     void* native_object,
@@ -1039,50 +1048,51 @@ extern "C" napi_status napi_wrap(napi_env env,
 {
     NAPI_PREMABLE
 
-    JSValue value = toJS(js_object);
-    if (!value || value.isUndefinedOrNull()) {
-        return napi_object_expected;
-    }
-
     auto* globalObject = toJS(env);
-
-    NapiRef** refPtr = nullptr;
-    if (auto* val = jsDynamicCast<NapiPrototype*>(value)) {
-        refPtr = &val->napiRef;
-    } else if (auto* val = jsDynamicCast<NapiClass*>(value)) {
-        refPtr = &val->napiRef;
+    auto& vm = globalObject->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+    JSValue jsc_value = toJS(js_object);
+    if (jsc_value.isEmpty()) {
+        return napi_invalid_arg;
     }
-
-    if (!refPtr) {
+    JSObject* jsc_object = jsc_value.getObject();
+    if (!jsc_object) {
+        scope.assertNoException();
         return napi_object_expected;
     }
 
-    if (*refPtr) {
-        // Calling napi_wrap() a second time on an object will return an error.
-        // To associate another native instance with the object, use
-        // napi_remove_wrap() first.
+    // NapiPrototype has an inline field to store a napi_ref, so we use that if we can
+    auto* napi_instance = jsDynamicCast<NapiPrototype*>(jsc_object);
+
+    const JSC::Identifier& propertyName = WebCore::builtinNames(vm).napiWrappedContentsPrivateName();
+
+    if (hasNapiWrap(vm, globalObject, jsc_object)) {
+        // already wrapped
+        scope.assertNoException();
         return napi_invalid_arg;
     }
 
+    // create a new weak reference (refcount 0)
     auto* ref = new NapiRef(globalObject, 0);
+    ref->weakValueRef.set(jsc_value, weakValueHandleOwner(), ref);
 
-    ref->weakValueRef.set(value, weakValueHandleOwner(), ref);
-
-    if (finalize_cb) {
-        ref->finalizer.finalize_cb = finalize_cb;
-        ref->finalizer.finalize_hint = finalize_hint;
-    }
+    ref->finalizer.finalize_cb = finalize_cb;
+    ref->finalizer.finalize_hint = finalize_hint;
+    ref->data = native_object;
 
-    if (native_object) {
-        ref->data = native_object;
+    if (napi_instance) {
+        napi_instance->napiRef = ref;
+    } else {
+        // wrap the ref in an external so that it can serve as a JSValue
+        auto* external = Bun::NapiExternal::create(globalObject->vm(), globalObject->NapiExternalStructure(), ref, nullptr, nullptr);
+        jsc_object->putDirect(vm, propertyName, JSValue(external));
+        RETURN_IF_EXCEPTION(scope, napi_pending_exception);
     }
 
-    *refPtr = ref;
-
     if (result) {
         *result = toNapi(ref);
     }
-
+    scope.assertNoException();
     return napi_ok;
 }
 
@@ -1091,35 +1101,48 @@ extern "C" napi_status napi_remove_wrap(napi_env env, napi_value js_object,
 {
     NAPI_PREMABLE
 
-    JSValue value = toJS(js_object);
-    if (!value || value.isUndefinedOrNull()) {
+    JSValue jsc_value = toJS(js_object);
+    if (jsc_value.isEmpty()) {
+        return napi_invalid_arg;
+    }
+    JSObject* jsc_object = jsc_value.getObject();
+    if (!js_object) {
         return napi_object_expected;
     }
+    // may be null
+    auto* napi_instance = jsDynamicCast<NapiPrototype*>(jsc_object);
 
-    NapiRef** refPtr = nullptr;
-    if (auto* val = jsDynamicCast<NapiPrototype*>(value)) {
-        refPtr = &val->napiRef;
-    } else if (auto* val = jsDynamicCast<NapiClass*>(value)) {
-        refPtr = &val->napiRef;
-    }
+    auto* globalObject = toJS(env);
+    auto& vm = globalObject->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+    NapiRef* ref = nullptr;
 
-    if (!refPtr) {
-        return napi_object_expected;
+    if (!hasNapiWrap(vm, globalObject, jsc_object)) {
+        return napi_invalid_arg;
     }
 
-    if (!(*refPtr)) {
-        // not sure if this should succeed or return an error
-        return napi_ok;
+    if (napi_instance) {
+        ref = napi_instance->napiRef;
+        napi_instance->napiRef = nullptr;
+    } else {
+        const JSC::Identifier& propertyName = WebCore::builtinNames(vm).napiWrappedContentsPrivateName();
+        JSValue contents_value = jsc_object->getDirect(vm, propertyName);
+        RETURN_IF_EXCEPTION(scope, napi_pending_exception);
+        // this cast asserts: we should not have stored anything but a NapiExternal here
+        auto* contents_external = jsCast<Bun::NapiExternal*>(contents_value);
+        ref = static_cast<NapiRef*>(contents_external->value());
+        jsc_object->deleteProperty(globalObject, propertyName);
+        RETURN_IF_EXCEPTION(scope, napi_pending_exception);
     }
 
-    auto* ref = *refPtr;
-    *refPtr = nullptr;
-
     if (result) {
         *result = ref->data;
     }
-    delete ref;
+    ref->finalizer.finalize_cb = nullptr;
 
+    // don't delete the ref: if weak, it'll delete itself when the JS object is deleted;
+    // if strong, native addon needs to clean it up.
+    // the external is garbage collected.
     return napi_ok;
 }
 
@@ -1128,23 +1151,40 @@ extern "C" napi_status napi_unwrap(napi_env env, napi_value js_object,
 {
     NAPI_PREMABLE
 
-    JSValue value = toJS(js_object);
-
-    if (!value.isObject()) {
-        return NAPI_OBJECT_EXPECTED;
+    JSValue jsc_value = toJS(js_object);
+    if (jsc_value.isEmpty()) {
+        return napi_invalid_arg;
     }
+    JSObject* jsc_object = jsc_value.getObject();
+    if (!jsc_object) {
+        return napi_object_expected;
+    }
+    auto* napi_instance = jsDynamicCast<NapiPrototype*>(jsc_object);
 
+    auto* globalObject = toJS(env);
+    auto& vm = globalObject->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+    const JSC::Identifier& propertyName = WebCore::builtinNames(vm).napiWrappedContentsPrivateName();
     NapiRef* ref = nullptr;
-    if (auto* val = jsDynamicCast<NapiPrototype*>(value)) {
-        ref = val->napiRef;
-    } else if (auto* val = jsDynamicCast<NapiClass*>(value)) {
-        ref = val->napiRef;
+
+    if (!hasNapiWrap(vm, globalObject, jsc_object)) {
+        return napi_invalid_arg;
+    }
+
+    if (napi_instance) {
+        ref = napi_instance->napiRef;
     } else {
-        ASSERT(false);
+        JSValue contents_value = jsc_object->getDirect(vm, propertyName);
+        RETURN_IF_EXCEPTION(scope, napi_pending_exception);
+
+        // this cast asserts: we should not have stored anything but a NapiExternal here
+        auto* contents_external = jsCast<Bun::NapiExternal*>(contents_value);
+        ref = static_cast<NapiRef*>(contents_external->value());
     }
+    ASSERT(ref);
 
-    if (ref && result) {
-        *result = ref ? ref->data : nullptr;
+    if (result) {
+        *result = ref->data;
     }
 
     return napi_ok;

src/js/builtins/BunBuiltinNames.h

@@ -255,6 +255,7 @@ using namespace JSC;
     macro(writing) \
     macro(written) \
     macro(napiDlopenHandle) \
+    macro(napiWrappedContents) \
     BUN_ADDITIONAL_BUILTIN_NAMES(macro)
 // --- END of BUN_COMMON_PRIVATE_IDENTIFIERS_EACH_PROPERTY_NAME ---
 

test/napi/napi-app/binding.gyp

@@ -10,14 +10,25 @@
                     "AdditionalOptions": ["/std:c++20"],
                 },
             },
-            "sources": ["main.cpp"],
+            "sources": ["main.cpp", "wrap_tests.cpp"],
             "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",
             ],
-        }
+        },
+        {
+            "target_name": "second_addon",
+            "sources": ["second_addon.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/main.cpp

@@ -1,6 +1,6 @@
-#include <node.h>
-
-#include <napi.h>
+#include "napi_with_version.h"
+#include "utils.h"
+#include "wrap_tests.h"
 
 #include <array>
 #include <cassert>
@@ -30,23 +30,6 @@ napi_value fail_fmt(napi_env env, const char *fmt, ...) {
   return fail(env, buf);
 }
 
-napi_value ok(napi_env env) {
-  napi_value result;
-  napi_get_undefined(env, &result);
-  return result;
-}
-
-static void run_gc(const Napi::CallbackInfo &info) {
-  info[0].As<Napi::Function>().Call(0, nullptr);
-}
-
-// calls napi_typeof and asserts it returns napi_ok
-static napi_valuetype get_typeof(napi_env env, napi_value value) {
-  napi_valuetype result;
-  assert(napi_typeof(env, value, &result) == napi_ok);
-  return result;
-}
-
 napi_value test_issue_7685(const Napi::CallbackInfo &info) {
   Napi::Env env(info.Env());
   Napi::HandleScope scope(env);
@@ -595,33 +578,6 @@ napi_value was_finalize_called(const Napi::CallbackInfo &info) {
   return ret;
 }
 
-static const char *napi_valuetype_to_string(napi_valuetype type) {
-  switch (type) {
-  case napi_undefined:
-    return "undefined";
-  case napi_null:
-    return "null";
-  case napi_boolean:
-    return "boolean";
-  case napi_number:
-    return "number";
-  case napi_string:
-    return "string";
-  case napi_symbol:
-    return "symbol";
-  case napi_object:
-    return "object";
-  case napi_function:
-    return "function";
-  case napi_external:
-    return "external";
-  case napi_bigint:
-    return "bigint";
-  default:
-    return "unknown";
-  }
-}
-
 // calls a function (the sole argument) which must throw. catches and returns
 // the thrown error
 napi_value call_and_get_exception(const Napi::CallbackInfo &info) {
@@ -1080,6 +1036,8 @@ Napi::Object InitAll(Napi::Env env, Napi::Object exports1) {
   exports.Set("try_add_tag", Napi::Function::New(env, try_add_tag));
   exports.Set("check_tag", Napi::Function::New(env, check_tag));
 
+  napitests::register_wrap_tests(env, exports);
+
   return exports;
 }