From 2fac54e35508871e335a95c5b3969be1c3bf62b4 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Wed, 29 Jul 2026 21:01:54 -0700 Subject: [PATCH 01/11] Fix napi_get_property_names conformance on JSC, ChakraCore and QuickJS napi_get_property_names is specified to return the enumerable string-keyed properties of an object *and of its prototype chain* -- the same set a `for...in` loop visits. Only the V8 backend did that, via GetPropertyNames configured with kIncludePrototypes | ONLY_ENUMERABLE | SKIP_SYMBOLS. The other three each diverged: | backend | enumerable-only | includes prototypes | throws | |----------------|-----------------|---------------------|--------| | V8 | yes | yes | no | | JavaScriptCore | n/a | n/a | yes | | ChakraCore | no | no | no | | QuickJS | yes | no | no | JavaScriptCore was outright broken: it called Object.getOwnPropertyNames with argc 0, so the `object` argument was never used and the call always threw "TypeError: undefined is not an object". ChakraCore used JsGetOwnPropertyNames, which is own-only and also reports non-enumerable properties. QuickJS used JS_GetOwnPropertyNames with JS_GPN_ENUM_ONLY, which is enumerable-only but still own-only. None of the three engines exposes a native equivalent of V8's key collection, so add a single shared implementation that walks the prototype chain explicitly, written purely against the public napi_* surface, and have all three backends delegate to it. Per level it takes Object.keys for the properties `for...in` reports, and Object.getOwnPropertyNames for the shadowing set: a non-enumerable own property is not reported itself, but it does hide a same-named enumerable property further up the chain. JavaScriptCore's JSObjectCopyPropertyNames was considered for that backend since it walks the chain natively, but it drops the shadowing rule, so the shared walk is used there too and all backends stay consistent. Adds nine script tests, exercised through a new napiGetPropertyNames global that calls Napi::Object::GetPropertyNames. Verified locally on ChakraCore and QuickJS (225 passing, 0 failing on both). As a negative control, five of the nine fail when the old ChakraCore implementation is restored. Fixes #216 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Core/Node-API/CMakeLists.txt | 12 +- Core/Node-API/Source/js_native_api_chakra.cc | 11 +- .../Source/js_native_api_javascriptcore.cc | 11 +- Core/Node-API/Source/js_native_api_quickjs.cc | 27 +---- Core/Node-API/Source/js_native_api_shared.cc | 114 ++++++++++++++++++ Core/Node-API/Source/js_native_api_shared.h | 22 ++++ Tests/UnitTests/Scripts/tests.ts | 86 +++++++++++++ Tests/UnitTests/Shared/Shared.cpp | 10 ++ 8 files changed, 260 insertions(+), 33 deletions(-) create mode 100644 Core/Node-API/Source/js_native_api_shared.cc create mode 100644 Core/Node-API/Source/js_native_api_shared.h 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/Source/js_native_api_chakra.cc b/Core/Node-API/Source/js_native_api_chakra.cc index 67066d92..700a011a 100644 --- a/Core/Node-API/Source/js_native_api_chakra.cc +++ b/Core/Node-API/Source/js_native_api_chakra.cc @@ -1,4 +1,5 @@ #include "js_native_api_chakra.h" +#include "js_native_api_shared.h" #include #include #include @@ -678,11 +679,13 @@ 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. + CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result)); + return napi_ok; } diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 6d1ade9c..4a8dfe65 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 @@ -964,13 +965,13 @@ 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 + // silently drops properties shadowed by a non-enumerable own property, so use + // the shared prototype-chain walk instead. + CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result)); return napi_ok; } diff --git a/Core/Node-API/Source/js_native_api_quickjs.cc b/Core/Node-API/Source/js_native_api_quickjs.cc index c9e2823d..2f090921 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,11 @@ 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_FreePropertyEnum(env->context, ptab, plen); - - *result = FromJSValue(env, arr); + + // `JS_GetOwnPropertyNames` is own-only, so use the shared prototype-chain + // walk instead. + CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result)); + napi_clear_last_error(env); return napi_ok; } 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..8bd2b00b --- /dev/null +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -0,0 +1,114 @@ +#include "js_native_api_shared.h" + +#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 GetUtf8Value(napi_env env, napi_value value, std::string& result) { + size_t length{}; + RETURN_IF_NOT_OK(napi_get_value_string_utf8(env, value, nullptr, 0, &length)); + + std::vector buffer(length + 1); + size_t copied{}; + RETURN_IF_NOT_OK(napi_get_value_string_utf8(env, value, buffer.data(), buffer.size(), &copied)); + + result.assign(buffer.data(), copied); + return napi_ok; + } + + 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; + } + + // Appends every element of the string array `names` to `shadowed`. + napi_status AddAll(napi_env env, napi_value names, std::unordered_set& shadowed) { + uint32_t count{}; + RETURN_IF_NOT_OK(napi_get_array_length(env, names, &count)); + + std::string key{}; + for (uint32_t index = 0; index < count; ++index) { + napi_value name{}; + RETURN_IF_NOT_OK(napi_get_element(env, names, index, &name)); + RETURN_IF_NOT_OK(GetUtf8Value(env, name, key)); + shadowed.insert(std::move(key)); + } + + return napi_ok; + } + } + + napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result) { + // `Object.keys` reports one level's own enumerable string-keyed properties + // in specification order, which is exactly what `for...in` visits at that + // level. `Object.getOwnPropertyNames` additionally reports the + // non-enumerable ones: `for...in` does not visit those, but they still + // shadow same-named properties further up the prototype chain, so they have + // to be tracked as well. + napi_value global{}; + napi_value objectConstructor{}; + napi_value keys{}; + napi_value getOwnPropertyNames{}; + RETURN_IF_NOT_OK(napi_get_global(env, &global)); + RETURN_IF_NOT_OK(napi_get_named_property(env, global, "Object", &objectConstructor)); + RETURN_IF_NOT_OK(napi_get_named_property(env, objectConstructor, "keys", &keys)); + RETURN_IF_NOT_OK(napi_get_named_property(env, objectConstructor, "getOwnPropertyNames", &getOwnPropertyNames)); + + napi_value names{}; + RETURN_IF_NOT_OK(napi_create_array(env, &names)); + uint32_t nameCount{}; + + std::unordered_set shadowed{}; + std::string key{}; + + napi_value current{}; + RETURN_IF_NOT_OK(napi_coerce_to_object(env, object, ¤t)); + + while (true) { + bool isObjectLike{}; + RETURN_IF_NOT_OK(IsObjectLike(env, current, isObjectLike)); + if (!isObjectLike) { + break; + } + + napi_value ownEnumerableNames{}; + RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, keys, 1, ¤t, &ownEnumerableNames)); + + uint32_t ownEnumerableCount{}; + RETURN_IF_NOT_OK(napi_get_array_length(env, ownEnumerableNames, &ownEnumerableCount)); + for (uint32_t index = 0; index < ownEnumerableCount; ++index) { + napi_value name{}; + RETURN_IF_NOT_OK(napi_get_element(env, ownEnumerableNames, index, &name)); + RETURN_IF_NOT_OK(GetUtf8Value(env, name, key)); + if (shadowed.find(key) == shadowed.end()) { + RETURN_IF_NOT_OK(napi_set_element(env, names, nameCount++, name)); + } + } + + napi_value ownNames{}; + RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, getOwnPropertyNames, 1, ¤t, &ownNames)); + RETURN_IF_NOT_OK(AddAll(env, ownNames, shadowed)); + + RETURN_IF_NOT_OK(napi_get_prototype(env, current, ¤t)); + } + + *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..ae3907d1 --- /dev/null +++ b/Core/Node-API/Source/js_native_api_shared.h @@ -0,0 +1,22 @@ +#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 { + // 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, + // ChakraCore and QuickJS have no equivalent, so this walks the prototype + // chain explicitly. See https://github.com/BabylonJS/JsRuntimeHost/issues/216. + // + // `object` is coerced with `napi_coerce_to_object`, as V8's `CHECK_TO_OBJECT` + // does. Callers are expected to have already validated `env` and `result`. + napi_status GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result); +} diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index cdc9416b..353ada71 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -7,6 +7,7 @@ Mocha.reporter('spec'); declare const hostPlatform: string; declare const setExitCode: (code: number) => void; +declare const napiGetPropertyNames: (object: any) => string[]; describe("AbortController", function () { @@ -1621,6 +1622,91 @@ 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 ChakraCore and QuickJS only reported own properties + // (ChakraCore additionally reported non-enumerable ones). + + function forIn(object: any): string[] { + const keys: string[] = []; + for (const key in object) { + keys.push(key); + } + return keys; + } + + 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("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(forIn(object)); + expect(napiGetPropertyNames(object)).to.deep.equal(["own", "middle"]); + }); +}); + + describe("Performance", function () { this.timeout(1000); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index a920fa1f..4c3b0c41 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -100,6 +100,16 @@ TEST(JavaScript, All) env.Global().Set("setExitCode", setExitCodeCallback); env.Global().Set("hostPlatform", Napi::Value::From(env, JSRUNTIMEHOST_PLATFORM)); + + // 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); }); Babylon::ScriptLoader loader{runtime}; From 9e3c304a1c397e1e6aadb8629aa4a521d701419e Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Wed, 29 Jul 2026 21:25:59 -0700 Subject: [PATCH 02/11] Fix napi_get_prototype on JSC and skip the for...in oracle where it is broken Two follow-ups from CI on the previous commit. napi_get_prototype on JavaScriptCore ran the result of JSObjectGetPrototype through JSValueToObject. At the top of a prototype chain that value is `null`, so the conversion threw "TypeError: null is not an object" instead of reporting the end of the chain, which made the chain impossible to walk and failed all nine new tests. Return the raw prototype value, as V8 does. It has no other callers in this repository: Blob.cpp deliberately uses Object.getPrototypeOf instead. The "matches for...in" assertion also failed on Hermes, but in the opposite direction: napi_get_property_names returned the correct ['own', 'middle'] while Hermes' own `for...in` returned ['own', 'middle', 'deep'], i.e. Hermes does not implement the rule that a non-enumerable own property shadows an inherited enumerable one. Probe for that behaviour at runtime rather than naming engines, and only use `for...in` as an oracle where it holds. The explicit expected-value assertion still runs everywhere. Re-verified on ChakraCore and QuickJS: 225 passing, 0 failing on both. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Core/Node-API/Source/js_native_api_javascriptcore.cc | 11 +++++++---- Tests/UnitTests/Scripts/tests.ts | 12 +++++++++++- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 4a8dfe65..922952eb 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -1329,13 +1329,16 @@ 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); - JSValueRef exception{}; - JSObjectRef prototype{JSValueToObject(env->context, JSObjectGetPrototype(env->context, ToJSObject(env, object)), &exception)}; - CHECK_JSC(env, exception); + // `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. + *result = ToNapi(JSObjectGetPrototype(env->context, ToJSObject(env, object))); - *result = ToNapi(prototype); return napi_ok; } diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 353ada71..bf4afbdd 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1637,6 +1637,14 @@ describe("napi_get_property_names (#216)", function () { 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. + 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"]); }); @@ -1701,8 +1709,10 @@ describe("napi_get_property_names (#216)", function () { object[Symbol("s")] = 4; Object.defineProperty(object, "deep", { value: 5, enumerable: false }); - expect(napiGetPropertyNames(object)).to.deep.equal(forIn(object)); expect(napiGetPropertyNames(object)).to.deep.equal(["own", "middle"]); + if (forInHonoursShadowing) { + expect(napiGetPropertyNames(object)).to.deep.equal(forIn(object)); + } }); }); From 3b857e7c4f5e539fe623a02d129787c806820ce8 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Wed, 29 Jul 2026 21:49:44 -0700 Subject: [PATCH 03/11] Implement Object::GetPropertyNames in the JSI Node-API adapter The JSI adapter provides the Napi C++ surface directly on top of JSI rather than over the C Node-API, and Object::GetPropertyNames was still a stub that threw std::runtime_error{"TODO"}, surfacing in script as "Error: Exception in HostFunction: TODO". jsi::Object::getPropertyNames returns the enumerable string-keyed properties of an object and of its prototype chain, which is exactly the specified behaviour, so forward to it. Verified locally against the ReactNative.V8Jsi runtime: all nine napi_get_property_names tests pass, 225 passing, 0 failing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Core/Node-API-JSI/Include/napi/napi-inl.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 From c3def88da9fa66f694366fa2269997dfa7feb5b4 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 30 Jul 2026 08:06:38 -0700 Subject: [PATCH 04/11] Test the ToObject coercion, and make null/undefined consistent Review feedback: the implementation coerces its argument with ToObject, but nothing covered that. `Napi::Object::GetPropertyNames` can only be called on an already-constructed `Napi::Object`, so the C++ harness structurally could not reach the coercion path. Expose the C entry point directly as `napiGetPropertyNamesRaw` and pass the raw value through. The Node-API-JSI backend implements the `Napi::` C++ surface straight on top of JSI and has no C Node-API at all, so the global is left undefined there (it already has a `JSRUNTIMEHOST_NAPI_ENGINE_JSI` define) and the six new tests skip themselves. Testing that also exposed a divergence for `null` and `undefined`, which have no object wrapper: V8 reports `napi_object_expected`, QuickJS's `napi_coerce_to_object` uses `Object(value)` and happily returns an empty object, and JavaScriptCore's throws. Check `napi_typeof` explicitly in the shared implementation so all three match V8 instead. Verified locally: ChakraCore 231 passing / 0 failing, QuickJS 231 / 0, JSI 225 / 0 with 6 pending. Negative control: with the null/undefined check removed, QuickJS fails exactly the two tests that cover it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Core/Node-API/Source/js_native_api_shared.cc | 12 +++++++ Tests/UnitTests/Scripts/tests.ts | 36 ++++++++++++++++++++ Tests/UnitTests/Shared/Shared.cpp | 35 +++++++++++++++++++ 3 files changed, 83 insertions(+) diff --git a/Core/Node-API/Source/js_native_api_shared.cc b/Core/Node-API/Source/js_native_api_shared.cc index 8bd2b00b..e394ea20 100644 --- a/Core/Node-API/Source/js_native_api_shared.cc +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -75,6 +75,18 @@ namespace napi_shared { std::unordered_set shadowed{}; std::string key{}; + // `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{}; RETURN_IF_NOT_OK(napi_coerce_to_object(env, object, ¤t)); diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index bf4afbdd..323b1ed1 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -8,6 +8,7 @@ Mocha.reporter('spec'); declare const hostPlatform: string; declare const setExitCode: (code: number) => void; declare const napiGetPropertyNames: (object: any) => string[]; +declare const napiGetPropertyNamesRaw: (object: any) => string[]; describe("AbortController", function () { @@ -1714,6 +1715,41 @@ describe("napi_get_property_names (#216)", function () { 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 () { + it("wraps a string primitive and reports its indices", function () { + expect(napiGetPropertyNamesRaw("ab")).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(); + }); + }); }); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 4c3b0c41..35bb3a83 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include namespace @@ -110,6 +111,40 @@ TEST(JavaScript, All) }, "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{}; + napi_get_and_clear_last_exception(rawEnv, &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}; From ad868a2ed61c3261abded2b04bd270adde494404 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 30 Jul 2026 08:44:40 -0700 Subject: [PATCH 05/11] Skip the primitive-wrapping cases on Hermes Hermes' Node-API is not implemented in this repository -- it comes from the Hermes dependency itself -- and it rejects primitives outright instead of applying ToObject, so the three wrapping tests failed there. It does reject null and undefined like everyone else, so those two still run. Hermes is documented as an experimental engine here and its napi is not ours to fix, so skip those cases explicitly rather than weakening the assertions for every backend. That needs an engine identifier in script, so plumb NAPI_JAVASCRIPT_ENGINE through as a `napiEngine` global alongside the existing `hostPlatform` one. Verified locally: ChakraCore 231 passing / 0 failing, QuickJS 231 / 0, JSI 225 / 0 with 6 pending. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Tests/UnitTests/CMakeLists.txt | 1 + Tests/UnitTests/Scripts/tests.ts | 26 ++++++++++++++++++-------- Tests/UnitTests/Shared/Shared.cpp | 1 + 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index 2dbc7619..c12b12f8 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -45,6 +45,7 @@ endif() add_executable(UnitTests ${SOURCES} ${SCRIPTS} ${TYPE_SCRIPTS} ${ASSETS}) target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}") +target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE="${NAPI_JAVASCRIPT_ENGINE}") # The V8JSI Node-API shim does not implement napi_create_dataview, so the # CreateDataViewRejectsOverflowingRange test is compiled out on that backend. diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 323b1ed1..5498bffa 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -9,6 +9,7 @@ declare const hostPlatform: string; declare const setExitCode: (code: number) => void; declare const napiGetPropertyNames: (object: any) => string[]; declare const napiGetPropertyNamesRaw: (object: any) => string[]; +declare const napiEngine: string; describe("AbortController", function () { @@ -1724,16 +1725,25 @@ describe("napi_get_property_names (#216)", function () { const describeCoercion = typeof napiGetPropertyNamesRaw === "function" ? describe : describe.skip; describeCoercion("argument coercion", function () { - it("wraps a string primitive and reports its indices", function () { - expect(napiGetPropertyNamesRaw("ab")).to.deep.equal(["0", "1"]); - }); + // 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. + const describePrimitives = napiEngine === "Hermes" ? describe.skip : describe; + + 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 number primitive, which has no enumerable properties", function () { - expect(napiGetPropertyNamesRaw(42)).to.deep.equal([]); - }); + 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("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 () { diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 35bb3a83..5f6c535e 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -101,6 +101,7 @@ TEST(JavaScript, All) env.Global().Set("setExitCode", setExitCodeCallback); env.Global().Set("hostPlatform", Napi::Value::From(env, JSRUNTIMEHOST_PLATFORM)); + env.Global().Set("napiEngine", Napi::Value::From(env, JSRUNTIMEHOST_NAPI_ENGINE)); // Exposes napi_get_property_names, via its C++ wrapper, so that the // script tests can compare it against `for...in`. See From 1767446abc01d93a07530535f6cb1e0aa547c171 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 30 Jul 2026 08:59:37 -0700 Subject: [PATCH 06/11] Define JSRUNTIMEHOST_NAPI_ENGINE for the Android build too Shared.cpp is compiled by two targets -- UnitTests and Android's UnitTestsJNI -- and only the former got the new define, so all four Android legs failed with "use of undeclared identifier 'JSRUNTIMEHOST_NAPI_ENGINE'". Mirror it next to JSRUNTIMEHOST_PLATFORM, exactly as that one is already handled in both places. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt index 0af5caa8..c870151c 100644 --- a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt +++ b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt @@ -22,6 +22,7 @@ add_library(UnitTestsJNI SHARED ${UNIT_TESTS_DIR}/Shared/Shared.cpp) target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}") +target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_NAPI_ENGINE="${NAPI_JAVASCRIPT_ENGINE}") target_compile_definitions(UnitTestsJNI PRIVATE ARCANA_TEST_HOOKS) target_include_directories(UnitTestsJNI From cbe9c16b96d155546c4f344b31047f6496ba0536 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Branimir=20Karad=C5=BEi=C4=87=20=28via=20Copilot=29?= <223556219+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 18:55:19 -0700 Subject: [PATCH 07/11] Address property-name review feedback Use Chakra terminology, keep a Hermes coercion tripwire linked to #219, and avoid building the final unused shadowing set. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79ad319b-72f9-4b93-9c81-57858177c0a7 --- Core/Node-API/Source/js_native_api_shared.cc | 15 +++++++++++---- Core/Node-API/Source/js_native_api_shared.h | 2 +- Tests/UnitTests/Scripts/tests.ts | 15 ++++++++++++--- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_shared.cc b/Core/Node-API/Source/js_native_api_shared.cc index e394ea20..7b3f991f 100644 --- a/Core/Node-API/Source/js_native_api_shared.cc +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -111,11 +111,18 @@ namespace napi_shared { } } - napi_value ownNames{}; - RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, getOwnPropertyNames, 1, ¤t, &ownNames)); - RETURN_IF_NOT_OK(AddAll(env, ownNames, shadowed)); + napi_value next{}; + RETURN_IF_NOT_OK(napi_get_prototype(env, current, &next)); + + bool hasNextLevel{}; + RETURN_IF_NOT_OK(IsObjectLike(env, next, hasNextLevel)); + if (hasNextLevel) { + napi_value ownNames{}; + RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, getOwnPropertyNames, 1, ¤t, &ownNames)); + RETURN_IF_NOT_OK(AddAll(env, ownNames, shadowed)); + } - RETURN_IF_NOT_OK(napi_get_prototype(env, current, ¤t)); + current = next; } *result = names; diff --git a/Core/Node-API/Source/js_native_api_shared.h b/Core/Node-API/Source/js_native_api_shared.h index ae3907d1..14d13840 100644 --- a/Core/Node-API/Source/js_native_api_shared.h +++ b/Core/Node-API/Source/js_native_api_shared.h @@ -13,7 +13,7 @@ namespace napi_shared { // // V8 gets this from a single `GetPropertyNames` call configured with // `kIncludePrototypes | ONLY_ENUMERABLE | SKIP_SYMBOLS`. JavaScriptCore, - // ChakraCore and QuickJS have no equivalent, so this walks the prototype + // Chakra and QuickJS have no equivalent, so this walks the prototype // chain explicitly. See https://github.com/BabylonJS/JsRuntimeHost/issues/216. // // `object` is coerced with `napi_coerce_to_object`, as V8's `CHECK_TO_OBJECT` diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 5498bffa..54f13120 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1628,8 +1628,8 @@ 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 ChakraCore and QuickJS only reported own properties - // (ChakraCore additionally reported non-enumerable ones). + // outright, while Chakra and QuickJS only reported own properties + // (Chakra additionally reported non-enumerable ones). function forIn(object: any): string[] { const keys: string[] = []; @@ -1643,6 +1643,7 @@ describe("napi_get_property_names (#216)", function () { // 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; @@ -1729,9 +1730,17 @@ describe("napi_get_property_names (#216)", function () { // 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. + // 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 = napiEngine === "Hermes" ? describe.skip : describe; + if (napiEngine === "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"]); From 0d7ab0666817e711afff07852a53729b3e70fe1b Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Fri, 31 Jul 2026 09:53:37 -0700 Subject: [PATCH 08/11] Terminate the prototype walk on a cycle and report last_error consistently A `getPrototypeOf` Proxy trap may return an object that is already on the chain. Nothing in the specification forbids it -- the invariants on that trap constrain only the non-extensible case -- so `Object.getPrototypeOf(p) === p` is reachable from script. V8 walks the chain recursively and so terminates with a `RangeError`; the shared walk introduced here is iterative and spun forever instead. Confirmed against a real build: the test process burned 56 seconds of CPU and never returned, and no JavaScript-level timeout can preempt it, because control never re-enters the engine. Stopping at the first repeated level is exact rather than a bail-out. Every level adds its full own-property-name set to `shadowed` before the walk advances, so a level reached a second time can only re-encounter names that are already shadowed; breaking there yields precisely the fixed point the non-terminating walk converges on. Separately, `napi_get_property_names` disagreed with `napi_get_last_error_info`. The shared walk is written against the public `napi_*` surface and so 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. A caller therefore saw `napi_object_expected` returned while the recorded error code was still `napi_ok`. The success path had the mirror-image problem: it left whatever error a previous call had recorded in place. Set and clear the error explicitly at all three call sites. Covered by four script tests for the cyclic cases and a native regression test for the error reporting; reverting either fix fails them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033 --- Core/Node-API/Source/js_native_api_chakra.cc | 15 ++++- .../Source/js_native_api_javascriptcore.cc | 14 ++++- Core/Node-API/Source/js_native_api_quickjs.cc | 12 +++- Core/Node-API/Source/js_native_api_shared.cc | 34 +++++++++++ Tests/UnitTests/Scripts/tests.ts | 48 +++++++++++++++ Tests/UnitTests/Shared/Shared.cpp | 59 +++++++++++++++++++ 6 files changed, 174 insertions(+), 8 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_chakra.cc b/Core/Node-API/Source/js_native_api_chakra.cc index 700a011a..0b45ff29 100644 --- a/Core/Node-API/Source/js_native_api_chakra.cc +++ b/Core/Node-API/Source/js_native_api_chakra.cc @@ -683,9 +683,18 @@ napi_status napi_get_property_names(napi_env env, CHECK_ARG(env, result); // `JsGetOwnPropertyNames` is own-only and includes non-enumerable properties, - // so use the shared prototype-chain walk instead. - CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result)); - + // 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)}; + 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_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 922952eb..680b355c 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -970,10 +970,18 @@ napi_status napi_get_property_names(napi_env env, // JavaScriptCore's `JSObjectCopyPropertyNames` walks the prototype chain but // silently drops properties shadowed by a non-enumerable own property, so use - // the shared prototype-chain walk instead. - CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result)); + // 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)}; + 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, diff --git a/Core/Node-API/Source/js_native_api_quickjs.cc b/Core/Node-API/Source/js_native_api_quickjs.cc index 2f090921..71a15c2d 100644 --- a/Core/Node-API/Source/js_native_api_quickjs.cc +++ b/Core/Node-API/Source/js_native_api_quickjs.cc @@ -1397,8 +1397,16 @@ napi_status napi_get_property_names(napi_env env, napi_value object, napi_value* CHECK_ARG(env, result); // `JS_GetOwnPropertyNames` is own-only, so use the shared prototype-chain - // walk instead. - CHECK_NAPI(napi_shared::GetEnumerablePropertyNames(env, object, result)); + // 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)}; + 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_shared.cc b/Core/Node-API/Source/js_native_api_shared.cc index 7b3f991f..23dcdc5c 100644 --- a/Core/Node-API/Source/js_native_api_shared.cc +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -50,6 +50,21 @@ namespace napi_shared { 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 GetEnumerablePropertyNames(napi_env env, napi_value object, napi_value* result) { @@ -73,6 +88,7 @@ namespace napi_shared { uint32_t nameCount{}; std::unordered_set shadowed{}; + std::vector visited{}; std::string key{}; // `ToObject` is what the specification (and the V8 implementation) applies @@ -97,6 +113,24 @@ namespace napi_shared { break; } + // A `getPrototypeOf` Proxy trap can return an object that is already on + // the chain -- nothing in the specification forbids it, so + // `Object.getPrototypeOf(p) === p` is reachable from script -- which + // makes this walk cyclic. V8 recurses and so terminates with a + // `RangeError`; this loop is iterative and would spin forever. + // + // Stopping at the repeat is exact rather than a bail-out: every level + // adds its own property names to `shadowed` before the walk continues, + // so a level visited a second time can only re-encounter names that are + // already shadowed. Breaking here therefore yields the same result the + // non-terminating walk converges on. + bool alreadyVisited{}; + RETURN_IF_NOT_OK(Contains(env, visited, current, alreadyVisited)); + if (alreadyVisited) { + break; + } + visited.push_back(current); + napi_value ownEnumerableNames{}; RETURN_IF_NOT_OK(napi_call_function(env, objectConstructor, keys, 1, ¤t, &ownEnumerableNames)); diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 54f13120..5e450686 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1769,6 +1769,54 @@ describe("napi_get_property_names (#216)", 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 = napiEngine === "Chakra" || napiEngine === "QuickJS" || napiEngine === "JavaScriptCore"; + const describeCycles = usesSharedWalk ? describe : describe.skip; + + describeCycles("cyclic prototype chains", function () { + this.timeout(5000); + + it("terminates when a proxy is its own prototype", function () { + let object: any; + object = new Proxy({ own: 1 }, { getPrototypeOf() { return object; } }); + expect(napiGetPropertyNames(object)).to.deep.equal(["own"]); + }); + + it("terminates on a two-object cycle and reports each level once", 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.deep.equal(["a", "b"]); + }); + + 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("propagates a throwing getPrototypeOf trap instead of hanging", function () { + const object = new Proxy({ own: 1 }, { + getPrototypeOf() { throw new Error("trap"); }, + }); + expect(() => napiGetPropertyNames(object)).to.throw(); + }); + }); }); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 5f6c535e..23a98735 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include namespace @@ -449,6 +450,64 @@ TEST(NodeApi, GetValueStringUtf16HandlesZeroBufsize) } #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) +{ + // Hermes supplies its own Node-API rather than the implementations in this + // repository, so this contract is not its to satisfy. Every backend that is + // ours is covered, including V8, which reaches napi_set_last_error through + // RETURN_STATUS_IF_FALSE. + if (std::string_view{JSRUNTIMEHOST_NAPI_ENGINE} == "Hermes") + { + GTEST_SKIP() << "Hermes 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(); From efc20cb558b5a9a839501b9e618ee7cef038855c Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Fri, 31 Jul 2026 12:06:25 -0700 Subject: [PATCH 09/11] Scope the new property-name tests to the backends they describe CI turned up two places where the tests asserted behaviour that belongs to an engine rather than to this change. JavaScriptCore's `napi_get_prototype` calls `JSObjectGetPrototype`, which reads the internal [[Prototype]] slot and never runs a proxy `getPrototypeOf` trap. Since a cycle can only be built with that trap, a trapped chain there reports the target's real prototype instead: the cycle is invisible, the throwing trap never fires, and the walk was never at risk on that backend to begin with. That is a pre-existing limitation of `napi_get_prototype`, not of the walk, and fixing it means giving JavaScriptCore `Reflect.getPrototypeOf` semantics, which is a separate change. Skip the three proxy-dependent cases there and say why. The acyclic-chain case needs no trap and still runs everywhere. The native error-reporting test asserted a contract V8 does not keep, and I was wrong to claim otherwise: `napi_get_property_names` there is vendored upstream Node code whose rejection leaves a pending exception, so the next call reports `napi_pending_exception` rather than `napi_object_expected`, and whose success path returns bare `napi_ok` through `GET_RETURN_STATUS` without clearing. Both are upstream's to define. Scope the test to the three backends that share the walk -- the ones this change actually touches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033 --- Tests/UnitTests/Scripts/tests.ts | 19 ++++++++++++++++--- Tests/UnitTests/Shared/Shared.cpp | 17 +++++++++++------ 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index 5e450686..e2eeebd3 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1784,16 +1784,29 @@ describe("napi_get_property_names (#216)", function () { const usesSharedWalk = napiEngine === "Chakra" || napiEngine === "QuickJS" || napiEngine === "JavaScriptCore"; const describeCycles = usesSharedWalk ? describe : describe.skip; + // A cycle can only be built with a `getPrototypeOf` trap, so observing one + // additionally requires `napi_get_prototype` to consult that trap. + // JavaScriptCore's does not: it calls `JSObjectGetPrototype`, which reads + // the internal [[Prototype]] slot directly and never runs proxy traps + // (`js_native_api_javascriptcore.cc:1348`). A trapped chain there simply + // reports the target's real prototype, so the cycle -- and the throwing + // trap -- are both invisible, and the walk was never at risk on that + // backend. That is a pre-existing limitation of `napi_get_prototype`, not + // of the walk, so it is left alone here; the termination check below is + // still correct and harmless on JavaScriptCore. + const proxyTrapsReachPrototypeWalk = usesSharedWalk && napiEngine !== "JavaScriptCore"; + const itTrapped = proxyTrapsReachPrototypeWalk ? it : it.skip; + describeCycles("cyclic prototype chains", function () { this.timeout(5000); - it("terminates when a proxy is its own prototype", function () { + itTrapped("terminates when a proxy is its own prototype", function () { let object: any; object = new Proxy({ own: 1 }, { getPrototypeOf() { return object; } }); expect(napiGetPropertyNames(object)).to.deep.equal(["own"]); }); - it("terminates on a two-object cycle and reports each level once", function () { + itTrapped("terminates on a two-object cycle and reports each level once", function () { let first: any; let second: any; first = new Proxy({ a: 1 }, { getPrototypeOf() { return second; } }); @@ -1810,7 +1823,7 @@ describe("napi_get_property_names (#216)", function () { expect(napiGetPropertyNames(leaf)).to.deep.equal(["own", "middle", "deep"]); }); - it("propagates a throwing getPrototypeOf trap instead of hanging", function () { + itTrapped("propagates a throwing getPrototypeOf trap instead of hanging", function () { const object = new Proxy({ own: 1 }, { getPrototypeOf() { throw new Error("trap"); }, }); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 23a98735..110dcfab 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -454,13 +454,18 @@ TEST(NodeApi, GetValueStringUtf16HandlesZeroBufsize) #if !defined(JSRUNTIMEHOST_NAPI_ENGINE_JSI) TEST(NodeApi, GetPropertyNamesReportsLastErrorConsistently) { - // Hermes supplies its own Node-API rather than the implementations in this - // repository, so this contract is not its to satisfy. Every backend that is - // ours is covered, including V8, which reaches napi_set_last_error through - // RETURN_STATUS_IF_FALSE. - if (std::string_view{JSRUNTIMEHOST_NAPI_ENGINE} == "Hermes") + // 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{JSRUNTIMEHOST_NAPI_ENGINE}; + if (engine != "Chakra" && engine != "QuickJS" && engine != "JavaScriptCore") { - GTEST_SKIP() << "Hermes supplies its own napi_get_property_names."; + GTEST_SKIP() << engine << " supplies its own napi_get_property_names."; } // Regression: napi_get_property_names rejects null/undefined with From fdd36a92ed12a9e36a3214e36942b2ed84bef7db Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Fri, 31 Jul 2026 13:00:35 -0700 Subject: [PATCH 10/11] Coerce the argument in napi_get_prototype on JavaScriptCore The conversion this function already performed was on the wrong operand. It belongs on the argument, which is where V8 puts it, and moving it there fixes a latent defect on the same line. The argument was handed straight to `ToJSObject`, which only asserts that its input is an object. A primitive therefore tripped the assert in debug builds and, in release, reinterpreted a non-object `JSValueRef` as a `JSObjectRef` before handing it to `JSObjectGetPrototype` -- undefined behaviour rather than a status. Coercing instead matches V8's `CHECK_TO_OBJECT`: a primitive yields its wrapper's prototype, and only `null` and `undefined` are rejected, with `napi_object_expected`. Nothing in the repository could reach this. The shared prototype walk is the only caller, and it recurses only into values it has already established are object-like, so the behaviour it depends on is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033 --- .../Source/js_native_api_javascriptcore.cc | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 680b355c..cd2fde72 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -1345,9 +1345,25 @@ napi_status napi_get_prototype(napi_env env, // `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. - *result = ToNapi(JSObjectGetPrototype(env->context, ToJSObject(env, object))); + // + // 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); + } - return napi_ok; + JSValueRef exception{}; + const JSObjectRef self{JSValueToObject(env->context, value, &exception)}; + CHECK_JSC(env, exception); + + *result = ToNapi(JSObjectGetPrototype(env->context, self)); + + return napi_clear_last_error(env); } napi_status napi_create_object(napi_env env, napi_value* result) { From 26a4a999607a98697b431417c4db614a8cfa771b Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Fri, 31 Jul 2026 15:22:38 -0700 Subject: [PATCH 11/11] Correct the JSObjectCopyPropertyNames comment The comment had the failure mode backwards. JavaScriptCore does not drop the shadowed inherited property; it reports it. `JSObject::getPropertyNames` walks the chain calling `getOwnPropertyNames` per level with `DontEnumPropertiesMode::Exclude`, so a non-enumerable own property is never added to the array and therefore cannot suppress a same-named enumerable property further up -- the inherited name survives where `for...in` correctly omits it. The conclusion is unchanged, and so is the code: it is still not a conforming replacement for the shared walk. Thanks to @matthargett for catching it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033 --- Core/Node-API/Source/js_native_api_javascriptcore.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index cd2fde72..7daa7bfd 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -968,9 +968,13 @@ napi_status napi_get_property_names(napi_env env, CHECK_ARG(env, object); CHECK_ARG(env, result); - // JavaScriptCore's `JSObjectCopyPropertyNames` walks the prototype chain but - // silently drops properties shadowed by a non-enumerable own property, so use - // the shared prototype-chain walk instead. It is written against the public + // 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