From 7f56a78ff7eab77f1316696aef1de3304b70e639 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 29 Jan 2020 19:20:18 -0800 Subject: [PATCH] objectwrap: remove wrap only on failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `napi_remove_wrap()` was intended for objects that are alive for which the native addon wishes to withdraw its native pointer, and perhaps replace it with another. Therefore we need not `napi_remove_wrap()` during gc/env-cleanup. It is sufficient to `napi_delete_reference()`, as `Reference` already does. We need only `napi_remove_wrap()` if the construction failed and therefore no gc callback will ever happen. This change also removes references to `ObjectWrapConstructionContext` from the header because the class is not used anymore. Fixes: https://github.com/nodejs/node-addon-api/issues/660 Reviewed-By: Tobias Nießen Reviewed-By: Chengzhong Wu Reviewed-By: Kevin Eady Reviewed-By: Michael Dawson --- napi-inl.h | 9 ++++++--- napi.h | 5 ++--- test/objectwrap-removewrap.cc | 10 +++++----- test/objectwrap-removewrap.js | 22 ++++++++++++++++++++-- 4 files changed, 33 insertions(+), 13 deletions(-) diff --git a/napi-inl.h b/napi-inl.h index 2855e80ef..dca1c4e5a 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -3121,8 +3121,9 @@ inline ObjectWrap::~ObjectWrap() { 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()) + if (!object.IsEmpty() && _construction_failed) { napi_remove_wrap(Env(), object, nullptr); + } } } @@ -3694,15 +3695,17 @@ inline napi_value ObjectWrap::ConstructorCallbackWrapper( napi_value wrapper = details::WrapCallback([&] { CallbackInfo callbackInfo(env, info); + T* instance = new T(callbackInfo); #ifdef NAPI_CPP_EXCEPTIONS - new T(callbackInfo); + instance->_construction_failed = false; #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(); delete instance; e.ThrowAsJavaScriptException(); + } else { + instance->_construction_failed = false; } # endif // NAPI_CPP_EXCEPTIONS return callbackInfo.This(); diff --git a/napi.h b/napi.h index 49435d59b..ff7b82226 100644 --- a/napi.h +++ b/napi.h @@ -138,7 +138,6 @@ namespace Napi { class CallbackInfo; class TypedArray; template class TypedArrayOf; - class ObjectWrapConstructionContext; typedef TypedArrayOf Int8Array; ///< Typed-array of signed 8-bit integers typedef TypedArrayOf Uint8Array; ///< Typed-array of unsigned 8-bit integers @@ -1403,7 +1402,6 @@ namespace Napi { class CallbackInfo { public: - friend class ObjectWrapConstructionContext; CallbackInfo(napi_env env, napi_callback_info info); ~CallbackInfo(); @@ -1429,7 +1427,6 @@ namespace Napi { napi_value _staticArgs[6]; napi_value* _dynamicArgs; void* _data; - ObjectWrapConstructionContext* _objectWrapConstructionContext; }; class PropertyDescriptor { @@ -1888,6 +1885,8 @@ namespace Napi { template static napi_callback WrapSetter(SetterTag) noexcept { return &This::WrappedMethod; } static napi_callback WrapSetter(SetterTag) noexcept { return nullptr; } + + bool _construction_failed = true; }; class HandleScope { diff --git a/test/objectwrap-removewrap.cc b/test/objectwrap-removewrap.cc index 31a8c6870..fdcec07c7 100644 --- a/test/objectwrap-removewrap.cc +++ b/test/objectwrap-removewrap.cc @@ -1,7 +1,6 @@ #include #include -#ifdef NAPI_CPP_EXCEPTIONS namespace { static int dtor_called = 0; @@ -21,7 +20,11 @@ Napi::Value GetDtorCalled(const Napi::CallbackInfo& info) { 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) { @@ -30,16 +33,13 @@ class Test : public Napi::ObjectWrap { } private: - DtorCounter dtor_ounter_; + DtorCounter dtor_counter_; }; } // 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 index fe7a8274a..01c8b9169 100644 --- a/test/objectwrap-removewrap.js +++ b/test/objectwrap-removewrap.js @@ -1,8 +1,16 @@ '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 = (binding) => { +const test = (bindingName) => { + const binding = require(bindingName); const Test = binding.objectwrap_removewrap.Test; const getDtorCalled = binding.objectwrap_removewrap.getDtorCalled; @@ -12,6 +20,16 @@ const test = (binding) => { }); 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(require(`./build/${buildType}/binding.node`)); +test(`./build/${buildType}/binding.node`); +test(`./build/${buildType}/binding_noexcept.node`);