[Extensions] Handle null ScriptContexts in GetModuleSystem()

[Extensions] Handle null ScriptContexts in GetModuleSystem()

GetModuleSystem() returns an object associated with a different JS
context for testing purposes (and is also one of the "why do we even
have that lever"-type things that we'd like to get rid of, but that's a
separate issue). Without site isolation, embedded frames (even those
from non-accessible contexts) are all hosted within the same process,
and we can retrieve a ScriptContext for an embedded frame. With site
isolation, this isn't necessarily true - there may be no corresponding
script context for an embedded frame (when the frame is hosted in
another process).

In both cases, we should never return an object that is inaccessible to
the current context, and so a null context is perfectly valid. Handle
this in GetModuleSystem() and add a general comment above the
GetContextByV8Object() method in ScriptContextSet.

Fixes ExtensionBindingsApiTest.ModuleSystem for PlzNavigate +
SitePerProcess

BUG=674734
Review-Url: https://codereview.chromium.org/2654623007
Cr-Commit-Position: refs/heads/master@{#447149}
Committed: https://chromium.googlesource.com/chromium/src/+/78d31a9fd4f364abc61bf19cbd8e7f6d10209c2a

[Devlin, 2017-01-25 19:33:14]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 19:33:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-01-25 20:25:09]

Description was changed from

==========
[Extensions] Handle null ScriptContexts in GetModuleSystem()

GetModuleSystem() returns an object associated with a different JS
context for testing purposes (and is also one of the "why do we even
have that lever"-type things that we'd like to get rid of, but that's a
separate issue). Without site isolation, embedded frames (even those
from non-accessible contexts) are all hosted within the same process,
and we can retrieve a ScriptContext for an embedded frame. With site
isolation, this isn't necessarily true - there may be no corresponding
script context for an embedded frame (when the frame is hosted in
another process).

In both cases, we should never return an object that is inaccessible to
the current context, and so a null context is perfectly valid. Handle
this in GetModuleSystem() and add a general comment above the
GetContextByV8Object() method in ScriptContextSet.

BUG=674734
==========

to

==========
[Extensions] Handle null ScriptContexts in GetModuleSystem()

GetModuleSystem() returns an object associated with a different JS
context for testing purposes (and is also one of the "why do we even
have that lever"-type things that we'd like to get rid of, but that's a
separate issue). Without site isolation, embedded frames (even those
from non-accessible contexts) are all hosted within the same process,
and we can retrieve a ScriptContext for an embedded frame. With site
isolation, this isn't necessarily true - there may be no corresponding
script context for an embedded frame (when the frame is hosted in
another process).

In both cases, we should never return an object that is inaccessible to
the current context, and so a null context is perfectly valid. Handle
this in GetModuleSystem() and add a general comment above the
GetContextByV8Object() method in ScriptContextSet.

Fixes ExtensionBindingsApiTest.ModuleSystem for PlzNavigate + SitePerProcess

BUG=674734
==========

[Devlin, 2017-01-25 20:25:20]

Description was changed from

==========
[Extensions] Handle null ScriptContexts in GetModuleSystem()

GetModuleSystem() returns an object associated with a different JS
context for testing purposes (and is also one of the "why do we even
have that lever"-type things that we'd like to get rid of, but that's a
separate issue). Without site isolation, embedded frames (even those
from non-accessible contexts) are all hosted within the same process,
and we can retrieve a ScriptContext for an embedded frame. With site
isolation, this isn't necessarily true - there may be no corresponding
script context for an embedded frame (when the frame is hosted in
another process).

In both cases, we should never return an object that is inaccessible to
the current context, and so a null context is perfectly valid. Handle
this in GetModuleSystem() and add a general comment above the
GetContextByV8Object() method in ScriptContextSet.

Fixes ExtensionBindingsApiTest.ModuleSystem for PlzNavigate + SitePerProcess

BUG=674734
==========

to

==========
[Extensions] Handle null ScriptContexts in GetModuleSystem()

GetModuleSystem() returns an object associated with a different JS
context for testing purposes (and is also one of the "why do we even
have that lever"-type things that we'd like to get rid of, but that's a
separate issue). Without site isolation, embedded frames (even those
from non-accessible contexts) are all hosted within the same process,
and we can retrieve a ScriptContext for an embedded frame. With site
isolation, this isn't necessarily true - there may be no corresponding
script context for an embedded frame (when the frame is hosted in
another process).

In both cases, we should never return an object that is inaccessible to
the current context, and so a null context is perfectly valid. Handle
this in GetModuleSystem() and add a general comment above the
GetContextByV8Object() method in ScriptContextSet.

Fixes ExtensionBindingsApiTest.ModuleSystem for PlzNavigate +
SitePerProcess

BUG=674734
==========

[Devlin, 2017-01-25 20:25:32]

rdevlin.cronin@chromium.org changed reviewers:
+ nasko@chromium.org

[Devlin, 2017-01-25 20:25:32]

As promised. :)

[commit-bot: I haz the power, 2017-01-25 20:29:06]

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

[commit-bot: I haz the power, 2017-01-25 20:29:07]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-01-25 20:45:56]

nasko@chromium.org changed reviewers:
+ dcheng@chromium.org

[nasko, 2017-01-25 20:45:56]

I'd defer to dcheng@ on this one, since it isn't an area I'm comfortable with.

[Devlin, 2017-01-27 20:13:43]

On 2017/01/25 20:45:56, nasko wrote:
> I'd defer to dcheng@ on this one, since it isn't an area I'm comfortable with.

friendly ping -> dcheng

[dcheng, 2017-01-27 20:18:20]

On 2017/01/27 20:13:43, Devlin (catching up) wrote:
> On 2017/01/25 20:45:56, nasko wrote:
> > I'd defer to dcheng@ on this one, since it isn't an area I'm comfortable
with.
> 
> friendly ping -> dcheng

Can you help me understand how GetModuleSystem is used? I'm wondering why we're
calling it on a cross-context object to begin with (and the circumstance where
that happens).

[Devlin, 2017-01-27 20:40:26]

On 2017/01/27 20:18:20, dcheng wrote:
> On 2017/01/27 20:13:43, Devlin (catching up) wrote:
> > On 2017/01/25 20:45:56, nasko wrote:
> > > I'd defer to dcheng@ on this one, since it isn't an area I'm comfortable
> with.
> > 
> > friendly ping -> dcheng
> 
> Can you help me understand how GetModuleSystem is used? I'm wondering why
we're
> calling it on a cross-context object to begin with (and the circumstance where
> that happens).

GetModuleSystem() is a terrible, terrible hack that we have for various custom
bindings.  In the long term, I want to kill it, but that's a bit away.  We use
it on cross-context objects in order to force certain API bindings to be
instantiated in the background page (again, something I want to kill).  In
theory, these cross-contexts should be safe, but because it's JS, monkeypatching
can happen, and we have extra checks.

The test this fixes tries to access a cross-context object without proper
access, and should fail (which it does).  The only thing is that it failed even
more spectacularly with site isolation, because we didn't nullcheck the context.

[dcheng, 2017-01-27 21:00:34]

On 2017/01/27 20:40:26, Devlin (catching up) wrote:
> On 2017/01/27 20:18:20, dcheng wrote:
> > On 2017/01/27 20:13:43, Devlin (catching up) wrote:
> > > On 2017/01/25 20:45:56, nasko wrote:
> > > > I'd defer to dcheng@ on this one, since it isn't an area I'm comfortable
> > with.
> > > 
> > > friendly ping -> dcheng
> > 
> > Can you help me understand how GetModuleSystem is used? I'm wondering why
> we're
> > calling it on a cross-context object to begin with (and the circumstance
where
> > that happens).
> 
> GetModuleSystem() is a terrible, terrible hack that we have for various custom
> bindings.  In the long term, I want to kill it, but that's a bit away.  We use
> it on cross-context objects in order to force certain API bindings to be
> instantiated in the background page (again, something I want to kill).  In
> theory, these cross-contexts should be safe, but because it's JS,
monkeypatching
> can happen, and we have extra checks.
> 
> The test this fixes tries to access a cross-context object without proper
> access, and should fail (which it does).  The only thing is that it failed
even
> more spectacularly with site isolation, because we didn't nullcheck the
context.

Right, I don't understand that though: currently, we *do* have v8::Contexts
everywhere, even for remote frames. How is the extensions system invoking
GetModuleSystem()?

[Devlin, 2017-01-27 21:48:56]

On 2017/01/27 21:00:34, dcheng wrote:
> On 2017/01/27 20:40:26, Devlin (catching up) wrote:
> > On 2017/01/27 20:18:20, dcheng wrote:
> > > On 2017/01/27 20:13:43, Devlin (catching up) wrote:
> > > > On 2017/01/25 20:45:56, nasko wrote:
> > > > > I'd defer to dcheng@ on this one, since it isn't an area I'm
comfortable
> > > with.
> > > > 
> > > > friendly ping -> dcheng
> > > 
> > > Can you help me understand how GetModuleSystem is used? I'm wondering why
> > we're
> > > calling it on a cross-context object to begin with (and the circumstance
> where
> > > that happens).
> > 
> > GetModuleSystem() is a terrible, terrible hack that we have for various
custom
> > bindings.  In the long term, I want to kill it, but that's a bit away.  We
use
> > it on cross-context objects in order to force certain API bindings to be
> > instantiated in the background page (again, something I want to kill).  In
> > theory, these cross-contexts should be safe, but because it's JS,
> monkeypatching
> > can happen, and we have extra checks.
> > 
> > The test this fixes tries to access a cross-context object without proper
> > access, and should fail (which it does).  The only thing is that it failed
> even
> > more spectacularly with site isolation, because we didn't nullcheck the
> context.
> 
> Right, I don't understand that though: currently, we *do* have v8::Contexts
> everywhere, even for remote frames. How is the extensions system invoking
> GetModuleSystem()?

You can see the test's use here:
https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/bin...

Basically:
var iframe;  // An iframe
chrome.test.getModuleSystem(iframe.contentWindow);

I've verified via extremely sophisticated logging (read: printf debuggery) that
with PlzNavigate + Site Isolation, we create the v8 context (and thus the
extension system's ScriptContext) for the iframe in a different process than the
process with the extension's background page (and the process with the
background page does not get notified of a v8 context creation for that iframe).
 Without PlzNavigate + Site Isolation, the script contexts are created in the
same process (and thus the ScriptContext object exists and is found in
GetModuleSystem).

If it would be helpful, I can upload a patch with my logs that you can run
locally.

[dcheng, 2017-01-30 23:54:11]

On 2017/01/27 21:48:56, Devlin wrote:
> On 2017/01/27 21:00:34, dcheng wrote:
> > On 2017/01/27 20:40:26, Devlin (catching up) wrote:
> > > On 2017/01/27 20:18:20, dcheng wrote:
> > > > On 2017/01/27 20:13:43, Devlin (catching up) wrote:
> > > > > On 2017/01/25 20:45:56, nasko wrote:
> > > > > > I'd defer to dcheng@ on this one, since it isn't an area I'm
> comfortable
> > > > with.
> > > > > 
> > > > > friendly ping -> dcheng
> > > > 
> > > > Can you help me understand how GetModuleSystem is used? I'm wondering
why
> > > we're
> > > > calling it on a cross-context object to begin with (and the circumstance
> > where
> > > > that happens).
> > > 
> > > GetModuleSystem() is a terrible, terrible hack that we have for various
> custom
> > > bindings.  In the long term, I want to kill it, but that's a bit away.  We
> use
> > > it on cross-context objects in order to force certain API bindings to be
> > > instantiated in the background page (again, something I want to kill).  In
> > > theory, these cross-contexts should be safe, but because it's JS,
> > monkeypatching
> > > can happen, and we have extra checks.
> > > 
> > > The test this fixes tries to access a cross-context object without proper
> > > access, and should fail (which it does).  The only thing is that it failed
> > even
> > > more spectacularly with site isolation, because we didn't nullcheck the
> > context.
> > 
> > Right, I don't understand that though: currently, we *do* have v8::Contexts
> > everywhere, even for remote frames. How is the extensions system invoking
> > GetModuleSystem()?
> 
> You can see the test's use here:
>
https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/bin...
> 
> Basically:
> var iframe;  // An iframe
> chrome.test.getModuleSystem(iframe.contentWindow);
> 
> I've verified via extremely sophisticated logging (read: printf debuggery)
that
> with PlzNavigate + Site Isolation, we create the v8 context (and thus the
> extension system's ScriptContext) for the iframe in a different process than
the
> process with the extension's background page (and the process with the
> background page does not get notified of a v8 context creation for that
iframe).

OK, I see. So we (currently) do create a v8::Context, even for a RemoteFrame.
However, we don't report it back to the embedder (so extensions won't know about
it, thus it's null here).

>  Without PlzNavigate + Site Isolation, the script contexts are created in the
> same process (and thus the ScriptContext object exists and is found in
> GetModuleSystem).
> 
> If it would be helpful, I can upload a patch with my logs that you can run
> locally.

This patch LGTM.

Out of curiosity... WebFrame::canAccessFrame() feels like a hack. Could we just
do the checks entirely in JS here (e.g. try to access the window's document)? Is
this JS executing in an isolated world, or is it possible for a page/extension
to hook these property accesses with defineGetter?

[Devlin, 2017-01-31 00:11:10]

On 2017/01/30 23:54:11, dcheng wrote:
> Out of curiosity... WebFrame::canAccessFrame() feels like a hack. Could we
just
> do the checks entirely in JS here (e.g. try to access the window's document)?
Is
> this JS executing in an isolated world, or is it possible for a page/extension
> to hook these property accesses with defineGetter?

I've learned the hard way that no security checks should be done in JS. :)

[Devlin, 2017-01-31 00:11:14]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2017-01-31 00:11:41]

On 2017/01/31 00:11:10, Devlin wrote:
> On 2017/01/30 23:54:11, dcheng wrote:
> > Out of curiosity... WebFrame::canAccessFrame() feels like a hack. Could we
> just
> > do the checks entirely in JS here (e.g. try to access the window's
document)?
> Is
> > this JS executing in an isolated world, or is it possible for a
page/extension
> > to hook these property accesses with defineGetter?
> 
> I've learned the hard way that no security checks should be done in JS. :)

(Typically, with the properly-crafted sequence of getters/setters, it's possible
to monkeypatch)

[commit-bot: I haz the power, 2017-01-31 00:11:56]

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-01-31 01:13:12]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485821474550450, "parent_rev":
"d91e0b2cf276c7586e1f923771227ce3175d54ed", "commit_rev":
"78d31a9fd4f364abc61bf19cbd8e7f6d10209c2a"}

[commit-bot: I haz the power, 2017-01-31 01:14:11]

Description was changed from

==========
[Extensions] Handle null ScriptContexts in GetModuleSystem()

GetModuleSystem() returns an object associated with a different JS
context for testing purposes (and is also one of the "why do we even
have that lever"-type things that we'd like to get rid of, but that's a
separate issue). Without site isolation, embedded frames (even those
from non-accessible contexts) are all hosted within the same process,
and we can retrieve a ScriptContext for an embedded frame. With site
isolation, this isn't necessarily true - there may be no corresponding
script context for an embedded frame (when the frame is hosted in
another process).

In both cases, we should never return an object that is inaccessible to
the current context, and so a null context is perfectly valid. Handle
this in GetModuleSystem() and add a general comment above the
GetContextByV8Object() method in ScriptContextSet.

Fixes ExtensionBindingsApiTest.ModuleSystem for PlzNavigate +
SitePerProcess

BUG=674734
==========

to

==========
[Extensions] Handle null ScriptContexts in GetModuleSystem()

GetModuleSystem() returns an object associated with a different JS
context for testing purposes (and is also one of the "why do we even
have that lever"-type things that we'd like to get rid of, but that's a
separate issue). Without site isolation, embedded frames (even those
from non-accessible contexts) are all hosted within the same process,
and we can retrieve a ScriptContext for an embedded frame. With site
isolation, this isn't necessarily true - there may be no corresponding
script context for an embedded frame (when the frame is hosted in
another process).

In both cases, we should never return an object that is inaccessible to
the current context, and so a null context is perfectly valid. Handle
this in GetModuleSystem() and add a general comment above the
GetContextByV8Object() method in ScriptContextSet.

Fixes ExtensionBindingsApiTest.ModuleSystem for PlzNavigate +
SitePerProcess

BUG=674734

Review-Url: https://codereview.chromium.org/2654623007
Cr-Commit-Position: refs/heads/master@{#447149}
Committed:
https://chromium.googlesource.com/chromium/src/+/78d31a9fd4f364abc61bf19cbd8e...
==========

[commit-bot: I haz the power, 2017-01-31 01:14:12]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/78d31a9fd4f364abc61bf19cbd8e...