napi: stop napi_get_property_names poisoning JSC's per-Structure own-keys cache

src/jsc/bindings/napi.cpp

@@ -48,6 +48,7 @@
 #include <JavaScriptCore/JSArray.h>
 #include <JavaScriptCore/JSPromise.h>
 #include <JavaScriptCore/ObjectConstructor.h>
+#include <JavaScriptCore/PropertyNameArray.h>
 #include <JavaScriptCore/ArrayBuffer.h>
 #include <JavaScriptCore/JSArrayBuffer.h>
 #include "JSFFIFunction.h"
@@ -1814,6 +1815,35 @@ extern "C" napi_status napi_create_typedarray(
 
 namespace Zig {
 
+// Walk the prototype chain collecting property names without touching JSC's
+// per-Structure own-keys cache. JSC::allPropertyKeys() stores the chain-walked
+// list there, poisoning Reflect.ownKeys/Object.keys for same-shaped objects.
+static JSArray* collectInheritedPropertyKeys(JSGlobalObject* globalObject, JSObject* object, PropertyNameMode propertyNameMode, DontEnumPropertiesMode dontEnumPropertiesMode)
+{
+    VM& vm = JSC::getVM(globalObject);
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
+    PropertyNameArrayBuilder properties(vm, propertyNameMode, PrivateSymbolMode::Exclude);
+    object->getPropertyNames(globalObject, properties, dontEnumPropertiesMode);
+    RETURN_IF_EXCEPTION(scope, nullptr);
+
+    unsigned numProperties = properties.size();
+    JSArray* keys = JSArray::create(vm, globalObject->originalArrayStructureForIndexingType(ArrayWithContiguous), numProperties);
+    for (unsigned i = 0; i < numProperties; i++) {
+        const auto& identifier = properties[i];
+        JSValue key;
+        if (propertyNameMode != PropertyNameMode::Strings && identifier.isSymbol()) {
+            ASSERT(!identifier.isPrivateName());
+            key = Symbol::create(vm, static_cast<SymbolImpl&>(*identifier.impl()));
+        } else {
+            key = jsOwnedString(vm, identifier.string());
+        }
+        keys->putDirectIndex(globalObject, i, key);
+        RETURN_IF_EXCEPTION(scope, nullptr);
+    }
+    return keys;
+}
+
 extern "C" napi_status napi_get_all_property_names(
     napi_env env, napi_value objectNapi, napi_key_collection_mode key_mode,
     napi_key_filter key_filter, napi_key_conversion key_conversion,
@@ -1842,7 +1872,7 @@ extern "C" napi_status napi_get_all_property_names(
 
     JSArray* exportKeys = nullptr;
     if (key_mode == napi_key_include_prototypes) {
-        exportKeys = allPropertyKeys(globalObject, object, jsc_property_mode, jsc_key_mode);
+        exportKeys = collectInheritedPropertyKeys(globalObject, object, jsc_property_mode, jsc_key_mode);
     } else {
         exportKeys = ownPropertyKeys(globalObject, object, jsc_property_mode, jsc_key_mode);
     }
@@ -2011,7 +2041,7 @@ extern "C" napi_status napi_get_property_names(napi_env env, napi_value object,
     Zig::GlobalObject* globalObject = toJS(env);
 
     JSC::EnsureStillAliveScope ensureStillAlive(jsValue);
-    JSValue value = JSC::allPropertyKeys(globalObject, jsObject, PropertyNameMode::Strings, DontEnumPropertiesMode::Exclude);
+    JSValue value = collectInheritedPropertyKeys(globalObject, jsObject, PropertyNameMode::Strings, DontEnumPropertiesMode::Exclude);
     NAPI_RETURN_IF_EXCEPTION(env);
     JSC::EnsureStillAliveScope ensureStillAlive1(value);
 

test/napi/napi-app/js_test_helpers.cpp

@@ -257,6 +257,30 @@ static napi_value make_empty_object(const Napi::CallbackInfo &info) {
   return object;
 }
 
+// get_property_names(object) -> array
+static napi_value get_property_names(const Napi::CallbackInfo &info) {
+  napi_env env = info.Env();
+  napi_value result;
+  NODE_API_CALL(env, napi_get_property_names(env, info[0], &result));
+  return result;
+}
+
+// get_all_property_names(object, key_mode, key_filter, key_conversion) -> array
+static napi_value get_all_property_names(const Napi::CallbackInfo &info) {
+  napi_env env = info.Env();
+  uint32_t key_mode, key_filter, key_conversion;
+  NODE_API_CALL(env, napi_get_value_uint32(env, info[1], &key_mode));
+  NODE_API_CALL(env, napi_get_value_uint32(env, info[2], &key_filter));
+  NODE_API_CALL(env, napi_get_value_uint32(env, info[3], &key_conversion));
+  napi_value result;
+  NODE_API_CALL(env,
+                napi_get_all_property_names(
+                    env, info[0], (napi_key_collection_mode)key_mode,
+                    (napi_key_filter)key_filter,
+                    (napi_key_conversion)key_conversion, &result));
+  return result;
+}
+
 // add_tag(object, lower, upper)
 static napi_value add_tag(const Napi::CallbackInfo &info) {
   Napi::Env env = info.Env();
@@ -447,6 +471,8 @@ void register_js_test_helpers(Napi::Env env, Napi::Object exports) {
   REGISTER_FUNCTION(env, exports, create_latin1_string);
   REGISTER_FUNCTION(env, exports, make_empty_array);
   REGISTER_FUNCTION(env, exports, make_empty_object);
+  REGISTER_FUNCTION(env, exports, get_property_names);
+  REGISTER_FUNCTION(env, exports, get_all_property_names);
   REGISTER_FUNCTION(env, exports, add_tag);
   REGISTER_FUNCTION(env, exports, try_add_tag);
   REGISTER_FUNCTION(env, exports, check_tag);

test/napi/napi-app/module.js

@@ -236,6 +236,43 @@ nativeTests.test_set_property = () => {
   }
 };
 
+nativeTests.test_property_names_cache_poisoning = () => {
+  // napi_key_include_prototypes = 0, napi_key_own_only = 1
+  // napi_key_all_properties = 0, napi_key_skip_symbols = 16
+  // napi_key_keep_numbers = 0
+  const mkA = () => ({ a: 1, b: 2 });
+  for (let i = 0; i < 20; i++) nativeTests.get_all_property_names(mkA(), 0, 0, 0);
+  console.log("Reflect.ownKeys after get_all_property_names(include_prototypes):", Reflect.ownKeys(mkA()).join(","));
+  console.log("Object.keys after get_all_property_names(include_prototypes):", Object.keys(mkA()).join(","));
+
+  const proto = { pEnum: 9 };
+  const mkB = () => {
+    const o = Object.create(proto);
+    o.w1 = 1;
+    o.w2 = 2;
+    return o;
+  };
+  for (let i = 0; i < 20; i++) nativeTests.get_property_names(mkB());
+  console.log("Object.keys after get_property_names:", Object.keys(mkB()).join(","));
+  console.log("Reflect.ownKeys after get_property_names:", Reflect.ownKeys(mkB()).join(","));
+
+  const mkC = () => ({ c: 1, d: 2 });
+  for (let i = 0; i < 20; i++) nativeTests.get_all_property_names(mkC(), 0, 16, 0);
+  console.log("Object.getOwnPropertyNames after skip_symbols chain walk:", Object.getOwnPropertyNames(mkC()).join(","));
+
+  // Own-only mode must still return only own keys and not poison anything.
+  const mkD = () => ({ e: 1, f: 2 });
+  for (let i = 0; i < 20; i++) nativeTests.get_all_property_names(mkD(), 1, 0, 0);
+  console.log("Reflect.ownKeys after get_all_property_names(own_only):", Reflect.ownKeys(mkD()).join(","));
+
+  // The napi result itself should still include inherited keys.
+  const apnResult = nativeTests.get_all_property_names(mkA(), 0, 0, 0);
+  console.log("napi include_prototypes result has own a,b:", apnResult.includes("a") && apnResult.includes("b"));
+  console.log("napi include_prototypes result has inherited toString:", apnResult.includes("toString"));
+  const gpnResult = nativeTests.get_property_names(mkB());
+  console.log("napi get_property_names result:", gpnResult.join(","));
+};
+
 nativeTests.test_number_integer_conversions_from_js = () => {
   const i32 = { min: -(2 ** 31), max: 2 ** 31 - 1 };
   const u32Max = 2 ** 32 - 1;

test/napi/napi.test.ts

@@ -574,6 +574,15 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => {
     });
   });
 
+  describe("napi_get_property_names / napi_get_all_property_names", () => {
+    it("does not poison JSC's per-Structure own-keys cache", async () => {
+      const output = await checkSameOutput("test_property_names_cache_poisoning", []);
+      expect(output).toContain("Reflect.ownKeys after get_all_property_names(include_prototypes): a,b");
+      expect(output).toContain("Object.keys after get_property_names: w1,w2");
+      expect(output).toContain("napi get_property_names result: w1,w2,pEnum");
+    });
+  });
+
   describe("napi_value <=> integer conversion", () => {
     it("works", async () => {
       await checkSameOutput("test_number_integer_conversions_from_js", []);