diff --git a/napi-inl.h b/napi-inl.h index f72e1daea..db3e53885 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -10,6 +10,7 @@ // Note: Do not include this file directly! Include "napi.h" instead. #include +#include #include #include #include @@ -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::ObjectWrap(const Napi::CallbackInfo& callbackInfo) { napi_value wrapper = callbackInfo.This(); napi_status status; napi_ref ref; - T* instance = static_cast(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* instanceRef = instance; + 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() && _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` destructor. This will prevent the + // `Reference` destructor from doing a double delete of this + // reference. + _ref = nullptr; + _env = nullptr; + } + } + } +} template inline T* ObjectWrap::Unwrap(Object wrapper) { @@ -3369,10 +3403,21 @@ inline napi_value ObjectWrap::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::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/napi.h b/napi.h index 2fc9b10f3..83834b4a1 100644 --- a/napi.h +++ b/napi.h @@ -1735,6 +1735,8 @@ namespace Napi { StaticAccessorCallbackData; typedef AccessorCallbackData InstanceAccessorCallbackData; + + bool _construction_failed = true; }; class HandleScope { diff --git a/test/bigint.js b/test/bigint.js index e4255172c..0af867b43 100644 --- a/test/bigint.js +++ b/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)$/, }); } diff --git a/test/binding.cc b/test/binding.cc index 5c3cd6b24..ad27484fe 100644 --- a/test/binding.cc +++ b/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)); diff --git a/test/binding.gyp b/test/binding.gyp index ced1a6802..d0be28ada 100644 --- a/test/binding.gyp +++ b/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', diff --git a/test/index.js b/test/index.js index e05c25a65..bff1708ca 100644 --- a/test/index.js +++ b/test/index.js @@ -48,6 +48,8 @@ let testModules = [ 'typedarray', '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..fdcec07c7 --- /dev/null +++ b/test/objectwrap-removewrap.cc @@ -0,0 +1,45 @@ +#include +#include + +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) { +#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; +} diff --git a/test/objectwrap-removewrap.js b/test/objectwrap-removewrap.js new file mode 100644 index 000000000..01c8b9169 --- /dev/null +++ b/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`); diff --git a/test/objectwrap.js b/test/objectwrap.js index 533c05f75..56d106c06 100644 --- a/test/objectwrap.js +++ b/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`)); diff --git a/test/objectwrap_constructor_exception.cc b/test/objectwrap_constructor_exception.cc new file mode 100644 index 000000000..015baa6a8 --- /dev/null +++ b/test/objectwrap_constructor_exception.cc @@ -0,0 +1,27 @@ +#include + +class ConstructorExceptionTest : + public Napi::ObjectWrap { +public: + ConstructorExceptionTest(const Napi::CallbackInfo& info) : + Napi::ObjectWrap(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; +} diff --git a/test/objectwrap_constructor_exception.js b/test/objectwrap_constructor_exception.js new file mode 100644 index 000000000..8428c8034 --- /dev/null +++ b/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`));