objectwrap: gracefully handle constructor exceptions

Ensure that no native instance pointer is associated with the JavaScript object under construction if the native constructor causes a JavaScript exception to be thrown. Fixes: #599
nodejs · Nov 14, 2019 · 9347e10 · 9347e10
1 parent df75e08
commit 9347e10
Show file tree

Hide file tree

Showing 6 changed files with 71 additions and 0 deletions.
diff --git a/napi-inl.h b/napi-inl.h
@@ -3370,9 +3370,30 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
   }
 
   T* instance;
+  auto handleConstructorException = [](const CallbackInfo& info) {
+    napi_status status = napi_remove_wrap(info.Env(), info.This(), nullptr);
+    NAPI_FATAL_IF_FAILED(status,
+        "ObjectWrap<T>::ConstructorCallbackWrapper",
+        "Failed to remove wrap from failed ObjectWrap instance construction");
+  };
   napi_value wrapper = details::WrapCallback([&] {
     CallbackInfo callbackInfo(env, info);
+#ifdef NAPI_CPP_EXCEPTIONS
+    try {
+      instance = new T(callbackInfo);
+    } catch (const Error& e) {
+      handleConstructorException(callbackInfo);
+      e.ThrowAsJavaScriptException();
+    }
+#else
     instance = new T(callbackInfo);
+    if (callbackInfo.Env().IsExceptionPending()) {
+      Error e = callbackInfo.Env().GetAndClearPendingException();
+      handleConstructorException(callbackInfo);
+      e.ThrowAsJavaScriptException();
+      delete instance;
+    }
+# endif  // NAPI_CPP_EXCEPTIONS
     return callbackInfo.This();
   });
 

diff --git a/test/binding.cc b/test/binding.cc
@@ -48,6 +48,7 @@ Object InitThreadSafeFunction(Env env);
 #endif
 Object InitTypedArray(Env env);
 Object InitObjectWrap(Env env);
+Object InitObjectWrapConstructorException(Env env);
 Object InitObjectReference(Env env);
 Object InitVersionManagement(Env env);
 Object InitThunkingManual(Env env);
@@ -100,6 +101,8 @@ Object Init(Env env, Object exports) {
 #endif
   exports.Set("typedarray", InitTypedArray(env));
   exports.Set("objectwrap", InitObjectWrap(env));
+  exports.Set("objectwrapConstructorException",
+      InitObjectWrapConstructorException(env));
   exports.Set("objectreference", InitObjectReference(env));
   exports.Set("version_management", InitVersionManagement(env));
   exports.Set("thunking_manual", InitThunkingManual(env));

diff --git a/test/binding.gyp b/test/binding.gyp
@@ -43,6 +43,7 @@
         'threadsafe_function/threadsafe_function.cc',
         'typedarray.cc',
         'objectwrap.cc',
+        'objectwrap_constructor_exception.cc',
         'objectreference.cc',
         'version_management.cc',
         'thunking_manual.cc',

diff --git a/test/index.js b/test/index.js
@@ -47,6 +47,7 @@ let testModules = [
   'typedarray',
   'typedarray-bigint',
   'objectwrap',
+  'objectwrap_constructor_exception',
   'objectreference',
   'version_management'
 ];

diff --git a/test/objectwrap_constructor_exception.cc b/test/objectwrap_constructor_exception.cc
@@ -0,0 +1,26 @@
+#include <napi.h>
+
+class ConstructorExceptionTest :
+  public Napi::ObjectWrap<ConstructorExceptionTest> {
+public:
+  ConstructorExceptionTest(const Napi::CallbackInfo& info) :
+    Napi::ObjectWrap<ConstructorExceptionTest>(info) {
+    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;
+}
diff --git a/test/objectwrap_constructor_exception.js b/test/objectwrap_constructor_exception.js
@@ -0,0 +1,19 @@
+'use strict';
+const buildType = process.config.target_defaults.default_configuration;
+const assert = require('assert');
+
+const test = (binding) => {
+  const { ConstructorExceptionTest } = binding.objectwrapConstructorException;
+  let gotException = false;
+  try {
+    new ConstructorExceptionTest();
+  } catch (anException) {
+    gotException = true;
+  }
+  global.gc();
+
+  assert.strictEqual(gotException, true);
+}
+
+test(require(`./build/${buildType}/binding.node`));
+test(require(`./build/${buildType}/binding_noexcept.node`));