Merge pull request #723 from gabrielschulhof/backport-4e885069-pr-475

[v2.x] Backport ObjectWrap reference-related changes

Author: NickNaso

napi-inl.h

@@ -10,6 +10,7 @@
 // Note: Do not include this file directly! Include "napi.h" instead.
 
 #include <algorithm>
+#include <atomic>
 #include <cstring>
 #include <mutex>
 #include <type_traits>
@@ -19,6 +20,8 @@ namespace Napi {
 // Helpers to handle functions exposed from C++.
 namespace details {
 
+extern std::atomic_bool needs_objectwrap_destructor_fix;
+
 // Attach a data item to an object and delete it when the object gets
 // garbage-collected.
 // TODO: Replace this code with `napi_add_finalizer()` whenever it becomes
@@ -251,11 +254,16 @@ struct AccessorCallbackData {
 // Module registration
 ////////////////////////////////////////////////////////////////////////////////
 
-#define NODE_API_MODULE(modname, regfunc)                 \
-  napi_value __napi_ ## regfunc(napi_env env,             \
-                                napi_value exports) {     \
-    return Napi::RegisterModule(env, exports, regfunc);   \
-  }                                                       \
+#define NODE_API_MODULE(modname, regfunc)                      \
+  namespace Napi {                                             \
+    namespace details {                                        \
+      std::atomic_bool needs_objectwrap_destructor_fix(false); \
+    }                                                          \
+  }                                                            \
+  napi_value __napi_ ## regfunc(napi_env env,                  \
+                                napi_value exports) {          \
+    return Napi::RegisterModule(env, exports, regfunc);        \
+  }                                                            \
   NAPI_MODULE(modname, __napi_ ## regfunc)
 
 // Adapt the NAPI_MODULE registration function:
@@ -264,6 +272,12 @@ struct AccessorCallbackData {
 inline napi_value RegisterModule(napi_env env,
                                  napi_value exports,
                                  ModuleRegisterCallback registerCallback) {
+  const napi_node_version* nver = Napi::VersionManagement::GetNodeVersion(env);
+  Napi::details::needs_objectwrap_destructor_fix =
+    (nver->major < 10 ||
+      (nver->major == 10 && nver->minor < 15) ||
+      (nver->major == 10 && nver->minor == 15 && nver->patch < 3));
+
   return details::WrapCallback([&] {
     return napi_value(registerCallback(Napi::Env(env),
                                        Napi::Object(env, exports)));
@@ -2968,16 +2982,36 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
   napi_value wrapper = callbackInfo.This();
   napi_status status;
   napi_ref ref;
-  T* instance = static_cast<T*>(this);
-  status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref);
+  status = napi_wrap(env, wrapper, this, FinalizeCallback, nullptr, &ref);
   NAPI_THROW_IF_FAILED_VOID(env, status);
 
-  Reference<Object>* instanceRef = instance;
+  Reference<Object>* instanceRef = this;
   *instanceRef = Reference<Object>(env, ref);
 }
 
-template<typename T>
-inline ObjectWrap<T>::~ObjectWrap() {}
+template <typename T>
+inline ObjectWrap<T>::~ObjectWrap() {
+  // If the JS object still exists at this point, remove the finalizer added
+  // through `napi_wrap()`.
+  if (!IsEmpty()) {
+    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() && _construction_failed) {
+      napi_remove_wrap(Env(), object, nullptr);
+
+      if (Napi::details::needs_objectwrap_destructor_fix) {
+        // If construction failed we delete the reference via
+        // `napi_remove_wrap()`, not via `napi_delete_reference()` in the
+        // `Reference<Object>` destructor. This will prevent the
+        // `Reference<Object>` destructor from doing a double delete of this
+        // reference.
+        _ref = nullptr;
+        _env = nullptr;
+      }
+    }
+  }
+}
 
 template<typename T>
 inline T* ObjectWrap<T>::Unwrap(Object wrapper) {
@@ -3369,10 +3403,21 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
     return nullptr;
   }
 
-  T* instance;
   napi_value wrapper = details::WrapCallback([&] {
     CallbackInfo callbackInfo(env, info);
-    instance = new T(callbackInfo);
+    T* instance = new T(callbackInfo);
+#ifdef NAPI_CPP_EXCEPTIONS
+    instance->_construction_failed = false;
+#else
+    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();
   });
 
@@ -3497,7 +3542,7 @@ inline napi_value ObjectWrap<T>::InstanceSetterCallbackWrapper(
 
 template <typename T>
 inline void ObjectWrap<T>::FinalizeCallback(napi_env env, void* data, void* /*hint*/) {
-  T* instance = reinterpret_cast<T*>(data);
+  ObjectWrap<T>* instance = static_cast<ObjectWrap<T>*>(data);
   instance->Finalize(Napi::Env(env));
   delete instance;
 }

napi.h

@@ -1735,6 +1735,8 @@ namespace Napi {
       StaticAccessorCallbackData;
     typedef AccessorCallbackData<InstanceGetterCallback, InstanceSetterCallback>
       InstanceAccessorCallbackData;
+
+    bool _construction_failed = true;
   };
 
   class HandleScope {

test/bigint.js

@@ -46,7 +46,7 @@ function test(binding) {
   });
 
   assert.throws(TestTooBigBigInt, {
-    name: 'RangeError',
-    message: 'Maximum BigInt size exceeded',
+    name: /^(RangeError|Error)$/,
+    message: /^(Maximum BigInt size exceeded|Invalid argument)$/,
   });
 }

test/binding.cc

@@ -49,6 +49,8 @@ Object InitThreadSafeFunction(Env env);
 #endif
 Object InitTypedArray(Env env);
 Object InitObjectWrap(Env env);
+Object InitObjectWrapConstructorException(Env env);
+Object InitObjectWrapRemoveWrap(Env env);
 Object InitObjectReference(Env env);
 Object InitVersionManagement(Env env);
 Object InitThunkingManual(Env env);
@@ -102,6 +104,9 @@ Object Init(Env env, Object exports) {
 #endif
   exports.Set("typedarray", InitTypedArray(env));
   exports.Set("objectwrap", InitObjectWrap(env));
+  exports.Set("objectwrapConstructorException",
+      InitObjectWrapConstructorException(env));
+  exports.Set("objectwrap_removewrap", InitObjectWrapRemoveWrap(env));
   exports.Set("objectreference", InitObjectReference(env));
   exports.Set("version_management", InitVersionManagement(env));
   exports.Set("thunking_manual", InitThunkingManual(env));

test/binding.gyp

@@ -44,6 +44,8 @@
         'threadsafe_function/threadsafe_function.cc',
         'typedarray.cc',
         'objectwrap.cc',
+        'objectwrap_constructor_exception.cc',
+        'objectwrap-removewrap.cc',
         'objectreference.cc',
         'version_management.cc',
         'thunking_manual.cc',

test/index.js

@@ -48,6 +48,8 @@ let testModules = [
   'typedarray',
   'typedarray-bigint',
   'objectwrap',
+  'objectwrap_constructor_exception',
+  'objectwrap-removewrap',
   'objectreference',
   'version_management'
 ];

test/objectwrap-removewrap.cc

@@ -0,0 +1,45 @@
+#include <napi.h>
+#include <assert.h>
+
+namespace {
+
+static int dtor_called = 0;
+
+class DtorCounter {
+ public:
+  ~DtorCounter() {
+    assert(dtor_called == 0);
+    dtor_called++;
+  }
+};
+
+Napi::Value GetDtorCalled(const Napi::CallbackInfo& info) {
+  return Napi::Number::New(info.Env(), dtor_called);
+}
+
+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) {
+    exports.Set("Test", DefineClass(env, "Test", {}));
+    exports.Set("getDtorCalled", Napi::Function::New(env, GetDtorCalled));
+  }
+
+private:
+  DtorCounter dtor_counter_;
+};
+
+}  // anonymous namespace
+
+Napi::Object InitObjectWrapRemoveWrap(Napi::Env env) {
+  Napi::Object exports = Napi::Object::New(env);
+  Test::Initialize(env, exports);
+  return exports;
+}

test/objectwrap-removewrap.js

@@ -0,0 +1,35 @@
+'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 = (bindingName) => {
+  const binding = require(bindingName);
+  const Test = binding.objectwrap_removewrap.Test;
+  const getDtorCalled = binding.objectwrap_removewrap.getDtorCalled;
+
+  assert.strictEqual(getDtorCalled(), 0);
+  assert.throws(() => {
+    new Test();
+  });
+  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(`./build/${buildType}/binding.node`);
+test(`./build/${buildType}/binding_noexcept.node`);

test/objectwrap.js

@@ -222,6 +222,9 @@ const test = (binding) => {
   // `Test` is needed for accessing exposed symbols
   testObj(new Test(), Test);
   testClass(Test);
+
+  // Make sure the C++ object can be garbage collected without issues.
+  setImmediate(global.gc);
 }
 
 test(require(`./build/${buildType}/binding.node`));

test/objectwrap_constructor_exception.cc

@@ -0,0 +1,27 @@
+#include <napi.h>
+
+class ConstructorExceptionTest :
+  public Napi::ObjectWrap<ConstructorExceptionTest> {
+public:
+  ConstructorExceptionTest(const Napi::CallbackInfo& info) :
+    Napi::ObjectWrap<ConstructorExceptionTest>(info) {
+    fprintf(stderr, "That was the interesting reference\n");
+    Napi::Error error = Napi::Error::New(info.Env(), "an exception");
+#ifdef NAPI_DISABLE_CPP_EXCEPTIONS
+    error.ThrowAsJavaScriptException();
+#else
+    throw error;
+#endif  // NAPI_DISABLE_CPP_EXCEPTIONS
+  }
+
+  static void Initialize(Napi::Env env, Napi::Object exports) {
+    const char* name = "ConstructorExceptionTest";
+    exports.Set(name, DefineClass(env, name, {}));
+  }
+};
+
+Napi::Object InitObjectWrapConstructorException(Napi::Env env) {
+  Napi::Object exports = Napi::Object::New(env);
+  ConstructorExceptionTest::Initialize(env, exports);
+  return exports;
+}