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.

Two different cases must be taken into consideration:

1. The exception in the constructor was caused by
   `ObjectWrap<T>::ObjectWrap` when the call to `napi_wrap()` failed.
2. The exception in the constructor was caused by the constructor of
   the subclass of `ObjectWrap<T>` after `napi_wrap()` was already
   successful.

Fixes: nodejs#599
Co-authored-by: blagoev <[email protected]>
PR-URL: nodejs#600
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

Author: Gabriel Schulhof

napi-inl.h

@@ -2666,6 +2666,37 @@ inline Object FunctionReference::New(const std::vector<napi_value>& args) const
 // CallbackInfo class
 ////////////////////////////////////////////////////////////////////////////////
 
+class ObjectWrapConstructionContext {
+ public:
+  ObjectWrapConstructionContext(CallbackInfo* info) {
+    info->_objectWrapConstructionContext = this;
+  }
+
+  static inline void SetObjectWrapped(const CallbackInfo& info) {
+    if (info._objectWrapConstructionContext == nullptr) {
+      Napi::Error::Fatal("ObjectWrapConstructionContext::SetObjectWrapped",
+                         "_objectWrapConstructionContext is NULL");
+    }
+    info._objectWrapConstructionContext->_objectWrapped = true;
+  }
+
+  inline void Cleanup(const CallbackInfo& info) {
+    if (_objectWrapped) {
+      napi_status status = napi_remove_wrap(info.Env(), info.This(), nullptr);
+
+      // There's already a pending exception if we are at this point, so we have
+      // no choice but to fatally fail here.
+      NAPI_FATAL_IF_FAILED(status,
+                           "ObjectWrapConstructionContext::Cleanup",
+                           "Failed to remove wrap from unsuccessfully "
+                           "constructed ObjectWrap instance");
+    }
+  }
+
+ private:
+  bool _objectWrapped = false;
+};
+
 inline CallbackInfo::CallbackInfo(napi_env env, napi_callback_info info)
     : _env(env), _info(info), _this(nullptr), _dynamicArgs(nullptr), _data(nullptr) {
   _argc = _staticArgCount;
@@ -2968,11 +2999,11 @@ 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;
+  ObjectWrapConstructionContext::SetObjectWrapped(callbackInfo);
+  Reference<Object>* instanceRef = this;
   *instanceRef = Reference<Object>(env, ref);
 }
 
@@ -3369,10 +3400,27 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(
     return nullptr;
   }
 
-  T* instance;
   napi_value wrapper = details::WrapCallback([&] {
     CallbackInfo callbackInfo(env, info);
-    instance = new T(callbackInfo);
+    ObjectWrapConstructionContext constructionContext(&callbackInfo);
+#ifdef NAPI_CPP_EXCEPTIONS
+    try {
+      new T(callbackInfo);
+    } catch (const Error& e) {
+      // Re-throw the error after removing the failed wrap.
+      constructionContext.Cleanup(callbackInfo);
+      throw e;
+    }
+#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();
+      constructionContext.Cleanup(callbackInfo);
+      e.ThrowAsJavaScriptException();
+      delete instance;
+    }
+# endif  // NAPI_CPP_EXCEPTIONS
     return callbackInfo.This();
   });
 

napi.h

@@ -132,6 +132,7 @@ namespace Napi {
   template <typename T> class Reference;
   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
@@ -1377,6 +1378,7 @@ namespace Napi {
 
   class CallbackInfo {
   public:
+    friend class ObjectWrapConstructionContext;
     CallbackInfo(napi_env env, napi_callback_info info);
     ~CallbackInfo();
 
@@ -1403,6 +1405,7 @@ namespace Napi {
     napi_value _staticArgs[6];
     napi_value* _dynamicArgs;
     void* _data;
+    ObjectWrapConstructionContext* _objectWrapConstructionContext;
   };
 
   class PropertyDescriptor {

test/binding.cc

@@ -49,6 +49,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);
@@ -102,6 +103,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));

test/binding.gyp

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

test/index.js

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

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;
+}

test/objectwrap_constructor_exception.js

@@ -0,0 +1,12 @@
+'use strict';
+const buildType = process.config.target_defaults.default_configuration;
+const assert = require('assert');
+
+const test = (binding) => {
+  const { ConstructorExceptionTest } = binding.objectwrapConstructorException;
+  assert.throws(() => (new ConstructorExceptionTest()), /an exception/);
+  global.gc();
+}
+
+test(require(`./build/${buildType}/binding.node`));
+test(require(`./build/${buildType}/binding_noexcept.node`));