Optionally invoke an interceptor on failed access checks

Optionally invoke an interceptor on failed access checks

This superseeds all-can-read/all-can-write properties

BUG=chromium:618305
R=verwaest@chromium.org

Committed: https://crrev.com/c34cc7a6ff20d7fb5a6ae1e7327278d3eb12e688
Cr-Commit-Position: refs/heads/master@{#37286}

[jochen (gone - plz use gerrit), 2016-06-21 14:36:48]

ptal - currently adding tests, but the basic functionality is there.

Once we moved Window to this API, we can provide an API to create an context
with an access check callback + named handler only (but without a global object)

[jochen (gone - plz use gerrit), 2016-06-21 14:39:31]

On 2016/06/21 at 14:36:48, jochen wrote:
> ptal - currently adding tests, but the basic functionality is there.
> 
> Once we moved Window to this API, we can provide an API to create an context
with an access check callback + named handler only (but without a global object)

btw, do we need to do something special for Location of an OOPIF? I guess since
it's from a getter on the remote window, it'll be created in your context
anyways?

[dcheng, 2016-06-21 14:56:55]

On 2016/06/21 14:39:31, jochen wrote:
> On 2016/06/21 at 14:36:48, jochen wrote:
> > ptal - currently adding tests, but the basic functionality is there.
> > 
> > Once we moved Window to this API, we can provide an API to create an context
> with an access check callback + named handler only (but without a global
object)
> 
> btw, do we need to do something special for Location of an OOPIF? I guess
since
> it's from a getter on the remote window, it'll be created in your context
> anyways?

I've been going back and forth on this. Originally, I was thinking that we could
just create it in the context of the accessing window. But then that feels kind
of weird: what if the accessing window gets detached before the remote window?
Because of that, I was thinking that we'd want a special object for Location as
well that lives on WindowProxy. But this seems kind of hacky as well...

[Toon Verwaest, 2016-06-22 11:30:16]

https://codereview.chromium.org/2087823002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2087823002/diff/20001/src/objects.cc#newcode1360
src/objects.cc:1360: Handle<InterceptorInfo>
JSObject::GetInterceptorForFailedAccessCheck(
Seems like this belongs on the LookupIterator, but I don't feel strongly about
it.

https://codereview.chromium.org/2087823002/diff/20001/src/objects.cc#newcode1385
src/objects.cc:1385: if (!interceptor.is_null()) {
if (interceptor.is_null())? Negation just confuses.

https://codereview.chromium.org/2087823002/diff/20001/src/objects.cc#newcode4264
src/objects.cc:4264: Maybe<bool> JSObject::SetPropertyWithInterceptorInternal(
namespace {} helper method?

https://codereview.chromium.org/2087823002/diff/20001/src/objects.cc#newcode5577
src/objects.cc:5577: JSObject::GetPropertyAttributesWithInterceptorInternal(
Same here?

https://codereview.chromium.org/2087823002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:18923: if (!maybe_constructor->IsJSFunction()) return NULL;
Shouldn't this be a DCHECK; in addition to a
DCHECK(receiver->map()->is_access_check_needed())?

https://codereview.chromium.org/2087823002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:18925: if (!constructor->shared()->IsApiFunction()) return NULL;
In which case this has to be an API function, so another DCHECK.

https://codereview.chromium.org/2087823002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:18928:
constructor->shared()->get_api_func_data()->access_check_info();
Unrelated to this particular CL, but we should probably make api-func-data
directly available on API functions rather than getting it from the SFI. We
could e.g., install it in the literals array field of the jsfunction upon
FunctionTemplate instantiation.

[jochen (gone - plz use gerrit), 2016-06-22 12:28:22]

ptal - addressed your comments, and tests should pass now. still working on new
tests

https://codereview.chromium.org/2087823002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2087823002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:18923: if (!maybe_constructor->IsJSFunction()) return NULL;
On 2016/06/22 at 11:30:16, Toon Verwaest wrote:
> Shouldn't this be a DCHECK; in addition to a
DCHECK(receiver->map()->is_access_check_needed())?

it's null for detached global proxies

https://codereview.chromium.org/2087823002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:18925: if (!constructor->shared()->IsApiFunction()) return NULL;
On 2016/06/22 at 11:30:16, Toon Verwaest wrote:
> In which case this has to be an API function, so another DCHECK.

it's not for the debug context.

[Toon Verwaest, 2016-06-22 13:25:21]

you still have a gcmole issue, but lgtm otherwise

[jochen (gone - plz use gerrit), 2016-06-27 08:02:17]

now with tests! and keys!

[Toon Verwaest, 2016-06-27 09:34:58]

https://codereview.chromium.org/2087823002/diff/160001/src/keys.cc
File src/keys.cc (right):

https://codereview.chromium.org/2087823002/diff/160001/src/keys.cc#newcode577
src/keys.cc:577: static_cast<PropertyFilter>(filter_ |
USE_ACCESS_CHECK_INTERCEPTOR);
We should get rid of these filters. Just duplicate the collecting path; we're
not going to walk the prototype chain anyway.

So I'd suggest keeping the old ALL_CAN_READ filter for the old case, removing it
once we migrated everyone over. And calling into a new method that just returns
the values from the interceptor directly, rather than reusing the complex
machinery of CollectOwn*; which becomes even more complex with these flags.

Keeping things separate will make both paths faster and less convoluted.

[Toon Verwaest, 2016-06-27 09:37:59]

https://codereview.chromium.org/2087823002/diff/160001/src/keys.cc
File src/keys.cc (right):

https://codereview.chromium.org/2087823002/diff/160001/src/keys.cc#newcode577
src/keys.cc:577: static_cast<PropertyFilter>(filter_ |
USE_ACCESS_CHECK_INTERCEPTOR);
On 2016/06/27 09:34:58, Toon Verwaest wrote:
> We should get rid of these filters. Just duplicate the collecting path; we're
> not going to walk the prototype chain anyway.
> 
> So I'd suggest keeping the old ALL_CAN_READ filter for the old case, removing
it
> once we migrated everyone over. And calling into a new method that just
returns
> the values from the interceptor directly, rather than reusing the complex
> machinery of CollectOwn*; which becomes even more complex with these flags.
> 
> Keeping things separate will make both paths faster and less convoluted.

I think you basically want to reuse the lower half of CollectInterceptorKeys and
only that for the !MayAccess case.

[Toon Verwaest, 2016-06-27 11:25:38]

lgtm

[jochen (gone - plz use gerrit), 2016-06-27 11:26:23]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2016-06-27 11:26:27]

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-06-27 11:48:13]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-27 11:49:16]

Description was changed from

==========
Optionally invoke an interceptor on failed access checks

This superseeds all-can-read/all-can-write properties

BUG=chromium:618305
R=verwaest@chromium.org
==========

to

==========
Optionally invoke an interceptor on failed access checks

This superseeds all-can-read/all-can-write properties

BUG=chromium:618305
R=verwaest@chromium.org

Committed: https://crrev.com/c34cc7a6ff20d7fb5a6ae1e7327278d3eb12e688
Cr-Commit-Position: refs/heads/master@{#37286}
==========

[commit-bot: I haz the power, 2016-06-27 11:49:20]

Patchset 10 (id:??) landed as
https://crrev.com/c34cc7a6ff20d7fb5a6ae1e7327278d3eb12e688
Cr-Commit-Position: refs/heads/master@{#37286}