Bug 189034 – for/in over a Proxy should not call [[GetOwnProperty]] trap twice per property

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Kevin Gibbons]

Consider the following program:

```
if (typeof console === 'undefined') console = { log: print };

let a = Object.create(null, {
  x: { enumerable: false, configurable: true, value: 0 },
});

let handler = {
  getOwnPropertyDescriptor(t, p) {
    console.log('gopd');
    let o = Reflect.getOwnPropertyDescriptor(t, p);
    o.enumerable = true;
    return o;
  },
};

let pa = new Proxy(a, handler);

for (let key in pa) {
  console.log('reached');
}
```

This prints nothing. It should print `gopd` and `reached`, like every other browser. The spec, in #sec-enumerate-object-properties, requires that for-in enumeration determines enumerability by calling [[GetOwnProperty]], which on proxies means an observable invocation of the getOwnPropertyDescriptor trap.

JSC appears to be relying on the enumerability of the target's property directly, which is bad.

This only happens if the `ownKeys` handler is not present, even with the default behavior. That is, adding `ownKeys(target) { return Reflect.ownKeys(target); },` to the proxy's handler causes the program to behave correctly.

See also https://bugs.webkit.org/show_bug.cgi?id=189030. These two might have the same root cause - from the observable behavior, it looks like some code is assuming that `ownKeys` only returns enumerable properties, which is not its behavior (even in JSC).

See also (and please comment on) this open spec bug about more precisely specifying the behavior of for-in, which prompted the investigation which lead me to discovering these issues: https://github.com/tc39/ecma262/issues/1281

[Caitlin Potter (:caitp)]

This is a dupe of https://bugs.webkit.org/show_bug.cgi?id=176810, afaik.

[Caitlin Potter (:caitp)]

My earlier comment was incorrect (as was pointed out elsewhere) --- I've written a fix for this, which I think is complete.

I'd like to run it on EWS, and if everything is green, I believe it will allow the complete removal of op_has_indexed_property, op_has_structure_property and op_has_generic_property (which AFAIK are not used by anything other than ForIn loops).

[Caitlin Potter (:caitp)]

Created attachment 367860 [details]
Patch

[EWS Watchlist]

Comment on attachment 367860 [details]
Patch

Attachment 367860 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11937584

New failing tests:
js/for-in-modify-in-loop.html

[EWS Watchlist]

Created attachment 367864 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 367860 [details]
Patch

Attachment 367860 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11937698

New failing tests:
js/for-in-modify-in-loop.html

[EWS Watchlist]

Created attachment 367866 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 367860 [details]
Patch

Attachment 367860 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11937763

New failing tests:
js/for-in-modify-in-loop.html

[EWS Watchlist]

Created attachment 367869 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[jsc-armv7 EWS]

Comment on attachment 367860 [details]
Patch

Attachment 367860 [details] did not pass jsc-armv7-ews (jsc-only):
Output: https://webkit-queues.webkit.org/results/11937875

New failing tests:
stress/for-in-delete-during-iteration.js.no-cjit-validate-phases
stress/for-in-side-effects.js.dfg-eager
stress/proxy-get-own-property.js.dfg-eager
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-dfg-eager-no-cjit
stress/proxy-get-own-property.js.no-cjit-collect-continuously
stress/for-in-side-effects.js.dfg-eager-no-cjit-validate
stress/for-in-side-effects.js.dfg-maximal-flush-validate-no-cjit
ChakraCore.yaml/ChakraCore/test/ControlFlow/forInObjectAddDelete.js.default
stress/proxy-get-own-property.js.dfg-maximal-flush-validate-no-cjit
stress/proxy-get-own-property.js.no-cjit-validate-phases
stress/for-in-delete-during-iteration.js.dfg-maximal-flush-validate-no-cjit
stress/for-in-delete-during-iteration.js.default
stress/for-in-delete-during-iteration.js.no-cjit-collect-continuously
stress/proxy-get-own-property.js.default
stress/proxy-get-own-property.js.no-llint
stress/for-in-delete-during-iteration.js.dfg-eager
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-no-llint
stress/for-in-side-effects.js.no-cjit-collect-continuously
stress/for-in-side-effects.js.no-cjit-validate-phases
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-no-cjit
ChakraCore.yaml/ChakraCore/test/ControlFlow/enumeration_adddelete.js.default
stress/for-in-side-effects.js.default
stress/for-in-delete-during-iteration.js.dfg-eager-no-cjit-validate
stress/proxy-get-own-property.js.dfg-eager-no-cjit-validate
stress/for-in-delete-during-iteration.js.no-llint
stress/for-in-side-effects.js.no-llint
ChakraCore.yaml/ChakraCore/test/Object/forinenumcache.js.default
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout
apiTests

[Caitlin Potter (:caitp)]

Ah, re-reading the spec, this isn't quite going to work, since For-In can't eagerly check the property descriptor immediately after [[OwnPropertyKeys]] :\

Will have to figure out another way. Ignore the R=? for now.

[EWS Watchlist]

Comment on attachment 367860 [details]
Patch

Attachment 367860 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11938412

New failing tests:
js/for-in-modify-in-loop.html

[EWS Watchlist]

Comment on attachment 367860 [details]
Patch

Attachment 367860 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/11938025

New failing tests:
stress/proxy-get-own-property.js.ftl-eager-no-cjit
stress/for-in-delete-during-iteration.js.no-cjit-validate-phases
stress/for-in-side-effects.js.dfg-eager
stress/proxy-get-own-property.js.ftl-no-cjit-no-inline-validate
stress/for-in-delete-during-iteration.js.ftl-eager
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-dfg-eager-no-cjit
stress/for-in-side-effects.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-no-llint
stress/for-in-side-effects.js.dfg-eager-no-cjit-validate
stress/proxy-get-own-property.js.ftl-eager
stress/for-in-delete-during-iteration.js.ftl-no-cjit-validate-sampling-profiler
stress/for-in-delete-during-iteration.js.ftl-no-cjit-b3o0
stress/for-in-delete-during-iteration.js.ftl-no-cjit-no-inline-validate
stress/for-in-delete-during-iteration.js.ftl-no-cjit-no-put-stack-validate
stress/proxy-get-own-property.js.ftl-no-cjit-no-put-stack-validate
ChakraCore.yaml/ChakraCore/test/ControlFlow/forInObjectAddDelete.js.default
stress/proxy-get-own-property.js.dfg-maximal-flush-validate-no-cjit
stress/proxy-get-own-property.js.no-cjit-validate-phases
stress/for-in-delete-during-iteration.js.no-cjit-collect-continuously
stress/for-in-delete-during-iteration.js.default
stress/for-in-delete-during-iteration.js.dfg-maximal-flush-validate-no-cjit
stress/for-in-delete-during-iteration.js.ftl-eager-no-cjit-b3o1
stress/proxy-get-own-property.js.default
stress/proxy-get-own-property.js.no-llint
stress/proxy-get-own-property.js.ftl-no-cjit-validate-sampling-profiler
stress/for-in-side-effects.js.ftl-no-cjit-validate-sampling-profiler
stress/for-in-delete-during-iteration.js.dfg-eager
stress/for-in-side-effects.js.ftl-no-cjit-no-put-stack-validate
stress/for-in-side-effects.js.dfg-maximal-flush-validate-no-cjit
stress/proxy-get-own-property.js.no-cjit-collect-continuously
stress/for-in-delete-during-iteration.js.ftl-no-cjit-small-pool
stress/for-in-side-effects.js.no-cjit-collect-continuously
stress/proxy-get-own-property.js.no-ftl
ChakraCore.yaml/ChakraCore/test/Object/forinenumcache.js.default
stress/for-in-side-effects.js.no-cjit-validate-phases
stress/for-in-delete-during-iteration.js.bytecode-cache
stress/for-in-side-effects.js.no-ftl
stress/for-in-side-effects.js.ftl-eager-no-cjit
stress/for-in-side-effects.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-no-cjit
ChakraCore.yaml/ChakraCore/test/ControlFlow/enumeration_adddelete.js.default
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-ftl-eager-no-cjit
stress/for-in-side-effects.js.default
stress/for-in-delete-during-iteration.js.dfg-eager-no-cjit-validate
stress/proxy-get-own-property.js.ftl-eager-no-cjit-b3o1
stress/for-in-delete-during-iteration.js.no-ftl
stress/for-in-side-effects.js.ftl-no-cjit-small-pool
stress/proxy-get-own-property.js.dfg-eager-no-cjit-validate
stress/for-in-delete-during-iteration.js.no-llint
stress/for-in-side-effects.js.no-llint
stress/for-in-delete-during-iteration.js.ftl-eager-no-cjit
stress/proxy-get-own-property.js.ftl-no-cjit-b3o0
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-ftl-no-cjit
stress/for-in-side-effects.js.ftl-no-cjit-b3o0
stress/for-in-side-effects.js.ftl-eager
stress/proxy-get-own-property.js.ftl-no-cjit-small-pool
stress/proxy-get-own-property.js.dfg-eager
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout
stress/for-in-side-effects.js.ftl-no-cjit-no-inline-validate
stress/proxy-get-own-property.js.bytecode-cache

[EWS Watchlist]

Created attachment 367878 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 367860 [details]
Patch

Attachment 367860 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/11938958

New failing tests:
stress/proxy-get-own-property.js.ftl-eager-no-cjit
stress/for-in-delete-during-iteration.js.no-cjit-validate-phases
stress/for-in-side-effects.js.dfg-eager
stress/proxy-get-own-property.js.ftl-no-cjit-no-inline-validate
stress/for-in-delete-during-iteration.js.ftl-eager
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-dfg-eager-no-cjit
stress/for-in-side-effects.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-no-llint
stress/for-in-side-effects.js.dfg-eager-no-cjit-validate
stress/proxy-get-own-property.js.ftl-eager
stress/for-in-delete-during-iteration.js.ftl-no-cjit-validate-sampling-profiler
stress/for-in-delete-during-iteration.js.ftl-no-cjit-b3o0
stress/for-in-delete-during-iteration.js.ftl-no-cjit-no-inline-validate
stress/for-in-delete-during-iteration.js.ftl-no-cjit-no-put-stack-validate
stress/proxy-get-own-property.js.ftl-no-cjit-no-put-stack-validate
ChakraCore.yaml/ChakraCore/test/ControlFlow/forInObjectAddDelete.js.default
stress/proxy-get-own-property.js.dfg-maximal-flush-validate-no-cjit
stress/proxy-get-own-property.js.no-cjit-validate-phases
stress/for-in-delete-during-iteration.js.no-cjit-collect-continuously
stress/for-in-delete-during-iteration.js.default
stress/for-in-delete-during-iteration.js.dfg-maximal-flush-validate-no-cjit
stress/for-in-delete-during-iteration.js.ftl-eager-no-cjit-b3o1
stress/proxy-get-own-property.js.default
stress/proxy-get-own-property.js.no-llint
stress/proxy-get-own-property.js.ftl-no-cjit-validate-sampling-profiler
stress/for-in-side-effects.js.ftl-no-cjit-validate-sampling-profiler
stress/for-in-delete-during-iteration.js.dfg-eager
stress/for-in-side-effects.js.ftl-no-cjit-no-put-stack-validate
stress/for-in-side-effects.js.dfg-maximal-flush-validate-no-cjit
stress/proxy-get-own-property.js.no-cjit-collect-continuously
stress/for-in-delete-during-iteration.js.ftl-no-cjit-small-pool
stress/for-in-side-effects.js.no-cjit-collect-continuously
stress/proxy-get-own-property.js.no-ftl
ChakraCore.yaml/ChakraCore/test/Object/forinenumcache.js.default
stress/for-in-side-effects.js.no-cjit-validate-phases
stress/for-in-delete-during-iteration.js.bytecode-cache
stress/for-in-side-effects.js.no-ftl
stress/for-in-side-effects.js.ftl-eager-no-cjit
stress/for-in-side-effects.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-no-cjit
ChakraCore.yaml/ChakraCore/test/ControlFlow/enumeration_adddelete.js.default
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-ftl-eager-no-cjit
stress/for-in-side-effects.js.default
stress/for-in-delete-during-iteration.js.dfg-eager-no-cjit-validate
stress/proxy-get-own-property.js.ftl-eager-no-cjit-b3o1
stress/for-in-delete-during-iteration.js.no-ftl
stress/for-in-side-effects.js.ftl-no-cjit-small-pool
stress/proxy-get-own-property.js.dfg-eager-no-cjit-validate
stress/for-in-delete-during-iteration.js.no-llint
stress/for-in-side-effects.js.no-llint
stress/for-in-delete-during-iteration.js.ftl-eager-no-cjit
stress/proxy-get-own-property.js.ftl-no-cjit-b3o0
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-ftl-no-cjit
stress/for-in-side-effects.js.ftl-no-cjit-b3o0
stress/for-in-side-effects.js.ftl-eager
stress/proxy-get-own-property.js.ftl-no-cjit-small-pool
stress/proxy-get-own-property.js.dfg-eager
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/for-in-modify-in-loop.js.layout
stress/for-in-side-effects.js.ftl-no-cjit-no-inline-validate
stress/proxy-get-own-property.js.bytecode-cache

[Alexey Shvayka]

Created attachment 417241 [details]
Patch

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #16)
> Created attachment 417241 [details]
> Patch

                         r271191                    patch

for-in-proxy        134.0518+-2.8758     ^     66.2318+-1.0129        ^ definitely 2.0240x faster

<geometric>         134.0518+-2.8758     ^     66.2318+-1.0129        ^ definitely 2.0240x faster

[Alexey Shvayka]

(In reply to Caitlin Potter (:caitp) from comment #3)
> Created attachment 367860 [details]
> Patch

This patch gets a lot things right:

1. JSModuleNamespaceObject.cpp change was landed as-is in r254390.
2. A bit different approach to fixing ProxyObject.cpp was landed in r254070.
3. proxy-for-in.js stress test will be landed as-is in the new patch.

Thanks Caitlin!

[Yusuke Suzuki]

Comment on attachment 417241 [details]
Patch

r=me

[EWS]

Committed r271305: <https://trac.webkit.org/changeset/271305>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417241 [details].

[Radar WebKit Bug Importer]

<rdar://problem/72936255>