diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 5c8583bc..6fa43e82 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,16 @@ 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 && _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); } } @@ -785,7 +821,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,34 +836,76 @@ struct napi_ref__ { } napi_status value(napi_env env, napi_value* result) const { - assert(_value); - 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; - } + *result = nullptr; + if (_value == nullptr) { + return napi_ok; + } + if (_kind != Kind::Object) { + *result = _value; + return napi_ok; + } + 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, + 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 +1067,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 +1094,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 +1116,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 +1140,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 +1181,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 +1202,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 +1220,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 +1243,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 +1265,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 +1287,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 +1310,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 +1412,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 +1449,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,15 +1765,27 @@ 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); + + // 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)}; @@ -2038,6 +2172,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 +2193,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 +2238,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 +2337,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 +2360,15 @@ napi_status napi_instanceof(napi_env env, bool* result) { CHECK_ENV(env); CHECK_ARG(env, object); + CHECK_ARG(env, constructor); CHECK_ARG(env, result); + // 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/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..90b7bdbf 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,148 @@ 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; + std::promise calledOnPrimitive; + + runtime.Dispatch([&coerced, &rejected, &calledOnPrimitive](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); + + // 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) +{ + // 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();