Bug 189034 - for/in over a Proxy should not call [[GetOwnProperty]] trap twice per property
Summary: for/in over a Proxy should not call [[GetOwnProperty]] trap twice per property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Trivial
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on: 38970 203818 212954
Blocks:
  Show dependency treegraph
 
Reported: 2018-08-27 16:36 PDT by Kevin Gibbons
Modified: 2021-01-08 10:53 PST (History)
16 users (show)

See Also:


Attachments
Patch (14.67 KB, patch)
2019-04-19 18:39 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (3.94 MB, application/zip)
2019-04-19 19:34 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews103 for mac-highsierra (3.26 MB, application/zip)
2019-04-19 19:44 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.81 MB, application/zip)
2019-04-19 20:37 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews206 for win-future (13.07 MB, application/zip)
2019-04-19 22:44 PDT, EWS Watchlist
no flags Details
Patch (8.58 KB, patch)
2021-01-07 20:38 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Gibbons 2018-08-27 16:36:11 PDT
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
Comment 1 Caitlin Potter (:caitp) 2019-02-22 10:51:34 PST
This is a dupe of https://bugs.webkit.org/show_bug.cgi?id=176810, afaik.
Comment 2 Caitlin Potter (:caitp) 2019-04-19 18:20:39 PDT
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).
Comment 3 Caitlin Potter (:caitp) 2019-04-19 18:39:42 PDT
Created attachment 367860 [details]
Patch
Comment 4 EWS Watchlist 2019-04-19 19:34:40 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-04-19 19:34:42 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-04-19 19:44:14 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-04-19 19:44:16 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2019-04-19 20:37:28 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2019-04-19 20:37:30 PDT Comment hidden (obsolete)
Comment 10 jsc-armv7 EWS 2019-04-19 21:28:48 PDT Comment hidden (obsolete)
Comment 11 Caitlin Potter (:caitp) 2019-04-19 21:31:55 PDT
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 12 EWS Watchlist 2019-04-19 22:44:10 PDT Comment hidden (obsolete)
Comment 13 EWS Watchlist 2019-04-19 22:44:10 PDT Comment hidden (obsolete)
Comment 14 EWS Watchlist 2019-04-19 22:44:22 PDT Comment hidden (obsolete)
Comment 15 EWS Watchlist 2019-04-20 01:42:47 PDT Comment hidden (obsolete)
Comment 16 Alexey Shvayka 2021-01-07 20:38:06 PST
Created attachment 417241 [details]
Patch
Comment 17 Alexey Shvayka 2021-01-07 20:41:36 PST
(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
Comment 18 Alexey Shvayka 2021-01-07 20:48:24 PST
(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 19 Yusuke Suzuki 2021-01-08 10:48:43 PST
Comment on attachment 417241 [details]
Patch

r=me
Comment 20 EWS 2021-01-08 10:52:32 PST
Committed r271305: <https://trac.webkit.org/changeset/271305>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417241 [details].
Comment 21 Radar WebKit Bug Importer 2021-01-08 10:53:19 PST
<rdar://problem/72936255>