Fix Proxy [[OwnPropertyKeys]] check for integer-index keys (#1609)#2095
Open
tangtaizong666 wants to merge 1 commit into
Open
Fix Proxy [[OwnPropertyKeys]] check for integer-index keys (#1609)#2095tangtaizong666 wants to merge 1 commit into
tangtaizong666 wants to merge 1 commit into
Conversation
…1609) JSProxy::ownPropertyKeys enforces the ES [[OwnPropertyKeys]] invariant by checking that every own key of the target is present in the ownKeys trap result using SameValue. Hermes represents integer-index own-property keys (e.g. "12345") internally as numbers, while the trap result is validated to hold only Strings and Symbols. SameValue(Number, String) is always false, so any integer-index own key on a non-extensible target spuriously failed the invariant and threw "ownKeys target is non-extensible but key is missing from trap result". Normalize the numeric index keys in targetKeys to their canonical string form before the invariant comparisons, mirroring how for-in enumeration already converts numeric array indices to strings. The returned key list is unchanged, so this only removes the spurious failure. Fixes facebook#1609.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reflect.ownKeys/Object.keys/Object.getOwnPropertyNameson aProxywhose target is non-extensible and has an integer-index own key (e.g.
"12345") throwseven 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::ownPropertyKeysenforces 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-indexown-property keys internally as numbers (
JSObject::getOwnPropertyKeysreturns
12345as a Number), whereas the trap result is validated to containonly Strings and Symbols and therefore holds the key as the String
"12345".SameValue(12345, "12345")is alwaysfalse, so the invariant check fails andthrows. 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
targetKeysto their canonical string form — the same conversion that for-inenumeration already performs for numeric array indices
(
Interpreter-slowpaths.cpp, "We must return the property as a string"). Thereturned 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.jscovering:preventExtensionstarget (configurable-keys loop),Object.freezed target (non-configurable-keys loop),The test passes with this change and fails on the unpatched engine with the
reported
TypeError. Existingtest/hermes/proxy.jscontinues to pass.Fixes #1609