Don't swap processes for chrome:// subframes.

Don't swap processes for chrome:// subframes.

When running in --site-per-process mode, cross-site subframes run in
different process from the parent and there is an exception for chrome://
URLs to ensure chrome://settings continues working.

In PlzNavigate mode, the check for keeping chrome:// subframes in the
same process as the parent was missing, since it is added in the logic
for transferring navigations, which is not used with PlzNavigate.

BUG=674734
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2650473002
Cr-Commit-Position: refs/heads/master@{#445452}
Committed: https://chromium.googlesource.com/chromium/src/+/c6eede7d4c764d788ca02a02ab45c0f72309905b

[nasko, 2017-01-20 19:39:05]

Description was changed from

==========
Don't swap processes for chrome:// subframes.

When running in --site-per-process mode, cross-site subframes run in
different process from the parent and there is an exception for chrome://
URLs to ensure chrome://settings continues working.

In PlzNavigate mode, the check for keeping chrome:// subframes in the
same process as the parent was missing, since it is added in the logic
for transferring navigations, which is not used with PlzNavigate.

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

to

==========
Don't swap processes for chrome:// subframes.

When running in --site-per-process mode, cross-site subframes run in
different process from the parent and there is an exception for chrome://
URLs to ensure chrome://settings continues working.

In PlzNavigate mode, the check for keeping chrome:// subframes in the
same process as the parent was missing, since it is added in the logic
for transferring navigations, which is not used with PlzNavigate.

BUG=674734
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2017-01-20 19:40:01]

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

[commit-bot: I haz the power, 2017-01-20 19:40:34]

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-01-20 20:16:43]

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

[commit-bot: I haz the power, 2017-01-20 20:16:44]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[nasko, 2017-01-20 22:01:37]

Patchset #2 (id:20001) has been deleted

[nasko, 2017-01-20 22:02:30]

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

[commit-bot: I haz the power, 2017-01-20 22:03:12]

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

[nasko, 2017-01-20 23:01:28]

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

[nasko, 2017-01-20 23:01:28]

Hey Charlie,
Can you review this CL for me? It fixes chrome:// pages to not create
out-of-process iframes when PlzNavigate and Site Isolation are combined using
the approach we discussed offline.
Thanks in advance!
Nasko

[commit-bot: I haz the power, 2017-01-20 23:27:27]

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

[commit-bot: I haz the power, 2017-01-20 23:27:28]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-20 23:32:11]

Thanks!  Looks generally good, though that's a big problem that the test found.

Can we remove the WebUI check from
NavigationHandleImpl::MaybeTransferAndProceedInternal, either now or after this
lands?  I'm hoping the new check will be sufficient for both PlzNavigate and
default mode.

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1324: // subframes for
now, since e.g. chrome://settings has multiple "cross-site"
Let's rephrase this a bit to more closely match what we're doing, since the code
isn't limited to --site-per-process, and simply skipping a swap for a chrome://
subframe isn't sufficient (e.g., it would be bad if the subframe started on the
web and then somehow went to chrome://).  Something like:

If the parent frame is a chrome:// page and the subframe is as well, keep the
subframe in the parent's process even if they would be considered different
sites.  This avoids unnecessary OOPIFs on pages like chrome://settings, which
currently has multiple "cross-site" subframes that don't need isolation.

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1328: GURL
parent_site_url = frame_tree_node_->parent()
nit: const ref

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1330:
->GetSiteInstance()
Let's define parent_site_instance as a local variable to make this and the line
below more legible.

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:3064:
chrome_child_url.spec().c_str());
How did you get around the CSP problem you mentioned?

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:3095: //
different SiteInstance from the parent frame.
WAT??!  I'm shocked this is allowed.  (And I'm glad you tested it!)  We should
not be letting a WebUI page load a web iframe, should we?

I hope this crashes after sammc's https://codereview.chromium.org/2566583002/
lands.  We should probably upgrade his DCHECK_EQ in
RenderFrameHostImpl::AllowBindings to a CHECK_EQ (separate from his CL to avoid
any risk of getting it reverted).

[nasko, 2017-01-20 23:51:58]

> Can we remove the WebUI check from
> NavigationHandleImpl::MaybeTransferAndProceedInternal, either now or 
> after this lands?  I'm hoping the new check will be sufficient for
> both PlzNavigate and default mode.

I would rather do it as a follow up if you don't mind.

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1324: // subframes for
now, since e.g. chrome://settings has multiple "cross-site"
On 2017/01/20 23:32:11, Charlie Reis wrote:
> Let's rephrase this a bit to more closely match what we're doing, since the
code
> isn't limited to --site-per-process, and simply skipping a swap for a
chrome://
> subframe isn't sufficient (e.g., it would be bad if the subframe started on
the
> web and then somehow went to chrome://).  Something like:
> 
> If the parent frame is a chrome:// page and the subframe is as well, keep the
> subframe in the parent's process even if they would be considered different
> sites.  This avoids unnecessary OOPIFs on pages like chrome://settings, which
> currently has multiple "cross-site" subframes that don't need isolation.
> 
> 

Done.

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1328: GURL
parent_site_url = frame_tree_node_->parent()
On 2017/01/20 23:32:11, Charlie Reis wrote:
> nit: const ref

No longer present.

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1330:
->GetSiteInstance()
On 2017/01/20 23:32:11, Charlie Reis wrote:
> Let's define parent_site_instance as a local variable to make this and the
line
> below more legible.

Done.

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:3064:
chrome_child_url.spec().c_str());
On 2017/01/20 23:32:11, Charlie Reis wrote:
> How did you get around the CSP problem you mentioned?

Found pages with less strict CSP :(.

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:3095: //
different SiteInstance from the parent frame.
On 2017/01/20 23:32:11, Charlie Reis wrote:
> WAT??!  I'm shocked this is allowed.  (And I'm glad you tested it!)  We should
> not be letting a WebUI page load a web iframe, should we?

We shouldn't :).

> I hope this crashes after sammc's https://codereview.chromium.org/2566583002/
> lands.  We should probably upgrade his DCHECK_EQ in
> RenderFrameHostImpl::AllowBindings to a CHECK_EQ (separate from his CL to
avoid
> any risk of getting it reverted).

I think it is also worthwhile to add an explicit check for this as a
NavigationThrottle and ensure we just outright block the navigation. Feel free
to file a bug and assign it to me to take care of.

[Charlie Reis, 2017-01-20 23:59:32]

On 2017/01/20 23:51:58, nasko wrote:
> > Can we remove the WebUI check from
> > NavigationHandleImpl::MaybeTransferAndProceedInternal, either now or 
> > after this lands?  I'm hoping the new check will be sufficient for
> > both PlzNavigate and default mode.
> 
> I would rather do it as a follow up if you don't mind.

That's fine.  Can you do a quick test locally to see if we do the right thing in
non-PlzNavigate if you comment out that old code?  Just want to make sure we've
done something sufficient for both cases, even if we don't remove the old one in
this CL.

Also, I'd suggest patching in sammc's
https://codereview.chromium.org/2566583002/ to see if that causes your test to
fail.  I expect that to land soon, and we shouldn't introduce a conflict with it
if possible.

Looks good apart from that.

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:3095: //
different SiteInstance from the parent frame.
On 2017/01/20 23:51:58, nasko wrote:
> On 2017/01/20 23:32:11, Charlie Reis wrote:
> > WAT??!  I'm shocked this is allowed.  (And I'm glad you tested it!)  We
should
> > not be letting a WebUI page load a web iframe, should we?
> 
> We shouldn't :).
> 
> > I hope this crashes after sammc's
https://codereview.chromium.org/2566583002/
> > lands.  We should probably upgrade his DCHECK_EQ in
> > RenderFrameHostImpl::AllowBindings to a CHECK_EQ (separate from his CL to
> avoid
> > any risk of getting it reverted).
> 
> I think it is also worthwhile to add an explicit check for this as a
> NavigationThrottle and ensure we just outright block the navigation. Feel free
> to file a bug and assign it to me to take care of.

Agreed.  I'll file it.

[nasko, 2017-01-21 00:38:29]

Patchset #4 (id:80001) has been deleted

[nasko, 2017-01-21 00:42:07]

On 2017/01/20 23:59:32, Charlie Reis wrote:
> On 2017/01/20 23:51:58, nasko wrote:
> > > Can we remove the WebUI check from
> > > NavigationHandleImpl::MaybeTransferAndProceedInternal, either now or 
> > > after this lands?  I'm hoping the new check will be sufficient for
> > > both PlzNavigate and default mode.
> > 
> > I would rather do it as a follow up if you don't mind.
> 
> That's fine.  Can you do a quick test locally to see if we do the right thing
in
> non-PlzNavigate if you comment out that old code?  Just want to make sure
we've
> done something sufficient for both cases, even if we don't remove the old one
in
> this CL.

I just removed the old code. It passes in both normal and PlzNavigate mode.

> Also, I'd suggest patching in sammc's
> https://codereview.chromium.org/2566583002/ to see if that causes your test to
> fail.  I expect that to land soon, and we shouldn't introduce a conflict with
it
> if possible.

I tried that, but the URLs I chose don't actually have WebUI bindings, so there
is no mismatch to be detected. Instead of wrangling with WebUI bindings, I'd
rather spend that time actually fixing the bug, so I'll do that next week.
 
> Looks good apart from that.
> 
>
https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
> File content/browser/frame_host/render_frame_host_manager_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/2650473002/diff/40001/content/browser/frame_h...
> content/browser/frame_host/render_frame_host_manager_browsertest.cc:3095: //
> different SiteInstance from the parent frame.
> On 2017/01/20 23:51:58, nasko wrote:
> > On 2017/01/20 23:32:11, Charlie Reis wrote:
> > > WAT??!  I'm shocked this is allowed.  (And I'm glad you tested it!)  We
> should
> > > not be letting a WebUI page load a web iframe, should we?
> > 
> > We shouldn't :).
> > 
> > > I hope this crashes after sammc's
> https://codereview.chromium.org/2566583002/
> > > lands.  We should probably upgrade his DCHECK_EQ in
> > > RenderFrameHostImpl::AllowBindings to a CHECK_EQ (separate from his CL to
> > avoid
> > > any risk of getting it reverted).
> > 
> > I think it is also worthwhile to add an explicit check for this as a
> > NavigationThrottle and ensure we just outright block the navigation. Feel
free
> > to file a bug and assign it to me to take care of.
> 
> Agreed.  I'll file it.

Thanks! I'll get on fixing it next week.

[nasko, 2017-01-21 00:42:26]

Removed the code from the transfer path.

[Charlie Reis, 2017-01-21 00:46:44]

https://codereview.chromium.org/2650473002/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (left):

https://codereview.chromium.org/2650473002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:754:
!ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
Removing this whole block looks wrong to me.  I expected to only remove this one
line, such that all navigations get sent through
IsRendererTransferNeededForNavigation.

Hopefully we'll see test failures?  If not, we need to look closely at whether
IsRendererTransferNeededForNavigation is still needed.

[nasko, 2017-01-21 00:47:45]

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

[commit-bot: I haz the power, 2017-01-21 00:48:00]

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

[nasko, 2017-01-21 01:10:46]

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

[commit-bot: I haz the power, 2017-01-21 01:11:18]

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-01-21 02:42:58]

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

[commit-bot: I haz the power, 2017-01-21 02:42:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nasko, 2017-01-23 16:51:49]

https://codereview.chromium.org/2650473002/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (left):

https://codereview.chromium.org/2650473002/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:754:
!ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
On 2017/01/21 00:46:44, Charlie Reis wrote:
> Removing this whole block looks wrong to me.  I expected to only remove this
one
> line, such that all navigations get sent through
> IsRendererTransferNeededForNavigation.
> 
> Hopefully we'll see test failures?  If not, we need to look closely at whether
> IsRendererTransferNeededForNavigation is still needed.

Yes, you are correct. I thought I ran successfully locally tests with this whole
block removed, but apparently it wasn't exactly that.

[nasko, 2017-01-23 17:49:07]

Charlie, I restored the transfer path code in order to unblock PlzNavigate. I
will investigate the strange failures in the WebUI tests and land the code
removal once I have a proper fix.

[nasko, 2017-01-23 17:49:20]

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

[commit-bot: I haz the power, 2017-01-23 17:49:48]

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

[Charlie Reis, 2017-01-23 18:03:14]

On 2017/01/23 17:49:07, nasko wrote:
> Charlie, I restored the transfer path code in order to unblock PlzNavigate. I
> will investigate the strange failures in the WebUI tests and land the code
> removal once I have a proper fix.

Sounds good, thanks.  LGTM.

[commit-bot: I haz the power, 2017-01-23 19:42:33]

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

[commit-bot: I haz the power, 2017-01-23 19:42:35]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-01-23 20:00:03]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2017-01-23 20:00:51]

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-23 20:08:04]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485201603721740,
"parent_rev": "d81b1c7481a410b5bc393cf6e0fb4179ca76e7eb", "commit_rev":
"c6eede7d4c764d788ca02a02ab45c0f72309905b"}

[commit-bot: I haz the power, 2017-01-23 20:08:35]

Description was changed from

==========
Don't swap processes for chrome:// subframes.

When running in --site-per-process mode, cross-site subframes run in
different process from the parent and there is an exception for chrome://
URLs to ensure chrome://settings continues working.

In PlzNavigate mode, the check for keeping chrome:// subframes in the
same process as the parent was missing, since it is added in the logic
for transferring navigations, which is not used with PlzNavigate.

BUG=674734
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't swap processes for chrome:// subframes.

When running in --site-per-process mode, cross-site subframes run in
different process from the parent and there is an exception for chrome://
URLs to ensure chrome://settings continues working.

In PlzNavigate mode, the check for keeping chrome:// subframes in the
same process as the parent was missing, since it is added in the logic
for transferring navigations, which is not used with PlzNavigate.

BUG=674734
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2650473002
Cr-Commit-Position: refs/heads/master@{#445452}
Committed:
https://chromium.googlesource.com/chromium/src/+/c6eede7d4c764d788ca02a02ab45...
==========

[commit-bot: I haz the power, 2017-01-23 20:08:36]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/c6eede7d4c764d788ca02a02ab45...