Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 4 months ago by Camillo Bruni Modified:
4 years, 4 months ago Reviewers:
caitp, adamk CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[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}
Patch Set 1 #
Total comments: 5
Patch Set 2 : addressing comments #Patch Set 3 : cleaning up test #
Total comments: 1
Patch Set 4 : fixing comments #Patch Set 5 : rebasing #Patch Set 6 : eval scoping is hard #
Created: 4 years, 4 months ago
Messages
Total messages: 22 (12 generated)
cbruni@chromium.org changed reviewers: + adamk@chromium.org, caitp@igalia.com
PTAL while investigating a possible backmerge for chromium:632305 I stumbled upon this minor hiccup.
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?
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 ==========
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...
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 ==========
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
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"
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 ==========
The CQ bit was checked by cbruni@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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 ==========
The CQ bit was checked by cbruni@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f4f06c50298b275b531e510f7521df48778c2b0a Cr-Commit-Position: refs/heads/master@{#38199} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
