Skip to content

Commit

Permalink
objectwrap: remove wrap only on failure
Browse files Browse the repository at this point in the history
`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<Object>`
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: nodejs#660
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
  • Loading branch information
Gabriel Schulhof committed Feb 8, 2020
1 parent 4d81618 commit 7f56a78
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 13 deletions.
9 changes: 6 additions & 3 deletions napi-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -3121,8 +3121,9 @@ inline ObjectWrap<T>::~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);
}
}
}

Expand Down Expand Up @@ -3694,15 +3695,17 @@ inline napi_value ObjectWrap<T>::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();
Expand Down
5 changes: 2 additions & 3 deletions napi.h
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,6 @@ namespace Napi {
class CallbackInfo;
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
Expand Down Expand Up @@ -1403,7 +1402,6 @@ namespace Napi {

class CallbackInfo {
public:
friend class ObjectWrapConstructionContext;
CallbackInfo(napi_env env, napi_callback_info info);
~CallbackInfo();

Expand All @@ -1429,7 +1427,6 @@ namespace Napi {
napi_value _staticArgs[6];
napi_value* _dynamicArgs;
void* _data;
ObjectWrapConstructionContext* _objectWrapConstructionContext;
};

class PropertyDescriptor {
Expand Down Expand Up @@ -1888,6 +1885,8 @@ namespace Napi {
template <InstanceSetterCallback setter>
static napi_callback WrapSetter(SetterTag<setter>) noexcept { return &This::WrappedMethod<setter>; }
static napi_callback WrapSetter(SetterTag<nullptr>) noexcept { return nullptr; }

bool _construction_failed = true;
};

class HandleScope {
Expand Down
10 changes: 5 additions & 5 deletions test/objectwrap-removewrap.cc
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#include <napi.h>
#include <assert.h>

#ifdef NAPI_CPP_EXCEPTIONS
namespace {

static int dtor_called = 0;
Expand All @@ -21,7 +20,11 @@ Napi::Value GetDtorCalled(const Napi::CallbackInfo& info) {
class Test : public Napi::ObjectWrap<Test> {
public:
Test(const Napi::CallbackInfo& info) : Napi::ObjectWrap<Test>(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) {
Expand All @@ -30,16 +33,13 @@ class Test : public Napi::ObjectWrap<Test> {
}

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;
}
22 changes: 20 additions & 2 deletions test/objectwrap-removewrap.js
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -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`);

0 comments on commit 7f56a78

Please sign in to comment.