[keys] Trigger [[getOwnPropertyDescriptor]] trap for Object.keys

[keys] Trigger [[getOwnPropertyDescriptor]] trap on proxies for Object.keys

This CL fixes a long-standing bug with Object.keys where the enumerability
check was omitted if the [ownKeys] trap is not present. The only distinction the
KeyAccumulator needs is whether it collects keys for for-in (is_for_in_) or not.
ForInFilter performs a separate step to filter out non-enumerable keys later-on
while in all the other use-cases we have to filter keys.

BUG=v8:1543, v8:5250
Committed: https://crrev.com/f4f06c50298b275b531e510f7521df48778c2b0a
Cr-Commit-Position: refs/heads/master@{#38199}

[Camillo Bruni, 2016-07-28 16:34:36]

cbruni@chromium.org changed reviewers:
+ adamk@chromium.org, caitp@igalia.com

[Camillo Bruni, 2016-07-28 16:34:37]

PTAL
while investigating a possible backmerge for chromium:632305 I stumbled upon
this minor hiccup.

[caitp, 2016-07-28 17:41:59]

Not an expert on the key accumulator, but the simplification looks nice.

https://codereview.chromium.org/2176113009/diff/1/test/mjsunit/es6/proxies-ke...
File test/mjsunit/es6/proxies-keys.js (right):

https://codereview.chromium.org/2176113009/diff/1/test/mjsunit/es6/proxies-ke...
test/mjsunit/es6/proxies-keys.js:38: assertThrows("Object.keys(proxy)", Number);
Is this asserting that eval("Object.keys(proxy)") will throw a Number instance?
that seems strane to me, does this pass?

[adamk, 2016-07-28 17:57:49]

Description was changed from

==========
[keys] Trigger [getOwnPropertyDescriptor] trap for Object.keysd

This CL fixes a long-standing bug with Object.keys where the enumerability
check was omitted if the [ownKeys] trap is not present.

BUG=v8:1543
==========

to

==========
[keys] Trigger [getOwnPropertyDescriptor] trap for Object.keys

This CL fixes a long-standing bug with Object.keys where the enumerability
check was omitted if the [ownKeys] trap is not present.

BUG=v8:1543
==========

[adamk, 2016-07-28 18:03:13]

https://codereview.chromium.org/2176113009/diff/1/src/keys.cc
File src/keys.cc (right):

https://codereview.chromium.org/2176113009/diff/1/src/keys.cc#newcode139
src/keys.cc:139: DCHECK(!is_for_in_);
I think this DCHECK can safely die now :)

https://codereview.chromium.org/2176113009/diff/1/test/mjsunit/es6/proxies-ke...
File test/mjsunit/es6/proxies-keys.js (right):

https://codereview.chromium.org/2176113009/diff/1/test/mjsunit/es6/proxies-ke...
test/mjsunit/es6/proxies-keys.js:26: // assertEquals(["foo", "bar"],
Object.keys(proxy));
I feel like I'm missing something, why is this test now commented out? Same for
the below...

[Camillo Bruni, 2016-07-29 08:24:14]

Description was changed from

==========
[keys] Trigger [getOwnPropertyDescriptor] trap for Object.keys

This CL fixes a long-standing bug with Object.keys where the enumerability
check was omitted if the [ownKeys] trap is not present.

BUG=v8:1543
==========

to

==========
[keys] Trigger [getOwnPropertyDescriptor] trap for Object.keys

This CL fixes a long-standing bug with Object.keys where the enumerability
check was omitted if the [ownKeys] trap is not present. The only distinction the
KeyAccumulator needs is whether it collects keys for for-in (is_for_in_) or not.
ForInFilter performs a separate step to filter out non-enumerable keys later-on 
while in all the other use-cases we have to filter keys.

BUG=v8:1543
==========

[Camillo Bruni, 2016-07-29 08:31:07]

added some more description.

https://codereview.chromium.org/2176113009/diff/1/src/keys.cc
File src/keys.cc (right):

https://codereview.chromium.org/2176113009/diff/1/src/keys.cc#newcode139
src/keys.cc:139: DCHECK(!is_for_in_);
On 2016/07/28 at 18:03:13, adamk wrote:
> I think this DCHECK can safely die now :)

better be sure :D

https://codereview.chromium.org/2176113009/diff/1/test/mjsunit/es6/proxies-ke...
File test/mjsunit/es6/proxies-keys.js (right):

https://codereview.chromium.org/2176113009/diff/1/test/mjsunit/es6/proxies-ke...
test/mjsunit/es6/proxies-keys.js:38: assertThrows("Object.keys(proxy)", Number);
On 2016/07/28 at 17:41:59, caitp wrote:
> Is this asserting that eval("Object.keys(proxy)") will throw a Number
instance? that seems strane to me, does this pass?

Yes, because we have the getOwnPropertyDescriptor trap set to throw a Number.
Given that the proxy has no trap for ownKeys (but the target does have keys) we
still have to check for enumerability according to
https://tc39.github.io/ecma262/#sec-enumerableownproperties

[adamk, 2016-07-29 21:09:30]

lgtm % comment

https://codereview.chromium.org/2176113009/diff/40001/test/mjsunit/es6/proxie...
File test/mjsunit/es6/proxies-keys.js (right):

https://codereview.chromium.org/2176113009/diff/40001/test/mjsunit/es6/proxie...
test/mjsunit/es6/proxies-keys.js:37: // Fall through to target if there is no
trap.
This comment should go below, and this comment should say "Fall through to
getOwnPropertyDescriptor if there is no trap"

[Camillo Bruni, 2016-08-01 08:30:26]

Description was changed from

==========
[keys] Trigger [getOwnPropertyDescriptor] trap for Object.keys

This CL fixes a long-standing bug with Object.keys where the enumerability
check was omitted if the [ownKeys] trap is not present. The only distinction the
KeyAccumulator needs is whether it collects keys for for-in (is_for_in_) or not.
ForInFilter performs a separate step to filter out non-enumerable keys later-on 
while in all the other use-cases we have to filter keys.

BUG=v8:1543
==========

to

==========
[keys] Trigger [getOwnPropertyDescriptor] trap for Object.keys

This CL fixes a long-standing bug with Object.keys where the enumerability
check was omitted if the [ownKeys] trap is not present. The only distinction the
KeyAccumulator needs is whether it collects keys for for-in (is_for_in_) or not.
ForInFilter performs a separate step to filter out non-enumerable keys later-on 
while in all the other use-cases we have to filter keys.

BUG=v8:1543, v8:5250
==========

[Camillo Bruni, 2016-08-01 08:30:52]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-08-01 08:30:52]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2176113009/#ps60001
(title: "fixing comments")

[commit-bot: I haz the power, 2016-08-01 08:30:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-01 08:47:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-01 08:47:54]

Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/5987)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[Camillo Bruni, 2016-08-01 08:52:20]

Description was changed from

==========
[keys] Trigger [getOwnPropertyDescriptor] trap for Object.keys

This CL fixes a long-standing bug with Object.keys where the enumerability
check was omitted if the [ownKeys] trap is not present. The only distinction the
KeyAccumulator needs is whether it collects keys for for-in (is_for_in_) or not.
ForInFilter performs a separate step to filter out non-enumerable keys later-on 
while in all the other use-cases we have to filter keys.

BUG=v8:1543, v8:5250
==========

to

==========
[keys] Trigger [[getOwnPropertyDescriptor]] trap for Object.keys

This CL fixes a long-standing bug with Object.keys where the enumerability
check was omitted if the [ownKeys] trap is not present. The only distinction the
KeyAccumulator needs is whether it collects keys for for-in (is_for_in_) or not.
ForInFilter performs a separate step to filter out non-enumerable keys later-on 
while in all the other use-cases we have to filter keys.

BUG=v8:1543, v8:5250
==========

[Camillo Bruni, 2016-08-01 09:44:22]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-08-01 09:44:22]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2176113009/#ps100001
(title: "eval scoping is hard")

[commit-bot: I haz the power, 2016-08-01 09:44:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Camillo Bruni, 2016-08-01 09:53:59]

Description was changed from

==========
[keys] Trigger [[getOwnPropertyDescriptor]] trap for Object.keys

This CL fixes a long-standing bug with Object.keys where the enumerability
check was omitted if the [ownKeys] trap is not present. The only distinction the
KeyAccumulator needs is whether it collects keys for for-in (is_for_in_) or not.
ForInFilter performs a separate step to filter out non-enumerable keys later-on 
while in all the other use-cases we have to filter keys.

BUG=v8:1543, v8:5250
==========

to

==========
[keys] Trigger [[getOwnPropertyDescriptor]] trap on proxies for Object.keys

This CL fixes a long-standing bug with Object.keys where the enumerability
check was omitted if the [ownKeys] trap is not present. The only distinction the
KeyAccumulator needs is whether it collects keys for for-in (is_for_in_) or not.
ForInFilter performs a separate step to filter out non-enumerable keys later-on 
while in all the other use-cases we have to filter keys.

BUG=v8:1543, v8:5250
==========

[commit-bot: I haz the power, 2016-08-01 10:08:59]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-01 10:11:03]

Description was changed from

==========
[keys] Trigger [[getOwnPropertyDescriptor]] trap on proxies for Object.keys

This CL fixes a long-standing bug with Object.keys where the enumerability
check was omitted if the [ownKeys] trap is not present. The only distinction the
KeyAccumulator needs is whether it collects keys for for-in (is_for_in_) or not.
ForInFilter performs a separate step to filter out non-enumerable keys later-on 
while in all the other use-cases we have to filter keys.

BUG=v8:1543, v8:5250
==========

to

==========
[keys] Trigger [[getOwnPropertyDescriptor]] trap on proxies for Object.keys

This CL fixes a long-standing bug with Object.keys where the enumerability
check was omitted if the [ownKeys] trap is not present. The only distinction the
KeyAccumulator needs is whether it collects keys for for-in (is_for_in_) or not.
ForInFilter performs a separate step to filter out non-enumerable keys later-on
while in all the other use-cases we have to filter keys.

BUG=v8:1543, v8:5250

Committed: https://crrev.com/f4f06c50298b275b531e510f7521df48778c2b0a
Cr-Commit-Position: refs/heads/master@{#38199}
==========

[commit-bot: I haz the power, 2016-08-01 10:11:03]

Patchset 6 (id:??) landed as
https://crrev.com/f4f06c50298b275b531e510f7521df48778c2b0a
Cr-Commit-Position: refs/heads/master@{#38199}