Add access check for observed objects

Add access check for observed objects

This change is mostly straightforward: for 'normal' sorts of change records,
simply don't deliver a changeRecord to a given observer callback if an access
the callback's Context is not allowed to "GET" or "HAS" changeRecord.name on
changeRecord.object, or if ACCESS_KEYS is disallowed.

For 'splice' records, the question of whether to hand it to an observer is trickier, since
there are multiple properties involved, and multiple types of possible information leakage.
Given that access-checked objects are very rare (only two in Blink, Window and Location),
and that they are not normally used as Arrays, it seems better to simply not emit any splice
records for such objects rather than spending lots of logic to attempt to avoid information
leakage for something that may never happen.

BUG=v8:2778
R=rossberg@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=16663

[rossberg, 2013-08-16 13:51:54]

This looks good so far, but unfortunately, I don't think it's enough to just
check GET access. For example, HAS or KEYS might be restricted, while GET is
allowed.

I think the accurate semantics would be something like this:

- if an observer does not have GET access to a property, it should not see
update changes for that property (but can still see delete or reconfiguration
events);

- if an observer does not have HAS access to a property, it should not see
update, delete or reconfiguration changes for that property;

- if an observer does not have KEYS access to an object, it should not see any
changes for that object.

Maybe it's not worth distinguishing the first and second, but you still need to
check for either access type. The last one applies to the whole object, as
opposed to individual properties, which is something that we need to distinguish
in the tests.

It's annoying that the API does not allow to ask about all access types in one
call. But I don't see much we can do about that. Fortunately, observe
performance on access-checked objects should not matter in practice, AFAICS.

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
File test/cctest/test-object-observe.cc (right):

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:479: TEST(ObserveNamedAccessCheck) {
Would be good to refine the test such that only individual properties are
blocked, while others pass (for both named and indexed). E.g. by replacing your
blockAccess mechanics with a black list.

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:488: CompileRun("records = null;"
Nit: can we make these proper 'var' declarations?

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:499: CompileRun("records2 = null;"
Same here.

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:503: "obj.foo = 'bar';"
Please add other forms of mutation, in particular, via Object.defineProperty and
via the API.

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:585: "[].pop.call(obj);"
Can we check some more complex array functions, too?

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:587: // TODO(adamk): Support splice records
What is this TODO about?

[adamk, 2013-08-16 21:48:47]

On 2013/08/16 13:51:54, rossberg wrote:
> This looks good so far, but unfortunately, I don't think it's enough to just
> check GET access. For example, HAS or KEYS might be restricted, while GET is
> allowed.
> 
> I think the accurate semantics would be something like this:
> 
> - if an observer does not have GET access to a property, it should not see
> update changes for that property (but can still see delete or reconfiguration
> events);
> 
> - if an observer does not have HAS access to a property, it should not see
> update, delete or reconfiguration changes for that property;
> 
> - if an observer does not have KEYS access to an object, it should not see any
> changes for that object.
> 
> Maybe it's not worth distinguishing the first and second, but you still need
to
> check for either access type. The last one applies to the whole object, as
> opposed to individual properties, which is something that we need to
distinguish
> in the tests.
> 
> It's annoying that the API does not allow to ask about all access types in one
> call. But I don't see much we can do about that. Fortunately, observe
> performance on access-checked objects should not matter in practice, AFAICS.

Thanks for mentioning ACCESS_KEYS. With that as a model, I think the above all
argues for just adding a new ACCESS_OBSERVE enum (as the one API change) and
following ACCESS_KEYS's lead of checking access via calling MayNamedAccess()
with undefined as the "key" to check for whether observation is allowed. We
could check both at the time of initial observation and when enqueueing.

It simply doesn't seem worthwhile to allow observation of only some changes to
an object (which is what it seems you're proposing with the set of complex rules
above), especially since in practice the only observed objects are Window and
Location, and moreover the only properties where access _is_ allowed tend to be
non-observable (they're provided by native accessors). Basically, if you're in a
place where an ACCESS_GET would fail, providing Object.observe doesn't provide
value.

What are your thoughts on that (minimal) change to the API?

[rossberg, 2013-08-19 10:19:59]

On 2013/08/16 21:48:47, adamk wrote:
> On 2013/08/16 13:51:54, rossberg wrote:
> > This looks good so far, but unfortunately, I don't think it's enough to just
> > check GET access. For example, HAS or KEYS might be restricted, while GET is
> > allowed.
> > 
> > I think the accurate semantics would be something like this:
> > 
> > - if an observer does not have GET access to a property, it should not see
> > update changes for that property (but can still see delete or
reconfiguration
> > events);
> > 
> > - if an observer does not have HAS access to a property, it should not see
> > update, delete or reconfiguration changes for that property;
> > 
> > - if an observer does not have KEYS access to an object, it should not see
any
> > changes for that object.
> > 
> > Maybe it's not worth distinguishing the first and second, but you still need
> to
> > check for either access type. The last one applies to the whole object, as
> > opposed to individual properties, which is something that we need to
> distinguish
> > in the tests.
> > 
> > It's annoying that the API does not allow to ask about all access types in
one
> > call. But I don't see much we can do about that. Fortunately, observe
> > performance on access-checked objects should not matter in practice, AFAICS.
> 
> Thanks for mentioning ACCESS_KEYS. With that as a model, I think the above all
> argues for just adding a new ACCESS_OBSERVE enum (as the one API change) and
> following ACCESS_KEYS's lead of checking access via calling MayNamedAccess()
> with undefined as the "key" to check for whether observation is allowed. We
> could check both at the time of initial observation and when enqueueing.
> 
> It simply doesn't seem worthwhile to allow observation of only some changes to
> an object (which is what it seems you're proposing with the set of complex
rules
> above), especially since in practice the only observed objects are Window and
> Location, and moreover the only properties where access _is_ allowed tend to
be
> non-observable (they're provided by native accessors). Basically, if you're in
a
> place where an ACCESS_GET would fail, providing Object.observe doesn't provide
> value.
> 
> What are your thoughts on that (minimal) change to the API?

No, as we discussed before, this won't fly.

It is not sufficient because we would still have to correctly deal with the
cases where ACCESS_OBSERVE was not filtered, but e.g. GET was -- which includes
all existing code in all embedders! The API allows these combinations
(supposedly for a reason), so we haven't won anything by adding a new flag, we'd
still need to check the other flags.

It is not necessary, because if we properly check the other flags then
ACCESS_OBSERVE would essentially be subsumed by ACCESS_KEYS. That is, we don't
provide any new useful functionality to users of the API.

It is not an option anyway because requiring everybody to set and handle
ACCESS_OBSERVE is a serious breaking change to the API at this point. It breaks
the security model of all existing embedders. And it probably breaks a lot of
them functionally, because their access callbacks do not expect the new value.

I acknowledge that proper access checking for Object.observe is an annoyance.
But the reason for that is not an incomplete API, but simply the fact that
Object.observe is such a big potential leak, for so many different pieces of
information! We can't cheat our way out of that, I'm afraid.

[adamk, 2013-08-19 17:15:59]

On 2013/08/19 10:19:59, rossberg wrote:
> On 2013/08/16 21:48:47, adamk wrote:
> > On 2013/08/16 13:51:54, rossberg wrote:
> > > This looks good so far, but unfortunately, I don't think it's enough to
just
> > > check GET access. For example, HAS or KEYS might be restricted, while GET
is
> > > allowed.
> > > 
> > > I think the accurate semantics would be something like this:
> > > 
> > > - if an observer does not have GET access to a property, it should not see
> > > update changes for that property (but can still see delete or
> reconfiguration
> > > events);
> > > 
> > > - if an observer does not have HAS access to a property, it should not see
> > > update, delete or reconfiguration changes for that property;
> > > 
> > > - if an observer does not have KEYS access to an object, it should not see
> any
> > > changes for that object.
> > > 
> > > Maybe it's not worth distinguishing the first and second, but you still
need
> > to
> > > check for either access type. The last one applies to the whole object, as
> > > opposed to individual properties, which is something that we need to
> > distinguish
> > > in the tests.
> > > 
> > > It's annoying that the API does not allow to ask about all access types in
> one
> > > call. But I don't see much we can do about that. Fortunately, observe
> > > performance on access-checked objects should not matter in practice,
AFAICS.
> > 
> > Thanks for mentioning ACCESS_KEYS. With that as a model, I think the above
all
> > argues for just adding a new ACCESS_OBSERVE enum (as the one API change) and
> > following ACCESS_KEYS's lead of checking access via calling MayNamedAccess()
> > with undefined as the "key" to check for whether observation is allowed. We
> > could check both at the time of initial observation and when enqueueing.
> > 
> > It simply doesn't seem worthwhile to allow observation of only some changes
to
> > an object (which is what it seems you're proposing with the set of complex
> rules
> > above), especially since in practice the only observed objects are Window
and
> > Location, and moreover the only properties where access _is_ allowed tend to
> be
> > non-observable (they're provided by native accessors). Basically, if you're
in
> a
> > place where an ACCESS_GET would fail, providing Object.observe doesn't
provide
> > value.
> > 
> > What are your thoughts on that (minimal) change to the API?
> 
> No, as we discussed before, this won't fly.
> 
> It is not sufficient because we would still have to correctly deal with the
> cases where ACCESS_OBSERVE was not filtered, but e.g. GET was -- which
includes
> all existing code in all embedders! The API allows these combinations
> (supposedly for a reason), so we haven't won anything by adding a new flag,
we'd
> still need to check the other flags.
> 
> It is not necessary, because if we properly check the other flags then
> ACCESS_OBSERVE would essentially be subsumed by ACCESS_KEYS. That is, we don't
> provide any new useful functionality to users of the API.
> 
> It is not an option anyway because requiring everybody to set and handle
> ACCESS_OBSERVE is a serious breaking change to the API at this point. It
breaks
> the security model of all existing embedders. And it probably breaks a lot of
> them functionally, because their access callbacks do not expect the new value.

Do you know of any other embedders that use access checks, besides Blink? I
would be surprised to learn that Node.js makes use of them. Basically I'm
asking: is this a theoretical concern (given that you don't know which embedders
are out there) or is it concrete?

[adamk, 2013-08-19 17:38:40]

On 2013/08/19 17:15:59, adamk wrote:
> Do you know of any other embedders that use access checks, besides Blink? I
> would be surprised to learn that Node.js makes use of them. Basically I'm
> asking: is this a theoretical concern (given that you don't know which
embedders
> are out there) or is it concrete?

To flesh this out a little more: it seems to me that the current API is not a
good fit for its use in Blink. Some mismatches:

- The optional data arg, though at first glance appearing to be used, is never
accessed in our security check callbacks.
- The actually useful "data", the current context, is passed only implicitly
(which makes the code in this patch a little weird, as it has to switch contexts
before calling the callback).
- Only ACCESS_HAS and ACCESS_GET are of interest to Blink. That is, ACCESS_SET
and ACCESS_KEYS are not distinguished at all (both use the same check, which
doesn't look at the key).

Thus, if Blink were to be the only embedder that uses the access check API, I
might be inclined to suggest fitting it more closely to the web security
use-case.

[rossberg, 2013-08-19 17:51:53]

On 2013/08/19 17:15:59, adamk wrote:
> On 2013/08/19 10:19:59, rossberg wrote:
> > On 2013/08/16 21:48:47, adamk wrote:
> > > On 2013/08/16 13:51:54, rossberg wrote:
> > > > This looks good so far, but unfortunately, I don't think it's enough to
> just
> > > > check GET access. For example, HAS or KEYS might be restricted, while
GET
> is
> > > > allowed.
> > > > 
> > > > I think the accurate semantics would be something like this:
> > > > 
> > > > - if an observer does not have GET access to a property, it should not
see
> > > > update changes for that property (but can still see delete or
> > reconfiguration
> > > > events);
> > > > 
> > > > - if an observer does not have HAS access to a property, it should not
see
> > > > update, delete or reconfiguration changes for that property;
> > > > 
> > > > - if an observer does not have KEYS access to an object, it should not
see
> > any
> > > > changes for that object.
> > > > 
> > > > Maybe it's not worth distinguishing the first and second, but you still
> need
> > > to
> > > > check for either access type. The last one applies to the whole object,
as
> > > > opposed to individual properties, which is something that we need to
> > > distinguish
> > > > in the tests.
> > > > 
> > > > It's annoying that the API does not allow to ask about all access types
in
> > one
> > > > call. But I don't see much we can do about that. Fortunately, observe
> > > > performance on access-checked objects should not matter in practice,
> AFAICS.
> > > 
> > > Thanks for mentioning ACCESS_KEYS. With that as a model, I think the above
> all
> > > argues for just adding a new ACCESS_OBSERVE enum (as the one API change)
and
> > > following ACCESS_KEYS's lead of checking access via calling
MayNamedAccess()
> > > with undefined as the "key" to check for whether observation is allowed.
We
> > > could check both at the time of initial observation and when enqueueing.
> > > 
> > > It simply doesn't seem worthwhile to allow observation of only some
changes
> to
> > > an object (which is what it seems you're proposing with the set of complex
> > rules
> > > above), especially since in practice the only observed objects are Window
> and
> > > Location, and moreover the only properties where access _is_ allowed tend
to
> > be
> > > non-observable (they're provided by native accessors). Basically, if
you're
> in
> > a
> > > place where an ACCESS_GET would fail, providing Object.observe doesn't
> provide
> > > value.
> > > 
> > > What are your thoughts on that (minimal) change to the API?
> > 
> > No, as we discussed before, this won't fly.
> > 
> > It is not sufficient because we would still have to correctly deal with the
> > cases where ACCESS_OBSERVE was not filtered, but e.g. GET was -- which
> includes
> > all existing code in all embedders! The API allows these combinations
> > (supposedly for a reason), so we haven't won anything by adding a new flag,
> we'd
> > still need to check the other flags.
> > 
> > It is not necessary, because if we properly check the other flags then
> > ACCESS_OBSERVE would essentially be subsumed by ACCESS_KEYS. That is, we
don't
> > provide any new useful functionality to users of the API.
> > 
> > It is not an option anyway because requiring everybody to set and handle
> > ACCESS_OBSERVE is a serious breaking change to the API at this point. It
> breaks
> > the security model of all existing embedders. And it probably breaks a lot
of
> > them functionally, because their access callbacks do not expect the new
value.
> 
> Do you know of any other embedders that use access checks, besides Blink? I
> would be surprised to learn that Node.js makes use of them. Basically I'm
> asking: is this a theoretical concern (given that you don't know which
embedders
> are out there) or is it concrete?

There are several browsers and browser-like environments besides Chrome that
embed either V8 or Blink+V8, e.g. Android, Opera, the tool chain Adobe is
working on, and some projects I know less about. And AFAIK even Blink embedders
typically add their own additional native objects to the runtime on top of what
Blink provides. So yes, AFAICT, this is a real issue. In particular, since it is
easy to introduce security holes unnoticed with such a change.

Even if it wasn't, I still think the current access API is the correct
abstraction (or it least close to it). It filters by kind of observable
information and its possible modifications, not by what language operations
invoke those. That is an important abstraction, because you don't want to make
the API and its uses dependent on the ever-changing zoo of operations the
language might provide. (Although admittedly it feels rather ES3 in its current
form.)

Is there any use case you envision that would care whether observe is 10x vs
just 3x slower on objects with access checks?

[adamk, 2013-08-19 18:24:29]

On 2013/08/19 17:51:53, rossberg wrote:
> On 2013/08/19 17:15:59, adamk wrote:
> > On 2013/08/19 10:19:59, rossberg wrote:
> > > On 2013/08/16 21:48:47, adamk wrote:
> > > > On 2013/08/16 13:51:54, rossberg wrote:
> > > > > This looks good so far, but unfortunately, I don't think it's enough
to
> > just
> > > > > check GET access. For example, HAS or KEYS might be restricted, while
> GET
> > is
> > > > > allowed.
> > > > > 
> > > > > I think the accurate semantics would be something like this:
> > > > > 
> > > > > - if an observer does not have GET access to a property, it should not
> see
> > > > > update changes for that property (but can still see delete or
> > > reconfiguration
> > > > > events);
> > > > > 
> > > > > - if an observer does not have HAS access to a property, it should not
> see
> > > > > update, delete or reconfiguration changes for that property;
> > > > > 
> > > > > - if an observer does not have KEYS access to an object, it should not
> see
> > > any
> > > > > changes for that object.
> > > > > 
> > > > > Maybe it's not worth distinguishing the first and second, but you
still
> > need
> > > > to
> > > > > check for either access type. The last one applies to the whole
object,
> as
> > > > > opposed to individual properties, which is something that we need to
> > > > distinguish
> > > > > in the tests.
> > > > > 
> > > > > It's annoying that the API does not allow to ask about all access
types
> in
> > > one
> > > > > call. But I don't see much we can do about that. Fortunately, observe
> > > > > performance on access-checked objects should not matter in practice,
> > AFAICS.
> > > > 
> > > > Thanks for mentioning ACCESS_KEYS. With that as a model, I think the
above
> > all
> > > > argues for just adding a new ACCESS_OBSERVE enum (as the one API change)
> and
> > > > following ACCESS_KEYS's lead of checking access via calling
> MayNamedAccess()
> > > > with undefined as the "key" to check for whether observation is allowed.
> We
> > > > could check both at the time of initial observation and when enqueueing.
> > > > 
> > > > It simply doesn't seem worthwhile to allow observation of only some
> changes
> > to
> > > > an object (which is what it seems you're proposing with the set of
complex
> > > rules
> > > > above), especially since in practice the only observed objects are
Window
> > and
> > > > Location, and moreover the only properties where access _is_ allowed
tend
> to
> > > be
> > > > non-observable (they're provided by native accessors). Basically, if
> you're
> > in
> > > a
> > > > place where an ACCESS_GET would fail, providing Object.observe doesn't
> > provide
> > > > value.
> > > > 
> > > > What are your thoughts on that (minimal) change to the API?
> > > 
> > > No, as we discussed before, this won't fly.
> > > 
> > > It is not sufficient because we would still have to correctly deal with
the
> > > cases where ACCESS_OBSERVE was not filtered, but e.g. GET was -- which
> > includes
> > > all existing code in all embedders! The API allows these combinations
> > > (supposedly for a reason), so we haven't won anything by adding a new
flag,
> > we'd
> > > still need to check the other flags.
> > > 
> > > It is not necessary, because if we properly check the other flags then
> > > ACCESS_OBSERVE would essentially be subsumed by ACCESS_KEYS. That is, we
> don't
> > > provide any new useful functionality to users of the API.
> > > 
> > > It is not an option anyway because requiring everybody to set and handle
> > > ACCESS_OBSERVE is a serious breaking change to the API at this point. It
> > breaks
> > > the security model of all existing embedders. And it probably breaks a lot
> of
> > > them functionally, because their access callbacks do not expect the new
> value.
> > 
> > Do you know of any other embedders that use access checks, besides Blink? I
> > would be surprised to learn that Node.js makes use of them. Basically I'm
> > asking: is this a theoretical concern (given that you don't know which
> embedders
> > are out there) or is it concrete?
> 
> There are several browsers and browser-like environments besides Chrome that
> embed either V8 or Blink+V8, e.g. Android, Opera, the tool chain Adobe is
> working on, and some projects I know less about. And AFAIK even Blink
embedders
> typically add their own additional native objects to the runtime on top of
what
> Blink provides. So yes, AFAICT, this is a real issue. In particular, since it
is
> easy to introduce security holes unnoticed with such a change.
> 
> Even if it wasn't, I still think the current access API is the correct
> abstraction (or it least close to it). It filters by kind of observable
> information and its possible modifications, not by what language operations
> invoke those. That is an important abstraction, because you don't want to make
> the API and its uses dependent on the ever-changing zoo of operations the
> language might provide. (Although admittedly it feels rather ES3 in its
current
> form.)

Thanks for the quick response, and for explaining your concerns in more detail.

> Is there any use case you envision that would care whether observe is 10x vs
> just 3x slower on objects with access checks?

It wasn't a performance concern I had in mind, only a complexity concern (e.g.,
that we might end up introducing more bugs by trying to follow the rules you
laid out in your first response). But I take your point that at least the API
provides abstraction between the concrete security concern (leaks between
different-origin frames) and the details of the language (how one can get access
to properties of an object).

So, to go back to your initial response: would you be OK with being minimally
permissive, and simply checking all three access types and dropping the record
if any of HAS, GET, or KEYS fails the access check? This would also happen to
avoid any performance change in practice in Blink, since the KEYS check will
fail fast.

[abarth-chromium, 2013-08-19 18:58:57]

On 2013/08/19 17:51:53, rossberg wrote:
> There are several browsers and browser-like environments besides Chrome that
> embed either V8 or Blink+V8,

Of the ones you've mentioned, I don't know of any other than Blink that turn on
access checks.  It's a super odd-ball thing to do.  Even Blink only does it for
two objects, and then only because of legacy HTML content.

[adamk, 2013-08-20 21:38:16]

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
File test/cctest/test-object-observe.cc (right):

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:479: TEST(ObserveNamedAccessCheck) {
On 2013/08/16 13:51:54, rossberg wrote:
> Would be good to refine the test such that only individual properties are
> blocked, while others pass (for both named and indexed). E.g. by replacing
your
> blockAccess mechanics with a black list.

Done.

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:488: CompileRun("records = null;"
On 2013/08/16 13:51:54, rossberg wrote:
> Nit: can we make these proper 'var' declarations?

Done.

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:499: CompileRun("records2 = null;"
On 2013/08/16 13:51:54, rossberg wrote:
> Same here.

Done.

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:503: "obj.foo = 'bar';"
On 2013/08/16 13:51:54, rossberg wrote:
> Please add other forms of mutation, in particular, via Object.defineProperty
and
> via the API.

Happy to expand this a little bit, but I'd be interested in what you think
counts as good coverage; see my question below about complex array methods.

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:585: "[].pop.call(obj);"
On 2013/08/16 13:51:54, rossberg wrote:
> Can we check some more complex array functions, too?

How complex do you want? It's not clear to me what stuff should be tested here,
vs just testing access checks in general and testing record enqueueing in
general.

We have tests in mjsunit for testing that we get the right records in
Object.observe; as long as there's a reasonably tight bottleneck in
src/object-observe.js for where we enqueue records, I wouldn't expect there to
be any increase in test coverage from testing, say, 2 array methods compared to
5 array methods.

I ask because writing tests in this style (as API tests) is super-verbose and
time consuming, compared to writing tests in JS, so if I can get more coverage
out of the JS tests I'd much rather do that.

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:587: // TODO(adamk): Support splice records
On 2013/08/16 13:51:54, rossberg wrote:
> What is this TODO about?

Made it longer.

[adamk, 2013-08-20 22:09:20]

Added some more tests, too; still interested in your thoughts on how much
coverage is enough here.

[rossberg, 2013-08-21 09:31:50]

On 2013/08/19 18:24:29, adamk wrote:
> So, to go back to your initial response: would you be OK with being minimally
> permissive, and simply checking all three access types and dropping the record
> if any of HAS, GET, or KEYS fails the access check? This would also happen to
> avoid any performance change in practice in Blink, since the KEYS check will
> fail fast.

Yes, that sounds perfectly reasonable to me.

[rossberg, 2013-08-21 10:26:41]

On 2013/08/19 18:58:57, abarth wrote:
> On 2013/08/19 17:51:53, rossberg wrote:
> > There are several browsers and browser-like environments besides Chrome that
> > embed either V8 or Blink+V8,
> 
> Of the ones you've mentioned, I don't know of any other than Blink that turn
on
> access checks.  It's a super odd-ball thing to do.  Even Blink only does it
for
> two objects, and then only because of legacy HTML content.

Do you know if there is a chance that Blink might get along without that some
day? We could probably eliminate thousands of lines of messy V8-internal code if
we could rip out access checking.

[rossberg, 2013-08-21 10:45:51]

If you also add checking for ACCESS_HAS then I think the CL is good to go.

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
File test/cctest/test-object-observe.cc (right):

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:585: "[].pop.call(obj);"
On 2013/08/20 21:38:16, adamk wrote:
> On 2013/08/16 13:51:54, rossberg wrote:
> > Can we check some more complex array functions, too?
> 
> How complex do you want? It's not clear to me what stuff should be tested
here,
> vs just testing access checks in general and testing record enqueueing in
> general.
> 
> We have tests in mjsunit for testing that we get the right records in
> Object.observe; as long as there's a reasonably tight bottleneck in
> src/object-observe.js for where we enqueue records, I wouldn't expect there to
> be any increase in test coverage from testing, say, 2 array methods compared
to
> 5 array methods.
> 
> I ask because writing tests in this style (as API tests) is super-verbose and
> time consuming, compared to writing tests in JS, so if I can get more coverage
> out of the JS tests I'd much rather do that.

Doesn't have to be crazy. I'd be content with one more method that creates more
than just a single straightforward splice record.

[adamk, 2013-08-21 16:34:27]

On 2013/08/21 10:45:51, rossberg wrote:
> If you also add checking for ACCESS_HAS then I think the CL is good to go.

This just means another call to May*Access(), right? Will add that, and rename
the runtime method to reflect new behavior.

[adamk, 2013-08-21 17:18:36]

Okay, added everything I think we needed. The tests have gotten a little
verbose; let me know if you'd like them refactored before commit.

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
File test/cctest/test-object-observe.cc (right):

https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-ob...
test/cctest/test-object-observe.cc:585: "[].pop.call(obj);"
On 2013/08/21 10:45:52, rossberg wrote:
> On 2013/08/20 21:38:16, adamk wrote:
> > On 2013/08/16 13:51:54, rossberg wrote:
> > > Can we check some more complex array functions, too?
> > 
> > How complex do you want? It's not clear to me what stuff should be tested
> here,
> > vs just testing access checks in general and testing record enqueueing in
> > general.
> > 
> > We have tests in mjsunit for testing that we get the right records in
> > Object.observe; as long as there's a reasonably tight bottleneck in
> > src/object-observe.js for where we enqueue records, I wouldn't expect there
to
> > be any increase in test coverage from testing, say, 2 array methods compared
> to
> > 5 array methods.
> > 
> > I ask because writing tests in this style (as API tests) is super-verbose
and
> > time consuming, compared to writing tests in JS, so if I can get more
coverage
> > out of the JS tests I'd much rather do that.
> 
> Doesn't have to be crazy. I'd be content with one more method that creates
more
> than just a single straightforward splice record.

The nature of splice records is that there aren't any array operations that
create more than one record, but I've updated the test to do more than one
operation on the object.

[rossberg, 2013-08-22 11:14:09]

> Okay, added everything I think we needed. The tests have gotten a little
> verbose; let me know if you'd like them refactored before commit.

LGTM, except that, yes, it would be good if you could refactor the tests to
avoid the repetition. Can't you just wrap the test body into a loop over an
array from which you take pointers to different AccessCheck functions?

(Please keep in mind that the tree is still not open for landing, though.)


> The nature of splice records is that there aren't any array operations that
> create more than one record, but I've updated the test to do more than one
> operation on the object.

OK. I thought there were some that can create multiple splice records under
certain conditions, but I probably misremembered. :)

[adamk, 2013-09-11 19:44:48]

On 2013/08/22 11:14:09, rossberg wrote:
> > Okay, added everything I think we needed. The tests have gotten a little
> > verbose; let me know if you'd like them refactored before commit.
> 
> LGTM, except that, yes, it would be good if you could refactor the tests to
> avoid the repetition. Can't you just wrap the test body into a loop over an
> array from which you take pointers to different AccessCheck functions?

Okay, refactored to avoid some repetition. Named and Indexed checks have a
single TEST block each, and I've added a global static to mark the type to
block. I also factored out a helper for generating the access-checked object in
the first place.

Landing now (test coverage is the same as before), happy to do more work on the
tests if you have any follow-up comments.

> (Please keep in mind that the tree is still not open for landing, though.)
> 
> 
> > The nature of splice records is that there aren't any array operations that
> > create more than one record, but I've updated the test to do more than one
> > operation on the object.
> 
> OK. I thought there were some that can create multiple splice records under
> certain conditions, but I probably misremembered. :)

[adamk, 2013-09-11 20:04:00]

Committed patchset #9 manually as r16663 (presubmit successful).