[es6] silence access-check failure for well-known symbol properties

[es6] silence access-check failure for well-known symbol properties

Symbols marked as "well-known" now return an undefined value when loaded with a failed access check, instead of throwing.

Currently, only @@isConcatSpreadable is marked as well-known, until the correct behaviour is properly specified.

BUG=v8:4289, 507553
LOG=N
R=adamk@chromium.org, jochen@chromium.org, verwaest@chromium.org

Committed: https://crrev.com/8561dbd655769cb63fc797e7ebd3923bee943212
Cr-Commit-Position: refs/heads/master@{#31131}

[caitp (gmail), 2015-07-09 13:53:42]

caitpotter88@gmail.com changed reviewers:
+ verwaest@chromium.org
- werwaest@chromium.org

[caitp (gmail), 2015-07-09 13:55:00]

Hi,

this is one approach which should maintain legacy behaviour when dealing with
objects from a different origin. If the @@isConcatSpreadable symbol is present,
Chromium can probably emit a warning as to why its value is ignored

[caitp (gmail), 2015-07-09 13:56:03]

On 2015/07/09 13:55:00, caitp wrote:
> Hi,
> 
> this is one approach which should maintain legacy behaviour when dealing with
> objects from a different origin. If the @@isConcatSpreadable symbol is
present,
> Chromium can probably emit a warning as to why its value is ignored

test coming soon

[caitp (gmail), 2015-07-09 14:20:49]

caitpotter88@gmail.com changed reviewers:
+ arv@chromium.org

[caitp (gmail), 2015-07-09 14:49:42]

https://codereview.chromium.org/1230793002/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1230793002/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:21910: Local<Value> result2 =
CompileRun("[].concat([global, global], protected)");
This is probably not doing what I want it to do

I would like context 1 to attempt to pass an array from context 2 to
ArrayConcat() --- and the spreading should occur because the value is an Array,
and regardless of whether @@isConcatSpreadable is set to undefined or not.

The API is sort of confusing so I'm not totally sure how to simulate that.

[adamk, 2015-07-09 15:56:04]

A few comments, but as I say below, I think verwaest@ is the right reviewer for
this approach.

The simpler approach of treating IsAccessCheckNeeded() as undefined seems like
nearly as good a solution, though: the only thing it disallows in practice is
making Window and Location objects ConcatSpreadable, something I'd hope no one
would ever try to do.

https://codereview.chromium.org/1230793002/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1230793002/diff/40001/src/objects.h#newcode1219
src/objects.h:1219: MUST_USE_RESULT static inline MaybeHandle<Object>
GetPropertyOrFallbackValue(
I'd wait for Toon's review before making more changes, but this needs a better
name if it's the approach we want to take. This name says nothing to me about
access checks.

https://codereview.chromium.org/1230793002/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1230793002/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:21910: Local<Value> result2 =
CompileRun("[].concat([global, global], protected)");
On 2015/07/09 at 14:49:41, caitp wrote:
> This is probably not doing what I want it to do
> 
> I would like context 1 to attempt to pass an array from context 2 to
ArrayConcat() --- and the spreading should occur because the value is an Array,
and regardless of whether @@isConcatSpreadable is set to undefined or not.
> 
> The API is sort of confusing so I'm not totally sure how to simulate that.

Is the idea just to check that the check is doing IsJSArray() instead of
instanceof? That seems like a separate concern from the security check test case
going on here.

[caitp (gmail), 2015-07-09 16:02:09]

https://codereview.chromium.org/1230793002/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1230793002/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:21910: Local<Value> result2 =
CompileRun("[].concat([global, global], protected)");
On 2015/07/09 15:56:04, adamk wrote:
> On 2015/07/09 at 14:49:41, caitp wrote:
> > This is probably not doing what I want it to do
> > 
> > I would like context 1 to attempt to pass an array from context 2 to
> ArrayConcat() --- and the spreading should occur because the value is an
Array,
> and regardless of whether @@isConcatSpreadable is set to undefined or not.
> > 
> > The API is sort of confusing so I'm not totally sure how to simulate that.
> 
> Is the idea just to check that the check is doing IsJSArray() instead of
> instanceof? That seems like a separate concern from the security check test
case
> going on here.

Per spec, if @@isConcatSpreadable is undefined, but the value is an exotic Array
object (or a real subclass), the value is always spread --- so EG, a
cross-origin Elements object would be spread due to subclassing Array.

So I just want to also assert that, even though the @@isConcatSpreadable value
is ignored, it will still perform the spread for a cross-origin exotic Array
object

[adamk, 2015-07-09 17:17:25]

I think I'm liking my "minimal" suggestion less now, since it could cause us to
throw exceptions in new places if there are access-checked objects on the
prototype chain (still rare, but that makes another case).

So the approach here does seem attractive to me, will see if I can think of a
good name that's still short for GetPropertyOrFallback.

https://codereview.chromium.org/1230793002/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1230793002/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:21910: Local<Value> result2 =
CompileRun("[].concat([global, global], protected)");
On 2015/07/09 at 16:02:09, caitp wrote:
> On 2015/07/09 15:56:04, adamk wrote:
> > On 2015/07/09 at 14:49:41, caitp wrote:
> > > This is probably not doing what I want it to do
> > > 
> > > I would like context 1 to attempt to pass an array from context 2 to
> > ArrayConcat() --- and the spreading should occur because the value is an
Array,
> > and regardless of whether @@isConcatSpreadable is set to undefined or not.
> > > 
> > > The API is sort of confusing so I'm not totally sure how to simulate that.
> > 
> > Is the idea just to check that the check is doing IsJSArray() instead of
> > instanceof? That seems like a separate concern from the security check test
case
> > going on here.
> 
> Per spec, if @@isConcatSpreadable is undefined, but the value is an exotic
Array object (or a real subclass), the value is always spread --- so EG, a
cross-origin Elements object would be spread due to subclassing Array.
> 
> So I just want to also assert that, even though the @@isConcatSpreadable value
is ignored, it will still perform the spread for a cross-origin exotic Array
object

I see what you're aiming for, but Arrays are never access checked (and at the
moment I don't believe there's any way to create an Array subclass that is
access checked). That's why it seems like a separate concern, test-wise. The V8
model is sadly brittle, but what it attempts to do is disallow
non-access-checked objects from ever being shared across origins by cutting off
access at the access-checking boundary (either the Window or Location objects).

[caitp (gmail), 2015-07-09 18:40:46]

On 2015/07/09 17:17:25, adamk wrote:
> I think I'm liking my "minimal" suggestion less now, since it could cause us
to
> throw exceptions in new places if there are access-checked objects on the
> prototype chain (still rare, but that makes another case).
> 
> So the approach here does seem attractive to me, will see if I can think of a
> good name that's still short for GetPropertyOrFallback.
> 
> https://codereview.chromium.org/1230793002/diff/40001/test/cctest/test-api.cc
> File test/cctest/test-api.cc (right):
> 
>
https://codereview.chromium.org/1230793002/diff/40001/test/cctest/test-api.cc...
> test/cctest/test-api.cc:21910: Local<Value> result2 =
> CompileRun("[].concat([global, global], protected)");
> On 2015/07/09 at 16:02:09, caitp wrote:
> > On 2015/07/09 15:56:04, adamk wrote:
> > > On 2015/07/09 at 14:49:41, caitp wrote:
> > > > This is probably not doing what I want it to do
> > > > 
> > > > I would like context 1 to attempt to pass an array from context 2 to
> > > ArrayConcat() --- and the spreading should occur because the value is an
> Array,
> > > and regardless of whether @@isConcatSpreadable is set to undefined or not.
> > > > 
> > > > The API is sort of confusing so I'm not totally sure how to simulate
that.
> > > 
> > > Is the idea just to check that the check is doing IsJSArray() instead of
> > > instanceof? That seems like a separate concern from the security check
test
> case
> > > going on here.
> > 
> > Per spec, if @@isConcatSpreadable is undefined, but the value is an exotic
> Array object (or a real subclass), the value is always spread --- so EG, a
> cross-origin Elements object would be spread due to subclassing Array.
> > 
> > So I just want to also assert that, even though the @@isConcatSpreadable
value
> is ignored, it will still perform the spread for a cross-origin exotic Array
> object
> 
> I see what you're aiming for, but Arrays are never access checked (and at the
> moment I don't believe there's any way to create an Array subclass that is
> access checked). That's why it seems like a separate concern, test-wise. The
V8
> model is sadly brittle, but what it attempts to do is disallow
> non-access-checked objects from ever being shared across origins by cutting
off
> access at the access-checking boundary (either the Window or Location
objects).

Fair enough, then the whole "result2" thing seems kind of pointless, the rest
should be okay?

[caitp (gmail), 2015-07-09 18:43:15]

https://codereview.chromium.org/1230793002/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1230793002/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:21910: Local<Value> result2 =
CompileRun("[].concat([global, global], protected)");
On 2015/07/09 17:17:25, adamk wrote:
> On 2015/07/09 at 16:02:09, caitp wrote:
> > On 2015/07/09 15:56:04, adamk wrote:
> > > On 2015/07/09 at 14:49:41, caitp wrote:
> > > > This is probably not doing what I want it to do
> > > > 
> > > > I would like context 1 to attempt to pass an array from context 2 to
> > > ArrayConcat() --- and the spreading should occur because the value is an
> Array,
> > > and regardless of whether @@isConcatSpreadable is set to undefined or not.
> > > > 
> > > > The API is sort of confusing so I'm not totally sure how to simulate
that.
> > > 
> > > Is the idea just to check that the check is doing IsJSArray() instead of
> > > instanceof? That seems like a separate concern from the security check
test
> case
> > > going on here.
> > 
> > Per spec, if @@isConcatSpreadable is undefined, but the value is an exotic
> Array object (or a real subclass), the value is always spread --- so EG, a
> cross-origin Elements object would be spread due to subclassing Array.
> > 
> > So I just want to also assert that, even though the @@isConcatSpreadable
value
> is ignored, it will still perform the spread for a cross-origin exotic Array
> object
> 
> I see what you're aiming for, but Arrays are never access checked (and at the
> moment I don't believe there's any way to create an Array subclass that is
> access checked). That's why it seems like a separate concern, test-wise. The
V8
> model is sadly brittle, but what it attempts to do is disallow
> non-access-checked objects from ever being shared across origins by cutting
off
> access at the access-checking boundary (either the Window or Location
objects).

oops, https://codereview.chromium.org/1230793002/#msg9 was meant for here

[adamk, 2015-07-09 18:59:13]

https://codereview.chromium.org/1230793002/diff/60001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1230793002/diff/60001/test/cctest/test-api.cc...
test/cctest/test-api.cc:21889: Local<Object> global_object =
proxy_object->GetPrototype()->ToObject(isolate);
I don't think you need this stuff any more.

https://codereview.chromium.org/1230793002/diff/60001/test/cctest/test-api.cc...
test/cctest/test-api.cc:21908:
CHECK(result1.As<Object>()->Get(0)->Equals(protected_object));
I think you also want a test for an access checked object where the access check
succeeds and it's concat-spreadable.

[caitp (gmail), 2015-07-09 19:14:00]

https://codereview.chromium.org/1230793002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1230793002/diff/20001/src/objects.h#newcode1219
src/objects.h:1219: MUST_USE_RESULT static inline MaybeHandle<Object>
GetPropertyOrFallbackValue(
How about something like "GetPropertyWithAccessCheckFallback" or
"GetPropertyWithAccessCheckFallbackValue"?

[adamk, 2015-07-09 23:07:15]

I'd like to wait for a bit more time on the public-script-coord thread, though I
don't think we need to wait on changing Blink's whole Window object
implementation.

[Toon Verwaest, 2015-07-10 13:22:16]

wow ...

do we really need this? I'm really really not in favor of any of this.

There's a spec-on-the-way for cross-origin access, which is very close to what
Chrome does. See https://etherpad.mozilla.org/html5-cross-origin-objects

We can definitely not just look behind access checks, so if we really need this,
then only returning a fallback value makes sense. But even that breaks what
would normally happen.

As adamk@ mentioned, access checks are only relevant for Window and Location. Do
we really need special concat-spreadable support for those objects? What's wrong
with just throwing as would happen if you wouldn't have a VM-supported
implementation?

I really don't like this; if it wasn't clear :)

https://codereview.chromium.org/1230793002/diff/80001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1230793002/diff/80001/test/cctest/test-api.cc...
test/cctest/test-api.cc:21863: if (!name->IsSymbol()) return true;
Access checks are in the process of being rewritten to not get anything in but
the receiver and the context from which access is requested. At this point type
is already *always* ACCESS_HAS and "name" is *always* undefined. Hence this
IsSymbol check makes no sense.

Access checks shouldn't be finegrained, and cannot partially allow access (minus
some really internal things like hash-codes). Either you have access from the
current context to an object, or you don't.

[caitp (gmail), 2015-07-10 13:54:01]

On 2015/07/10 13:22:16, Toon Verwaest wrote:
> wow ...
> 
> do we really need this? I'm really really not in favor of any of this.
> 
> There's a spec-on-the-way for cross-origin access, which is very close to what
> Chrome does. See https://etherpad.mozilla.org/html5-cross-origin-objects
> 
> We can definitely not just look behind access checks, so if we really need
this,
> then only returning a fallback value makes sense. But even that breaks what
> would normally happen.

Yes, and that's not great, but what would you rather do? We can't just try to
retrieve the property and throw --- we need to either not access the property
(which will break access-checked objects that pass the access check --- maybe
that's fine, maybe not) --- or to just ignore the failed access check and
fallback on a sane behaviour.

> As adamk@ mentioned, access checks are only relevant for Window and Location.
Do
> we really need special concat-spreadable support for those objects? What's
wrong
> with just throwing as would happen if you wouldn't have a VM-supported
> implementation?

If we don't do anything, and just leave it as-is, we break things (as seen in
https://code.google.com/p/chromium/issues/detail?id=507553)

If we know the object is special in this way, then sure we could just avoid
retrieving the property, which accomplishes the same thing as this --- with one
exception: if an access check is needed, but allowed, it does the wrong thing
even for safe objects.

"What is the use-case for spreading Window or Location"? It doesn't matter what
the use-case is, because failing the access check breaks "concat" period,
whether they intended to spread or not.

> I really don't like this; if it wasn't clear :)

Well, what would you like? That's a more helpful thing.

> https://codereview.chromium.org/1230793002/diff/80001/test/cctest/test-api.cc
> File test/cctest/test-api.cc (right):
> 
>
https://codereview.chromium.org/1230793002/diff/80001/test/cctest/test-api.cc...
> test/cctest/test-api.cc:21863: if (!name->IsSymbol()) return true;
> Access checks are in the process of being rewritten to not get anything in but
> the receiver and the context from which access is requested. At this point
type
> is already *always* ACCESS_HAS and "name" is *always* undefined. Hence this
> IsSymbol check makes no sense.
> 
> Access checks shouldn't be finegrained, and cannot partially allow access
(minus
> some really internal things like hash-codes). Either you have access from the
> current context to an object, or you don't.

Well, it makes the test simpler --- it doesn't have to be a real-world example.
If it's going to stop working, then I'll put the time in to make it work. But
there isn't much point of that if we're not going to do it this way. So, given
that we can't just throw when the access check fails, and you don't like the
fallback value idea, what would you prefer? Constructive criticism is
appreciated

[Toon Verwaest, 2015-07-10 15:21:12]

I think this needs to be thought trough by people writing the spec. This is just
an ad-hoc workaround, which will likely not match whatever is later decided as
the actual spec.

If you really need to get something in *right now* for whatever reason, I'd only
sign off on *only* changing runtime-array.cc/IsConcatSpreadable to include:
if (obj->IsJSObject() && !isolate->MayAccess(Handle<JSObject>::cast(obj))) {
  // or whatever value is the default non-spreadable value.
  return isolate->heap()->false_value();
}

[Toon Verwaest, 2015-07-10 15:21:34]

verwaest@chromium.org changed reviewers:
+ rossberg@chromium.org

[Toon Verwaest, 2015-07-10 15:21:35]

+andreas

[Toon Verwaest, 2015-07-10 15:30:37]

(that should have been obj->IsAccessCheckNeeded() &&
!isolate->MayAccess(Handle<JSObject>::cast(obj)) I guess)

[adamk, 2015-07-10 17:24:39]

Just to link discussions together further, the spec-world discussion about this
is taking place at:

https://lists.w3.org/Archives/Public/public-script-coord/2015JulSep/0022.html

[caitp (gmail), 2015-07-11 17:09:02]

On 2015/07/10 17:24:39, adamk wrote:
> Just to link discussions together further, the spec-world discussion about
this
> is taking place at:
> 
> https://lists.w3.org/Archives/Public/public-script-coord/2015JulSep/0022.html

Per the discussion in the etherpad, the best thing to do is probably just to
install [Unforgeable] + readonly @@toStringTag/@@isConcatSpreadable/etc
attributes on cross-origin objects, and ignore the security check.

Preferred? Not preferred?

[caitp (gmail), 2015-07-11 17:21:23]

On 2015/07/11 17:09:02, caitp wrote:
> On 2015/07/10 17:24:39, adamk wrote:
> > Just to link discussions together further, the spec-world discussion about
> this
> > is taking place at:
> > 
> >
https://lists.w3.org/Archives/Public/public-script-coord/2015JulSep/0022.html
> 
> Per the discussion in the etherpad, the best thing to do is probably just to
> install [Unforgeable] + readonly @@toStringTag/@@isConcatSpreadable/etc
> attributes on cross-origin objects, and ignore the security check.
> 
> Preferred? Not preferred?

by "ignore the security check", I mean with an IDL attribute, not in the VM, of
course

[Toon Verwaest, 2015-07-13 13:12:19]

Yes. That what I meant with my all-can-read suggestion; that's the same thing.
If that's a workable solution I'd much prefer that. I presume we'd only need it
on window though, not on location?

[adamk, 2015-07-13 18:12:49]

On 2015/07/11 at 17:09:02, caitpotter88 wrote:
> On 2015/07/10 17:24:39, adamk wrote:
> > Just to link discussions together further, the spec-world discussion about
this
> > is taking place at:
> > 
> >
https://lists.w3.org/Archives/Public/public-script-coord/2015JulSep/0022.html
> 
> Per the discussion in the etherpad, the best thing to do is probably just to
install [Unforgeable] + readonly @@toStringTag/@@isConcatSpreadable/etc
attributes on cross-origin objects, and ignore the security check.
> 
> Preferred? Not preferred?

My reading of the etherpad doesn't match this interpretation. In the current
Blink implementation, an [Unforgeable] attribute will have the same value to all
observers (not just cross-origin accesses). The draft "spec" in the etherpad
suggests that each access to a frame from another origin generates a new Window
object.

[caitp (gmail), 2015-07-13 18:20:59]

On 2015/07/13 18:12:49, adamk wrote:
> On 2015/07/11 at 17:09:02, caitpotter88 wrote:
> > On 2015/07/10 17:24:39, adamk wrote:
> > > Just to link discussions together further, the spec-world discussion about
> this
> > > is taking place at:
> > > 
> > >
> https://lists.w3.org/Archives/Public/public-script-coord/2015JulSep/0022.html
> > 
> > Per the discussion in the etherpad, the best thing to do is probably just to
> install [Unforgeable] + readonly @@toStringTag/@@isConcatSpreadable/etc
> attributes on cross-origin objects, and ignore the security check.
> > 
> > Preferred? Not preferred?
> 
> My reading of the etherpad doesn't match this interpretation. In the current
> Blink implementation, an [Unforgeable] attribute will have the same value to
all
> observers (not just cross-origin accesses). The draft "spec" in the etherpad
> suggests that each access to a frame from another origin generates a new
Window
> object.

[Unforgeable] attributes are non-configurable (which is good, they can't be
reconfigured as untrusted accessors) --- the downside is that they live on the
instance rather than prototype, but for Window this probably doesn't matter
much. This alone should be enough to ensure that the value is trustworthy (after
all, per the etherpad, properties have/return primitive values are able to be
whitelisted for cross-origin access).

Making them readonly on top of that hurts the customization a little bit, but
maybe goes a step further in making sure they're always safe?

That's what I'm getting at, anyway

[adamk, 2015-07-13 18:25:07]

On 2015/07/13 at 18:20:59, caitpotter88 wrote:
> On 2015/07/13 18:12:49, adamk wrote:
> > On 2015/07/11 at 17:09:02, caitpotter88 wrote:
> > > On 2015/07/10 17:24:39, adamk wrote:
> > > > Just to link discussions together further, the spec-world discussion
about
> > this
> > > > is taking place at:
> > > > 
> > > >
> >
https://lists.w3.org/Archives/Public/public-script-coord/2015JulSep/0022.html
> > > 
> > > Per the discussion in the etherpad, the best thing to do is probably just
to
> > install [Unforgeable] + readonly @@toStringTag/@@isConcatSpreadable/etc
> > attributes on cross-origin objects, and ignore the security check.
> > > 
> > > Preferred? Not preferred?
> > 
> > My reading of the etherpad doesn't match this interpretation. In the current
> > Blink implementation, an [Unforgeable] attribute will have the same value to
all
> > observers (not just cross-origin accesses). The draft "spec" in the etherpad
> > suggests that each access to a frame from another origin generates a new
Window
> > object.
> 
> [Unforgeable] attributes are non-configurable (which is good, they can't be
reconfigured as untrusted accessors) --- the downside is that they live on the
instance rather than prototype, but for Window this probably doesn't matter
much. This alone should be enough to ensure that the value is trustworthy (after
all, per the etherpad, properties have/return primitive values are able to be
whitelisted for cross-origin access).
> 
> Making them readonly on top of that hurts the customization a little bit, but
maybe goes a step further in making sure they're always safe?

If we use an [Unforgeable] attribute and disabled access checks, we would
definitely need to make it readonly. Which would hurt customization, indeed, as
well as being observably different from what Boris described in the thread,
since Object.hasOwnProperty(window, Symbol.isConcatSpreadable) would be true for
same-origin windows in Blink but false in Firefox.

> That's what I'm getting at, anyway

[Toon Verwaest, 2015-07-14 07:22:50]

With an all-can-read property you can still return whatever you want for iframes
that don't have access. See
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
for an example. The getter could return an internally stored mutable property to
origins that do have access, while returning a default value to origins without
access.

[Toon Verwaest, 2015-07-14 07:27:19]

But obviously it's still visibly different from what Boris suggests, given that
the property is there.

As I've suggested in another thread, we *could* use all-can-read interceptors to
achieve both all-can-read and absent, but that's kinda ugly ... We could support
"invisible" all-can-read accessors that pretend to be absent as long as you read
it as a regular property, but returns the value returned by the getter when read
from origin without access. Not exactly sure what's best.

The easiest from our side would be to make the property always be there
(non-deletable) on window.

[Toon Verwaest, 2015-07-18 07:59:19]

What about setting a bit on symbols whether they are "absent on access check
failure", and extend GetProperty(Attributes)WithFailedAccessCheck to also handle
such symbols in addition to all_can_read accessors and interceptors? That way it
becomes very easy to support a new symbol by just setting the bit, and the logic
isn't spread throughout the system but localized to those 2 methods.

[adamk, 2015-09-29 21:48:28]

On 2015/07/18 07:59:19, Toon Verwaest wrote:
> What about setting a bit on symbols whether they are "absent on access check
> failure", and extend GetProperty(Attributes)WithFailedAccessCheck to also
handle
> such symbols in addition to all_can_read accessors and interceptors? That way
it
> becomes very easy to support a new symbol by just setting the bit, and the
logic
> isn't spread throughout the system but localized to those 2 methods.

Caitlin, would you be interested in implementing Toon's suggestion here? I'd be
happy to pick it up myself.

[adamk, 2015-09-29 21:49:18]

On 2015/09/29 21:48:28, adamk wrote:
> On 2015/07/18 07:59:19, Toon Verwaest wrote:
> > What about setting a bit on symbols whether they are "absent on access check
> > failure", and extend GetProperty(Attributes)WithFailedAccessCheck to also
> handle
> > such symbols in addition to all_can_read accessors and interceptors? That
way
> it
> > becomes very easy to support a new symbol by just setting the bit, and the
> logic
> > isn't spread throughout the system but localized to those 2 methods.
> 
> Caitlin, would you be interested in implementing Toon's suggestion here? I'd
be
> happy to pick it up myself.

I should add: it sounds to me like this is what SpiderMonkey is planning on
doing, and it it least works fine for @@isConcatSpreadable (not sure if it's
sufficient for @@toStringTag; depends on what behavior we want for toString on
cross-domain frames).

[caitp (gmail), 2015-09-30 04:26:02]

On 2015/09/29 21:49:18, adamk wrote:
> On 2015/09/29 21:48:28, adamk wrote:
> > On 2015/07/18 07:59:19, Toon Verwaest wrote:
> > > What about setting a bit on symbols whether they are "absent on access
check
> > > failure", and extend GetProperty(Attributes)WithFailedAccessCheck to also
> > handle
> > > such symbols in addition to all_can_read accessors and interceptors? That
> way
> > it
> > > becomes very easy to support a new symbol by just setting the bit, and the
> > logic
> > > isn't spread throughout the system but localized to those 2 methods.
> > 
> > Caitlin, would you be interested in implementing Toon's suggestion here? I'd
> be
> > happy to pick it up myself.
> 
> I should add: it sounds to me like this is what SpiderMonkey is planning on
> doing, and it it least works fine for @@isConcatSpreadable (not sure if it's
> sufficient for @@toStringTag; depends on what behavior we want for toString on
> cross-domain frames).

I'll work on it Thursday

[jochen (gone - plz use gerrit), 2015-09-30 09:24:01]

include/ lgtm

[caitp (gmail), 2015-10-01 14:58:46]

Patchset #6 (id:100001) has been deleted

[caitp (gmail), 2015-10-01 15:16:47]

https://codereview.chromium.org/1230793002/diff/140001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1230793002/diff/140001/src/objects.cc#newcode662
src/objects.cc:662: access_check_failed = false;
This alternative implementation doesn't change the nature of [[Get]] in general,
but provides an alternative way of getting properties when special behaviour is
required in the case of access checks.

This duplicates some code, and the method name isn't very good. But I think this
is more general and applies to other well-known Symbols (eg if something special
is done for @@toStringTag instead of treating it as undefined --- not spec'd yet
but it sounds like it's a possibility?).

What are peoples thoughts?

[Toon Verwaest, 2015-10-01 15:30:13]

I still stand by my previous comments. I don't want access checks to be exposed
by GetProperty.

[caitp (gmail), 2015-10-01 16:03:48]

On 2015/10/01 15:30:13, Toon Verwaest wrote:
> I still stand by my previous comments. I don't want access checks to be
exposed
> by GetProperty.

How about something like "GetWellKnownSymbolHook()" instead? It doesn't really
matter what it's called, but whatever it is needs to let the caller decide what
happens when an access check fails.

My problem with the approach is that it would prevent ordinary [[Get]]s of
well-known symbols to throw, which is probably not what we want to do. They
still don't leak any information, but it's inconsistent with the rest of the
platform. So I would prefer to just give internal code the option to ignore the
thing.

I'll send another patch set with your idea in mind anyways, though.

[caitp (gmail), 2015-10-01 16:23:55]

New patch set does not throw for loads of spec'd symbol hooks ever, so but still
throws on writes, and probably throws on [[HasProperty]] tests too.

[Toon Verwaest, 2015-10-02 08:05:27]

Last patch looks good from my side. Some additional comments.

https://codereview.chromium.org/1230793002/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1230793002/diff/160001/src/objects.cc#newcode997
src/objects.cc:997: if (!it->IsElement()) {
Rather than if (!it->IsElement()) etc, you can just use
Handle<Name> name = it->GetName();
if (name->IsSymbol() && Symbol::cast(*name)->is_well_known_symbol()) {
  return it->factory()->undefined_value();
}

https://codereview.chromium.org/1230793002/diff/160001/src/objects.cc#newcode...
src/objects.cc:1001: RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(it->isolate(),
Object);
I don't see why this statement (RETURN_EX..) is needed.

https://codereview.chromium.org/1230793002/diff/160001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1230793002/diff/160001/src/objects.h#newcode1217
src/objects.h:1217: LanguageMode language_mode = SLOPPY);
This declaration can now be dropped.

[Toon Verwaest, 2015-10-02 08:09:42]

@adamk: if we later need other behavior for other symbols, we could switch in
...WithFailedAccessCheck over a well-known-symbol "id". So far (and it seems
like the only thing that makes sense to me) any spec'd behavior on failed access
check has a predictable result. E.g., Object.isExtensible(otherwWindow) *always*
returns true, even if the underlying object isn't extensible in as visible in
its origin.

[caitp (gmail), 2015-10-02 10:01:15]

https://codereview.chromium.org/1230793002/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1230793002/diff/160001/src/objects.cc#newcode997
src/objects.cc:997: if (!it->IsElement()) {
On 2015/10/02 08:05:27, Toon Verwaest wrote:
> Rather than if (!it->IsElement()) etc, you can just use
> Handle<Name> name = it->GetName();
> if (name->IsSymbol() && Symbol::cast(*name)->is_well_known_symbol()) {
>   return it->factory()->undefined_value();
> }

GetName() converts elements to strings, which isn't needed, but name() has a
DCHECK(!IsElement()). So, it seemed better to avoid the allocation, from my pov.

I can change it if people prefer that

https://codereview.chromium.org/1230793002/diff/160001/src/objects.cc#newcode...
src/objects.cc:1001: RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(it->isolate(),
Object);
On 2015/10/02 08:05:27, Toon Verwaest wrote:
> I don't see why this statement (RETURN_EX..) is needed.

I guess it isn't really needed, will remove

https://codereview.chromium.org/1230793002/diff/160001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1230793002/diff/160001/src/objects.h#newcode1217
src/objects.h:1217: LanguageMode language_mode = SLOPPY);
On 2015/10/02 08:05:27, Toon Verwaest wrote:
> This declaration can now be dropped.

Acknowledged.

[Toon Verwaest, 2015-10-02 10:07:02]

https://codereview.chromium.org/1230793002/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1230793002/diff/160001/src/objects.cc#newcode997
src/objects.cc:997: if (!it->IsElement()) {
On 2015/10/02 10:01:15, caitp wrote:
> On 2015/10/02 08:05:27, Toon Verwaest wrote:
> > Rather than if (!it->IsElement()) etc, you can just use
> > Handle<Name> name = it->GetName();
> > if (name->IsSymbol() && Symbol::cast(*name)->is_well_known_symbol()) {
> >   return it->factory()->undefined_value();
> > }
> 
> GetName() converts elements to strings, which isn't needed, but name() has a
> DCHECK(!IsElement()). So, it seemed better to avoid the allocation, from my
pov.
> 
> I can change it if people prefer that

That is possibly true. It could already have been cached. Adding IsElement()
only avoids this possible cost (in an exceptional path that's not performance
sensitive), but does introduce extra unnecessary complexity here.

[caitp (gmail), 2015-10-02 10:41:56]

https://codereview.chromium.org/1230793002/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1230793002/diff/160001/src/objects.cc#newcode997
src/objects.cc:997: if (!it->IsElement()) {
On 2015/10/02 10:07:01, Toon Verwaest wrote:
> On 2015/10/02 10:01:15, caitp wrote:
> > On 2015/10/02 08:05:27, Toon Verwaest wrote:
> > > Rather than if (!it->IsElement()) etc, you can just use
> > > Handle<Name> name = it->GetName();
> > > if (name->IsSymbol() && Symbol::cast(*name)->is_well_known_symbol()) {
> > >   return it->factory()->undefined_value();
> > > }
> > 
> > GetName() converts elements to strings, which isn't needed, but name() has a
> > DCHECK(!IsElement()). So, it seemed better to avoid the allocation, from my
> pov.
> > 
> > I can change it if people prefer that
> 
> That is possibly true. It could already have been cached. Adding IsElement()
> only avoids this possible cost (in an exceptional path that's not performance
> sensitive), but does introduce extra unnecessary complexity here.

Done.

https://codereview.chromium.org/1230793002/diff/160001/src/objects.cc#newcode...
src/objects.cc:1001: RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(it->isolate(),
Object);
On 2015/10/02 10:01:15, caitp wrote:
> On 2015/10/02 08:05:27, Toon Verwaest wrote:
> > I don't see why this statement (RETURN_EX..) is needed.
> 
> I guess it isn't really needed, will remove

Done.

https://codereview.chromium.org/1230793002/diff/160001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1230793002/diff/160001/src/objects.h#newcode1217
src/objects.h:1217: LanguageMode language_mode = SLOPPY);
On 2015/10/02 10:01:15, caitp wrote:
> On 2015/10/02 08:05:27, Toon Verwaest wrote:
> > This declaration can now be dropped.
> 
> Acknowledged.

Done.

[Toon Verwaest, 2015-10-02 10:54:01]

lgtm, thanks

[caitp (gmail), 2015-10-02 12:02:38]

caitpotter88@gmail.com changed reviewers:
- arv@chromium.org

[caitp (gmail), 2015-10-02 13:10:53]

I've written http://caitp.github.io/spidermonkey-wks-security/ to help track all
of the behaviours of cross-origin access to symbol hooks. It's missing a lot of
stuff, but it might be useful later on

[adamk, 2015-10-05 16:41:14]

I'm happy that Toon is happy with this approach. I'm still a bit worried about
the "not yet specced" part, though, especially as we're adding this new behavior
to existing Symbols. Is there a reason not to start this with only allowing
window[@@isConcatSpreadable] to return undefined?

[caitp (gmail), 2015-10-05 16:44:56]

On 2015/10/05 16:41:14, adamk wrote:
> I'm happy that Toon is happy with this approach. I'm still a bit worried about
> the "not yet specced" part, though, especially as we're adding this new
behavior
> to existing Symbols. Is there a reason not to start this with only allowing
> window[@@isConcatSpreadable] to return undefined?

If it's necessary, we could turn off this special handling by simply not setting
the well-known symbol bit during native context setup, so that seems like it's
probably okay.

To only enable this for @@isConcatSpreadable, that's pretty easy too

[caitp (gmail), 2015-10-05 20:18:13]

On 2015/10/05 16:44:56, caitp wrote:
> On 2015/10/05 16:41:14, adamk wrote:
> > I'm happy that Toon is happy with this approach. I'm still a bit worried
about
> > the "not yet specced" part, though, especially as we're adding this new
> behavior
> > to existing Symbols. Is there a reason not to start this with only allowing
> > window[@@isConcatSpreadable] to return undefined?
> 
> If it's necessary, we could turn off this special handling by simply not
setting
> the well-known symbol bit during native context setup, so that seems like it's
> probably okay.
> 
> To only enable this for @@isConcatSpreadable, that's pretty easy too

Anyways, I've added a patch set which limits the special-casing to just
@@IsConcatSpreadable, and it should be trivial to add other symbols as needed.

[adamk, 2015-10-05 20:21:59]

lgtm with an updated CL description noting that this only changes behavior for
@@isConcatSpreadable for now.

[adamk, 2015-10-06 16:54:54]

The CQ bit was checked by adamk@chromium.org

[adamk, 2015-10-06 16:54:54]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1230793002/#ps200001
(title: "Only special-case @@isConcatSpreadable")

[commit-bot: I haz the power, 2015-10-06 16:55:02]

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

[commit-bot: I haz the power, 2015-10-06 16:56:25]

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

[commit-bot: I haz the power, 2015-10-06 16:56:25]

Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10413)
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/6408)

[caitp (gmail), 2015-10-06 16:58:27]

On 2015/10/06 16:56:25, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
>   v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
>   v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
>   v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
>   v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
>   v8_mac_rel on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10413)
>   v8_presubmit on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/6408)

[caitp (gmail), 2015-10-06 16:59:14]

On 2015/10/06 16:58:27, caitp wrote:
> On 2015/10/06 16:56:25, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
> >   v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
> >   v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
> >   v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
> >   v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
> >   v8_mac_rel on tryserver.v8 (JOB_FAILED,
> > http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10413)
> >   v8_presubmit on tryserver.v8 (JOB_FAILED,
> > http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/6408)

I'll send a rebase, surprised it's bitrotten already.

Sorry about the duplicate emails, Canary seems to be quite jumpy about
submitting these rietveld forms today

[caitp (gmail), 2015-10-06 17:05:34]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-10-06 17:05:34]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
jochen@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1230793002/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-06 17:05:43]

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

[commit-bot: I haz the power, 2015-10-06 17:16:54]

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

[commit-bot: I haz the power, 2015-10-06 17:16:55]

Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10414)

[caitp (gmail), 2015-10-06 17:45:29]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-10-06 17:45:30]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
jochen@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1230793002/#ps230001
(title: "Ensure @@isConcatSpreadable is actually installed now that its in a
separate list")

[commit-bot: I haz the power, 2015-10-06 17:45:48]

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

[commit-bot: I haz the power, 2015-10-06 18:10:29]

Committed patchset #12 (id:230001)

[commit-bot: I haz the power, 2015-10-06 18:10:57]

Patchset 12 (id:??) landed as
https://crrev.com/8561dbd655769cb63fc797e7ebd3923bee943212
Cr-Commit-Position: refs/heads/master@{#31131}