Merge branch 'add-napi_remove_wrap' of github.com:addaleax/node-api

Fix crashes when the ctor of an ObjectWrap subclass throws.
See nodejs#475

Author: fprenoveau

napi-inl.h

@@ -2920,16 +2920,25 @@ 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())
+      napi_remove_wrap(Env(), object, nullptr);
+  }
+}
 
 template<typename T>
 inline T* ObjectWrap<T>::Unwrap(Object wrapper) {
@@ -3449,7 +3458,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;
 }

test/binding.cc

@@ -42,6 +42,7 @@ Object InitThreadSafeFunction(Env env);
 #endif
 Object InitTypedArray(Env env);
 Object InitObjectWrap(Env env);
+Object InitObjectWrapRemoveWrap(Env env);
 Object InitObjectReference(Env env);
 Object InitVersionManagement(Env env);
 Object InitThunkingManual(Env env);
@@ -87,6 +88,7 @@ Object Init(Env env, Object exports) {
 #endif
   exports.Set("typedarray", InitTypedArray(env));
   exports.Set("objectwrap", InitObjectWrap(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

@@ -37,6 +37,7 @@
         'threadsafe_function/threadsafe_function.cc',
         'typedarray.cc',
         'objectwrap.cc',
+        'objectwrap-removewrap.cc',
         'objectreference.cc',
         'version_management.cc',
         'thunking_manual.cc',

test/index.js

@@ -42,6 +42,7 @@ let testModules = [
   'typedarray',
   'typedarray-bigint',
   'objectwrap',
+  'objectwrap-removewrap',
   'objectreference',
   'version_management'
 ];

test/objectwrap-removewrap.cc

@@ -0,0 +1,45 @@
+#include <napi.h>
+#include <assert.h>
+
+#ifdef NAPI_CPP_EXCEPTIONS
+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) {
+    throw Napi::Error::New(Env(), "Some error");
+  }
+
+  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_ounter_;
+};
+
+}  // 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;
+}

test/objectwrap-removewrap.js

@@ -0,0 +1,17 @@
+'use strict';
+const buildType = process.config.target_defaults.default_configuration;
+const assert = require('assert');
+
+const test = (binding) => {
+  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.
+}
+
+test(require(`./build/${buildType}/binding.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`));