Fix napi_get_property_names conformance on JSC, ChakraCore and QuickJS

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

Core/Node-API/CMakeLists.txt

@@ -51,21 +51,27 @@ 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")
     elseif(NAPI_JAVASCRIPT_ENGINE STREQUAL "JavaScriptCore")
         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")

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 <napi/js_native_api.h>
 #include <array>
 #include <cassert>
@@ -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<JsValueRef>(object);
-  JsValueRef propertyNames;
-  CHECK_JSRT(env, JsGetOwnPropertyNames(obj, &propertyNames));
-  *result = reinterpret_cast<napi_value>(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;
 }
 

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 <algorithm>
 #include <cassert>
 #include <cmath>
@@ -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;
 }
 

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 <napi/js_native_api.h>
 #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;
 }

Core/Node-API/Source/js_native_api_shared.cc

@@ -0,0 +1,126 @@
+#include "js_native_api_shared.h"
+
+#include <napi/js_native_api.h>
+
+#include <string>
+#include <unordered_set>
+#include <vector>
+
+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<char> 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<std::string>& 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<std::string> 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, &current));
+
+    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, &current, &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, &current, &ownNames));
+      RETURN_IF_NOT_OK(AddAll(env, ownNames, shadowed));
+
+      RETURN_IF_NOT_OK(napi_get_prototype(env, current, &current));
+    }
+
+    *result = names;
+    return napi_ok;
+  }
+
+  #undef RETURN_IF_NOT_OK
+}

Core/Node-API/Source/js_native_api_shared.h

@@ -0,0 +1,22 @@
+#pragma once
+
+#include <napi/js_native_api_types.h>
+
+// 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);
+}

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

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.