Bug 189034

Summary: for/in over a Proxy should not call [[GetOwnProperty]] trap twice per property
Product: WebKit Reporter: Kevin Gibbons <bakkot>
Component: JavaScriptCoreAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Trivial CC: ashvayka, caitp, darin, ews-watchlist, fpizlo, guijemont, guijemont+jsc-armv7-ews, keith_miller, mark.lam, msaboff, rniwa, ross.kirsling, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=205983
Bug Depends on: 38970, 203818, 212954    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews105 for mac-highsierra-wk2
none
Archive of layout-test-results from ews103 for mac-highsierra
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Archive of layout-test-results from ews206 for win-future
none
Patch none

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>