LookupIterator should find private symbols on JSProxies

LookupIterator should find private symbols on JSProxies

BUG=chromium:571365
R=verwaest@chromium.org,neis@chromium.org
LOG=n

Committed: https://crrev.com/9b7035d96ce794cb03fb3db3152f3ee90148e066
Cr-Commit-Position: refs/heads/master@{#33358}

[neis, 2016-01-13 13:53:48]

https://codereview.chromium.org/1587633002/diff/1/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/1587633002/diff/1/src/lookup.cc#newcode614
src/lookup.cc:614: if (name_.is_null() || !name_->IsPrivate()) return JSPROXY;
This looks wrong.  I think you should just return JSProxy unconditionally.  But
before you do that, you should modify the proxy operations (JSProxy::GetProperty
etc) to always consult the proxy's property store for private symbols.

[Toon Verwaest, 2016-01-15 12:24:14]

I'm actually fine with this implementation. I'd also be fine with Georg's
suggestion; so I'll leave it up to him :-)

I'd replace name_.is_null() with IsElement() though.

[jochen (gone - plz use gerrit), 2016-01-15 13:32:45]

ptal

now with support for all the *Private APIs \o/

[jochen (gone - plz use gerrit), 2016-01-15 14:19:34]

Description was changed from

==========
LookupIterator should find private symbols on JSProxies

Currently no way to test this, as there's no way to install symbols
right now.

BUG=chromium:571365
R=verwaest@chromium.org,neis@chromium.org
LOG=n
==========

to

==========
LookupIterator should find private symbols on JSProxies

BUG=chromium:571365
R=verwaest@chromium.org,neis@chromium.org
LOG=n
==========

[jochen (gone - plz use gerrit), 2016-01-15 14:19:45]

ptal

addressed neis@ feedback

[Toon Verwaest, 2016-01-15 14:22:43]

Mostly nits

https://codereview.chromium.org/1587633002/diff/40001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/1587633002/diff/40001/src/lookup.cc#newcode521
src/lookup.cc:521: handle(object->global_dictionary());
Handle<GlobalDictionary> dictionary(
    JSObject::cast(*holder)->global_dictionary());

https://codereview.chromium.org/1587633002/diff/40001/src/lookup.cc#newcode529
src/lookup.cc:529: object->WriteToField(descriptor_number(), *value);
JSObject::cast(*holder)->WriteToField(descriptor_number(), *value)

https://codereview.chromium.org/1587633002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1587633002/diff/40001/src/objects.cc#newcode6206
src/objects.cc:6206: Handle<JSObject>::cast(object)->global_dictionary());
JSObject::cast(*object)->

https://codereview.chromium.org/1587633002/diff/40001/src/objects.cc#newcode6235
src/objects.cc:6235: if (it->GetReceiver()->IsJSProxy()) {
We don't need this branch anymore now right?

https://codereview.chromium.org/1587633002/diff/40001/src/objects.cc#newcode7124
src/objects.cc:7124: if (property_name->IsPrivate()) {
DCHECK(!property_name->IsPrivate()) ?

https://codereview.chromium.org/1587633002/diff/40001/src/objects.cc#newcode7127
src/objects.cc:7127: NewTypeError(MessageTemplate::kProxyPrivate));
Get rid of kProxyPrivate?

https://codereview.chromium.org/1587633002/diff/40001/src/objects.cc#newcode7197
src/objects.cc:7197: Handle<Name> private_name,
Handle<Symbol> ?

https://codereview.chromium.org/1587633002/diff/40001/src/objects.cc#newcode7209
src/objects.cc:7209: LookupIterator it = LookupIterator::PropertyOrElement(
LookupIterator it(proxy, private_name); should suffice

https://codereview.chromium.org/1587633002/diff/40001/src/objects.cc#newcode7224
src/objects.cc:7224: if (*dict != *result) proxy->set_properties(*result);
if (!dict.is_identical_to(result)) proxy->set_properties(*result);

[jochen (gone - plz use gerrit), 2016-01-15 14:44:49]

addressed Toons comments (and replied to those I didn't)

https://codereview.chromium.org/1587633002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1587633002/diff/40001/src/objects.cc#newcode6235
src/objects.cc:6235: if (it->GetReceiver()->IsJSProxy()) {
On 2016/01/15 at 14:22:42, Toon Verwaest wrote:
> We don't need this branch anymore now right?

below, we cast the receiver to JSObject which won't work (and the entire loop
assumes it's a proxy)

https://codereview.chromium.org/1587633002/diff/40001/src/objects.cc#newcode7127
src/objects.cc:7127: NewTypeError(MessageTemplate::kProxyPrivate));
On 2016/01/15 at 14:22:42, Toon Verwaest wrote:
> Get rid of kProxyPrivate?

I still need it (see next patchset). tl;dr builtins could still try to set
symbols in expected ways, and at least the unit tests expect this to thro

[neis, 2016-01-15 14:49:12]

I think I will change the private-symbols.js test such that it only uses the
*_PRIVATE functions defined in macros.py.  Then you can turn the checks in
AddPrivateData into proper CHECKs.

[neis, 2016-01-15 14:49:13]

I think I will change the private-symbols.js test such that it only uses the
*_PRIVATE functions defined in macros.py.  Then you can turn the checks in
AddPrivateData into proper CHECKs.

[jochen (gone - plz use gerrit), 2016-01-15 14:58:13]

SET_PRIVATE() doesn't set DONT_ENUM properties, so that won't be enough.

[jochen (gone - plz use gerrit), 2016-01-15 15:49:23]

now with green bots \o/

[Toon Verwaest, 2016-01-18 09:10:05]

lgtm.

As an overall guideline we shouldn't be "hardening" code by adding throws all
over the place for things that shouldn't happen. If you really want to harden,
add a CHECK. That way we can discover that it's broken in chromecrash.

https://codereview.chromium.org/1587633002/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1587633002/diff/100001/src/objects.cc#newcode...
src/objects.cc:7197: if (!PropertyDescriptor::IsDataDescriptor(desc) ||
I really think we should DCHECK or even CHECK here rather than throw. This
should really not happen. If builtins do this they are broken. I guess a CHECK
would be ideal to crash if we get it wrong somewhere.

https://codereview.chromium.org/1587633002/diff/100001/src/objects.cc#newcode...
src/objects.cc:7203: Handle<Object> value(
value = if the right-side is already a handle.

[jochen (gone - plz use gerrit), 2016-01-18 12:14:42]

Georg said he'd update the mjsunit test to not run into this exception, and will
turn those into CHECKs

[jochen (gone - plz use gerrit), 2016-01-18 12:18:43]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-01-18 12:18:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1587633002/#ps120001
(title: "updates")

[commit-bot: I haz the power, 2016-01-18 12:18:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1587633002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1587633002/120001

[neis, 2016-01-18 12:19:01]

On 2016/01/18 12:14:42, jochen wrote:
> Georg said he'd update the mjsunit test to not run into this exception, and
will
> turn those into CHECKs

Yes.

Lgtm.

[commit-bot: I haz the power, 2016-01-18 12:41:15]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-18 12:41:37]

Description was changed from

==========
LookupIterator should find private symbols on JSProxies

BUG=chromium:571365
R=verwaest@chromium.org,neis@chromium.org
LOG=n
==========

to

==========
LookupIterator should find private symbols on JSProxies

BUG=chromium:571365
R=verwaest@chromium.org,neis@chromium.org
LOG=n

Committed: https://crrev.com/9b7035d96ce794cb03fb3db3152f3ee90148e066
Cr-Commit-Position: refs/heads/master@{#33358}
==========

[commit-bot: I haz the power, 2016-01-18 12:41:37]

Patchset 7 (id:??) landed as
https://crrev.com/9b7035d96ce794cb03fb3db3152f3ee90148e066
Cr-Commit-Position: refs/heads/master@{#33358}