Fix napi_get_property_names conformance on JSC, Chakra and QuickJS

[bkaradzic-microsoft]

[Updated by Copilot on behalf of @bghgary]

Fixes #216.

Context

napi_get_property_names should return enumerable string-keyed properties across the prototype chain, matching for...in. Before this PR, JavaScriptCore threw, Chakra and QuickJS returned only own properties, and JSI had an unimplemented stub. The three Node-API backends now share a property-name walk; JSI uses its own implementation.

The walk must account for non-enumerable own properties hiding inherited names, terminate when a Proxy returns a repeated prototype, and keep the returned status consistent with napi_get_last_error_info. JavaScriptCore’s napi_get_prototype also needed correction for primitives and the null end of a prototype chain.

Review follow-up

A retained JavaScriptCore context could collect reference sentinels after their environment was deleted, causing a use-after-free. This branch addresses that lifetime defect as well as the conformance changes.

Coverage and limits

The feature-named unit tests cover own and inherited names, shadowing, symbols, arrays, prototype termination, error reporting, and reference lifetime. Proxy-dependent cases are skipped on JavaScriptCore because its prototype lookup does not invoke the Proxy trap. Hermes’ for...in shadowing discrepancy remains tracked in #219; napi_get_all_property_names remains V8-only.

[Copilot]

Pull request overview

This PR aligns napi_get_property_names behavior across the JavaScriptCore, ChakraCore, and QuickJS backends to match the Node-API/V8 semantics: enumerable, string-keyed property names including the prototype chain (i.e., matching for...in, including the “non-enumerable own property shadows inherited enumerable” rule).

Changes:

• Added an engine-agnostic implementation (napi_shared::GetEnumerablePropertyNames) that walks the prototype chain using Object.keys plus Object.getOwnPropertyNames to enforce for...in-equivalent semantics.
• Updated JavaScriptCore, ChakraCore, and QuickJS napi_get_property_names implementations to delegate to the shared helper (and added missing CHECK_ARG(env, object) on JSC/Chakra).
• Added script-level regression tests and a test harness global (napiGetPropertyNames) to validate behavior against for...in.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
Tests/UnitTests/Shared/Shared.cpp	Exposes napiGetPropertyNames to script tests via the C++ harness.
Tests/UnitTests/Scripts/tests.ts	Adds regression tests for napi_get_property_names semantics (prototype chain, enumerability, symbols, shadowing, arrays).
Core/Node-API/Source/js_native_api_shared.h	Declares shared helper for napi_get_property_names semantics.
Core/Node-API/Source/js_native_api_shared.cc	Implements the shared prototype-chain walk using only public napi_* APIs.
Core/Node-API/Source/js_native_api_quickjs.cc	Replaces own-only QuickJS implementation with shared helper.
Core/Node-API/Source/js_native_api_javascriptcore.cc	Fixes JSC implementation (previously throwing) by delegating to shared helper and validating object.
Core/Node-API/Source/js_native_api_chakra.cc	Replaces own-only / non-enumerable-including Chakra implementation with shared helper and validates object.
Core/Node-API/CMakeLists.txt	Adds shared helper sources to non-V8 backend builds.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

Core/Node-API/Source/js_native_api_chakra.cc:688

• napi_shared::GetEnumerablePropertyNames can return napi_object_expected (for null/undefined) without setting env->last_error. Since CHECK_NAPI assumes last_error is already set, callers may observe stale napi_get_last_error_info data. Set last_error explicitly for this status before returning it.

  // `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));

Core/Node-API/Source/js_native_api_javascriptcore.cc:976

• napi_shared::GetEnumerablePropertyNames can return napi_object_expected (for null/undefined) without setting env->last_error. Because CHECK_NAPI assumes last_error was already set, this can leave stale last-error info visible via napi_get_last_error_info when an error is returned. Set last_error explicitly for this status before returning.

  // 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;

Tests/UnitTests/Shared/Shared.cpp:142

• When napi_get_property_names fails with a pending JavaScript exception, the current code clears and discards the original exception and instead throws a generic status-based error. This makes debugging failures harder and can hide the real engine error. If an exception is pending, rethrow that exception value instead of discarding it.

                    // 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);
                    }

Core/Node-API/Source/js_native_api_quickjs.cc:1403

• napi_shared::GetEnumerablePropertyNames can return napi_object_expected (for null/undefined) without setting env->last_error. Because CHECK_NAPI assumes the callee already set last_error, this can leave stale last-error info visible via napi_get_last_error_info even though the API returned an error. Handle this status explicitly and set last_error before returning it.

  // `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);

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

LGTM. Comments inline.

[matthargett]

thanks for coming at N-API! hopefully this means you can review my existing work in this area :D a few review notes:

• This does not duplicate the implementation work in #189, #217, rebeckerspecialties#8, or #10 but it does overlap with PR #116’s already-vendored test_object conformance case, although that directory is not currently enabled.

• Please use PR #189’s JSR_NAPI_ENGINE_* definition family rather than adding a parallel engine-name macro. The current merge of this PR branch against my N-API stack has one Android CMake conflict for exactly this reason.

• Make a focused v1-compatible NodeApi/CTS addon durable ABI test, retaining this PR’s additional shadowing, coercion, poisoned-intrinsic, proxy-cycle, and JSI cases.

• Please design the shared collector so it can support v6 napi_get_all_property_names, or link a required follow-up; the combined v7 stack (#189 ) otherwise still exposes a non-V8 symbol gap.

a couple of other security things to guard against:

• rethrow the pending value in the raw test bridge instead of clearing and replacing it with a generic error
• avoid UTF-8 strings as property-key identity because distinct lone-surrogate keys may collapse

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Core/Node-API/Source/js_native_api_shared.cc:23

• GetUtf8Value converts property names via napi_get_value_string_utf8 and then uses the UTF-8 bytes for shadowing/deduping. In this repo’s V8 implementation, napi_get_value_string_utf8 uses REPLACE_INVALID_UTF8, which can make distinct JS strings (e.g., ones containing unpaired surrogates) collapse to the same UTF-8 sequence. That can break the shadowing rule and cause incorrect results for exotic-but-valid property names. Prefer tracking keys using UTF-16 code units (napi_get_value_string_utf16 + std::u16string) or otherwise comparing actual JS strings rather than their UTF-8 encoding.

    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);

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

Concerns inline.

[Copilot]

Copilot review overview

🟡 Changes recommended

The shared walk depends on mutable JavaScript intrinsics and uses quadratic name deduplication.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Property enumeration depends on mutable JavaScript globals · New

Resolved since last review (1)

• napi_get_prototype calls ToJSObject(env, object), which has an internal assert that the input is…

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

LGTM.