Make FunctionTemplate::HasInstance checks work with remote contexts.

Make instance checks understand remote contexts.

https://crrev.com/2500363002 updated FunctionTemplate::HasInstance to
follow the hidden prototype chain of a global proxy to the global
object. However, remote contexts don't have a global object to check;
instead, teach the instance check knows about the conventions of
global proxy setup and have it also check the constructor's prototype. 

Similarly, also teach Object::FindInstanceInPrototypeChain about the
unusual conventions for remote contexts.

BUG=527190
Review-Url: https://codereview.chromium.org/2698683003
Cr-Commit-Position: refs/heads/master@{#43263}
Committed: https://chromium.googlesource.com/v8/v8/+/692cccce2695b900f181eb2fdfe83e4dec0d4bf1

[dcheng, 2017-02-16 07:11:30]

Description was changed from

==========
Make FunctionTemplate::HasInstance checks work with remote contexts.

Remote contexts don't have a global object to check; instead, check
the constructor's prototype.

BUG=527190
==========

to

==========
Make FunctionTemplate::HasInstance checks work with remote contexts.

https://codereview.chromium.org/2500363002 updated this check to follow
the hidden prototype chain of a global proxy to the global object.
However, remote contexts don't have a global object to check; instead,
teach the instance check knows about the conventions of global proxy setup and
have it also check the constructor's prototype. 

BUG=527190
==========

[dcheng, 2017-02-16 07:11:30]

dcheng@chromium.org changed reviewers:
+ jochen@chromium.org, yukishiino@chromium.org

[dcheng, 2017-02-16 07:12:45]

https://codereview.chromium.org/2698683003/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2698683003/diff/20001/src/api.cc#newcode6577
src/api.cc:6577: // Otherwise, fallback to testing the prototype chain.
I tried to remove this section, but it makes a bunch of worker things fail. I'm
going to investigate and see if I can remove this exception for workers
(probably in a followup?)

[Yuki, 2017-02-16 07:29:02]

LGTM on my side.
Defer to jochen.

[dcheng, 2017-02-16 08:00:12]

Note that with this and https://codereview.chromium.org/2693203003/ applied, the
patch to use remote contexts /almost/ works.

However, there are still some issues:
- EventTarget also needs to use LenientThis, to properly handle dispatchEvent
- However, after making EventTarget a LenientThis, security checks don't appear
to work correctly...
- Similarly, a few other things like __lookupGetter__ seem to be incorrectly
bypassing checks...

https://codereview.chromium.org/2698683003/diff/30001/src/api.cc
File src/api.cc (left):

https://codereview.chromium.org/2698683003/diff/30001/src/api.cc#oldcode6554
src/api.cc:6554: return
self->IsTemplateFor(iter.GetCurrent<i::JSGlobalObject>());
I was forgetting to walk the parent templates; after fixing this, I don't need
to inspect the global object at all.

[dcheng, 2017-02-16 08:01:21]

On 2017/02/16 08:00:12, dcheng wrote:
> Note that with this and https://codereview.chromium.org/2693203003/ applied,
the
> patch to use remote contexts /almost/ works.
> 
> However, there are still some issues:
> - EventTarget also needs to use LenientThis, to properly handle dispatchEvent
> - However, after making EventTarget a LenientThis, security checks don't
appear
> to work correctly...
> - Similarly, a few other things like __lookupGetter__ seem to be incorrectly
> bypassing checks...

And just to be clear, these regressions only affect contexts created with
NewRemoteContext(). So it doesn't regress existing behavior, just
--site-per-process.

> 
> https://codereview.chromium.org/2698683003/diff/30001/src/api.cc
> File src/api.cc (left):
> 
> https://codereview.chromium.org/2698683003/diff/30001/src/api.cc#oldcode6554
> src/api.cc:6554: return
> self->IsTemplateFor(iter.GetCurrent<i::JSGlobalObject>());
> I was forgetting to walk the parent templates; after fixing this, I don't need
> to inspect the global object at all.

[dcheng, 2017-02-16 10:32:05]

Description was changed from

==========
Make FunctionTemplate::HasInstance checks work with remote contexts.

https://codereview.chromium.org/2500363002 updated this check to follow
the hidden prototype chain of a global proxy to the global object.
However, remote contexts don't have a global object to check; instead,
teach the instance check knows about the conventions of global proxy setup and
have it also check the constructor's prototype. 

BUG=527190
==========

to

==========
Make instance checks understand remote contexts.

https://crrev.com/2500363002 updated FunctionTemplate::HasInstance to
follow the hidden prototype chain of a global proxy to the global
object. However, remote contexts don't have a global object to check;
instead, teach the instance check knows about the conventions of
global proxy setup and have it also check the constructor's prototype. 

Similarly, also teach Object::FindInstanceInPrototypeChain about the
unusual conventions for remote contexts.

BUG=527190
==========

[dcheng, 2017-02-16 10:35:23]

PTAL, I had to update FindInstanceInPrototypeChain as well.

In the long run, I think I'm still going to investigate making
https://codereview.chromium.org/2677653002/ work; I think it would reduce the
need for these special cases. In the short-term, this unblocks the patch that
actually switches Chromium to using remote contexts.

[dcheng, 2017-02-16 10:40:17]

One other surprising thing I noticed: the cross-origin interceptors for deletes
(and possibly other things) aren't actually ever invoked. Is it worth fixing
this? It's not (currently) needed to make all this functionality work. If not,
should we add a comment to the documentation for SetAccessCheckAndHandler?

[Yuki, 2017-02-16 11:40:11]

On 2017/02/16 10:40:17, dcheng wrote:
> One other surprising thing I noticed: the cross-origin interceptors for
deletes
> (and possibly other things) aren't actually ever invoked. Is it worth fixing
> this? It's not (currently) needed to make all this functionality work. If not,
> should we add a comment to the documentation for SetAccessCheckAndHandler?

I don't think it's urgent, but some of cross origin-accessible properties are
deletable in the spec.  Someday we will need to fix it, I think.

[jochen (gone - plz use gerrit), 2017-02-16 12:44:12]

lgtm

[jochen (gone - plz use gerrit), 2017-02-16 12:44:26]

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

[jochen (gone - plz use gerrit), 2017-02-16 12:44:26]

toon fyi

[dcheng, 2017-02-16 21:14:09]

Updating this so it doesn't depend on the CreationContext() patch and landing
based on the LGTMs already received.

[dcheng, 2017-02-16 21:23:20]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-02-16 21:23:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2698683003/#ps110001
(title: "Break dependency on CreationContext patch")

[commit-bot: I haz the power, 2017-02-16 21:23:30]

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, 2017-02-16 21:46:16]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1487280200987710,
"parent_rev": "d1d4b9ce51d2ef3e9bd760e70642acddac30846f", "commit_rev":
"692cccce2695b900f181eb2fdfe83e4dec0d4bf1"}

[commit-bot: I haz the power, 2017-02-16 21:46:24]

Description was changed from

==========
Make instance checks understand remote contexts.

https://crrev.com/2500363002 updated FunctionTemplate::HasInstance to
follow the hidden prototype chain of a global proxy to the global
object. However, remote contexts don't have a global object to check;
instead, teach the instance check knows about the conventions of
global proxy setup and have it also check the constructor's prototype. 

Similarly, also teach Object::FindInstanceInPrototypeChain about the
unusual conventions for remote contexts.

BUG=527190
==========

to

==========
Make instance checks understand remote contexts.

https://crrev.com/2500363002 updated FunctionTemplate::HasInstance to
follow the hidden prototype chain of a global proxy to the global
object. However, remote contexts don't have a global object to check;
instead, teach the instance check knows about the conventions of
global proxy setup and have it also check the constructor's prototype. 

Similarly, also teach Object::FindInstanceInPrototypeChain about the
unusual conventions for remote contexts.

BUG=527190

Review-Url: https://codereview.chromium.org/2698683003
Cr-Commit-Position: refs/heads/master@{#43263}
Committed:
https://chromium.googlesource.com/v8/v8/+/692cccce2695b900f181eb2fdfe83e4dec0...
==========

[commit-bot: I haz the power, 2017-02-16 21:46:27]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/v8/v8/+/692cccce2695b900f181eb2fdfe83e4dec0...