n-api: add ability to remove a wrapping

Calling napi_wrap() twice on the same object has the result of returning
napi_invalid_arg on the second call. However, sometimes it is necessary
to replace the native pointer associated with a given object. This new
API allows one to remove an existing pointer, returning the object to
its pristine, non-wrapped state.

PR-URL: https://github.com/nodejs/node/pull/14658
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fixes: https://github.com/nodejs/abi-stable-node/issues/266

doc/api/n-api.md

@@ -3152,7 +3152,9 @@ Afterward, additional manipulation of the wrapper's prototype chain may cause
 
 *Note*: Calling `napi_wrap()` a second time on an object that already has a
 native instance associated with it by virtue of a previous call to
-`napi_wrap()` will cause an error to be returned.
+`napi_wrap()` will cause an error to be returned. If you wish to associate
+another native instance with the given object, call `napi_remove_wrap()` on it
+first.
 
 ### *napi_unwrap*
 <!-- YAML
@@ -3165,8 +3167,8 @@ napi_status napi_unwrap(napi_env env,
 ```
 
  - `[in] env`: The environment that the API is invoked under.
- - `[in] js_object`: The object associated with the C++ class instance.
+ - `[in] js_object`: The object associated with the native instance.
- - `[out] result`: Pointer to the wrapped C++ class instance.
+ - `[out] result`: Pointer to the wrapped native instance.
 
 Returns `napi_ok` if the API succeeded.
 
@@ -3179,6 +3181,28 @@ method or accessor, then the `this` argument to the callback is the wrapper
 object; the wrapped C++ instance that is the target of the call can be obtained
 then by calling `napi_unwrap()` on the wrapper object.
 
+### *napi_remove_wrap*
+<!-- YAML
+added: REPLACEME
+-->
+```C
+napi_status napi_remove_wrap(napi_env env,
+                             napi_value js_object,
+                             void** result);
+```
+
+ - `[in] env`: The environment that the API is invoked under.
+ - `[in] js_object`: The object associated with the native instance.
+ - `[out] result`: Pointer to the wrapped native instance.
+
+Returns `napi_ok` if the API succeeded.
+
+Retrieves a native instance that was previously wrapped in the JavaScript
+object `js_object` using `napi_wrap()` and removes the wrapping, thereby
+restoring the JavaScript object's prototype chain. If a finalize callback was
+associated with the wrapping, it will no longer be called when the JavaScript
+object becomes garbage-collected.
+
 ## Asynchronous Operations
 
 Addon modules often need to leverage async helpers from libuv as part of their

src/node_api.cc

@@ -674,6 +674,8 @@ v8::Local<v8::Object> CreateAccessorCallbackData(napi_env env,
   return cbdata;
 }
 
+int kWrapperFields = 3;
+
 // Pointer used to identify items wrapped by N-API. Used by FindWrapper and
 // napi_wrap().
 const char napi_wrap_name[] = "N-API Wrapper";
@@ -682,7 +684,8 @@ const char napi_wrap_name[] = "N-API Wrapper";
 // wrapper would be the first in the chain, but it is OK for other objects to
 // be inserted in the prototype chain.
 bool FindWrapper(v8::Local<v8::Object> obj,
-                 v8::Local<v8::Object>* result = nullptr) {
+                 v8::Local<v8::Object>* result = nullptr,
+                 v8::Local<v8::Object>* parent = nullptr) {
   v8::Local<v8::Object> wrapper = obj;
 
   do {
@@ -690,8 +693,11 @@ bool FindWrapper(v8::Local<v8::Object> obj,
     if (proto.IsEmpty() || !proto->IsObject()) {
       return false;
     }
+    if (parent != nullptr) {
+      *parent = wrapper;
+    }
     wrapper = proto.As<v8::Object>();
-    if (wrapper->InternalFieldCount() == 2) {
+    if (wrapper->InternalFieldCount() == kWrapperFields) {
       v8::Local<v8::Value> external = wrapper->GetInternalField(1);
       if (external->IsExternal() &&
           external.As<v8::External>()->Value() == v8impl::napi_wrap_name) {
@@ -745,6 +751,29 @@ napi_env GetEnv(v8::Local<v8::Context> context) {
   return result;
 }
 
+napi_status Unwrap(napi_env env,
+                   napi_value js_object,
+                   void** result,
+                   v8::Local<v8::Object>* wrapper,
+                   v8::Local<v8::Object>* parent = nullptr) {
+  CHECK_ARG(env, js_object);
+  CHECK_ARG(env, result);
+
+  v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(js_object);
+  RETURN_STATUS_IF_FALSE(env, value->IsObject(), napi_invalid_arg);
+  v8::Local<v8::Object> obj = value.As<v8::Object>();
+
+  RETURN_STATUS_IF_FALSE(
+    env, v8impl::FindWrapper(obj, wrapper, parent), napi_invalid_arg);
+
+  v8::Local<v8::Value> unwrappedValue = (*wrapper)->GetInternalField(0);
+  RETURN_STATUS_IF_FALSE(env, unwrappedValue->IsExternal(), napi_invalid_arg);
+
+  *result = unwrappedValue.As<v8::External>()->Value();
+
+  return napi_ok;
+}
+
 }  // end of namespace v8impl
 
 // Intercepts the Node-V8 module registration callback. Converts parameters
@@ -2266,62 +2295,78 @@ napi_status napi_wrap(napi_env env,
   // Create a wrapper object with an internal field to hold the wrapped pointer
   // and a second internal field to identify the owner as N-API.
   v8::Local<v8::ObjectTemplate> wrapper_template;
-  ENV_OBJECT_TEMPLATE(env, wrap, wrapper_template, 2);
+  ENV_OBJECT_TEMPLATE(env, wrap, wrapper_template, v8impl::kWrapperFields);
 
   auto maybe_object = wrapper_template->NewInstance(context);
   CHECK_MAYBE_EMPTY(env, maybe_object, napi_generic_failure);
-
   v8::Local<v8::Object> wrapper = maybe_object.ToLocalChecked();
-  wrapper->SetInternalField(1, v8::External::New(isolate,
-    reinterpret_cast<void*>(const_cast<char*>(v8impl::napi_wrap_name))));
 
   // Store the pointer as an external in the wrapper.
   wrapper->SetInternalField(0, v8::External::New(isolate, native_object));
+  wrapper->SetInternalField(1, v8::External::New(isolate,
+    reinterpret_cast<void*>(const_cast<char*>(v8impl::napi_wrap_name))));
 
   // Insert the wrapper into the object's prototype chain.
   v8::Local<v8::Value> proto = obj->GetPrototype();
   CHECK(wrapper->SetPrototype(context, proto).FromJust());
   CHECK(obj->SetPrototype(context, wrapper).FromJust());
 
+  v8impl::Reference* reference = nullptr;
   if (result != nullptr) {
     // The returned reference should be deleted via napi_delete_reference()
     // ONLY in response to the finalize callback invocation. (If it is deleted
     // before then, then the finalize callback will never be invoked.)
     // Therefore a finalize callback is required when returning a reference.
     CHECK_ARG(env, finalize_cb);
-    v8impl::Reference* reference = v8impl::Reference::New(
+    reference = v8impl::Reference::New(
         env, obj, 0, false, finalize_cb, native_object, finalize_hint);
     *result = reinterpret_cast<napi_ref>(reference);
   } else if (finalize_cb != nullptr) {
     // Create a self-deleting reference just for the finalize callback.
-    v8impl::Reference::New(
+    reference = v8impl::Reference::New(
         env, obj, 0, true, finalize_cb, native_object, finalize_hint);
   }
 
+  if (reference != nullptr) {
+    wrapper->SetInternalField(2, v8::External::New(isolate, reference));
+  }
+
   return GET_RETURN_STATUS(env);
 }
 
-napi_status napi_unwrap(napi_env env, napi_value js_object, void** result) {
+napi_status napi_unwrap(napi_env env, napi_value obj, void** result) {
   // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
   // JS exceptions.
   CHECK_ENV(env);
-  CHECK_ARG(env, js_object);
+  v8::Local<v8::Object> wrapper;
-  CHECK_ARG(env, result);
+  return napi_set_last_error(env, v8impl::Unwrap(env, obj, result, &wrapper));
-
+}
-  v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(js_object);
-  RETURN_STATUS_IF_FALSE(env, value->IsObject(), napi_invalid_arg);
-  v8::Local<v8::Object> obj = value.As<v8::Object>();
 
+napi_status napi_remove_wrap(napi_env env, napi_value obj, void** result) {
+  NAPI_PREAMBLE(env);
   v8::Local<v8::Object> wrapper;
-  RETURN_STATUS_IF_FALSE(
+  v8::Local<v8::Object> parent;
-    env, v8impl::FindWrapper(obj, &wrapper), napi_invalid_arg);
+  napi_status status = v8impl::Unwrap(env, obj, result, &wrapper, &parent);
+  if (status != napi_ok) {
+    return napi_set_last_error(env, status);
+  }
 
-  v8::Local<v8::Value> unwrappedValue = wrapper->GetInternalField(0);
+  v8::Local<v8::Value> external = wrapper->GetInternalField(2);
-  RETURN_STATUS_IF_FALSE(env, unwrappedValue->IsExternal(), napi_invalid_arg);
+  if (external->IsExternal()) {
+    v8impl::Reference::Delete(
+        static_cast<v8impl::Reference*>(external.As<v8::External>()->Value()));
+  }
 
-  *result = unwrappedValue.As<v8::External>()->Value();
+  if (!parent.IsEmpty()) {
+    v8::Maybe<bool> maybe = parent->SetPrototype(
+        env->isolate->GetCurrentContext(), wrapper->GetPrototype());
+    CHECK_MAYBE_NOTHING(env, maybe, napi_generic_failure);
+    if (!maybe.FromMaybe(false)) {
+      return napi_set_last_error(env, napi_generic_failure);
+    }
+  }
 
-  return napi_clear_last_error(env);
+  return GET_RETURN_STATUS(env);
 }
 
 napi_status napi_create_external(napi_env env,

src/node_api.h

@@ -362,6 +362,9 @@ NAPI_EXTERN napi_status napi_wrap(napi_env env,
 NAPI_EXTERN napi_status napi_unwrap(napi_env env,
                                     napi_value js_object,
                                     void** result);
+NAPI_EXTERN napi_status napi_remove_wrap(napi_env env,
+                                         napi_value js_object,
+                                         void** result);
 NAPI_EXTERN napi_status napi_create_external(napi_env env,
                                              void* data,
                                              napi_finalize finalize_cb,

test/addons-napi/test_general/test.js

@@ -1,4 +1,5 @@
 'use strict';
+// Flags: --expose-gc
 
 const common = require('../../common');
 const test_general = require(`./build/${common.buildType}/test_general`);
@@ -56,10 +57,37 @@ assert.strictEqual(release, process.release.name);
 // for null
 assert.strictEqual(test_general.testNapiTypeof(null), 'null');
 
-const x = {};
+// Ensure that garbage collecting an object with a wrapped native item results
+// in the finalize callback being called.
+let w = {};
+test_general.wrap(w, []);
+w = null;
+global.gc();
+assert.strictEqual(test_general.derefItemWasCalled(), true,
+                   'deref_item() was called upon garbage collecting a ' +
+                   'wrapped object');
 
 // Assert that wrapping twice fails.
+const x = {};
 test_general.wrap(x, 25);
 assert.throws(function() {
   test_general.wrap(x, 'Blah');
 }, Error);
+
+// Ensure that wrapping, removing the wrap, and then wrapping again works.
+const y = {};
+test_general.wrap(y, -12);
+test_general.removeWrap(y);
+assert.doesNotThrow(function() {
+  test_general.wrap(y, 're-wrap!');
+}, Error, 'Wrapping twice succeeds if a remove_wrap() separates the instances');
+
+// Ensure that removing a wrap and garbage collecting does not fire the
+// finalize callback.
+let z = {};
+test_general.testFinalizeWrap(z);
+test_general.removeWrap(z);
+z = null;
+global.gc();
+assert.strictEqual(test_general.finalizeWasCalled(), false,
+                   'finalize callback was not called upon garbage collection');

test/addons-napi/test_general/test_general.c

@@ -138,17 +138,29 @@ napi_value testNapiTypeof(napi_env env, napi_callback_info info) {
   return result;
 }
 
+static bool deref_item_called = false;
 static void deref_item(napi_env env, void* data, void* hint) {
   (void) hint;
 
+  deref_item_called = true;
   NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, (napi_ref)data));
 }
 
+napi_value deref_item_was_called(napi_env env, napi_callback_info info) {
+  napi_value it_was_called;
+
+  NAPI_CALL(env, napi_get_boolean(env, deref_item_called, &it_was_called));
+
+  return it_was_called;
+}
+
 napi_value wrap(napi_env env, napi_callback_info info) {
   size_t argc = 2;
   napi_value argv[2];
   napi_ref payload;
 
+  deref_item_called = false;
+
   NAPI_CALL(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL));
   NAPI_CALL(env, napi_create_reference(env, argv[1], 1, &payload));
   NAPI_CALL(env, napi_wrap(env, argv[0], payload, deref_item, NULL, NULL));
@@ -156,6 +168,43 @@ napi_value wrap(napi_env env, napi_callback_info info) {
   return NULL;
 }
 
+napi_value remove_wrap(napi_env env, napi_callback_info info) {
+  size_t argc = 1;
+  napi_value wrapped;
+  void* data;
+
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &wrapped, NULL, NULL));
+  NAPI_CALL(env, napi_remove_wrap(env, wrapped, &data));
+  if (data != NULL) {
+    NAPI_CALL(env, napi_delete_reference(env, (napi_ref)data));
+  }
+
+  return NULL;
+}
+
+static bool finalize_called = false;
+static void test_finalize(napi_env env, void* data, void* hint) {
+  finalize_called = true;
+}
+
+napi_value test_finalize_wrap(napi_env env, napi_callback_info info) {
+  size_t argc = 1;
+  napi_value js_object;
+
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &js_object, NULL, NULL));
+  NAPI_CALL(env, napi_wrap(env, js_object, NULL, test_finalize, NULL, NULL));
+
+  return NULL;
+}
+
+napi_value finalize_was_called(napi_env env, napi_callback_info info) {
+  napi_value it_was_called;
+
+  NAPI_CALL(env, napi_get_boolean(env, finalize_called, &it_was_called));
+
+  return it_was_called;
+}
+
 void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
   napi_property_descriptor descriptors[] = {
     DECLARE_NAPI_PROPERTY("testStrictEquals", testStrictEquals),
@@ -169,6 +218,10 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
     DECLARE_NAPI_PROPERTY("testNapiErrorCleanup", testNapiErrorCleanup),
     DECLARE_NAPI_PROPERTY("testNapiTypeof", testNapiTypeof),
     DECLARE_NAPI_PROPERTY("wrap", wrap),
+    DECLARE_NAPI_PROPERTY("removeWrap", remove_wrap),
+    DECLARE_NAPI_PROPERTY("testFinalizeWrap", test_finalize_wrap),
+    DECLARE_NAPI_PROPERTY("finalizeWasCalled", finalize_was_called),
+    DECLARE_NAPI_PROPERTY("derefItemWasCalled", deref_item_was_called),
   };
 
   NAPI_CALL_RETURN_VOID(env, napi_define_properties(