diff --git a/napi-inl.h b/napi-inl.h index c9fa63875..2855e80ef 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -2702,37 +2702,6 @@ inline Object FunctionReference::New(const std::vector& 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; @@ -3140,13 +3109,22 @@ inline ObjectWrap::ObjectWrap(const Napi::CallbackInfo& callbackInfo) { status = napi_wrap(env, wrapper, this, FinalizeCallback, nullptr, &ref); NAPI_THROW_IF_FAILED_VOID(env, status); - ObjectWrapConstructionContext::SetObjectWrapped(callbackInfo); Reference* instanceRef = this; *instanceRef = Reference(env, ref); } -template -inline ObjectWrap::~ObjectWrap() {} +template +inline ObjectWrap::~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 inline T* ObjectWrap::Unwrap(Object wrapper) { @@ -3716,23 +3694,15 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( napi_value wrapper = details::WrapCallback([&] { CallbackInfo callbackInfo(env, info); - 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; - } + new T(callbackInfo); #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; + e.ThrowAsJavaScriptException(); } # endif // NAPI_CPP_EXCEPTIONS return callbackInfo.This(); @@ -3859,7 +3829,7 @@ inline napi_value ObjectWrap::InstanceSetterCallbackWrapper( template inline void ObjectWrap::FinalizeCallback(napi_env env, void* data, void* /*hint*/) { - T* instance = reinterpret_cast(data); + ObjectWrap* instance = static_cast*>(data); instance->Finalize(Napi::Env(env)); delete instance; } diff --git a/test/binding.cc b/test/binding.cc index bfed9ae5f..aa9db6e41 100644 --- a/test/binding.cc +++ b/test/binding.cc @@ -51,6 +51,7 @@ Object InitThreadSafeFunction(Env env); 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); @@ -107,6 +108,7 @@ Object Init(Env env, Object exports) { 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)); diff --git a/test/binding.gyp b/test/binding.gyp index a84ab073d..b6777808d 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -42,6 +42,7 @@ 'typedarray.cc', 'objectwrap.cc', 'objectwrap_constructor_exception.cc', + 'objectwrap-removewrap.cc', 'objectreference.cc', 'version_management.cc', 'thunking_manual.cc', diff --git a/test/index.js b/test/index.js index 5f8bdea90..1bd1d9144 100644 --- a/test/index.js +++ b/test/index.js @@ -50,6 +50,7 @@ let testModules = [ 'typedarray-bigint', 'objectwrap', 'objectwrap_constructor_exception', + 'objectwrap-removewrap', 'objectreference', 'version_management' ]; diff --git a/test/objectwrap-removewrap.cc b/test/objectwrap-removewrap.cc new file mode 100644 index 000000000..31a8c6870 --- /dev/null +++ b/test/objectwrap-removewrap.cc @@ -0,0 +1,45 @@ +#include +#include + +#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 { +public: + Test(const Napi::CallbackInfo& info) : Napi::ObjectWrap(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; +} diff --git a/test/objectwrap-removewrap.js b/test/objectwrap-removewrap.js new file mode 100644 index 000000000..fe7a8274a --- /dev/null +++ b/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`)); diff --git a/test/objectwrap.js b/test/objectwrap.js index e1281d3d2..a1a56136f 100644 --- a/test/objectwrap.js +++ b/test/objectwrap.js @@ -273,6 +273,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`));