objectwrap: gracefully handle constructor exceptions #600

gabrielschulhof · 2019-11-13T08:00:53Z

Move the napi_wrap() step out of the
Napi::ObjectWrap<T>::ObjectWrap() constructor to ensure that no
native instance pointer is associated with the JavaScript object under
construction if the native constructor should cause a JavaScript
exception to be thrown.

Fixes: #599

NickNaso · 2019-11-13T11:59:16Z

@gabrielschulhof If I well understood you delayed the creation of the object reference in a function ConstructorCallbackWrapper that then we use in the DefineClass to pass the callback to underlying napi_define_class N-API function. Is it right?

dananderson · 2019-11-13T17:00:47Z

@gabrielschulhof My initial solution to this was the changes you proposed; however, the solution removes some functionality that may break existing ObjectWrap users (maybe this is a reasonable tradeoff). With the proposed change, native subclasses of ObjectWrap can no longer reference the javascript via Value() and Unwrap() could not be used.

class MyClass : public ObjectWrap<MyClass> {
  MyClass(const CallbackInfo& info) : ObjectWrap<MyClass>(info) {
    // info.This() could be used, but the myDependency constructor could call Unwrap()
    myDependency.New({ this->Value() });
  }
};

The above code feels contrived, but the rough equivalent in javascript seems more common.

class MyClass {
  constructor() {
    this.dependency = new Dependency(this)
  }
}

An alternate solution would be to introduce an ObjectWrap overridable Construct() function. Subclasses would put constructor logic in the new function. Basically, separating the C++/napi_ref construction from the constructor logic. Then, ConstructCallWrapper could do:

instance = new T(); // <- registers napi_ref, same finailizer
instance->Construct();
return callbackInfo.This();

This could also be a breaking change and I have not thought through all of the use cases.

gabrielschulhof · 2019-11-13T18:01:27Z

@dananderson good point! In that case, IINM the best we can do is, in ObjectWrap<T>::ConstructorCallbackWrapper:

  T* instance;
  napi_value wrapper = details::WrapCallback([&] {
    CallbackInfo callbackInfo(env, info);
#ifdef NAPI_CPP_EXCEPTIONS
    try {
      instance = new T(callbackInfo);
    } catch (const std::exception& e) {
      napi_remove_wrap(env, info.This(), instance);
      throw e;
    }
#else
    if (env.IsExceptionPending()) {
      napi_remove_wrap(env, info.This(), instance, ...);
      delete instance;
    }
#endif

Still, the scenario you present lends me to believe that the constructor chain which ultimately throws the exception may, in real addons, have lots of side effects which need to be cleaned up. At least with this modification it will fail with an exception rather than a crash, so that should spotlight the place where things need fixing, and should hopefully get people to look at all the side effects of their constructors.

dananderson · 2019-11-13T19:11:39Z

@gabrielschulhof I believe napi_remove_wrap() does not remove the finalizer (and C++this) from the napi_ref, so the GC will still try to delete the invalid pointer. I could be wrong, but this is my understanding from my quick look at the related V8 code (Reference). And, I did not see a way in V8 to disassociate the finalizer from the Reference.

Another solution is to use finalizer hint. Though, I think this is a bit messy and would require a heap allocation.

I don't see a great solution to this without breaking user code.

gabrielschulhof · 2019-11-14T23:57:32Z

@dananderson AFAICT with the change in this PR the v8impl::Reference used for the wrapping is actually deleted twice.

When C++ exceptions are used, it is deleted when the error is thrown, because the superclass of ObjectWrap<T> is Reference<Object> which will call napi_delete_reference() as part of its destructor. At this point, the reference is not actually deleted, but it is marked as a self-deleting reference. Then, in the new code, napi_remove_wrap() is called on the JavaScript object. This causes the v8impl::Reference to be retrieved from the wrapped object and deleted again. By this point it is marked as self-deleting, so it will actually be deleted. As part of the v8impl::Reference destructor, its _persistent member is also destroyed. _persistent is a v8::Global so it will be Reset() as part of its destruction, causing the weak callback to be removed from the JS object.

When C++ exceptions are not used, napi_remove_wrap() is called first, as part of the explicit exception handling added in this PR. Then, when the instance is explicitly deleted, the Reference<T> destructor calls napi_delete_reference(). Again, v8impl::Reference::Delete() is called twice, to the same effect.

Either way, it looks like the failed object construction is now cleaned up correctly.

gabrielschulhof · 2019-11-14T23:59:19Z

... although it looks like Node.js v8.x does not have all our v8impl::Reference fixes, because on that version the code does crash.

dmitryash · 2019-11-16T11:06:56Z

I would prefer calling napi_wrap() directly in ConstructorCallbackWrapper() after T instance is constructed if it is possible:

std::unique_ptr<T> instance(new T(cb_info));
napi_wrap(...);
instance.release();
return args.This();

gabrielschulhof · 2019-11-16T18:31:50Z

@dananderson pointed out that moving the napi_wrap() out of the constructor will prevent subclasses from calling Unwrap() in their constructor.

dmitryash · 2019-11-16T21:04:23Z

Just to make sure. Is it really needed to call Unwrap() inside a constructor if there is this pointer? Calling Unwrap() looks similar to calling a virtual function, the object is not competely constructed yet. To get JS value is enough to get This() from callback info instance but with care. JS and C++ are different so that there is not direct mapping between them. Some constructs cannot be implemented directly. However, I do not like an idea if separare Construct() because it is not natural in C++.

dmitryash · 2019-11-16T22:10:31Z

Moreover this code in ObjectWrap constructor may be an undefined behavior and incorrect

node-addon-api/napi-inl.h

Lines 2971 to 2973 in 24d75dd

    
           T* instance = static_cast<T*>(this); 
        
           status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref); 
        
           NAPI_THROW_IF_FAILED_VOID(env, status);

According to standard

static_cast of a pointer to storage without an object is only allowed when casting to (possibly cv-qualified) void*.

Object of T is not constructed but static_cast is used to cast to T*. So that napi_wrap() should not be called in base class, i.e ObjectWrap.

I'm not an expert in JS but this code looks broken and prints an unexpected value (however strictly defined):

class Dependency {
    constructor(my_class) {
        console.log("Value: " + my_class.value)
    }
}

class MyClass {
    constructor() {
        this.dependency = new Dependency(this)
        this.value = 5
    }
}

const c = new MyClass()

This code is similar to unwrapping and using this in constructor of C++ class and it may have unexpected results and I'm not sure that in C++ it is strictly defined (more likely undefined).
To recap, my opinion is not to allow users to call Unwrap within constructors. Instead Unwrapcan throw Error to detect code/design which probably does some bad things.

napi-inl.h

Ensure that no native instance pointer is associated with the JavaScript object under construction if the native constructor causes a JavaScript exception to be thrown. Two different cases must be taken into consideration: 1. The exception in the constructor was caused by `ObjectWrap<T>::ObjectWrap` when the call to `napi_wrap()` failed. 2. The exception in the constructor was caused by the constructor of the subclass of `ObjectWrap<T>` after `napi_wrap()` was already successful. Fixes: nodejs#599 Co-authored-by: blagoev <[email protected]> PR-URL: nodejs#600

gabrielschulhof · 2020-01-01T01:12:08Z

CI:

Version	Job	Status
v14.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/1507/	✓
v12.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/1508/	✓
v10.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/1509/	✓
v8.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/1510/	✗

That's right – v8.x doesn't have the patch in core that makes references work properly! Good thing it's going EOL post haste 🙂

gabrielschulhof · 2020-01-01T01:13:14Z

napi-inl.h

@@ -3106,11 +3137,11 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
  napi_value wrapper = callbackInfo.This();
  napi_status status;
  napi_ref ref;
-  T* instance = static_cast<T*>(this);


@dmitryash it's actually possible to avoid this cast, because this works just fine in the code below.

@gabrielschulhof While I agree that this is nicer this way, it also requires updating the cast in FinalizeCallback like #475 does

@addaleax I think #475 will mostly supersede this PR.

blagoev · 2020-01-02T09:40:46Z

@gabrielschulhof It seems to me both #638 and #600 latest changes have some drawbacks in the implementation, but I guess we can't make it ideal so lets go with these #600 last changes here. I wish to make some naming suggestions to make the names more generic. Please consider them but if you think it does not improve or it's not worth it, then just ignore them and merge after CI works. I will make comments inline with the code tab.
cheers

blagoev · 2020-01-02T09:56:12Z

test/objectwrap_constructor_exception.js

+  try {
+    new ConstructorExceptionTest();
+  } catch (anException) {
+    gotException = true;


If we use assert.throws(() => then the test will fail if there is no exception in the constructor. And currently the global.gc() call will fail the process leaving npm to show a generic error message. I think it's slightly better if we fail the test if there is no exception from the constructor. Just a notch better. what do you think? Or just swap the assert before the gc call maybe.

blagoev · 2020-01-02T10:15:57Z

@gabrielschulhof I will post the naming suggestion here for readability

ObjectWrapCleanup -> ObjectWrapCreationContext
ObjectWrapCleanup._wrap_worked -> ObjectWrapCreationContext._objectWrapped (i think c++ addon do not use snake style names or if it does then this becomes _object_wrapped)
ObjectWrapCleanup.MarkWrapOK() -> ObjectWrapCreationContext.SetObjectWrapped() (like setting a single bit)
ObjectWrapCleanup.RemoveWrap() -> ObjectWrapCreationContext.Cleanup()

CallbackInfo->_object_wrap_cleanup -> CallbackInfo->_objectWrapCreationContext (snake style or not)

My way of thinking about this is the caller of new T() needs to setup a creation context which might get used in the future for something else in addition to cleanup the wrapping on fail. For example some other creation parameter can be passed in before the wrapping occurs. Hence the change to more generic name.

Otherwise I think the implementation is great and fixes the issue. So 👍 for a merge. I am not sure about the CI.

cheers

gabrielschulhof · 2020-01-02T21:34:02Z

@blagoev I went with ObjectWrapConstructionContext and equivalents.

gabrielschulhof · 2020-01-02T21:45:15Z

This fix will not work on v8.x so we need to wait for #643 to land.

Ensure that no native instance pointer is associated with the JavaScript object under construction if the native constructor causes a JavaScript exception to be thrown. Two different cases must be taken into consideration: 1. The exception in the constructor was caused by `ObjectWrap<T>::ObjectWrap` when the call to `napi_wrap()` failed. 2. The exception in the constructor was caused by the constructor of the subclass of `ObjectWrap<T>` after `napi_wrap()` was already successful. Fixes: nodejs#599 Co-authored-by: blagoev <[email protected]> PR-URL: nodejs#600

gabrielschulhof · 2020-01-07T00:43:54Z

@KevinEady @mhdawson could you PTAL? I removed the "blocked" label, because the PR removing v8.x and v6.x support has landed.

mhdawson · 2020-01-09T22:56:28Z

test/objectwrap_constructor_exception.js

+
+const test = (binding) => {
+  const { ConstructorExceptionTest } = binding.objectwrapConstructorException;
+  assert.throws(() => (new ConstructorExceptionTest()));


Could this be improved to validate that we get the same exception as we expect?

mhdawson

LGTM

gabrielschulhof · 2020-01-09T23:32:29Z

CI:

Version	Job	Status
v14.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/1578/	✓
v12.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/1579/	✓
v10.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/1580/	✓

Ensure that no native instance pointer is associated with the JavaScript object under construction if the native constructor causes a JavaScript exception to be thrown. Two different cases must be taken into consideration: 1. The exception in the constructor was caused by `ObjectWrap<T>::ObjectWrap` when the call to `napi_wrap()` failed. 2. The exception in the constructor was caused by the constructor of the subclass of `ObjectWrap<T>` after `napi_wrap()` was already successful. Fixes: #599 Co-authored-by: blagoev <[email protected]> PR-URL: #600 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

gabrielschulhof · 2020-01-09T23:59:40Z

Landed in 86384f9.

Ensure that no native instance pointer is associated with the JavaScript object under construction if the native constructor causes a JavaScript exception to be thrown. Two different cases must be taken into consideration: 1. The exception in the constructor was caused by `ObjectWrap<T>::ObjectWrap` when the call to `napi_wrap()` failed. 2. The exception in the constructor was caused by the constructor of the subclass of `ObjectWrap<T>` after `napi_wrap()` was already successful. Fixes: nodejs#599 Co-authored-by: blagoev <[email protected]> PR-URL: nodejs#600 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Ensure that no native instance pointer is associated with the JavaScript object under construction if the native constructor causes a JavaScript exception to be thrown. Two different cases must be taken into consideration: 1. The exception in the constructor was caused by `ObjectWrap<T>::ObjectWrap` when the call to `napi_wrap()` failed. 2. The exception in the constructor was caused by the constructor of the subclass of `ObjectWrap<T>` after `napi_wrap()` was already successful. Fixes: nodejs/node-addon-api#599 Co-authored-by: blagoev <[email protected]> PR-URL: nodejs/node-addon-api#600 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

gabrielschulhof force-pushed the issue-599 branch 2 times, most recently from bf8519d to 9347e10 Compare November 14, 2019 03:09

gabrielschulhof force-pushed the issue-599 branch from 9347e10 to f9a8178 Compare November 15, 2019 04:31

gabrielschulhof mentioned this pull request Dec 25, 2019

fixes v8 GC access violation for zombie objects #638

Closed

gabrielschulhof commented Dec 30, 2019

View reviewed changes

napi-inl.h Outdated Show resolved Hide resolved

gabrielschulhof added the blocked label Dec 30, 2019

gabrielschulhof force-pushed the issue-599 branch from f9a8178 to 683bfcc Compare December 31, 2019 20:55

gabrielschulhof removed the blocked label Dec 31, 2019

gabrielschulhof force-pushed the issue-599 branch from 683bfcc to a1acd41 Compare January 1, 2020 00:11

gabrielschulhof force-pushed the issue-599 branch from a1acd41 to 0cc1985 Compare January 1, 2020 00:27

gabrielschulhof force-pushed the issue-599 branch from 0cc1985 to 7ee9bfd Compare January 1, 2020 00:29

gabrielschulhof commented Jan 1, 2020

View reviewed changes

gabrielschulhof requested review from legendecas, mhdawson and KevinEady January 1, 2020 01:20

blagoev reviewed Jan 2, 2020

View reviewed changes

gabrielschulhof added the blocked label Jan 2, 2020

blagoev approved these changes Jan 3, 2020

View reviewed changes

gabrielschulhof removed the blocked label Jan 7, 2020

Gabriel Schulhof and others added 3 commits January 6, 2020 16:27

remove static cast in constructor

b00638c

perform some renames to better reflect semantics

868b4bd

gabrielschulhof force-pushed the issue-599 branch from 759800a to 868b4bd Compare January 7, 2020 00:27

mhdawson reviewed Jan 9, 2020

View reviewed changes

mhdawson approved these changes Jan 9, 2020

View reviewed changes

test for a specific exception

bd465de

gabrielschulhof closed this Jan 9, 2020

gabrielschulhof mentioned this pull request Jan 10, 2020

src: fix ObjectWrap destruction #475

Closed

gabrielschulhof deleted the issue-599 branch February 1, 2021 16:39

gabrielschulhof restored the issue-599 branch February 1, 2021 16:40

gabrielschulhof deleted the issue-599 branch March 7, 2021 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

objectwrap: gracefully handle constructor exceptions #600

objectwrap: gracefully handle constructor exceptions #600

gabrielschulhof commented Nov 13, 2019

NickNaso commented Nov 13, 2019

dananderson commented Nov 13, 2019

gabrielschulhof commented Nov 13, 2019

dananderson commented Nov 13, 2019

gabrielschulhof commented Nov 14, 2019

gabrielschulhof commented Nov 14, 2019

dmitryash commented Nov 16, 2019

gabrielschulhof commented Nov 16, 2019

dmitryash commented Nov 16, 2019

dmitryash commented Nov 16, 2019

gabrielschulhof commented Jan 1, 2020

gabrielschulhof Jan 1, 2020

addaleax Jan 14, 2020

gabrielschulhof Jan 15, 2020

blagoev commented Jan 2, 2020

blagoev Jan 2, 2020

blagoev commented Jan 2, 2020

gabrielschulhof commented Jan 2, 2020

gabrielschulhof commented Jan 2, 2020

gabrielschulhof commented Jan 7, 2020 •

edited

Loading

mhdawson Jan 9, 2020

gabrielschulhof Jan 9, 2020

mhdawson left a comment

gabrielschulhof commented Jan 9, 2020

gabrielschulhof commented Jan 9, 2020

objectwrap: gracefully handle constructor exceptions #600

objectwrap: gracefully handle constructor exceptions #600

Conversation

gabrielschulhof commented Nov 13, 2019

NickNaso commented Nov 13, 2019

dananderson commented Nov 13, 2019

gabrielschulhof commented Nov 13, 2019

dananderson commented Nov 13, 2019

gabrielschulhof commented Nov 14, 2019

gabrielschulhof commented Nov 14, 2019

dmitryash commented Nov 16, 2019

gabrielschulhof commented Nov 16, 2019

dmitryash commented Nov 16, 2019

dmitryash commented Nov 16, 2019

gabrielschulhof commented Jan 1, 2020

gabrielschulhof Jan 1, 2020

Choose a reason for hiding this comment

addaleax Jan 14, 2020

Choose a reason for hiding this comment

gabrielschulhof Jan 15, 2020

Choose a reason for hiding this comment

blagoev commented Jan 2, 2020

blagoev Jan 2, 2020

Choose a reason for hiding this comment

blagoev commented Jan 2, 2020

gabrielschulhof commented Jan 2, 2020

gabrielschulhof commented Jan 2, 2020

gabrielschulhof commented Jan 7, 2020 • edited Loading

mhdawson Jan 9, 2020

Choose a reason for hiding this comment

gabrielschulhof Jan 9, 2020

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Jan 9, 2020

gabrielschulhof commented Jan 9, 2020

gabrielschulhof commented Jan 7, 2020 •

edited

Loading