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

[legendecas]

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]

[gabrielschulhof]

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]

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]

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]

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

1. 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.
2. 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]

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]

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

[vmoroz]

@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]

@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.