Napi::Env::GetAndClearPendingException assumes exceptions are Error objects #912

legendecas · 2021-02-20T03:31:57Z

On common checks on each node-api calls in node-addon-api (e.g. NAPI_THROW_IF_FAILED), Napi::Error::New(env) is used to construct proper error representative types to indicate the exception value. In https://github.com/nodejs/node-addon-api/blob/main/napi.h#L1354 Napi::Error extends Napi::ObjectReference, which in the term of itself is correct, JavaScript errors are objects. However, exceptions are not, they can be any JavaScript values like string, or number, although the meaning might not be clear when throwing primitives like numbers but it is still valid JavaScript codes.

Thus, in the case of throwing non-object and non-function types between the border of node-addon-api and javascript (or using napi_throw values directly in c++ land), node-addon-api complains (fatal error) that Napi::Error cannot be constructed with non-objects and non-functions.

#include <napi.h>

namespace {
void Test(const Napi::CallbackInfo& info) {
  info[0].As<Napi::Function>().Call({});
}

Napi::Object InitAll(Napi::Env env, Napi::Object exports) {
  exports.Set("test", Napi::Function::New<Test>(env));
}

NODE_API_MODULE(addon, InitAll)
}

example of calling into the addon:

const addon = require('./build/Debug/test_napi_exception.node');
addon.test(() => {
    throw 'foobar';
});

This is what we will get:

FATAL ERROR: Error::Error napi_create_reference
 1: 0xadd3a0 node::Abort() [../node/out/Release/node]
 2: 0xade3dc node::OnFatalError(char const*, char const*) [../node/out/Release/node]
 3: 0xade479  [../node/out/Release/node]
 4: 0xaa454b napi_fatal_error [../node/out/Release/node]
 5: 0x7f8e504cb8d6 Napi::Error::Error() [/disks/chengzhong.wcz/repro-napi-v8impl-refbase-double-free/build/Debug/test_napi_exception.node]
 6: 0x7f8e504cb9c4 Napi::Error::Error(napi_env__*, napi_value__*) [/disks/chengzhong.wcz/repro-napi-v8impl-refbase-double-free/build/Debug/test_napi_exception.node]
 7: 0x7f8e504cb840 Napi::Error::New(napi_env__*) [/disks/chengzhong.wcz/repro-napi-v8impl-refbase-double-free/build/Debug/test_napi_exception.node]
 8: 0x7f8e504cb4ac Napi::Function::Call(napi_value__*, unsigned long, napi_value__* const*) const [/disks/chengzhong.wcz/repro-napi-v8impl-refbase-double-free/build/Debug/test_napi_exception.node]
 9: 0x7f8e504cb3da Napi::Function::Call(napi_value__*, std::initializer_list<napi_value__*> const&) const [/disks/chengzhong.wcz/repro-napi-v8impl-refbase-double-free/build/Debug/test_napi_exception.node]
10: 0x7f8e504cb36e Napi::Function::Call(std::initializer_list<napi_value__*> const&) const [/disks/chengzhong.wcz/repro-napi-v8impl-refbase-double-free/build/Debug/test_napi_exception.node]
11: 0x7f8e504cad59  [/disks/chengzhong.wcz/repro-napi-v8impl-refbase-double-free/build/Debug/test_napi_exception.node]
12: 0x7f8e504caf17  [/disks/chengzhong.wcz/repro-napi-v8impl-refbase-double-free/build/Debug/test_napi_exception.node]
13: 0x7f8e504cafb0  [/disks/chengzhong.wcz/repro-napi-v8impl-refbase-double-free/build/Debug/test_napi_exception.node]
14: 0x7f8e504caf84  [/disks/chengzhong.wcz/repro-napi-v8impl-refbase-double-free/build/Debug/test_napi_exception.node]
15: 0xa88545  [../node/out/Release/node]
16: 0xd65c3a  [../node/out/Release/node]
17: 0xd677ec v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [../node/out/Release/node]
18: 0x15f1699  [../node/out/Release/node]

The text was updated successfully, but these errors were encountered:

gabrielschulhof · 2021-02-21T19:51:58Z

Our C++-to-JS exception conversion code requires that exceptions be JS objects so we can keep persistent references to them (#31). We may need a workaround though.

gabrielschulhof · 2021-03-05T16:55:45Z

One possible way forward as discussed in the @nodejs/node-api meeting is to provide a variant of GetAndClearException() that returns a Napi::Value rather than a Napi::Error, and only if C++ exception handling is turned off, making clear in the documentation the implications wrt. HandleScope of not having a persistent reference.

github-actions · 2021-06-04T02:04:48Z

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

legendecas · 2021-06-23T16:55:45Z

After investigating the problem, I believe the best solution (not introduce any breaking change) will be:

if the thrown value is primitive, we fetch the value and keep a "reference" by value, and convert them back to JavaScript once the value is requested.
if the thrown value is object, or function, just do what we are doing right now.

The key point is to prevent calling into JavaScript again in the exception handling to prevent from causing the VM state to be changed.

However, the (1) can apply on null, undefined, boolean, number, string, bigint, but not on symbol. symbols are unique values, we can not fetch the native value of the symbol nor can we convert some native representative back to symbol.

A TC39 stage 2 proposal Symbols as WeakMap keys is trying to loosen the WeakMap keys type constraint to allow symbol as WeakMap keys, which is very similar to the case we are facing (somewhat). I opened nodejs/node#39131 to discuss losing the constraint on node-api.

KevinEady · 2021-07-23T15:24:03Z

As discussed in today's Node-API meeting, we need to determine if various JavaScript engines allow creating references on primitive values. If this is the case, then the fix can be done in core via allowing references on all types. Otherwise, this will need fixes in node-addon-api instead.

Engine	Allows References on Primitives	Notes
V8
Apple	Yes	See JSValueProtect, allows referencing any JSValue
Firefox/Spidermonkey	Yes	See JS::PersistentRooted.
JerryScript	Yes	See Reference counting. The example creates a reference on a primitive (`3.14`)
React Native / Hermes

mhdawson · 2021-07-30T15:19:39Z

@vmoroz can you help confirm whether or not React Native/Hermes supports creating references on primitive values?

vmoroz · 2021-08-02T20:21:44Z

@vmoroz can you help confirm whether or not React Native/Hermes supports creating references on primitive values?

The JSI in ReactNative/Hermes does not differentiate between local and referenced values. All values are either references (object/string/symbol) or C++ primitives such as number/bool. The jsi::Value that can be of any JS type is defined as a union: https://github.com/facebook/react-native/blob/034c6dfe34d240cf7c6314e767716317fa554351/ReactCommon/jsi/jsi/jsi.h#L938 .
I.e. there are no references for number/bool, but strings and symbols are always references.

To implement JSI adapter for the NAPI I had to introduce a new type napi_ext_ref to work around the limitations of the NAPI references. It was mostly needed to support strings/symbols, to do better ref counting (auto-delete if ref count zero), and to do better weak ref counting (there can be multiple weak refs with their own ref count).

I would suggest that NAPI should adopt an improved reference type that feels and behaves similar to std::shared_ptr/std::weak_ptr from the C++ standard library.

mhdawson · 2021-08-06T15:19:18Z

@vmoroz I think we asked the wrong question last time. Based on discussion in the meeting today what we were trying to ask was:

@vmoroz can you help confirm whether or not React Native/Hermes supports creating persistent references on primitive values? In V8 references are either local or persistent and we get references for both primitive and object values. To be able to keep a reference outside of a specific scope we need to create persistent references in both cases. We want to make sure we can do the equivalent of creating a persistent reference for both primitive and object values in React Native/Hermes.

vmoroz · 2021-08-13T15:01:01Z

@mhdawson, yes, Hermes internally seems to have a mechanism creating persistent references for both primitive and object values. It can be done by providing a GC callback that reports root values. It is used in the JSI API implementation used by React Native. See https://github.com/facebook/hermes/blob/f3421c66a052bb12f400d527b858f4208b827c8b/API/hermes/hermes.cpp#L304

Though the JSI does not expose the references to numbers, bools, null, and undefined. These are converted from C++ values: https://github.com/facebook/hermes/blob/f3421c66a052bb12f400d527b858f4208b827c8b/API/hermes/hermes.cpp#L673

I am starting to look at the implementation of NAPI for Hermes, and hope to find soon if there any limitations with exposing references to numbers, bools, undefined, and null.

KevinEady · 2021-08-20T15:26:41Z

We discussed in today's meeting, for this particular application of throwing exceptions of primitive types: we really only need Symbol support from the engine, as we can handle the remaining primitives in the library (by taking the native primitive value and converting to/from JS values). It sounds like Hermes supports Symbol references so we should be able to move forward with this approach and lift the "only Function or Object" restriction on references to include Symbols.

mhdawson · 2021-08-27T15:09:00Z

Next step is for somebody to open a PR in core to extend support to Symbol. Effectively it should just be no longer blocking it on Symbols.

JckXia · 2021-08-27T17:03:21Z

Hey @mhdawson, I would like to take a stab at this and just have a question. Is it that all that we needed to do in node core is to add a check inside napi_create_reference to allow v8_value to be a symbol ? Thanks!

mhdawson · 2021-08-27T19:08:59Z

@JckXia my understand is that we have an existing check that prevents it from being a symbol. It would be to loosen that existing check to allow symbol. That might be exactly what you said but I'd have to look at the code to know :)

mhdawson · 2021-09-17T16:05:01Z

Change in Node-addon api

In inline Error::Error(napi_env env, napi_value value)

If created reference fails, create wrapper object, store value on object, create reference on that object

In code that throws Error as JavaScript (ThrowAsJavaScriptException()), pull value off object if we created it and throw that
Override Value() on Error so that it it does unwrap as well
Document that if you cast the Error to a C ref then you get the wrapper object if it was primitive (and possibly symbol) and how you can get the inner value.

JckXia · 2021-09-18T16:55:06Z

Hey @mhdawson , I just have some question with regard to implementing this change. Inside the ThrowAsJavaScriptException function, there's a call to napi_throw using the Value() func inherited from the Reference class. Could we leave the code inside ThrowAsJavaScriptException as is and simply override the Value() on error to do the unwrapping?

Also it looks like the Value() function from the base Reference class is not virtual, so what would the best way to override this function method from the Error class?

Thanks!

mhdawson · 2021-09-20T14:45:53Z

@JckXia overriding Value() sounds like the right answer as then all callers will get the right object.

In terms Value not being virtual, do you see any reason why it should not be?

JckXia · 2021-09-21T02:24:23Z

@mhdawson When I tried to mark Value as virtual from the Reference base class, I got an error saying
After doing some research it seems to suggest that to do this we have to mark the ObjectReference dtor as a virtual function as well. I am not too sure but I feel like this would require us to restructure a lot of unrelated code within napi.h. What I am doing at the moment is to "override" the Value function by defining it inside the Error class, which seems to work. (#1075) Thanks!

github-actions · 2021-12-21T00:14:34Z

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

mhdawson · 2021-12-21T14:19:37Z

@JckXia I think this one as fixed by one of your PRs right?

JckXia · 2021-12-21T23:09:50Z

@mhdawson Yep this is addressed by PR #1075 under node addon api

mhdawson · 2021-12-22T14:30:49Z

Closing as @JckXia confirmed it was addressed in #1075

legendecas changed the title ~~Napi::Error definition not compliant with ECMAScript throw-ables~~ Napi::Env::GetAndClearPendingException assumes exceptions are Error objects Feb 20, 2021

legendecas mentioned this issue Feb 22, 2021

Crash on node api add-on finalization nodejs/node#37236

Closed

KevinEady mentioned this issue Mar 20, 2021

src: fix a crashing issue in Error::ThrowAsJavaScriptException #902

Closed

legendecas mentioned this issue May 24, 2021

[BUG] node REPL crashes creating ObjectWrap in getter #1000

Closed

github-actions bot added the stale label Jun 4, 2021

legendecas removed the stale label Jun 4, 2021

KevinEady mentioned this issue Jun 4, 2021

Stale issues in node-addon-api to discuss nodejs/abi-stable-node#415

Open

JckXia mentioned this issue Aug 29, 2021

node-api: Loosen restriction on the type of value that we can create a ref on nodejs/node#39926

Closed

github-actions bot added the stale label Dec 21, 2021

mhdawson removed the stale label Dec 21, 2021

mhdawson closed this as completed Dec 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Napi::Env::GetAndClearPendingException assumes exceptions are Error objects #912

Napi::Env::GetAndClearPendingException assumes exceptions are Error objects #912

legendecas commented Feb 20, 2021 •

edited

Loading

gabrielschulhof commented Feb 21, 2021

gabrielschulhof commented Mar 5, 2021

github-actions bot commented Jun 4, 2021

legendecas commented Jun 23, 2021 •

edited

Loading

KevinEady commented Jul 23, 2021 •

edited

Loading

mhdawson commented Jul 30, 2021

vmoroz commented Aug 2, 2021

mhdawson commented Aug 6, 2021

vmoroz commented Aug 13, 2021

KevinEady commented Aug 20, 2021

mhdawson commented Aug 27, 2021

JckXia commented Aug 27, 2021

mhdawson commented Aug 27, 2021

mhdawson commented Sep 17, 2021

JckXia commented Sep 18, 2021

mhdawson commented Sep 20, 2021

JckXia commented Sep 21, 2021

github-actions bot commented Dec 21, 2021

mhdawson commented Dec 21, 2021

JckXia commented Dec 21, 2021

mhdawson commented Dec 22, 2021

Napi::Env::GetAndClearPendingException assumes exceptions are Error objects #912

Napi::Env::GetAndClearPendingException assumes exceptions are Error objects #912

Comments

legendecas commented Feb 20, 2021 • edited Loading

gabrielschulhof commented Feb 21, 2021

gabrielschulhof commented Mar 5, 2021

github-actions bot commented Jun 4, 2021

legendecas commented Jun 23, 2021 • edited Loading

KevinEady commented Jul 23, 2021 • edited Loading

mhdawson commented Jul 30, 2021

vmoroz commented Aug 2, 2021

mhdawson commented Aug 6, 2021

vmoroz commented Aug 13, 2021

KevinEady commented Aug 20, 2021

mhdawson commented Aug 27, 2021

JckXia commented Aug 27, 2021

mhdawson commented Aug 27, 2021

mhdawson commented Sep 17, 2021

JckXia commented Sep 18, 2021

mhdawson commented Sep 20, 2021

JckXia commented Sep 21, 2021

github-actions bot commented Dec 21, 2021

mhdawson commented Dec 21, 2021

JckXia commented Dec 21, 2021

mhdawson commented Dec 22, 2021

legendecas commented Feb 20, 2021 •

edited

Loading

legendecas commented Jun 23, 2021 •

edited

Loading

KevinEady commented Jul 23, 2021 •

edited

Loading