From cefd4561b72a1b27fd9656181c424e550742f70a Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Mon, 14 Sep 2026 05:24:07 -0500 Subject: [PATCH 1/3] JSC Node-API: reference primitives and coerce property receivers ToJSObject is a reinterpret_cast whose assert is compiled out in RelWithDebInfo, so every entry point that handed a caller value straight to a JSObject* API tripped JavaScriptCore's RELEASE_ASSERT (JSCell::getOwnPropertySlot) on a primitive. The reachable case is any non-object exception: node-addon-api wraps a pending exception with napi_create_reference, and the watchdog's termination exception is the bare string "JavaScript execution terminated.", as is `throw "text"`. - napi_create_reference follows Node: objects keep the sentinel scheme, symbols are held for the life of the reference (the C API has no weak handle for them), other primitives are strong while the count is positive and released at zero (Node-API 10; napi_invalid_arg before). The status of the reference's init is now propagated instead of being dropped, napi_get_reference_value reports NULL instead of leaving *result unset, and napi_reference_unref refuses an already-zero count. - Property get/set/has/delete and napi_get_prototype coerce the receiver with ToObject as Node does; null/undefined report napi_object_expected with the TypeError pending. - napi_wrap/unwrap/add_finalizer, napi_get_value_external, napi_get_array_length, napi_call_function, napi_new_instance and napi_instanceof validate their object/function argument first. --- .../Source/js_native_api_javascriptcore.cc | 170 +++++++++++++++--- 1 file changed, 150 insertions(+), 20 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 5c8583bc..79313859 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -222,6 +222,28 @@ namespace { return napi_set_last_error(env, napi_pending_exception); } + // Property access follows ToObject(), as it does in Node: a primitive receiver is boxed, while + // null and undefined leave their TypeError pending and report napi_object_expected. Handing a + // primitive straight to JSObject* entry points is a RELEASE_ASSERT inside JavaScriptCore. + napi_status ToJSObjectCoerced(napi_env env, napi_value value, JSObjectRef* result) { + CHECK_ARG(env, value); + const JSValueRef js_value{ToJSValue(value)}; + if (JSValueIsObject(env->context, js_value)) { + *result = ToJSObject(env, value); + return napi_ok; + } + + JSValueRef exception{}; + *result = JSValueToObject(env->context, js_value, &exception); + if (*result == nullptr) { + if (exception != nullptr) { + env->last_exception = exception; + } + return napi_set_last_error(env, napi_object_expected); + } + return napi_ok; + } + napi_status napi_set_error_code(napi_env env, napi_value error, napi_value code, @@ -645,6 +667,7 @@ namespace { } static napi_status Wrap(napi_env env, napi_value object, WrapperInfo** result) { + RETURN_STATUS_IF_FALSE(env, JSValueIsObject(env->context, ToJSValue(object)), napi_invalid_arg); WrapperInfo* info{}; CHECK_NAPI(Unwrap(env, object, &info)); if (info == nullptr) { @@ -662,6 +685,7 @@ namespace { } static napi_status Unwrap(napi_env env, napi_value object, WrapperInfo** result) { + RETURN_STATUS_IF_FALSE(env, JSValueIsObject(env->context, ToJSValue(object)), napi_invalid_arg); CHECK_NAPI(NativeInfo::Query(env, ToJSObject(env, object), result)); return napi_ok; } @@ -733,6 +757,10 @@ struct napi_ref__ { napi_status init(napi_env env, napi_value value, uint32_t count) { assert(!_value); + if (!JSValueIsObject(env->context, ToJSValue(value))) { + return init_primitive(env, value, count); + } + _value = value; _count = count; @@ -766,7 +794,7 @@ struct napi_ref__ { } void deinit(napi_env env) { - if (_count != 0) { + if (_protected) { unprotect(env); } @@ -775,8 +803,11 @@ struct napi_ref__ { } void ref(napi_env env) { - assert(_value); - if (_count++ == 0) { + if (_value == nullptr) { + // A primitive released at count zero cannot come back; Node reports a count of zero too. + return; + } + if (_count++ == 0 && !_protected) { protect(env); } } @@ -785,7 +816,13 @@ struct napi_ref__ { assert(_value); assert(_count != 0); if (--_count == 0) { - unprotect(env); + if (_kind == Kind::Object) { + unprotect(env); + } else if (_kind == Kind::Primitive) { + unprotect(env); + _value = nullptr; + } + // A symbol stays protected: see init_primitive. } } @@ -794,7 +831,14 @@ struct napi_ref__ { } napi_status value(napi_env env, napi_value* result) const { - assert(_value); + *result = nullptr; + if (_value == nullptr) { + return napi_ok; + } + if (_kind != Kind::Object) { + *result = _value; + return napi_ok; + } if (env->active_ref_values.find(_value) != env->active_ref_values.end()) { std::uintptr_t objectId{}; // NOTE: This check is needed for the same reason we need a similar check in the init function. @@ -809,19 +853,49 @@ struct napi_ref__ { } private: + enum class Kind { + Object, + Symbol, + Primitive, + }; + + // Primitives carry no identity to hang a sentinel on, and the JavaScriptCore C API has no weak + // handle for them, so they are held strongly while the count is positive and released once it + // reaches zero, which is what Node does for values it cannot hold weakly. Symbols are the + // exception: a weak reference to one has to keep resolving for as long as the symbol is alive, + // and that cannot be observed through the C API, so they stay protected for the life of the + // reference. Before Node-API 10 only symbols were referenceable among the non-objects. + napi_status init_primitive(napi_env env, napi_value value, uint32_t count) { + const bool symbol{JSValueIsSymbol(env->context, ToJSValue(value))}; +#if NAPI_VERSION < 10 + RETURN_STATUS_IF_FALSE(env, symbol, napi_invalid_arg); +#endif + _kind = symbol ? Kind::Symbol : Kind::Primitive; + _count = count; + if (symbol || _count != 0) { + _value = value; + protect(env); + } + return napi_ok; + } + void protect(napi_env env) { _iter = env->strong_refs.insert(env->strong_refs.end(), this); JSValueProtect(env->context, ToJSValue(_value)); + _protected = true; } void unprotect(napi_env env) { env->strong_refs.erase(_iter); JSValueUnprotect(env->context, ToJSValue(_value)); + _protected = false; } napi_value _value{}; uint32_t _count{}; std::uintptr_t _objectId{}; + Kind _kind{Kind::Object}; + bool _protected{false}; std::list::iterator _iter{}; }; @@ -983,13 +1057,16 @@ napi_status napi_set_property(napi_env env, CHECK_ARG(env, key); CHECK_ARG(env, value); + JSObjectRef target{}; + CHECK_NAPI(ToJSObjectCoerced(env, object, &target)); + JSValueRef exception{}; JSString key_str{ToJSString(env, key, &exception)}; CHECK_JSC(env, exception); JSObjectSetProperty( env->context, - ToJSObject(env, object), + target, key_str, ToJSValue(value), kJSPropertyAttributeNone, @@ -1007,13 +1084,16 @@ napi_status napi_has_property(napi_env env, CHECK_ARG(env, result); CHECK_ARG(env, key); + JSObjectRef target{}; + CHECK_NAPI(ToJSObjectCoerced(env, object, &target)); + JSValueRef exception{}; JSString key_str{ToJSString(env, key, &exception)}; CHECK_JSC(env, exception); *result = JSObjectHasProperty( env->context, - ToJSObject(env, object), + target, key_str); return napi_ok; } @@ -1026,13 +1106,16 @@ napi_status napi_get_property(napi_env env, CHECK_ARG(env, key); CHECK_ARG(env, result); + JSObjectRef target{}; + CHECK_NAPI(ToJSObjectCoerced(env, object, &target)); + JSValueRef exception{}; JSString key_str{ToJSString(env, key, &exception)}; CHECK_JSC(env, exception); *result = ToNapi(JSObjectGetProperty( env->context, - ToJSObject(env, object), + target, key_str, &exception)); CHECK_JSC(env, exception); @@ -1047,13 +1130,16 @@ napi_status napi_delete_property(napi_env env, CHECK_ENV(env); CHECK_ARG(env, result); + JSObjectRef target{}; + CHECK_NAPI(ToJSObjectCoerced(env, object, &target)); + JSValueRef exception{}; JSString key_str{ToJSString(env, key, &exception)}; CHECK_JSC(env, exception); *result = JSObjectDeleteProperty( env->context, - ToJSObject(env, object), + target, key_str, &exception); CHECK_JSC(env, exception); @@ -1085,10 +1171,13 @@ napi_status napi_set_named_property(napi_env env, CHECK_ENV(env); CHECK_ARG(env, value); + JSObjectRef target{}; + CHECK_NAPI(ToJSObjectCoerced(env, object, &target)); + JSValueRef exception{}; JSObjectSetProperty( env->context, - ToJSObject(env, object), + target, JSString(utf8name), ToJSValue(value), kJSPropertyAttributeNone, @@ -1103,11 +1192,14 @@ napi_status napi_has_named_property(napi_env env, const char* utf8name, bool* result) { CHECK_ENV(env); - CHECK_ARG(env, object); + CHECK_ARG(env, result); + + JSObjectRef target{}; + CHECK_NAPI(ToJSObjectCoerced(env, object, &target)); *result = JSObjectHasProperty( env->context, - ToJSObject(env, object), + target, JSString(utf8name)); return napi_ok; @@ -1118,12 +1210,15 @@ napi_status napi_get_named_property(napi_env env, const char* utf8name, napi_value* result) { CHECK_ENV(env); - CHECK_ARG(env, object); + CHECK_ARG(env, result); + + JSObjectRef target{}; + CHECK_NAPI(ToJSObjectCoerced(env, object, &target)); JSValueRef exception{}; *result = ToNapi(JSObjectGetProperty( env->context, - ToJSObject(env, object), + target, JSString(utf8name), &exception)); CHECK_JSC(env, exception); @@ -1138,10 +1233,13 @@ napi_status napi_set_element(napi_env env, CHECK_ENV(env); CHECK_ARG(env, value); + JSObjectRef target{}; + CHECK_NAPI(ToJSObjectCoerced(env, object, &target)); + JSValueRef exception{}; JSObjectSetPropertyAtIndex( env->context, - ToJSObject(env, object), + target, index, ToJSValue(value), &exception); @@ -1157,10 +1255,13 @@ napi_status napi_has_element(napi_env env, CHECK_ENV(env); CHECK_ARG(env, result); + JSObjectRef target{}; + CHECK_NAPI(ToJSObjectCoerced(env, object, &target)); + JSValueRef exception{}; JSValueRef value{JSObjectGetPropertyAtIndex( env->context, - ToJSObject(env, object), + target, index, &exception)}; CHECK_JSC(env, exception); @@ -1176,10 +1277,13 @@ napi_status napi_get_element(napi_env env, CHECK_ENV(env); CHECK_ARG(env, result); + JSObjectRef target{}; + CHECK_NAPI(ToJSObjectCoerced(env, object, &target)); + JSValueRef exception{}; *result = ToNapi(JSObjectGetPropertyAtIndex( env->context, - ToJSObject(env, object), + target, index, &exception)); CHECK_JSC(env, exception); @@ -1196,13 +1300,16 @@ napi_status napi_delete_element(napi_env env, napi_value index_value{ToNapi(JSValueMakeNumber(env->context, index))}; + JSObjectRef target{}; + CHECK_NAPI(ToJSObjectCoerced(env, object, &target)); + JSValueRef exception{}; JSString index_str{ToJSString(env, index_value, &exception)}; CHECK_JSC(env, exception); *result = JSObjectDeleteProperty( env->context, - ToJSObject(env, object), + target, index_str, &exception); CHECK_JSC(env, exception); @@ -1295,6 +1402,8 @@ napi_status napi_get_array_length(napi_env env, CHECK_ARG(env, value); CHECK_ARG(env, result); + RETURN_STATUS_IF_FALSE(env, JSValueIsArray(env->context, ToJSValue(value)), napi_array_expected); + JSValueRef exception{}; JSValueRef length = JSObjectGetProperty( env->context, @@ -1330,8 +1439,11 @@ napi_status napi_get_prototype(napi_env env, CHECK_ENV(env); CHECK_ARG(env, result); + JSObjectRef target{}; + CHECK_NAPI(ToJSObjectCoerced(env, object, &target)); + JSValueRef exception{}; - JSObjectRef prototype{JSValueToObject(env->context, JSObjectGetPrototype(env->context, ToJSObject(env, object)), &exception)}; + JSObjectRef prototype{JSValueToObject(env->context, JSObjectGetPrototype(env->context, target), &exception)}; CHECK_JSC(env, exception); *result = ToNapi(prototype); @@ -1643,9 +1755,14 @@ napi_status napi_call_function(napi_env env, napi_value* result) { CHECK_ENV(env); CHECK_ARG(env, recv); + CHECK_ARG(env, func); if (argc > 0) { CHECK_ARG(env, argv); } + // Only object-ness is checked here: a non-callable object surfaces as the TypeError that + // Function.prototype.call raises, whereas some JavaScriptCore builds report JSObjectMakeConstructor + // constructors as not-a-function (see napi_typeof). + RETURN_STATUS_IF_FALSE(env, JSValueIsObject(env->context, ToJSValue(func)), napi_function_expected); JSValueRef exception{}; JSValueRef return_value{JSObjectCallAsFunction( @@ -2038,6 +2155,8 @@ napi_status napi_get_value_external(napi_env env, napi_value value, void** resul CHECK_ARG(env, value); CHECK_ARG(env, result); + RETURN_STATUS_IF_FALSE(env, JSValueIsObject(env->context, ToJSValue(value)), napi_invalid_arg); + ExternalInfo* info = NativeInfo::Get(ToJSObject(env, value)); *result = (info != nullptr && info->Type() == NativeType::External) ? info->Data() : nullptr; return napi_ok; @@ -2057,7 +2176,11 @@ napi_status napi_create_reference(napi_env env, return napi_set_last_error(env, napi_generic_failure); } - ref->init(env, value, initial_refcount); + const napi_status status{ref->init(env, value, initial_refcount)}; + if (status != napi_ok) { + delete ref; + return status; + } *result = ref; return napi_ok; @@ -2098,6 +2221,7 @@ napi_status napi_reference_ref(napi_env env, napi_ref ref, uint32_t* result) { napi_status napi_reference_unref(napi_env env, napi_ref ref, uint32_t* result) { CHECK_ENV(env); CHECK_ARG(env, ref); + RETURN_STATUS_IF_FALSE(env, ref->count() != 0, napi_generic_failure); ref->unref(env); if (result != nullptr) { @@ -2196,6 +2320,10 @@ napi_status napi_new_instance(napi_env env, CHECK_ARG(env, argv); } CHECK_ARG(env, result); + RETURN_STATUS_IF_FALSE(env, + JSValueIsObject(env->context, ToJSValue(constructor)) && + JSObjectIsConstructor(env->context, ToJSObject(env, constructor)), + napi_function_expected); JSValueRef exception{}; *result = ToNapi(JSObjectCallAsConstructor( @@ -2215,7 +2343,9 @@ napi_status napi_instanceof(napi_env env, bool* result) { CHECK_ENV(env); CHECK_ARG(env, object); + CHECK_ARG(env, constructor); CHECK_ARG(env, result); + RETURN_STATUS_IF_FALSE(env, JSValueIsObject(env->context, ToJSValue(constructor)), napi_function_expected); JSValueRef exception{}; *result = JSValueIsInstanceOfConstructor( From 2a07b6afd83211edab48d78806b4f18f5aed8648 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Mon, 14 Sep 2026 05:29:24 -0500 Subject: [PATCH 2/3] Tests: cover primitive exceptions, receivers and references in Node-API PrimitiveExceptionSurvivesNativeCatch runs on every engine: `throw 'plain text'` must surface as a catchable Napi::Error whose Message() is callable, with the runtime still usable afterwards. It killed the process on JavaScriptCore before the previous commit. The coercion and reference semantics are Node's, and the other engines diverge (Chakra references any value, some reject a primitive receiver), so PropertyAccessCoercesPrimitiveReceiver and ReferencesToPrimitivesFollowNode build for JavaScriptCore only, behind a new JSRUNTIMEHOST_NAPI_ENGINE_JAVASCRIPTCORE test define. The reference test pins both the pre-10 napi_invalid_arg branch and the Node-API 10 hold-then-release branch. --- Tests/UnitTests/CMakeLists.txt | 7 ++ Tests/UnitTests/Shared/Shared.cpp | 134 ++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+) diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index f3676d7d..bdeb42e8 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -60,6 +60,13 @@ if(NAPI_JAVASCRIPT_ENGINE STREQUAL "JSI") target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_JSI) endif() +# The JavaScriptCore backend follows Node's ToObject coercion of property receivers and its +# reference rules for primitives; other engines diverge (Chakra references any value, some reject +# a primitive receiver outright), so the tests that pin those semantics build for it only. +if(NAPI_JAVASCRIPT_ENGINE STREQUAL "JavaScriptCore") + target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_JAVASCRIPTCORE) +endif() + target_link_libraries(UnitTests PRIVATE AppRuntime PRIVATE Console diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 1c7e9ff7..ac070566 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -1,5 +1,6 @@ #include "Shared.h" #include +#include #include #include #include @@ -829,6 +830,139 @@ TEST(NodeApi, AdjacentEscapableScopesEscapeIndependently) #endif +// The V8JSI shim surfaces a script `throw` of a primitive as a jsi::JSError rather than a +// Napi::Error, which AppRuntime's dispatch treats as fatal, so this case cannot run there. +#if !defined(JSRUNTIMEHOST_NAPI_ENGINE_JSI) +TEST(NodeApi, PrimitiveExceptionSurvivesNativeCatch) +{ + // Regression: a JavaScript `throw` of a non-object reaches node-addon-api's + // Napi::Error, which wraps the pending exception with napi_create_reference. + // The JavaScriptCore backend handed the primitive to a JSObject* entry point + // (a reinterpret_cast whose assert is compiled out) and tripped a + // RELEASE_ASSERT inside the engine. Its execution-time-limit termination + // exception is such a string, so terminating a busy worker killed the + // process. + Babylon::AppRuntime runtime{}; + + std::promise caught; + std::promise runtimeStillWorks; + + runtime.Dispatch([&caught, &runtimeStillWorks](Napi::Env env) { + bool sawError{false}; + try + { + // Napi::Eval rather than Env::RunScript: the JSI shim has no RunScript and + // Hermes only implements the 3-argument napi_run_script. + Napi::Eval(env, "throw 'plain text';", "primitive-exception.js"); + } + catch (const Napi::Error& error) + { + // Must be callable whether the backend held the string itself or + // wrapped it in an object. + (void)error.Message(); + sawError = true; + } + caught.set_value(sawError); + + const auto sum = Napi::Eval(env, "1 + 1", "primitive-exception.js"); + runtimeStillWorks.set_value(sum.IsNumber() && sum.As().Int32Value() == 2); + }); + + EXPECT_TRUE(caught.get_future().get()); + EXPECT_TRUE(runtimeStillWorks.get_future().get()); +} +#endif + +#if defined(JSRUNTIMEHOST_NAPI_ENGINE_JAVASCRIPTCORE) +TEST(NodeApi, PropertyAccessCoercesPrimitiveReceiver) +{ + // Node coerces the receiver of the property entry points with ToObject: the + // "length" of a string reads through its wrapper, while null and undefined + // report napi_object_expected and leave the TypeError pending. The + // JavaScriptCore backend used to reinterpret the primitive as an object. + Babylon::AppRuntime runtime{}; + + std::promise coerced; + std::promise rejected; + + runtime.Dispatch([&coerced, &rejected](Napi::Env env) { + napi_env nenv{env}; + + napi_value text{Napi::String::New(env, "hello")}; + napi_value length{}; + int32_t value{}; + coerced.set_value( + napi_get_named_property(nenv, text, "length", &length) == napi_ok && + napi_get_value_int32(nenv, length, &value) == napi_ok && + value == 5); + + napi_value undefined{env.Undefined()}; + napi_value ignored{}; + const napi_status status{napi_get_named_property(nenv, undefined, "length", &ignored)}; + bool pending{false}; + napi_is_exception_pending(nenv, &pending); + napi_value exception{}; + napi_get_and_clear_last_exception(nenv, &exception); + rejected.set_value(status == napi_object_expected && pending); + }); + + EXPECT_TRUE(coerced.get_future().get()); + EXPECT_TRUE(rejected.get_future().get()); +} + +TEST(NodeApi, ReferencesToPrimitivesFollowNode) +{ + // Symbols have always been referenceable, and a weak reference keeps + // resolving while the symbol is alive. Other primitives are refused before + // Node-API 10; from 10 on they are held while the count is positive and + // released at zero, when the value reads back as NULL. + Babylon::AppRuntime runtime{}; + + std::promise primitivesHandled; + std::promise symbolResolves; + + runtime.Dispatch([&primitivesHandled, &symbolResolves](Napi::Env env) { + napi_env nenv{env}; + + napi_value text{Napi::String::New(env, "held")}; + napi_ref ref{}; + const napi_status status{napi_create_reference(nenv, text, 1, &ref)}; +#if NAPI_VERSION >= 10 + napi_value value{}; + uint32_t count{1}; + bool ok{status == napi_ok && + napi_get_reference_value(nenv, ref, &value) == napi_ok && + value != nullptr && + Napi::Value(env, value).As().Utf8Value() == "held" && + napi_reference_unref(nenv, ref, &count) == napi_ok && + count == 0}; + value = text; + ok = ok && + napi_get_reference_value(nenv, ref, &value) == napi_ok && + value == nullptr && + napi_reference_unref(nenv, ref, &count) == napi_generic_failure && + napi_delete_reference(nenv, ref) == napi_ok; + primitivesHandled.set_value(ok); +#else + primitivesHandled.set_value(status == napi_invalid_arg); +#endif + + napi_value symbol{Napi::Symbol::New(env, "tag")}; + napi_ref symbolRef{}; + napi_value resolved{}; + symbolResolves.set_value( + napi_create_reference(nenv, symbol, 0, &symbolRef) == napi_ok && + napi_get_reference_value(nenv, symbolRef, &resolved) == napi_ok && + resolved != nullptr && + Napi::Value(env, resolved).StrictEquals(Napi::Value(env, symbol)) && + napi_delete_reference(nenv, symbolRef) == napi_ok); + }); + + EXPECT_TRUE(primitivesHandled.get_future().get()); + EXPECT_TRUE(symbolResolves.get_future().get()); +} +#endif + int RunTests() { testing::InitGoogleTest(); From f8e8152c0441739a83ae67b14a9daa37f13be4e4 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Mon, 14 Sep 2026 07:03:39 -0500 Subject: [PATCH 3/3] =?UTF-8?q?JSC=20Node-API:=20address=20review=20?= =?UTF-8?q?=E2=80=94=20dead=20weak=20targets,=20primitive=20receivers,=20i?= =?UTF-8?q?nstanceof=20constructors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - napi_reference_ref no longer promotes a weak object reference whose target was collected (or whose address a newer object reuses); the liveness check that napi_get_reference_value already performs is shared and the count stays at zero, as in Node. - napi_call_function boxes a primitive receiver (ToObject) and maps undefined/null to the null receiver instead of reinterpreting the value. - napi_instanceof requires a function or constructor for the constructor argument (napi_function_expected), not any object. - The JavaScriptCore-only regression tests also build into the Android UnitTestsJNI target, and the receiver coercion is covered. --- .../Source/js_native_api_javascriptcore.cc | 43 ++++++++++++++----- .../Android/app/src/main/cpp/CMakeLists.txt | 5 +++ Tests/UnitTests/Shared/Shared.cpp | 11 ++++- 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 79313859..6fa43e82 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -807,6 +807,11 @@ struct napi_ref__ { // A primitive released at count zero cannot come back; Node reports a count of zero too. return; } + if (_count == 0 && _kind == Kind::Object && !IsObjectAlive(env)) { + // The weak target has been collected (or its address reused by another object): promoting + // it would protect a stale pointer. Node likewise leaves such a reference at zero. + return; + } if (_count++ == 0 && !_protected) { protect(env); } @@ -839,20 +844,25 @@ struct napi_ref__ { *result = _value; return napi_ok; } - if (env->active_ref_values.find(_value) != env->active_ref_values.end()) { - std::uintptr_t objectId{}; - // NOTE: This check is needed for the same reason we need a similar check in the init function. - // See the comment in init for more details. - CHECK_NAPI(ReferenceInfo::GetObjectId(env, _value, &objectId)); - if (objectId == _objectId) { - *result = _value; - } + if (IsObjectAlive(env)) { + *result = _value; } return napi_ok; } private: + // Whether the weakly tracked object is still the one this reference was created for. The + // sentinel finalizer removes the active entry once the object is collected, and the object id + // check catches an address reused by a newer object before that finalizer ran (see init). + bool IsObjectAlive(napi_env env) const { + if (env->active_ref_values.find(_value) == env->active_ref_values.end()) { + return false; + } + std::uintptr_t objectId{}; + return ReferenceInfo::GetObjectId(env, _value, &objectId) == napi_ok && objectId == _objectId; + } + enum class Kind { Object, Symbol, @@ -1764,11 +1774,18 @@ napi_status napi_call_function(napi_env env, // constructors as not-a-function (see napi_typeof). RETURN_STATUS_IF_FALSE(env, JSValueIsObject(env->context, ToJSValue(func)), napi_function_expected); + // The receiver may be any value. A primitive is boxed as ToObject would; undefined and null + // become the null receiver, for which JavaScriptCore supplies the global object. + JSObjectRef receiver{}; + if (!JSValueIsUndefined(env->context, ToJSValue(recv)) && !JSValueIsNull(env->context, ToJSValue(recv))) { + CHECK_NAPI(ToJSObjectCoerced(env, recv, &receiver)); + } + JSValueRef exception{}; JSValueRef return_value{JSObjectCallAsFunction( env->context, ToJSObject(env, func), - JSValueIsUndefined(env->context, ToJSValue(recv)) ? nullptr : ToJSObject(env, recv), + receiver, argc, ToJSValues(argv), &exception)}; @@ -2345,7 +2362,13 @@ napi_status napi_instanceof(napi_env env, CHECK_ARG(env, object); CHECK_ARG(env, constructor); CHECK_ARG(env, result); - RETURN_STATUS_IF_FALSE(env, JSValueIsObject(env->context, ToJSValue(constructor)), napi_function_expected); + // Either predicate: some JavaScriptCore builds report JSObjectMakeConstructor constructors as + // not-a-function (see napi_typeof). + RETURN_STATUS_IF_FALSE(env, + JSValueIsObject(env->context, ToJSValue(constructor)) && + (JSObjectIsFunction(env->context, ToJSObject(env, constructor)) || + JSObjectIsConstructor(env->context, ToJSObject(env, constructor))), + napi_function_expected); JSValueRef exception{}; *result = JSValueIsInstanceOfConstructor( diff --git a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt index 2e31e0fb..9b1c9482 100644 --- a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt +++ b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt @@ -29,6 +29,11 @@ if(NAPI_JAVASCRIPT_ENGINE STREQUAL "V8") target_link_libraries(UnitTestsJNI PRIVATE AppRuntimeInternal) endif() +# Mirrors Tests/UnitTests/CMakeLists.txt so the JavaScriptCore-only Node-API tests run here too. +if(NAPI_JAVASCRIPT_ENGINE STREQUAL "JavaScriptCore") + target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_JAVASCRIPTCORE) +endif() + target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}") target_compile_definitions(UnitTestsJNI PRIVATE NAPI_JAVASCRIPT_ENGINE="${NAPI_JAVASCRIPT_ENGINE}") target_compile_definitions(UnitTestsJNI PRIVATE ARCANA_TEST_HOOKS) diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index ac070566..90b7bdbf 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -884,8 +884,9 @@ TEST(NodeApi, PropertyAccessCoercesPrimitiveReceiver) std::promise coerced; std::promise rejected; + std::promise calledOnPrimitive; - runtime.Dispatch([&coerced, &rejected](Napi::Env env) { + runtime.Dispatch([&coerced, &rejected, &calledOnPrimitive](Napi::Env env) { napi_env nenv{env}; napi_value text{Napi::String::New(env, "hello")}; @@ -904,10 +905,18 @@ TEST(NodeApi, PropertyAccessCoercesPrimitiveReceiver) napi_value exception{}; napi_get_and_clear_last_exception(nenv, &exception); rejected.set_value(status == napi_object_expected && pending); + + // A primitive receiver is boxed for napi_call_function as well. + napi_value toUpperCase{env.Global().Get("String").As().Get("prototype").As().Get("toUpperCase")}; + napi_value upper{}; + calledOnPrimitive.set_value( + napi_call_function(nenv, text, toUpperCase, 0, nullptr, &upper) == napi_ok && + Napi::Value(env, upper).As().Utf8Value() == "HELLO"); }); EXPECT_TRUE(coerced.get_future().get()); EXPECT_TRUE(rejected.get_future().get()); + EXPECT_TRUE(calledOnPrimitive.get_future().get()); } TEST(NodeApi, ReferencesToPrimitivesFollowNode)