[keys] propagate PropertyFilter to proxy targets in KeyAccumulator

[keys] propagate PropertyFilter to proxy targets in KeyAccumulator

BUG=v8:5174, v8:1543
R=cbruni@chromium.org, littledan@chromium.org

Committed: https://crrev.com/08d0012dda22306eee124b62a3de9829f3961ba8
Cr-Commit-Position: refs/heads/master@{#37634}

[caitp, 2016-07-08 17:47:13]

PTAL --- finally set up to use the right email account

[Dan Ehrenberg, 2016-07-08 21:33:11]

Could you also include the regression test from the bug?

[caitp, 2016-07-08 21:38:31]

On 2016/07/08 21:33:11, Dan Ehrenberg wrote:
> Could you also include the regression test from the bug?

added

[caitp, 2016-07-08 21:40:36]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 21:40:48]

Dry run: 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-07-08 21:40:50]

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

[commit-bot: I haz the power, 2016-07-08 21:40:51]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[Dan Ehrenberg, 2016-07-08 21:47:26]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 21:47:37]

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

[Dan Ehrenberg, 2016-07-08 22:02:07]

https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc
File src/keys.cc (right):

https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc#newcode862
src/keys.cc:862: KeyAccumulator::GetKeys(target, KeyCollectionMode::kOwnOnly,
filter_,
It looks like the relevant change here is passing the filter_, where previously,
ALL_PROPERTIES was passed by JSReciever::OwnPropertyKeys.

What is the function of getting the key conversion to be passed through? I don't
see the harm in converting numeric keys to strings here, and Object.keys
parameterizes it that way anyway.

[caitp, 2016-07-08 22:08:27]

On 2016/07/08 22:02:07, Dan Ehrenberg wrote:
> https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc
> File src/keys.cc (right):
> 
> https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc#newcode862
> src/keys.cc:862: KeyAccumulator::GetKeys(target, KeyCollectionMode::kOwnOnly,
> filter_,
> It looks like the relevant change here is passing the filter_, where
previously,
> ALL_PROPERTIES was passed by JSReciever::OwnPropertyKeys.
> 
> What is the function of getting the key conversion to be passed through? I
don't
> see the harm in converting numeric keys to strings here, and Object.keys
> parameterizes it that way anyway.

basically, Object.values/entries were duplicating some old code from
KeyAccumulator, and the original version of this change didn't update them to
explicitly using kConvertToString (which it depends on) --- so not changing this
stuff caused crashes.

The change to GetKeysConversion lets us reuse existing code and still work
correctly, which is nice.

[caitp, 2016-07-08 22:08:30]

On 2016/07/08 22:02:07, Dan Ehrenberg wrote:
> https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc
> File src/keys.cc (right):
> 
> https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc#newcode862
> src/keys.cc:862: KeyAccumulator::GetKeys(target, KeyCollectionMode::kOwnOnly,
> filter_,
> It looks like the relevant change here is passing the filter_, where
previously,
> ALL_PROPERTIES was passed by JSReciever::OwnPropertyKeys.
> 
> What is the function of getting the key conversion to be passed through? I
don't
> see the harm in converting numeric keys to strings here, and Object.keys
> parameterizes it that way anyway.

basically, Object.values/entries were duplicating some old code from
KeyAccumulator, and the original version of this change didn't update them to
explicitly using kConvertToString (which it depends on) --- so not changing this
stuff caused crashes.

The change to GetKeysConversion lets us reuse existing code and still work
correctly, which is nice.

[commit-bot: I haz the power, 2016-07-08 22:11:32]

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

[commit-bot: I haz the power, 2016-07-08 22:11:33]

Dry run: This issue passed the CQ dry run.

[caitp, 2016-07-08 22:25:36]

On 2016/07/08 22:08:30, caitp wrote:
> On 2016/07/08 22:02:07, Dan Ehrenberg wrote:
> > https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc
> > File src/keys.cc (right):
> > 
> > https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc#newcode862
> > src/keys.cc:862: KeyAccumulator::GetKeys(target,
KeyCollectionMode::kOwnOnly,
> > filter_,
> > It looks like the relevant change here is passing the filter_, where
> previously,
> > ALL_PROPERTIES was passed by JSReciever::OwnPropertyKeys.
> > 
> > What is the function of getting the key conversion to be passed through? I
> don't
> > see the harm in converting numeric keys to strings here, and Object.keys
> > parameterizes it that way anyway.
> 
> basically, Object.values/entries were duplicating some old code from
> KeyAccumulator, and the original version of this change didn't update them to
> explicitly using kConvertToString (which it depends on) --- so not changing
this
> stuff caused crashes.
> 
> The change to GetKeysConversion lets us reuse existing code and still work
> correctly, which is nice.

I just realized that didn't really explain the problem. Anyways, it may be
possible to get it working, but previously it did not, and this was the easiest
way to keep it simple.

[Dan Ehrenberg, 2016-07-08 22:28:22]

On 2016/07/08 at 22:08:30, caitp wrote:
> On 2016/07/08 22:02:07, Dan Ehrenberg wrote:
> > https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc
> > File src/keys.cc (right):
> > 
> > https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc#newcode862
> > src/keys.cc:862: KeyAccumulator::GetKeys(target,
KeyCollectionMode::kOwnOnly,
> > filter_,
> > It looks like the relevant change here is passing the filter_, where
previously,
> > ALL_PROPERTIES was passed by JSReciever::OwnPropertyKeys.
> > 
> > What is the function of getting the key conversion to be passed through? I
don't
> > see the harm in converting numeric keys to strings here, and Object.keys
> > parameterizes it that way anyway.
> 
> basically, Object.values/entries were duplicating some old code from
KeyAccumulator, and the original version of this change didn't update them to
explicitly using kConvertToString (which it depends on) --- so not changing this
stuff caused crashes.
> 
> The change to GetKeysConversion lets us reuse existing code and still work
correctly, which is nice.

That all sounds good. What I'm wondering about is why you threaded through they
keys conversion type, rather than just hard-coding kConvertToString in
CollectOwnJSProxyTargetKeys as it was previously. What code path will use a
different value, or where does the crash come in?

[caitp, 2016-07-08 22:44:43]

On 2016/07/08 22:28:22, Dan Ehrenberg wrote:
> On 2016/07/08 at 22:08:30, caitp wrote:
> > On 2016/07/08 22:02:07, Dan Ehrenberg wrote:
> > > https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc
> > > File src/keys.cc (right):
> > > 
> > >
https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc#newcode862
> > > src/keys.cc:862: KeyAccumulator::GetKeys(target,
> KeyCollectionMode::kOwnOnly,
> > > filter_,
> > > It looks like the relevant change here is passing the filter_, where
> previously,
> > > ALL_PROPERTIES was passed by JSReciever::OwnPropertyKeys.
> > > 
> > > What is the function of getting the key conversion to be passed through? I
> don't
> > > see the harm in converting numeric keys to strings here, and Object.keys
> > > parameterizes it that way anyway.
> > 
> > basically, Object.values/entries were duplicating some old code from
> KeyAccumulator, and the original version of this change didn't update them to
> explicitly using kConvertToString (which it depends on) --- so not changing
this
> stuff caused crashes.
> > 
> > The change to GetKeysConversion lets us reuse existing code and still work
> correctly, which is nice.
> 
> That all sounds good. What I'm wondering about is why you threaded through
they
> keys conversion type, rather than just hard-coding kConvertToString in
> CollectOwnJSProxyTargetKeys as it was previously. What code path will use a
> different value, or where does the crash come in?

So, we don't validate proxy keys when they don't have a trap --- which means
that it isn't guaranteed that they come back as what they should be.

What would up happening is, numbers were encoded as Smis, and trying to cast
them to Handle<Name> and then relying on Name::GetIterator() causes a crash.

But, you're right, hard coding with kConvertToString probably works around that.
I'll give that a shot.

[caitp, 2016-07-08 22:45:24]

On 2016/07/08 22:44:43, caitp wrote:
> On 2016/07/08 22:28:22, Dan Ehrenberg wrote:
> > On 2016/07/08 at 22:08:30, caitp wrote:
> > > On 2016/07/08 22:02:07, Dan Ehrenberg wrote:
> > > > https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc
> > > > File src/keys.cc (right):
> > > > 
> > > >
> https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc#newcode862
> > > > src/keys.cc:862: KeyAccumulator::GetKeys(target,
> > KeyCollectionMode::kOwnOnly,
> > > > filter_,
> > > > It looks like the relevant change here is passing the filter_, where
> > previously,
> > > > ALL_PROPERTIES was passed by JSReciever::OwnPropertyKeys.
> > > > 
> > > > What is the function of getting the key conversion to be passed through?
I
> > don't
> > > > see the harm in converting numeric keys to strings here, and Object.keys
> > > > parameterizes it that way anyway.
> > > 
> > > basically, Object.values/entries were duplicating some old code from
> > KeyAccumulator, and the original version of this change didn't update them
to
> > explicitly using kConvertToString (which it depends on) --- so not changing
> this
> > stuff caused crashes.
> > > 
> > > The change to GetKeysConversion lets us reuse existing code and still work
> > correctly, which is nice.
> > 
> > That all sounds good. What I'm wondering about is why you threaded through
> they
> > keys conversion type, rather than just hard-coding kConvertToString in
> > CollectOwnJSProxyTargetKeys as it was previously. What code path will use a
> > different value, or where does the crash come in?
> 
> So, we don't validate proxy keys when they don't have a trap --- which means
> that it isn't guaranteed that they come back as what they should be.
> 
> What would end up happening is, numbers were encoded as Smis, and trying to
cast
> them to Handle<Name> and then relying on Name::GetIterator() causes a crash.
> 
> But, you're right, hard coding with kConvertToString probably works around
that.
> I'll give that a shot.

[Dan Ehrenberg, 2016-07-08 22:47:34]

On 2016/07/08 at 22:44:43, caitp wrote:
> On 2016/07/08 22:28:22, Dan Ehrenberg wrote:
> > On 2016/07/08 at 22:08:30, caitp wrote:
> > > On 2016/07/08 22:02:07, Dan Ehrenberg wrote:
> > > > https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc
> > > > File src/keys.cc (right):
> > > > 
> > > >
https://codereview.chromium.org/2129193003/diff/60001/src/keys.cc#newcode862
> > > > src/keys.cc:862: KeyAccumulator::GetKeys(target,
> > KeyCollectionMode::kOwnOnly,
> > > > filter_,
> > > > It looks like the relevant change here is passing the filter_, where
> > previously,
> > > > ALL_PROPERTIES was passed by JSReciever::OwnPropertyKeys.
> > > > 
> > > > What is the function of getting the key conversion to be passed through?
I
> > don't
> > > > see the harm in converting numeric keys to strings here, and Object.keys
> > > > parameterizes it that way anyway.
> > > 
> > > basically, Object.values/entries were duplicating some old code from
> > KeyAccumulator, and the original version of this change didn't update them
to
> > explicitly using kConvertToString (which it depends on) --- so not changing
this
> > stuff caused crashes.
> > > 
> > > The change to GetKeysConversion lets us reuse existing code and still work
> > correctly, which is nice.
> > 
> > That all sounds good. What I'm wondering about is why you threaded through
they
> > keys conversion type, rather than just hard-coding kConvertToString in
> > CollectOwnJSProxyTargetKeys as it was previously. What code path will use a
> > different value, or where does the crash come in?
> 
> So, we don't validate proxy keys when they don't have a trap --- which means
that it isn't guaranteed that they come back as what they should be.
> 
> What would up happening is, numbers were encoded as Smis, and trying to cast
them to Handle<Name> and then relying on Name::GetIterator() causes a crash.
> 
> But, you're right, hard coding with kConvertToString probably works around
that. I'll give that a shot.

The suggestion wasn't to try to work around it, just to come up with the minimal
change to fix the bug. Not that refactoring cleanups are something I don't like;
I just don't see how threading this flag through is a cleanup.

[caitp, 2016-07-08 22:53:51]

the suggestion was implemented in the last patchset

[caitp, 2016-07-08 22:54:20]

Description was changed from

==========
[keys] propagate PropertyFilter, GetKeysConversion, etc to proxy targets

BUG=v8:5174, v8:1543
R=cbruni@chromium.org, littledan@chromium.org
==========

to

==========
[keys] propagate PropertyFilter to proxy targets in KeyAccumulator

BUG=v8:5174, v8:1543
R=cbruni@chromium.org, littledan@chromium.org
==========

[Dan Ehrenberg, 2016-07-08 22:58:13]

lgtm

[Camillo Bruni, 2016-07-11 08:08:34]

lgtm

[caitp, 2016-07-11 10:09:34]

The CQ bit was checked by caitp@igalia.com

[commit-bot: I haz the power, 2016-07-11 10:09:43]

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-07-11 10:38:36]

Description was changed from

==========
[keys] propagate PropertyFilter to proxy targets in KeyAccumulator

BUG=v8:5174, v8:1543
R=cbruni@chromium.org, littledan@chromium.org
==========

to

==========
[keys] propagate PropertyFilter to proxy targets in KeyAccumulator

BUG=v8:5174, v8:1543
R=cbruni@chromium.org, littledan@chromium.org
==========

[commit-bot: I haz the power, 2016-07-11 10:38:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-11 10:39:46]

Description was changed from

==========
[keys] propagate PropertyFilter to proxy targets in KeyAccumulator

BUG=v8:5174, v8:1543
R=cbruni@chromium.org, littledan@chromium.org
==========

to

==========
[keys] propagate PropertyFilter to proxy targets in KeyAccumulator

BUG=v8:5174, v8:1543
R=cbruni@chromium.org, littledan@chromium.org

Committed: https://crrev.com/08d0012dda22306eee124b62a3de9829f3961ba8
Cr-Commit-Position: refs/heads/master@{#37634}
==========

[commit-bot: I haz the power, 2016-07-11 10:39:47]

Patchset 5 (id:??) landed as
https://crrev.com/08d0012dda22306eee124b62a3de9829f3961ba8
Cr-Commit-Position: refs/heads/master@{#37634}