objectwrap: remove wrap only on failure

`napi_remove_wrap()` was intended for objects that are alive for which the native addon wishes to withdraw its native pointer, and perhaps replace it with another. Therefore we need not `napi_remove_wrap()` during gc/env-cleanup. It is sufficient to `napi_delete_reference()`, as `Reference<Object>` already does. We need only `napi_remove_wrap()` if the construction failed and therefore no gc callback will ever happen. This change also removes references to `ObjectWrapConstructionContext` from the header because the class is not used anymore. Fixes: #660 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Kevin Eady <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
nodejs · Feb 8, 2020 · 7f56a78 · 7f56a78
1 parent 4d81618
commit 7f56a78
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 13 deletions.
diff --git a/napi-inl.h b/napi-inl.h
@@ -3121,8 +3121,9 @@ inline ObjectWrap<T>::~ObjectWrap() {
     Object object = Value();
     // It is not valid to call `napi_remove_wrap()` with an empty `object`.
     // This happens e.g. during garbage collection.
-    if (!object.IsEmpty())
+    if (!object.IsEmpty() && _construction_failed) {
       napi_remove_wrap(Env(), object, nullptr);
+    }
   }
 }
 
@@ -3694,15 +3695,17 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
 
   napi_value wrapper = details::WrapCallback([&] {
     CallbackInfo callbackInfo(env, info);
+    T* instance = new T(callbackInfo);
 #ifdef NAPI_CPP_EXCEPTIONS
-    new T(callbackInfo);
+    instance->_construction_failed = false;
 #else
-    T* instance = new T(callbackInfo);
     if (callbackInfo.Env().IsExceptionPending()) {
       // We need to clear the exception so that removing the wrap might work.
       Error e = callbackInfo.Env().GetAndClearPendingException();
       delete instance;
       e.ThrowAsJavaScriptException();
+    } else {
+      instance->_construction_failed = false;
     }
 # endif  // NAPI_CPP_EXCEPTIONS
     return callbackInfo.This();

diff --git a/napi.h b/napi.h
@@ -138,7 +138,6 @@ namespace Napi {
   class CallbackInfo;
   class TypedArray;
   template <typename T> class TypedArrayOf;
-  class ObjectWrapConstructionContext;
 
   typedef TypedArrayOf<int8_t> Int8Array;     ///< Typed-array of signed 8-bit integers
   typedef TypedArrayOf<uint8_t> Uint8Array;   ///< Typed-array of unsigned 8-bit integers
@@ -1403,7 +1402,6 @@ namespace Napi {
 
   class CallbackInfo {
   public:
-    friend class ObjectWrapConstructionContext;
     CallbackInfo(napi_env env, napi_callback_info info);
     ~CallbackInfo();
 
@@ -1429,7 +1427,6 @@ namespace Napi {
     napi_value _staticArgs[6];
     napi_value* _dynamicArgs;
     void* _data;
-    ObjectWrapConstructionContext* _objectWrapConstructionContext;
   };
 
   class PropertyDescriptor {
@@ -1888,6 +1885,8 @@ namespace Napi {
     template <InstanceSetterCallback setter>
     static napi_callback WrapSetter(SetterTag<setter>) noexcept { return &This::WrappedMethod<setter>; }
     static napi_callback WrapSetter(SetterTag<nullptr>) noexcept { return nullptr; }
+
+    bool _construction_failed = true;
   };
 
   class HandleScope {

diff --git a/test/objectwrap-removewrap.cc b/test/objectwrap-removewrap.cc
@@ -1,7 +1,6 @@
 #include <napi.h>
 #include <assert.h>
 
-#ifdef NAPI_CPP_EXCEPTIONS
 namespace {
 
 static int dtor_called = 0;
@@ -21,7 +20,11 @@ Napi::Value GetDtorCalled(const Napi::CallbackInfo& info) {
 class Test : public Napi::ObjectWrap<Test> {
 public:
   Test(const Napi::CallbackInfo& info) : Napi::ObjectWrap<Test>(info) {
+#ifdef NAPI_CPP_EXCEPTIONS
     throw Napi::Error::New(Env(), "Some error");
+#else
+    Napi::Error::New(Env(), "Some error").ThrowAsJavaScriptException();
+#endif
   }
 
   static void Initialize(Napi::Env env, Napi::Object exports) {
@@ -30,16 +33,13 @@ class Test : public Napi::ObjectWrap<Test> {
   }
 
 private:
-  DtorCounter dtor_ounter_;
+  DtorCounter dtor_counter_;
 };
 
 }  // anonymous namespace
-#endif  // NAPI_CPP_EXCEPTIONS
 
 Napi::Object InitObjectWrapRemoveWrap(Napi::Env env) {
   Napi::Object exports = Napi::Object::New(env);
-#ifdef NAPI_CPP_EXCEPTIONS
   Test::Initialize(env, exports);
-#endif
   return exports;
 }
diff --git a/test/objectwrap-removewrap.js b/test/objectwrap-removewrap.js
@@ -1,8 +1,16 @@
 'use strict';
+
+if (process.argv[2] === 'child') {
+  // Create a single wrapped instance then exit.
+  return new (require(process.argv[3]).objectwrap.Test)();
+}
+
 const buildType = process.config.target_defaults.default_configuration;
 const assert = require('assert');
+const { spawnSync } = require('child_process');
 
-const test = (binding) => {
+const test = (bindingName) => {
+  const binding = require(bindingName);
   const Test = binding.objectwrap_removewrap.Test;
   const getDtorCalled = binding.objectwrap_removewrap.getDtorCalled;
 
@@ -12,6 +20,16 @@ const test = (binding) => {
   });
   assert.strictEqual(getDtorCalled(), 1);
   global.gc();  // Does not crash.
+
+  // Start a child process that creates a single wrapped instance to ensure that
+  // it is properly freed at its exit. It must not segfault.
+  // Re: https://github.com/nodejs/node-addon-api/issues/660
+  const child = spawnSync(process.execPath, [
+    __filename, 'child', bindingName
+  ]);
+  assert.strictEqual(child.signal, null);
+  assert.strictEqual(child.status, 0);
 }
 
-test(require(`./build/${buildType}/binding.node`));
+test(`./build/${buildType}/binding.node`);
+test(`./build/${buildType}/binding_noexcept.node`);