OOPIF: Prevent process swap when not sharing the StoragePartition.

OOPIF: Prevent process swap when not sharing the StoragePartition.

Even if the url should warrant a process swap, check if the newly
created SiteInstance would use the same storage partition as its parent.
If that's not the case, the subframe should not swap processes, as there
is not support for having an OOPIF that does not share the storage
partition of its parent.

This patch was taken out of @clamy's CL in:
https://codereview.chromium.org/2727633005/

BUG=685074, 612711, 615585
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[arthursonzogni, 2017-03-06 12:56:04]

Description was changed from

==========
OOPIF: Prevent process swap when not sharing the StoragePartition.

Even if the url should warrant a process swap, check if the newly
created SiteInstance would use the same storage partition as its parent.
If that's not the case, the subframe should not swap processes, as there
is not support for having an OOPIF that does not share the storage
partition of its parent.

This patch was taken out of @clamy's CL in:
https://codereview.chromium.org/2727633005/

BUG=685074, 612711, 615585
==========

to

==========
OOPIF: Prevent process swap when not sharing the StoragePartition.

Even if the url should warrant a process swap, check if the newly
created SiteInstance would use the same storage partition as its parent.
If that's not the case, the subframe should not swap processes, as there
is not support for having an OOPIF that does not share the storage
partition of its parent.

This patch was taken out of @clamy's CL in:
https://codereview.chromium.org/2727633005/

BUG=685074, 612711, 615585
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[arthursonzogni, 2017-03-06 12:56:41]

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

[commit-bot: I haz the power, 2017-03-06 12:56:50]

Dry run: 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-03-06 13:43:38]

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

[commit-bot: I haz the power, 2017-03-06 13:43:39]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-03-06 14:09:11]

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

[arthursonzogni, 2017-03-06 14:09:11]

Hi Nasko,

Please could you take a look?
You already have started reviewing it from @clamy's CL in:
https://codereview.chromium.org/2727633005/

It might be nice to create some test for this, but I have no
idea how to make an iframe with a different StoragePartition
other than this test:
SiteDetailsBrowserTest.PlatformAppsErrorPagesArentOOPIF when
https://codereview.chromium.org/2715933002/ and
https://codereview.chromium.org/2655463006/ are applied.

+CC @creis, @alexmos, @lazyboy, @clamy.

[nasko, 2017-03-06 18:12:06]

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

[nasko, 2017-03-06 18:12:07]

Adding creis@, to ensure we are all on the same page. I think it is a good idea
to enforce that we don't have subframes in different StoragePartition from their
parents, but not sure if this is the best way to do it. 

Also, to be noted, this is PlzNavigate mode only check, so the behavior in
regular mode navigation will not change. I assume this is intentional?

https://codereview.chromium.org/2732883003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2732883003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:780: // is not support
for having an OOPIF that does not share the storage
nit: StoragePartition.

[Charlie Reis, 2017-03-06 20:46:40]

This approach seems like it could force very sensitive URLs to stay in a
possibly compromised process, even if the other process model logic would say a
swap is required.  I don't think that's the outcome we want.  :)

The goal seems like it should be to either (1) make it an error to load a
subframe in a different StoragePartition and ensure that no Chrome code ever
tries to, or (2) decide that we should support it and make sure that it works in
all cases.  I'm assuming we want to do (1) for now, since (2) seems like a
larger project.

We were chatting last week about alternatives here, and Alex mentioned a few
options: we could leave the old page in place and cancel the load (as we do
today, with some extra magic making the outcome unclear to the parent), or we
could put an empty data URL or blank page into the parent's process for the CSP
blocking case.  These would be just for the time being, while we work out what
the best way to handle errors is.  Maybe one of those could unblock the PlzNav
CSP work?

Nasko, Alex, and I will try to chat about it this afternoon.

[nasko, 2017-03-06 23:06:18]

On 2017/03/06 20:46:40, Charlie Reis (OOO til Mar 6) wrote:
> This approach seems like it could force very sensitive URLs to stay in a
> possibly compromised process, even if the other process model logic would say
a
> swap is required.  I don't think that's the outcome we want.  :)
> 
> The goal seems like it should be to either (1) make it an error to load a
> subframe in a different StoragePartition and ensure that no Chrome code ever
> tries to, or (2) decide that we should support it and make sure that it works
in
> all cases.  I'm assuming we want to do (1) for now, since (2) seems like a
> larger project.
> 
> We were chatting last week about alternatives here, and Alex mentioned a few
> options: we could leave the old page in place and cancel the load (as we do
> today, with some extra magic making the outcome unclear to the parent), or we
> could put an empty data URL or blank page into the parent's process for the
CSP
> blocking case.  These would be just for the time being, while we work out what
> the best way to handle errors is.  Maybe one of those could unblock the PlzNav
> CSP work?
> 
> Nasko, Alex, and I will try to chat about it this afternoon.

We did discuss this and we came up with a solution - keep error pages resulting
from ERR_BLOCKED_BY_CLIENT in the same process as the document that initiated
the navigation. As such, this CL is a NOT LGTM and should be closed.

[arthursonzogni, 2017-03-07 09:05:11]

Description was changed from

==========
OOPIF: Prevent process swap when not sharing the StoragePartition.

Even if the url should warrant a process swap, check if the newly
created SiteInstance would use the same storage partition as its parent.
If that's not the case, the subframe should not swap processes, as there
is not support for having an OOPIF that does not share the storage
partition of its parent.

This patch was taken out of @clamy's CL in:
https://codereview.chromium.org/2727633005/

BUG=685074, 612711, 615585
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
OOPIF: Prevent process swap when not sharing the StoragePartition.

Even if the url should warrant a process swap, check if the newly
created SiteInstance would use the same storage partition as its parent.
If that's not the case, the subframe should not swap processes, as there
is not support for having an OOPIF that does not share the storage
partition of its parent.

This patch was taken out of @clamy's CL in:
https://codereview.chromium.org/2727633005/

BUG=685074, 612711, 615585
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========