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/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..922952eb 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; } @@ -1328,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/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..7b3f991f --- /dev/null +++ b/Core/Node-API/Source/js_native_api_shared.cc @@ -0,0 +1,133 @@ +#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{}; + + // `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)); + + 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 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)); + } + + 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..14d13840 --- /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, + // 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` + // 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/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 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 cdc9416b..54f13120 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -7,6 +7,9 @@ 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[]; +declare const napiEngine: string; describe("AbortController", function () { @@ -1621,6 +1624,154 @@ 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). + + 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("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 = 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"]); + }); + + 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(); + }); + }); +}); + + describe("Performance", function () { this.timeout(1000); diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index a920fa1f..5f6c535e 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include namespace @@ -100,6 +101,51 @@ 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 + // 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{}; + 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};