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
  • Loading branch information
Gabriel Schulhof committed Jan 29, 2020
1 parent 4648420 commit f135cb2
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 7 deletions.
8 changes: 4 additions & 4 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,17 +3695,16 @@ inline napi_value ObjectWrap<T>::ConstructorCallbackWrapper(

napi_value wrapper = details::WrapCallback([&] {
CallbackInfo callbackInfo(env, info);
#ifdef NAPI_CPP_EXCEPTIONS
new T(callbackInfo);
#else
T* instance = new T(callbackInfo);
#ifndef NAPI_CPP_EXCEPTIONS
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();
}
# endif // NAPI_CPP_EXCEPTIONS
instance->_construction_failed = false;
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

0 comments on commit f135cb2

Please sign in to comment.