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
This is a dupe of https://bugs.webkit.org/show_bug.cgi?id=176810, afaik.
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).
Created attachment 367860 [details] Patch
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
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
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
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
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
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
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
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.
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
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
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
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
Created attachment 417241 [details] Patch
(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
(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!
Comment on attachment 417241 [details] Patch r=me
Committed r271305: <https://trac.webkit.org/changeset/271305> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417241 [details].
<rdar://problem/72936255>