diff --git a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp index 8fe33493..e651d202 100644 --- a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp +++ b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp @@ -65,6 +65,8 @@ namespace Babylon Run(env); + // Strong intrinsic references must be released while the Chakra runtime is alive. + Napi::PrepareForRuntimeDisposal(env); ThrowIfFailed(JsSetCurrentContext(JS_INVALID_REFERENCE)); ThrowIfFailed(JsDisposeRuntime(jsRuntime)); diff --git a/Core/Node-API-JSI/Include/napi/napi-inl.h b/Core/Node-API-JSI/Include/napi/napi-inl.h index 14fb745c..47120224 100644 --- a/Core/Node-API-JSI/Include/napi/napi-inl.h +++ b/Core/Node-API-JSI/Include/napi/napi-inl.h @@ -772,7 +772,10 @@ inline bool Object::Delete(uint32_t index) { } inline Array Object::GetPropertyNames() const { - throw std::runtime_error{"TODO"}; + // `jsi::Object::getPropertyNames` returns the enumerable string-keyed + // properties of this object and of its prototype chain, which is exactly what + // `napi_get_property_names` is specified to produce. + return {_env, _object->getPropertyNames(_env->rt)}; } // TODO: not implemented diff --git a/Core/Node-API/CMakeLists.txt b/Core/Node-API/CMakeLists.txt index 5f495695..04977aa5 100644 --- a/Core/Node-API/CMakeLists.txt +++ b/Core/Node-API/CMakeLists.txt @@ -51,13 +51,17 @@ if(NAPI_BUILD_ABI) set(SOURCES ${SOURCES} "Source/env_quickjs.cc" "Source/js_native_api_quickjs.cc" - "Source/js_native_api_quickjs.h") + "Source/js_native_api_quickjs.h" + "Source/js_native_api_shared.cc" + "Source/js_native_api_shared.h") set(LINK_LIBRARIES ${LINK_LIBRARIES} PUBLIC qjs) elseif(NAPI_JAVASCRIPT_ENGINE STREQUAL "Chakra") set(SOURCES ${SOURCES} "Source/env_chakra.cc" "Source/js_native_api_chakra.cc" - "Source/js_native_api_chakra.h") + "Source/js_native_api_chakra.h" + "Source/js_native_api_shared.cc" + "Source/js_native_api_shared.h") set(LINK_LIBRARIES ${LINK_LIBRARIES} INTERFACE "chakrart.lib") @@ -65,7 +69,9 @@ if(NAPI_BUILD_ABI) set(SOURCES ${SOURCES} "Source/env_javascriptcore.cc" "Source/js_native_api_javascriptcore.cc" - "Source/js_native_api_javascriptcore.h") + "Source/js_native_api_javascriptcore.h" + "Source/js_native_api_shared.cc" + "Source/js_native_api_shared.h") if(ANDROID) set(V8_PACKAGE_NAME "jsc-android") diff --git a/Core/Node-API/Include/Engine/Chakra/napi/env.h b/Core/Node-API/Include/Engine/Chakra/napi/env.h index 4245fba1..3f0acf02 100644 --- a/Core/Node-API/Include/Engine/Chakra/napi/env.h +++ b/Core/Node-API/Include/Engine/Chakra/napi/env.h @@ -6,6 +6,8 @@ namespace Napi { Napi::Env Attach(); + void PrepareForRuntimeDisposal(Napi::Env); + void Detach(Napi::Env); Napi::Value Eval(Napi::Env env, const char* source, const char* sourceUrl); diff --git a/Core/Node-API/Source/env_chakra.cc b/Core/Node-API/Source/env_chakra.cc index 0c0be91c..95e52710 100644 --- a/Core/Node-API/Source/env_chakra.cc +++ b/Core/Node-API/Source/env_chakra.cc @@ -1,6 +1,7 @@ #include #include "js_native_api_chakra.h" #include +#include #include namespace @@ -30,6 +31,11 @@ namespace Napi ThrowIfFailed(JsGetPrototype(object, &prototype)); ThrowIfFailed(JsGetPropertyIdFromName(L"hasOwnProperty", &propertyId)); ThrowIfFailed(JsGetProperty(prototype, propertyId, &env_ptr->has_own_property_function)); + if (napi_shared::CapturePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + { + delete env_ptr; + throw std::runtime_error{"Napi::Attach: failed to capture property-name intrinsics"}; + } JsValueRef wrapSymbolDescription; ThrowIfFailed(JsPointerToString(L"BabylonNative_External", 22, &wrapSymbolDescription)); @@ -41,9 +47,19 @@ namespace Napi return {env_ptr}; } + void PrepareForRuntimeDisposal(Env env) + { + napi_env env_ptr{env}; + if (napi_shared::ReleasePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + { + throw std::runtime_error{"Napi::PrepareForRuntimeDisposal: failed to release property-name intrinsics"}; + } + } + void Detach(Env env) { napi_env env_ptr{env}; + PrepareForRuntimeDisposal(env); delete env_ptr; } } diff --git a/Core/Node-API/Source/env_javascriptcore.cc b/Core/Node-API/Source/env_javascriptcore.cc index d97db3a6..75e91015 100644 --- a/Core/Node-API/Source/env_javascriptcore.cc +++ b/Core/Node-API/Source/env_javascriptcore.cc @@ -1,18 +1,28 @@ #include #include #include "js_native_api_javascriptcore.h" +#include namespace Napi { Napi::Env Attach(JSGlobalContextRef context) { napi_env env_ptr{new napi_env__{context}}; + if (napi_shared::CapturePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + { + delete env_ptr; + throw std::runtime_error{"Napi::Attach: failed to capture property-name intrinsics"}; + } return {env_ptr}; } void Detach(Napi::Env env) { napi_env env_ptr{env}; + if (napi_shared::ReleasePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + { + throw std::runtime_error{"Napi::Detach: failed to release property-name intrinsics"}; + } delete env_ptr; } diff --git a/Core/Node-API/Source/env_quickjs.cc b/Core/Node-API/Source/env_quickjs.cc index 6cf36e18..d24eb004 100644 --- a/Core/Node-API/Source/env_quickjs.cc +++ b/Core/Node-API/Source/env_quickjs.cc @@ -56,6 +56,12 @@ namespace Napi } env_ptr->has_own_property_function = hasOwnProperty; + if (napi_shared::CapturePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + { + JS_FreeValue(context, hasOwnProperty); + delete env_ptr; + throw std::runtime_error{"Napi::Attach: failed to capture property-name intrinsics"}; + } return {env_ptr}; } @@ -65,6 +71,10 @@ namespace Napi napi_env env_ptr{env}; if (env_ptr) { + if (napi_shared::ReleasePropertyNameIntrinsics(env_ptr, env_ptr->property_name_intrinsics) != napi_ok) + { + throw std::runtime_error{"Napi::Detach: failed to release property-name intrinsics"}; + } // Release every strong napi_ref still outstanding. This mirrors // the V8 impl (napi_env__::DeleteMe) and is essential on QuickJS: // any surviving strong ref pins a JS value from outside the GC diff --git a/Core/Node-API/Source/js_native_api_chakra.cc b/Core/Node-API/Source/js_native_api_chakra.cc index 6e5d3e72..064ac7e6 100644 --- a/Core/Node-API/Source/js_native_api_chakra.cc +++ b/Core/Node-API/Source/js_native_api_chakra.cc @@ -1,8 +1,11 @@ #include "js_native_api_chakra.h" +#include "js_native_api_shared.h" #include +#include #include #include #include +#include #include #include #include @@ -48,13 +51,14 @@ JsErrorCode JsCopyStringUtf16(_In_ JsValueRef value, _Out_opt_ char16_t* buffer, size_t stringLength; CHECK_JSRT_ERROR_CODE(JsStringToPointer(value, &stringValue, &stringLength)); + const size_t copied = buffer == nullptr ? stringLength : std::min(bufferSize, stringLength); if (length != nullptr) { - *length = stringLength; + *length = copied; } - if (buffer != nullptr) { + if (buffer != nullptr && copied != 0) { static_assert(sizeof(char16_t) == sizeof(wchar_t)); - memcpy_s(buffer, bufferSize, stringValue, stringLength * sizeof(wchar_t)); + std::memcpy(buffer, stringValue, copied * sizeof(char16_t)); } return JsErrorCode::JsNoError; @@ -678,11 +682,22 @@ napi_status napi_get_property_names(napi_env env, napi_value object, napi_value* result) { CHECK_ENV(env); + CHECK_ARG(env, object); CHECK_ARG(env, result); - JsValueRef obj = reinterpret_cast(object); - JsValueRef propertyNames; - CHECK_JSRT(env, JsGetOwnPropertyNames(obj, &propertyNames)); - *result = reinterpret_cast(propertyNames); + + // `JsGetOwnPropertyNames` is own-only and includes non-enumerable properties, + // so use the shared prototype-chain walk instead. It is written against the + // public `napi_*` surface and so cannot reach `napi_set_last_error`; do it + // here, since `CHECK_NAPI` only propagates the status and the preceding call + // inside the walk will have cleared the last error. The success path likewise + // has to clear it, so that a rejection recorded by an earlier call does not + // survive as the last error of a call that succeeded. + const napi_status status{napi_shared::GetEnumerablePropertyNames(env, object, result, env->property_name_intrinsics)}; + if (status != napi_ok) { + return napi_set_last_error(env, status); + } + + napi_clear_last_error(env); return napi_ok; } diff --git a/Core/Node-API/Source/js_native_api_chakra.h b/Core/Node-API/Source/js_native_api_chakra.h index 2dfa59de..84a7f2a7 100644 --- a/Core/Node-API/Source/js_native_api_chakra.h +++ b/Core/Node-API/Source/js_native_api_chakra.h @@ -5,6 +5,7 @@ #include #include +#include "js_native_api_shared.h" #include #include #include @@ -13,6 +14,7 @@ struct napi_env__ { JsSourceContext source_context = JS_SOURCE_CONTEXT_NONE; napi_extended_error_info last_error{ nullptr, nullptr, 0, napi_ok }; JsValueRef has_own_property_function = JS_INVALID_REFERENCE; + napi_shared::PropertyNameIntrinsics property_name_intrinsics{}; JsPropertyIdRef wrap_property_id = JS_INVALID_REFERENCE; diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 5c8583bc..62657877 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -1,4 +1,5 @@ #include "js_native_api_javascriptcore.h" +#include "js_native_api_shared.h" #include #include #include @@ -73,7 +74,7 @@ namespace { size_t length{JSStringGetLength(_string)}; const JSChar* chars{JSStringGetCharactersPtr(_string)}; size_t size{std::min(length, bufsize - 1)}; - std::memcpy(buf, chars, size); + std::memcpy(buf, chars, size * sizeof(JSChar)); buf[size] = 0; if (result != nullptr) { *result = size; @@ -964,15 +965,27 @@ napi_status napi_get_property_names(napi_env env, napi_value object, napi_value* result) { CHECK_ENV(env); + CHECK_ARG(env, object); CHECK_ARG(env, result); - napi_value global{}, object_ctor{}, function{}; - CHECK_NAPI(napi_get_global(env, &global)); - CHECK_NAPI(napi_get_named_property(env, global, "Object", &object_ctor)); - CHECK_NAPI(napi_get_named_property(env, object_ctor, "getOwnPropertyNames", &function)); - CHECK_NAPI(napi_call_function(env, object_ctor, function, 0, nullptr, result)); + // JavaScriptCore's `JSObjectCopyPropertyNames` walks the prototype chain, but + // it does not apply the shadowing rule: `JSObject::getPropertyNames` calls + // `getOwnPropertyNames` per level with `DontEnumPropertiesMode::Exclude`, so + // a non-enumerable own property is never added to the array and so cannot + // suppress a same-named enumerable property further up the chain. The + // inherited name is reported where `for...in` correctly omits it. Use the + // shared prototype-chain walk instead. It is written against the public + // `napi_*` surface and so cannot reach `napi_set_last_error`; do it here, + // since `CHECK_NAPI` only propagates the status and the preceding call inside + // the walk will have cleared the last error. The success path likewise has to + // clear it, so that a rejection recorded by an earlier call does not survive + // as the last error of a call that succeeded. + const napi_status status{napi_shared::GetEnumerablePropertyNames(env, object, result, env->property_name_intrinsics)}; + if (status != napi_ok) { + return napi_set_last_error(env, status); + } - return napi_ok; + return napi_clear_last_error(env); } napi_status napi_set_property(napi_env env, @@ -1328,14 +1341,33 @@ napi_status napi_get_prototype(napi_env env, napi_value object, napi_value* result) { CHECK_ENV(env); + CHECK_ARG(env, object); CHECK_ARG(env, result); + // `JSObjectGetPrototype` already yields a JSValueRef, and that value is + // `null` at the top of a prototype chain. Running it through + // `JSValueToObject` threw "TypeError: null is not an object" there instead of + // reporting the end of the chain, which made the chain impossible to walk. + // V8 likewise returns the raw prototype value. + // + // The conversion belongs on the argument rather than the result. V8 coerces + // there (`CHECK_TO_OBJECT`), so a primitive yields its wrapper's prototype + // and only `null`/`undefined` are rejected. Passing the argument straight to + // `ToJSObject` instead would assert in debug and, in release, reinterpret a + // non-object `JSValueRef` as a `JSObjectRef` -- so a primitive was undefined + // behaviour rather than a status. + const JSValueRef value{ToJSValue(object)}; + if (JSValueIsNull(env->context, value) || JSValueIsUndefined(env->context, value)) { + return napi_set_last_error(env, napi_object_expected); + } + JSValueRef exception{}; - JSObjectRef prototype{JSValueToObject(env->context, JSObjectGetPrototype(env->context, ToJSObject(env, object)), &exception)}; + const JSObjectRef self{JSValueToObject(env->context, value, &exception)}; CHECK_JSC(env, exception); - *result = ToNapi(prototype); - return napi_ok; + *result = ToNapi(JSObjectGetPrototype(env->context, self)); + + return napi_clear_last_error(env); } napi_status napi_create_object(napi_env env, napi_value* result) { diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.h b/Core/Node-API/Source/js_native_api_javascriptcore.h index 8d8dbd02..7cc4f38a 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.h +++ b/Core/Node-API/Source/js_native_api_javascriptcore.h @@ -2,6 +2,7 @@ #include #include +#include "js_native_api_shared.h" #include #include #include @@ -13,6 +14,7 @@ struct napi_env__ { JSGlobalContextRef context{}; JSValueRef last_exception{}; napi_extended_error_info last_error{nullptr, nullptr, 0, napi_ok}; + napi_shared::PropertyNameIntrinsics property_name_intrinsics{}; std::unordered_map active_ref_values{}; std::list strong_refs{}; diff --git a/Core/Node-API/Source/js_native_api_quickjs.cc b/Core/Node-API/Source/js_native_api_quickjs.cc index 6db7f662..723295ce 100644 --- a/Core/Node-API/Source/js_native_api_quickjs.cc +++ b/Core/Node-API/Source/js_native_api_quickjs.cc @@ -1,4 +1,5 @@ #include "js_native_api_quickjs.h" +#include "js_native_api_shared.h" #include #if defined(__clang__) #pragma clang diagnostic push @@ -1394,27 +1395,19 @@ napi_status napi_get_property_names(napi_env env, napi_value object, napi_value* CHECK_ENV(env); CHECK_ARG(env, object); CHECK_ARG(env, result); - - JSValue jsObject = ToJSValue(object); - - JSPropertyEnum* ptab; - uint32_t plen; - - if (JS_GetOwnPropertyNames(env->context, &ptab, &plen, jsObject, - JS_GPN_STRING_MASK | JS_GPN_ENUM_ONLY) < 0) { - return napi_set_last_error(env, napi_generic_failure); - } - - JSValue arr = JS_NewArray(env->context); - - for (uint32_t i = 0; i < plen; i++) { - JSValue name = JS_AtomToString(env->context, ptab[i].atom); - JS_SetPropertyUint32(env->context, arr, i, name); + + // `JS_GetOwnPropertyNames` is own-only, so use the shared prototype-chain + // walk instead. It is written against the public `napi_*` surface and so + // cannot reach `napi_set_last_error`; do it here, since `CHECK_NAPI` only + // propagates the status and the preceding call inside the walk will have + // cleared the last error. The success path likewise has to clear it, so that + // a rejection recorded by an earlier call does not survive as the last error + // of a call that succeeded. + const napi_status status{napi_shared::GetEnumerablePropertyNames(env, object, result, env->property_name_intrinsics)}; + if (status != napi_ok) { + return napi_set_last_error(env, status); } - - JS_FreePropertyEnum(env->context, ptab, plen); - - *result = FromJSValue(env, arr); + napi_clear_last_error(env); return napi_ok; } diff --git a/Core/Node-API/Source/js_native_api_quickjs.h b/Core/Node-API/Source/js_native_api_quickjs.h index 38f9118f..e1bc84b6 100644 --- a/Core/Node-API/Source/js_native_api_quickjs.h +++ b/Core/Node-API/Source/js_native_api_quickjs.h @@ -9,6 +9,7 @@ #pragma clang diagnostic pop #endif #include +#include "js_native_api_shared.h" #include #include #include @@ -27,6 +28,7 @@ struct napi_env__ { JSContext* current_context = nullptr; napi_extended_error_info last_error{ nullptr, nullptr, 0, napi_ok }; JSValue has_own_property_function = JS_UNDEFINED; + napi_shared::PropertyNameIntrinsics property_name_intrinsics{}; const std::thread::id thread_id{std::this_thread::get_id()}; diff --git a/Core/Node-API/Source/js_native_api_shared.cc b/Core/Node-API/Source/js_native_api_shared.cc new file mode 100644 index 00000000..9d74b9b6 --- /dev/null +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -0,0 +1,194 @@ +#include "js_native_api_shared.h" + +#include + +#include +#include +#include +#include + +namespace napi_shared { + namespace { + #define RETURN_IF_NOT_OK(expression) \ + do { \ + const napi_status status__{(expression)}; \ + if (status__ != napi_ok) { \ + return status__; \ + } \ + } while (0) + + napi_status IsObjectLike(napi_env env, napi_value value, bool& result) { + napi_valuetype type{}; + RETURN_IF_NOT_OK(napi_typeof(env, value, &type)); + result = (type == napi_object || type == napi_function || type == napi_external); + return napi_ok; + } + + // Whether `value` is strictly equal to something already in `seen`. + napi_status Contains(napi_env env, const std::vector& seen, napi_value value, bool& result) { + for (const napi_value candidate : seen) { + bool equal{}; + RETURN_IF_NOT_OK(napi_strict_equals(env, candidate, value, &equal)); + if (equal) { + result = true; + return napi_ok; + } + } + + result = false; + return napi_ok; + } + } + + napi_status ReleasePropertyNameIntrinsics(napi_env env, PropertyNameIntrinsics& intrinsics) { + napi_status firstError{napi_ok}; + for (napi_ref* ref : {&intrinsics.object_constructor, &intrinsics.own_names, + &intrinsics.own_descriptor, &intrinsics.prototype}) { + if (*ref != nullptr) { + const napi_status status{napi_delete_reference(env, *ref)}; + if (status == napi_ok) { + *ref = nullptr; + } else if (firstError == napi_ok) { + firstError = status; + } + } + } + return firstError; + } + + napi_status CapturePropertyNameIntrinsics(napi_env env, PropertyNameIntrinsics& intrinsics) { + napi_value global{}; + napi_value functions[4]{}; + RETURN_IF_NOT_OK(napi_get_global(env, &global)); + RETURN_IF_NOT_OK(napi_get_named_property(env, global, "Object", &functions[0])); + RETURN_IF_NOT_OK(napi_get_named_property(env, functions[0], "getOwnPropertyNames", &functions[1])); + RETURN_IF_NOT_OK(napi_get_named_property(env, functions[0], "getOwnPropertyDescriptor", &functions[2])); + RETURN_IF_NOT_OK(napi_get_named_property(env, functions[0], "getPrototypeOf", &functions[3])); + for (const napi_value function : functions) { + napi_valuetype type{}; + RETURN_IF_NOT_OK(napi_typeof(env, function, &type)); + if (type != napi_function) { + return napi_function_expected; + } + } + + napi_ref* refs[]{&intrinsics.object_constructor, &intrinsics.own_names, + &intrinsics.own_descriptor, &intrinsics.prototype}; + for (size_t index = 0; index < 4; ++index) { + const napi_status status{napi_create_reference(env, functions[index], 1, refs[index])}; + if (status != napi_ok) { + const napi_status cleanupStatus{ReleasePropertyNameIntrinsics(env, intrinsics)}; + return cleanupStatus == napi_ok ? status : cleanupStatus; + } + } + return napi_ok; + } + + napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result, + const PropertyNameIntrinsics& intrinsics) { + // Take one own-key snapshot per prototype level, then inspect each + // descriptor to determine enumerability. This matches `for...in` for + // proxies, whose `ownKeys` trap must not be invoked twice at one level. + napi_value global{}; + napi_value objectConstructor{}; + napi_value getOwnPropertyNames{}; + napi_value getOwnPropertyDescriptor{}; + napi_value getPrototypeOf{}; + RETURN_IF_NOT_OK(napi_get_global(env, &global)); + RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.object_constructor, &objectConstructor)); + RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.own_names, &getOwnPropertyNames)); + RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.own_descriptor, &getOwnPropertyDescriptor)); + RETURN_IF_NOT_OK(napi_get_reference_value(env, intrinsics.prototype, &getPrototypeOf)); + + napi_value names{}; + RETURN_IF_NOT_OK(napi_create_array(env, &names)); + uint32_t nameCount{}; + + std::unordered_set shadowed{}; + std::vector visited{}; + + // `ToObject` is what the specification (and the V8 implementation) applies + // to the argument, so a primitive is wrapped and its properties reported. + // `null` and `undefined` have no wrapper, and V8 reports that as + // `napi_object_expected`; check explicitly rather than relying on + // `napi_coerce_to_object`, whose behaviour for those two values differs + // between engines (QuickJS yields an empty object, JavaScriptCore throws). + napi_valuetype type{}; + RETURN_IF_NOT_OK(napi_typeof(env, object, &type)); + if (type == napi_null || type == napi_undefined) { + return napi_object_expected; + } + + napi_value current{object}; + if (type != napi_object && type != napi_function && type != napi_external) { + RETURN_IF_NOT_OK(napi_call_function(env, global, objectConstructor, 1, &object, ¤t)); + } + + while (true) { + bool isObjectLike{}; + RETURN_IF_NOT_OK(IsObjectLike(env, current, isObjectLike)); + if (!isObjectLike) { + break; + } + + bool alreadyVisited{}; + RETURN_IF_NOT_OK(Contains(env, visited, current, alreadyVisited)); + if (alreadyVisited) { + RETURN_IF_NOT_OK(napi_throw_range_error(env, nullptr, "Cyclic prototype chain")); + return napi_pending_exception; + } + visited.push_back(current); + + napi_value ownNames{}; + RETURN_IF_NOT_OK(napi_call_function(env, global, getOwnPropertyNames, 1, ¤t, &ownNames)); + + uint32_t ownNameCount{}; + RETURN_IF_NOT_OK(napi_get_array_length(env, ownNames, &ownNameCount)); + for (uint32_t index = 0; index < ownNameCount; ++index) { + napi_value name{}; + RETURN_IF_NOT_OK(napi_get_element(env, ownNames, index, &name)); + + size_t length{}; + RETURN_IF_NOT_OK(napi_get_value_string_utf16(env, name, nullptr, 0, &length)); + std::u16string key(length + 1, u'\0'); + size_t copied{}; + RETURN_IF_NOT_OK(napi_get_value_string_utf16(env, name, key.data(), key.size(), &copied)); + key.resize(copied); + if (shadowed.find(key) != shadowed.end()) { + continue; + } + + napi_value descriptorArgs[]{current, name}; + napi_value descriptor{}; + RETURN_IF_NOT_OK(napi_call_function( + env, global, getOwnPropertyDescriptor, 2, descriptorArgs, &descriptor)); + + napi_valuetype descriptorType{}; + RETURN_IF_NOT_OK(napi_typeof(env, descriptor, &descriptorType)); + if (descriptorType == napi_undefined) { + continue; + } + + shadowed.insert(std::move(key)); + + napi_value enumerableValue{}; + RETURN_IF_NOT_OK(napi_get_named_property(env, descriptor, "enumerable", &enumerableValue)); + bool enumerable{}; + RETURN_IF_NOT_OK(napi_get_value_bool(env, enumerableValue, &enumerable)); + if (enumerable) { + RETURN_IF_NOT_OK(napi_set_element(env, names, nameCount++, name)); + } + } + + napi_value next{}; + RETURN_IF_NOT_OK(napi_call_function(env, global, getPrototypeOf, 1, ¤t, &next)); + + current = next; + } + + *result = names; + return napi_ok; + } + + #undef RETURN_IF_NOT_OK +} diff --git a/Core/Node-API/Source/js_native_api_shared.h b/Core/Node-API/Source/js_native_api_shared.h new file mode 100644 index 00000000..eb84be21 --- /dev/null +++ b/Core/Node-API/Source/js_native_api_shared.h @@ -0,0 +1,35 @@ +#pragma once + +#include + +// Engine-agnostic pieces of the Node-API surface, implemented purely in terms +// of the public `napi_*` entry points so that every backend behaves the same. +// Backends whose engine offers a faithful native equivalent should keep using +// it; these helpers exist for the ones that do not. +namespace napi_shared { + // Strong references keep these built-ins independent of later changes to + // globalThis.Object and its static methods. + struct PropertyNameIntrinsics { + napi_ref object_constructor{}; + napi_ref own_names{}; + napi_ref own_descriptor{}; + napi_ref prototype{}; + }; + + napi_status CapturePropertyNameIntrinsics(napi_env env, PropertyNameIntrinsics& intrinsics); + napi_status ReleasePropertyNameIntrinsics(napi_env env, PropertyNameIntrinsics& intrinsics); + + // Implements `napi_get_property_names` semantics: the names of all + // enumerable string-keyed properties of `object` and of its prototype chain, + // as an array of strings, matching a `for...in` enumeration. + // + // V8 gets this from a single `GetPropertyNames` call configured with + // `kIncludePrototypes | ONLY_ENUMERABLE | SKIP_SYMBOLS`. JavaScriptCore, + // Chakra and QuickJS have no equivalent, so this walks the prototype + // chain explicitly. See https://github.com/BabylonJS/JsRuntimeHost/issues/216. + // + // Primitives are wrapped with the captured Object constructor, matching + // V8's `CHECK_TO_OBJECT`. Callers validate `env` and `result`. + napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result, + const PropertyNameIntrinsics& intrinsics); +} diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 23b8e4e5..9cae6e35 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -8,6 +8,8 @@ Mocha.reporter('spec'); declare const hostPlatform: string; declare const hostEngine: string; declare const setExitCode: (code: number) => void; +declare const napiGetPropertyNames: (object: any) => string[]; +declare const napiGetPropertyNamesRaw: (object: any) => string[]; describe("AbortController", function () { @@ -1696,6 +1698,274 @@ describe("napi class prototype isolation (#172)", function () { }); +describe("napi_get_property_names (#216)", function () { + // Regression coverage for #216: napi_get_property_names must report the + // enumerable string-keyed properties of an object *and its prototype + // chain*, i.e. exactly what `for...in` visits. JavaScriptCore used to throw + // outright, while Chakra and QuickJS only reported own properties + // (Chakra additionally reported non-enumerable ones). + // Chakra does not define globalThis; a non-strict function returns the global object. + const globalObject = Function("return this")(); + + function forIn(object: any): string[] { + const keys: string[] = []; + for (const key in object) { + keys.push(key); + } + return keys; + } + + // Some engines' own `for...in` does not implement the shadowing rule that + // the tests below rely on -- Hermes reports an inherited property that a + // non-enumerable own property is supposed to hide. Probe for that rather + // than name engines, and only use `for...in` as an oracle where it holds. + // https://github.com/BabylonJS/JsRuntimeHost/issues/219 tracks the gap. + const shadowingProbe = Object.create({ probe: 1 }); + Object.defineProperty(shadowingProbe, "probe", { value: 2, enumerable: false }); + const forInHonoursShadowing = forIn(shadowingProbe).length === 0; + + it("returns own enumerable string keys", function () { + expect(napiGetPropertyNames({ a: 1, b: 2 })).to.deep.equal(["a", "b"]); + }); + + it("includes enumerable properties inherited from the prototype chain", function () { + const object = Object.create({ inherited: 1 }); + object.own = 2; + expect(napiGetPropertyNames(object)).to.deep.equal(["own", "inherited"]); + }); + + it("excludes non-enumerable own properties", function () { + const object = { visible: 1 }; + Object.defineProperty(object, "hidden", { value: 2, enumerable: false }); + expect(napiGetPropertyNames(object)).to.deep.equal(["visible"]); + }); + + it("excludes symbol keys", function () { + const object: any = { a: 1 }; + object[Symbol("s")] = 2; + expect(napiGetPropertyNames(object)).to.deep.equal(["a"]); + }); + + it("reports a shadowed inherited property only once", function () { + const object = Object.create({ shared: 1 }); + object.shared = 2; + expect(napiGetPropertyNames(object)).to.deep.equal(["shared"]); + }); + + it("distinguishes property names containing different lone surrogates", function () { + const object = Object.create({ ["\udc00"]: 1 }); + object["\ud800"] = 2; + expect(napiGetPropertyNames(object)).to.deep.equal(["\ud800", "\udc00"]); + }); + + it("takes one ownKeys snapshot per prototype level", function () { + let ownKeysCalls = 0; + const object = new Proxy({ own: 1 }, { + ownKeys(target) { + ++ownKeysCalls; + return Reflect.ownKeys(target); + }, + }); + expect(napiGetPropertyNames(object)).to.deep.equal(["own"]); + expect(ownKeysCalls).to.equal(1); + }); + + it("does not depend on mutable Object globals", function () { + const objectConstructor = Object; + const ownNames = Object.getOwnPropertyNames; + const ownDescriptor = Object.getOwnPropertyDescriptor; + const prototype = Object.getPrototypeOf; + const object = Object.create({ inherited: 1 }); + object.own = 2; + let names: string[] | undefined; + + try { + Object.getOwnPropertyNames = () => ["forged"]; + Object.getOwnPropertyDescriptor = () => ({ enumerable: false }); + Object.getPrototypeOf = () => null; + Reflect.set(globalObject, "Object", {}); + names = napiGetPropertyNames(object); + } finally { + Reflect.set(globalObject, "Object", objectConstructor); + Object.getOwnPropertyNames = ownNames; + Object.getOwnPropertyDescriptor = ownDescriptor; + Object.getPrototypeOf = prototype; + } + + expect(names).to.deep.equal(["own", "inherited"]); + }); + + it("deduplicates large sets of enumerable and non-enumerable names", function () { + const object = Object.create({ inherited: 1 }); + const expected: string[] = []; + for (let index = 0; index < 1024; ++index) { + const name = `key${index}`; + const enumerable = index % 2 === 0; + Object.defineProperty(object, name, { value: index, enumerable }); + if (enumerable) { + expected.push(name); + } + } + expected.push("inherited"); + expect(napiGetPropertyNames(object)).to.deep.equal(expected); + }); + + it("omits an inherited property shadowed by a non-enumerable own property", function () { + const object = Object.create({ shared: 1 }); + Object.defineProperty(object, "shared", { value: 2, enumerable: false }); + expect(napiGetPropertyNames(object)).to.deep.equal([]); + }); + + it("excludes class methods, which are non-enumerable", function () { + class Point { + x: number; + y: number; + constructor() { + this.x = 1; + this.y = 2; + } + length(): number { + return 0; + } + } + expect(napiGetPropertyNames(new Point())).to.deep.equal(["x", "y"]); + }); + + it("reports array indices as strings and omits the non-enumerable length", function () { + expect(napiGetPropertyNames(["a", "b"])).to.deep.equal(["0", "1"]); + }); + + it("matches for...in over a multi-level prototype chain", function () { + const grandparent = { deep: 0 }; + const parent: any = Object.create(grandparent); + parent.middle = 1; + Object.defineProperty(parent, "hiddenMiddle", { value: 2, enumerable: false }); + + const object: any = Object.create(parent); + object.own = 3; + object[Symbol("s")] = 4; + Object.defineProperty(object, "deep", { value: 5, enumerable: false }); + + expect(napiGetPropertyNames(object)).to.deep.equal(["own", "middle"]); + if (forInHonoursShadowing) { + expect(napiGetPropertyNames(object)).to.deep.equal(forIn(object)); + } + }); + + // `Napi::Object::GetPropertyNames` requires an object, so the coercion the + // C entry point performs on its argument is only reachable through + // `napiGetPropertyNamesRaw`. That global is undefined on the JSI backend, + // which implements the `Napi::` C++ surface directly on JSI and has no C + // Node-API to call. + const describeCoercion = typeof napiGetPropertyNamesRaw === "function" ? describe : describe.skip; + + describeCoercion("argument coercion", function () { + // Hermes' Node-API is not implemented in this repository -- it comes from + // the Hermes dependency itself -- and it rejects primitives outright + // rather than applying ToObject. Hermes is an experimental engine here, + // so assert the specified behaviour everywhere it is ours to control and + // skip the wrapping cases on Hermes rather than weakening them. The + // upstream gap is tracked by + // https://github.com/BabylonJS/JsRuntimeHost/issues/219. + const describePrimitives = hostEngine === "Hermes" ? describe.skip : describe; + + if (hostEngine === "Hermes") { + it("rejects primitives instead of applying ToObject (upstream gap)", function () { + expect(() => napiGetPropertyNamesRaw("ab")).to.throw(); + }); + } + + describePrimitives("of a primitive", function () { + it("wraps a string primitive and reports its indices", function () { + expect(napiGetPropertyNamesRaw("ab")).to.deep.equal(["0", "1"]); + }); + + it("wraps a primitive after the global Object binding is replaced", function () { + const objectConstructor = Object; + let names: string[] | undefined; + try { + Reflect.set(globalObject, "Object", {}); + names = napiGetPropertyNamesRaw("ab"); + } finally { + Reflect.set(globalObject, "Object", objectConstructor); + } + expect(names).to.deep.equal(["0", "1"]); + }); + + it("wraps a number primitive, which has no enumerable properties", function () { + expect(napiGetPropertyNamesRaw(42)).to.deep.equal([]); + }); + + it("wraps a boolean primitive, which has no enumerable properties", function () { + expect(napiGetPropertyNamesRaw(true)).to.deep.equal([]); + }); + }); + + it("still reports the prototype chain of a real object", function () { + const object = Object.create({ inherited: 1 }); + object.own = 2; + expect(napiGetPropertyNamesRaw(object)).to.deep.equal(["own", "inherited"]); + }); + + it("fails for null", function () { + expect(() => napiGetPropertyNamesRaw(null)).to.throw(); + }); + + it("fails for undefined", function () { + expect(() => napiGetPropertyNamesRaw(undefined)).to.throw(); + }); + }); + + // A `getPrototypeOf` Proxy trap may put an object back onto its own + // prototype chain -- no specification invariant forbids it -- which makes + // the chain cyclic. V8 collects keys recursively and so terminates with a + // `RangeError`; the shared walk is iterative and used to spin forever, + // burning CPU in native code with no way for script or the test timeout to + // interrupt it. + // + // These only apply to the backends that use the shared walk. V8 and Hermes + // bring their own key collection, and the JSI adapter forwards to + // `jsi::Object::getPropertyNames`, so their behaviour here is not ours to + // specify. + const usesSharedWalk = hostEngine === "Chakra" || hostEngine === "QuickJS" || hostEngine === "JavaScriptCore"; + const describeCycles = usesSharedWalk ? describe : describe.skip; + + describeCycles("cyclic prototype chains", function () { + this.timeout(5000); + + it("throws for a proxy that is its own prototype", function () { + let object: any; + object = new Proxy({ own: 1 }, { getPrototypeOf() { return object; } }); + expect(() => napiGetPropertyNames(object)).to.throw(RangeError); + }); + + it("throws for a two-object prototype cycle", function () { + let first: any; + let second: any; + first = new Proxy({ a: 1 }, { getPrototypeOf() { return second; } }); + second = new Proxy({ b: 2 }, { getPrototypeOf() { return first; } }); + expect(() => napiGetPropertyNames(first)).to.throw(RangeError); + }); + + it("still reports a long acyclic chain in full", function () { + const base = { deep: 1 }; + const middle = Object.create(base); + middle.middle = 2; + const leaf = Object.create(middle); + leaf.own = 3; + expect(napiGetPropertyNames(leaf)).to.deep.equal(["own", "middle", "deep"]); + }); + + it("preserves an exception from a throwing getPrototypeOf trap", function () { + const object = new Proxy({ own: 1 }, { + getPrototypeOf() { throw new Error("trap"); }, + }); + expect(() => napiGetPropertyNames(object)).to.throw("trap"); + }); + }); +}); + + describe("Performance", function () { this.timeout(1000); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 1c7e9ff7..df48ab4d 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -20,6 +20,8 @@ #include #include #include +#include +#include #include namespace @@ -101,6 +103,53 @@ TEST(JavaScript, All) env.Global().Set("hostPlatform", Napi::Value::From(env, JSRUNTIMEHOST_PLATFORM)); env.Global().Set("hostEngine", Napi::Value::From(env, NAPI_JAVASCRIPT_ENGINE)); + + // Exposes napi_get_property_names, via its C++ wrapper, so that the + // script tests can compare it against `for...in`. See + // https://github.com/BabylonJS/JsRuntimeHost/issues/216. + auto getPropertyNamesCallback = Napi::Function::New( + env, [](const Napi::CallbackInfo& info) -> Napi::Value { + return info[0].As().GetPropertyNames(); + }, + "napiGetPropertyNames"); + env.Global().Set("napiGetPropertyNames", getPropertyNamesCallback); + +#ifndef JSRUNTIMEHOST_NAPI_ENGINE_JSI + // `Napi::Object::GetPropertyNames` can only be reached through an + // already-constructed `Napi::Object`, so it cannot exercise the + // `ToObject` coercion that `napi_get_property_names` performs on its + // argument. Expose the C entry point directly for those cases. The JSI + // backend implements the `Napi::` C++ surface straight on top of JSI and + // has no C Node-API at all, so this global is left undefined there and + // the coercion tests skip themselves. + auto getPropertyNamesRawCallback = Napi::Function::New( + env, [](const Napi::CallbackInfo& info) -> Napi::Value { + napi_env rawEnv{info.Env()}; + napi_value result{}; + const napi_status status{napi_get_property_names(rawEnv, info[0], &result)}; + if (status != napi_ok) + { + // A failed call may or may not have left a JavaScript + // exception pending; surface either as a thrown error so + // that the script tests can assert on it uniformly. + bool isExceptionPending{}; + if (napi_is_exception_pending(rawEnv, &isExceptionPending) == napi_ok && isExceptionPending) + { + napi_value error{}; + if (napi_get_and_clear_last_exception(rawEnv, &error) == napi_ok) + { + throw Napi::Error{info.Env(), error}; + } + } + + throw Napi::Error::New(info.Env(), "napi_get_property_names failed with status " + std::to_string(status)); + } + + return Napi::Value{rawEnv, result}; + }, + "napiGetPropertyNamesRaw"); + env.Global().Set("napiGetPropertyNamesRaw", getPropertyNamesRawCallback); +#endif }); Babylon::ScriptLoader loader{runtime}; @@ -457,6 +506,39 @@ TEST(NodeApi, GetValueStringUtf16HandlesZeroBufsize) EXPECT_TRUE(normalWorks.get_future().get()); } +TEST(NodeApi, GetValueStringUtf16CopiesAndTruncates) +{ + Babylon::AppRuntime runtime{}; + std::promise copiedCorrectly; + + runtime.Dispatch([&copiedCorrectly](Napi::Env env) { + napi_env nenv{env}; + const char16_t input[]{u'a', static_cast(0xD800), u'b'}; + napi_value value{}; + if (napi_create_string_utf16(nenv, input, 3, &value) != napi_ok) + { + copiedCorrectly.set_value(false); + return; + } + + size_t length{}; + char16_t truncated[2]{u'?', u'?'}; + size_t truncatedLength{}; + char16_t complete[4]{}; + size_t completeLength{}; + const bool correct = + napi_get_value_string_utf16(nenv, value, nullptr, 0, &length) == napi_ok && length == 3 && + napi_get_value_string_utf16(nenv, value, truncated, 2, &truncatedLength) == napi_ok && + truncatedLength == 1 && truncated[0] == u'a' && truncated[1] == u'\0' && + napi_get_value_string_utf16(nenv, value, complete, 4, &completeLength) == napi_ok && + completeLength == 3 && complete[0] == u'a' && complete[1] == input[1] && + complete[2] == u'b' && complete[3] == u'\0'; + copiedCorrectly.set_value(correct); + }); + + EXPECT_TRUE(copiedCorrectly.get_future().get()); +} + // Closes an escapable handle scope however the test leaves it. Without this, a // failing assertion returns with the scope still open, the enclosing // Napi::HandleScope then fails to close, and Napi::Error::Fatal throws out of its @@ -829,6 +911,69 @@ TEST(NodeApi, AdjacentEscapableScopesEscapeIndependently) #endif +// The V8JSI shim has no C Node-API at all, so this only builds elsewhere. +#if !defined(JSRUNTIMEHOST_NAPI_ENGINE_JSI) +TEST(NodeApi, GetPropertyNamesReportsLastErrorConsistently) +{ + // This asserts the contract for the three backends this change touches -- + // the ones that share the prototype walk. V8's napi_get_property_names is + // vendored upstream Node code with different behaviour on both counts: a + // rejected call leaves a pending exception, so a following call reports + // napi_pending_exception rather than napi_object_expected, and its success + // path returns bare napi_ok through GET_RETURN_STATUS without clearing. + // Both are upstream's to define, not ours to redefine here. Hermes and the + // JSI adapter likewise supply their own. + const std::string_view engine{NAPI_JAVASCRIPT_ENGINE}; + if (engine != "Chakra" && engine != "QuickJS" && engine != "JavaScriptCore") + { + GTEST_SKIP() << engine << " supplies its own napi_get_property_names."; + } + + // Regression: napi_get_property_names rejects null/undefined with + // napi_object_expected, but the shared walk is written against the public + // napi_* surface and cannot reach napi_set_last_error. CHECK_NAPI only + // propagates the status, and the napi_typeof performed just before the + // rejection clears the last error on success, so the returned status and + // napi_get_last_error_info() disagreed: the caller saw + // napi_object_expected while the recorded error code was still napi_ok. + // Node-API's contract is that the two agree. + Babylon::AppRuntime runtime{}; + + std::promise nullConsistent; + std::promise undefinedConsistent; + std::promise successClears; + + runtime.Dispatch([&nullConsistent, &undefinedConsistent, &successClears](Napi::Env env) { + napi_env nenv{env}; + + const auto check = [nenv](napi_value value) { + napi_value names{nullptr}; + const napi_status status{napi_get_property_names(nenv, value, &names)}; + + const napi_extended_error_info* info{nullptr}; + napi_get_last_error_info(nenv, &info); + + return status == napi_object_expected && info != nullptr && info->error_code == status; + }; + + nullConsistent.set_value(check(napi_value{env.Null()})); + undefinedConsistent.set_value(check(napi_value{env.Undefined()})); + + // The success path must leave no stale error behind. + napi_value names{nullptr}; + const napi_status status{napi_get_property_names(nenv, napi_value{Napi::Object::New(env)}, &names)}; + + const napi_extended_error_info* info{nullptr}; + napi_get_last_error_info(nenv, &info); + successClears.set_value(status == napi_ok && info != nullptr && info->error_code == napi_ok); + }); + + EXPECT_TRUE(nullConsistent.get_future().get()); + EXPECT_TRUE(undefinedConsistent.get_future().get()); + EXPECT_TRUE(successClears.get_future().get()); +} +#endif + int RunTests() { testing::InitGoogleTest();