Fix Proxy [[OwnPropertyKeys]] check for integer-index keys (#1609)

[tangtaizong666]

Summary

Reflect.ownKeys / Object.keys / Object.getOwnPropertyNames on a Proxy
whose target is non-extensible and has an integer-index own key (e.g.
"12345") throws

TypeError: ownKeys target is non-extensible but key is missing from trap result

even when the trap faithfully returns the target's own keys. This breaks common
state libraries such as Valtio (see #1609), where a frozen snapshot object with
numeric string keys can no longer be enumerated.

Root cause. JSProxy::ownPropertyKeys enforces the ES [[OwnPropertyKeys]]
invariant (steps 19 and 21) by checking that every own key of the target is
present in the trap result using SameValue. Hermes represents integer-index
own-property keys internally as numbers (JSObject::getOwnPropertyKeys
returns 12345 as a Number), whereas the trap result is validated to contain
only Strings and Symbols and therefore holds the key as the String "12345".
SameValue(12345, "12345") is always false, so the invariant check fails and
throws. The comparison only runs for non-extensible targets, which is why the
bug only surfaces once the target is frozen / preventExtensions'd.

Fix. Before the invariant comparisons, normalize any numeric index keys in
targetKeys to their canonical string form — the same conversion that for-in
enumeration already performs for numeric array indices
(Interpreter-slowpaths.cpp, "We must return the property as a string"). The
returned key list is unchanged (it still comes from the trap result), so this
only removes the spurious invariant failure.

Test Plan

New regression test test/hermes/regress-proxy-ownkeys-numeric-key.js covering:

• an integer-index key on a preventExtensions target (configurable-keys loop),
• integer-index keys on a Object.freezed target (non-configurable-keys loop),
• mixed integer/string key ordering, and
• integer-index keys alongside symbols.

$ ./build-rel/bin/hermes -Xes6-proxy -non-strict -O -target=HBC \
    test/hermes/regress-proxy-ownkeys-numeric-key.js
case1: 12345
case2: 0,42
case3: 1,2,b,a
case4: 7,Symbol(s)

The test passes with this change and fails on the unpatched engine with the
reported TypeError. Existing test/hermes/proxy.js continues to pass.

Fixes #1609

[xhon-pelushi]

Built static_h (e9edc8b) with and without this patch and checked it against V8. The diagnosis is right, the fix is right, and it's a bigger win than the description claims. Three suggestions below, none blocking.

Root cause confirmed in source

• JSObject.cpp:507 encodes integer-index own keys with HermesValue::encodeTrustedNumberValue, so targetKeys really does hold 12345 as a Number.
• isSameValue (Operations.cpp:125) returns false on the getTag() mismatch before it ever reaches the deep string comparison, so SameValue(12345, "12345") is false as described.
• inTrapResult (JSProxy.cpp:1507) is the only place in JSProxy.cpp that compares a key. The other three isSameValue calls — lines 201, 912, 1047 — compare descriptor and trap values, not keys. So this is the only affected comparison and the fix doesn't need a sibling.

Two things I checked because they'd have made the patch wrong:

• Mutating targetKeys in place is safe. JSObject::getOwnPropertyKeys allocates a fresh array per call, and targetKeys is never returned — every return path hands back trapResult or the separate step-6a array. The normalized list is only ever used for the comparisons.
• The approach is already the in-repo idiom, not a new invention. getOwnPropertyKeysAsStrings (lib/VM/JSLib/Object.cpp:490) is the same in-place number→string JSArray::setElementAt loop with the same GC-marker flushing, and the single-element version appears at Interpreter-slowpaths.cpp:570 and StaticH.cpp:2365. Placing the block after the step-17 early return also keeps it off the common all-extensible path.

It reproduces, and the fix holds

Unpatched, the new test fails exactly as claimed, and both invariant messages are reachable depending on the shape:

$ ./build-rel/bin/hermes -Xes6-proxy -non-strict -O -target=HBC test/hermes/regress-proxy-ownkeys-numeric-key.js
Uncaught TypeError: ownKeys target is non-extensible but key is missing from trap result
    at keys (native)
    at global (test/hermes/regress-proxy-ownkeys-numeric-key.js:34:28)

$ ./build-rel/bin/hermes -Xes6-proxy i1609.js          # Object.freeze({123:'x'}) behind a passthrough trap
Uncaught TypeError: ownKeys target key is non-configurable but not present in trap result

Patched, the test prints the expected four lines and the issue repro returns ["123"], matching V8.

Regression sweep. I ran all 498 test/hermes/*.js under one uniform invocation on both builds and diffed. Six files differed; five of them (array-literal-large, date-constructor, narrow-trusted-regress, put-to-transient, regress-error-stack-native-stack-overflow) also differ base-against-base on a rerun, so they're nondeterministic and not attributable. The only real change is regress-proxy-ownkeys-numeric-key.js going 1 → 0. test/hermes/proxy.js is byte-identical.

Differential test against V8. 27 cases covering index keys, ordering, symbols, boundary indices, non-canonical numeric-looking keys, the negative invariant cases, and the consumers (Object.keys, getOwnPropertyNames, JSON.stringify, spread, for-in, nested proxies), run on node 18 and both Hermes builds:

	disagreements with V8
Hermes before	15 of 27
Hermes after	0 of 27

Importantly the five negative cases still throw — trap omitting the index, returning the wrong index, returning an extra key, returning a Number, and returning duplicates all still raise TypeError, same as V8. The fix removes the spurious failures without loosening the invariant.

Stress. 5000 integer-index keys on a frozen target, Reflect.ownKeys 50 times: no crash or handle-scope trouble, and the result matches V8 (len=5000, first/last both string).

1. The description under-sells the scope: it isn't only non-extensible targets

The comparison only runs for non-extensible targets, which is why the bug only surfaces once the target is frozen / preventExtensions'd.

Step 19's loop runs whenever targetNonconfigurableKeys is non-empty, independent of extensibility — if (*extensibleRes) return is step 20, after it. So a perfectly extensible object with one non-configurable integer-index property also throws:

var o = {};
Object.defineProperty(o, '5', {value: 1, configurable: false, enumerable: true});
Reflect.ownKeys(new Proxy(o, {ownKeys: t => Reflect.ownKeys(t)}));
// before: TypeError: ownKeys target key is non-configurable but not present in trap result
// after:  ["5"]     (V8: ["5"])

That's also the message @tmikov got in the first repro on #1609, so it's the path the original reporter hit. Worth fixing the wording since it makes the change look narrower than it is.

The other case I'd highlight is a frozen array, which I'd guess is at least as common in the wild as numeric string keys:

Reflect.ownKeys(new Proxy(Object.freeze([1,2,3]), {ownKeys: t => Reflect.ownKeys(t)}));
// before: TypeError    after: ["0","1","2","length"]   (V8: same)

2. Two test cases worth adding

The four cases in the new test all have extensibleTarget === false, so they only ever exercise the step-21 loop plus step 19. Neither of the shapes above is covered:

• extensible target with a non-configurable integer-index key — the only case where step 19 fires and step 20 returns before step 21, i.e. the one path where the normalization matters but step 21 never runs.
• Object.freeze([1,2,3]) behind a proxy — index keys alongside "length", and the likely real-world shape.

3. Minor: the loop converts more keys than get compared

The normalization walks every numeric entry in targetKeys, but when extensibleTarget is true only the nonConfigurable entries are ever passed to inTrapResult — step 20 returns before the step-21 loop. So an extensible target with one non-configurable index key among a thousand index keys pays a thousand numberToStringPrimitive allocations to perform one comparison. Converting inside inTrapResult, or restricting the loop to the indices that will actually be compared, would avoid that. Very much a nit given Proxy's overall cost here, but it's nearly free to fix.

What I did not verify

FileCheck isn't built in my tree, so the regression sweep used one uniform flag set across test/hermes/*.js rather than each test's own RUN line and compared base-vs-patched output rather than checking CHECK: directives — good for spotting differences caused by this patch, not a substitute for check-hermes. I also only exercised the -target=HBC CLI path; JSProxy::ownPropertyKeys is the single implementation (reached from JSObject.cpp:432), so the SH path should be covered too, but I didn't run it, and nothing was tested on-device under React Native.