Fix process transfers for blob urls of sites requiring dedicated processes.

Fix process transfers for blob urls of sites requiring dedicated processes.

RenderFrameHostManager::IsRendererTransferNeededForNavigation had a bug
where it passed an effective url, instead of an effective SITE url, to
a function that was expecting the latter.

Add a test that exercises this case. Add a CHECK to content shell browser
client to verify that we're actually getting site urls all the time.

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

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

[ncarter (slow), 2016-09-09 18:25:15]

Description was changed from

==========
Fix process transfers for blob urls of sites requiring dedicated processes.

RenderFrameHostManager::IsRendererTransferNeededForNavigation had a bug
where it passed an effective url, instead of an effective SITE url, to
a function that was expecting the latter.

Add a test that exercises this case. Add a CHECK to content shell browser
client to verify that we're actually getting site urls all the time.

BUG=644966
==========

to

==========
Fix process transfers for blob urls of sites requiring dedicated processes.

RenderFrameHostManager::IsRendererTransferNeededForNavigation had a bug
where it passed an effective url, instead of an effective SITE url, to
a function that was expecting the latter.

Add a test that exercises this case. Add a CHECK to content shell browser
client to verify that we're actually getting site urls all the time.

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

[ncarter (slow), 2016-09-09 18:25:37]

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

[commit-bot: I haz the power, 2016-09-09 18:25:59]

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

[ncarter (slow), 2016-09-09 18:27:14]

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

[ncarter (slow), 2016-09-09 18:27:15]

Charlie, PTAL

[Charlie Reis, 2016-09-09 18:54:39]

Quick questions to make sure I understand the fix before I review the test and
the ShellBrowserContentClient change.

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

https://codereview.chromium.org/2322673005/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1463:
SiteInstanceImpl::DoesSiteRequireDedicatedProcess(context, site_url)) {
Can you explain why was passing the effective URL wrong?

I noticed that DoesSiteRequireDedicatedProcess is documented to expect an
effective_url (in site_instance_impl.h).  Same for
ContentBrowserClient::DoesSiteRequireDedicatedProcess.  Should we change that
documentation?  The parameter names are also inconsistent: it's effective_url
until we get to chrome_content_browser_client.cc, when it becomes
effective_site_url.

There are two other callsites: SII::RequiresDedicatedProcess (which passes a
site URL) and RFHM::DetermineSiteInstanceForURL (which apparently passes a
normal URL, neither effective nor site, in TDI mode).  Is the latter one a
problem?

[commit-bot: I haz the power, 2016-09-09 19:43:15]

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

[commit-bot: I haz the power, 2016-09-09 19:43:16]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-09-09 20:03:18]

On 2016/09/09 18:54:39, Charlie Reis (slow) wrote:
> Quick questions to make sure I understand the fix before I review the test and
> the ShellBrowserContentClient change.
> 
>
https://codereview.chromium.org/2322673005/diff/1/content/browser/frame_host/...
> File content/browser/frame_host/render_frame_host_manager.cc (right):
> 
>
https://codereview.chromium.org/2322673005/diff/1/content/browser/frame_host/...
> content/browser/frame_host/render_frame_host_manager.cc:1463:
> SiteInstanceImpl::DoesSiteRequireDedicatedProcess(context, site_url)) {
> Can you explain why was passing the effective URL wrong?
> 
> I noticed that DoesSiteRequireDedicatedProcess is documented to expect an
> effective_url (in site_instance_impl.h).  Same for
> ContentBrowserClient::DoesSiteRequireDedicatedProcess.  Should we change that
> documentation?  The parameter names are also inconsistent: it's effective_url
> until we get to chrome_content_browser_client.cc, when it becomes
> effective_site_url.
> 
> There are two other callsites: SII::RequiresDedicatedProcess (which passes a
> site URL) and RFHM::DetermineSiteInstanceForURL (which apparently passes a
> normal URL, neither effective nor site, in TDI mode).  Is the latter one a
> problem?

Good catch. The latter one is a problem; and (I think) would affect the
interaction of TDI and --site-per-process. Based on your observation I think
I'll push the call to GetSiteURL down into
SiteInstanceImpl::DoesSiteRequireDedicatedProcess(), and remove the 'must be an
effective url' restriction from its argument, since that's not how it's used in
practice.

The comments about 'must be an effective url' etc simply didn't anticipate
blob/filesystem urls; they were concerned with the url and effective origin
having different schemes, but not via the blob/filesystem mechanism. It's
unfortunate that I didn't catch this in my earlier experimentation with blob
urls; the tests we did write just didn't quite target this exact scenario; it
just happens that getting it right is easier in --site-per-process.

Having a type container for Site URLs might be helpful here as a preventative
improvement.

[ncarter (slow), 2016-09-09 20:23:10]

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

[commit-bot: I haz the power, 2016-09-09 20:23:43]

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

[ncarter (slow), 2016-09-09 20:32:32]

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

[commit-bot: I haz the power, 2016-09-09 20:33:09]

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

[Charlie Reis, 2016-09-09 21:18:24]

Looks good-- just a few bits of cleanup.

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

https://codereview.chromium.org/2322673005/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:701: // extension process,
is a special place with magical powers; we want to protect
Ha!

https://codereview.chromium.org/2322673005/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:703: class
IsolateIcelandFrameTreeBrowserTest : public ContentBrowserTest {
Is there a reason this doesn't derive from FrameTreeBrowserTest?  That would
avoid duplicating SetUpOnMainThread and the need to move GetOriginFromRenderer
outside the class.

https://codereview.chromium.org/2322673005/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:739: // behaviors are for
this navigation to either be cancelled, or for it to
I'm a little unclear here.  Is there a mode where the navigation is canceled
(like --site-per-process or something similar), or are you just explaining that
it would be acceptable behavior if we later decided to go that way?  I imagine
the test would fail if the navigation were canceled, as written.

https://codereview.chromium.org/2322673005/diff/40001/content/browser/site_in...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/2322673005/diff/40001/content/browser/site_in...
content/browser/site_instance_impl.cc:372: GURL site_url =
GetSiteForURL(browser_context, url);
nit: Let's add a comment explaining why we want the site URL here, rather than a
normal URL or an effective URL.  (Worth elaborating, since we've been bitten by
it now, and I know I'll forget.)

https://codereview.chromium.org/2322673005/diff/40001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2322673005/diff/40001/content/public/browser/...
content/public/browser/content_browser_client.h:204: const GURL&
effective_site_url);
Can you also update the parameter name in the following places?

content_browser_client.cc
chrome_content_browser_client.h
shell_content_browser_client.h

https://codereview.chromium.org/2322673005/diff/40001/content/shell/browser/s...
File content/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/2322673005/diff/40001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.cc:153: url::Origin
effective_url_origin(effective_site_url);
origin?

https://codereview.chromium.org/2322673005/diff/40001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.cc:159: bool result =
base::MatchPattern(effective_url_origin.Serialize(), pattern);
return base...

[commit-bot: I haz the power, 2016-09-09 21:35:44]

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

[commit-bot: I haz the power, 2016-09-09 21:35:44]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-09-09 21:56:32]

ptal

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

https://codereview.chromium.org/2322673005/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:701: // extension process,
is a special place with magical powers; we want to protect
On 2016/09/09 21:18:23, Charlie Reis (slow) wrote:
> Ha!

Acknowledged.

https://codereview.chromium.org/2322673005/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:703: class
IsolateIcelandFrameTreeBrowserTest : public ContentBrowserTest {
On 2016/09/09 21:18:23, Charlie Reis (slow) wrote:
> Is there a reason this doesn't derive from FrameTreeBrowserTest?  That would
> avoid duplicating SetUpOnMainThread and the need to move GetOriginFromRenderer
> outside the class.

CrossProcessFrameTreeBrowserTest didn't derive from FrameTreeBrowserTest, and I
was following that pattern.

https://codereview.chromium.org/2322673005/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:739: // behaviors are for
this navigation to either be cancelled, or for it to
On 2016/09/09 21:18:23, Charlie Reis (slow) wrote:
> I'm a little unclear here.  Is there a mode where the navigation is canceled
> (like --site-per-process or something similar), or are you just explaining
that
> it would be acceptable behavior if we later decided to go that way?  I imagine
> the test would fail if the navigation were canceled, as written.

I removed the speculative language.

https://codereview.chromium.org/2322673005/diff/40001/content/browser/site_in...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/2322673005/diff/40001/content/browser/site_in...
content/browser/site_instance_impl.cc:372: GURL site_url =
GetSiteForURL(browser_context, url);
On 2016/09/09 21:18:23, Charlie Reis (slow) wrote:
> nit: Let's add a comment explaining why we want the site URL here, rather than
a
> normal URL or an effective URL.  (Worth elaborating, since we've been bitten
by
> it now, and I know I'll forget.)

Done.

https://codereview.chromium.org/2322673005/diff/40001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2322673005/diff/40001/content/public/browser/...
content/public/browser/content_browser_client.h:204: const GURL&
effective_site_url);
On 2016/09/09 21:18:24, Charlie Reis (slow) wrote:
> Can you also update the parameter name in the following places?
> 
> content_browser_client.cc
> chrome_content_browser_client.h
> shell_content_browser_client.h

Done.

https://codereview.chromium.org/2322673005/diff/40001/content/shell/browser/s...
File content/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/2322673005/diff/40001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.cc:153: url::Origin
effective_url_origin(effective_site_url);
On 2016/09/09 21:18:24, Charlie Reis (slow) wrote:
> origin?

Done.

https://codereview.chromium.org/2322673005/diff/40001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.cc:159: bool result =
base::MatchPattern(effective_url_origin.Serialize(), pattern);
On 2016/09/09 21:18:24, Charlie Reis (slow) wrote:
> return base...

Done.

[Charlie Reis, 2016-09-09 22:00:58]

Thanks!  LGTM.

[ncarter (slow), 2016-09-09 22:01:24]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2016-09-09 22:02:06]

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-09-09 23:09:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-09 23:13:19]

Description was changed from

==========
Fix process transfers for blob urls of sites requiring dedicated processes.

RenderFrameHostManager::IsRendererTransferNeededForNavigation had a bug
where it passed an effective url, instead of an effective SITE url, to
a function that was expecting the latter.

Add a test that exercises this case. Add a CHECK to content shell browser
client to verify that we're actually getting site urls all the time.

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

to

==========
Fix process transfers for blob urls of sites requiring dedicated processes.

RenderFrameHostManager::IsRendererTransferNeededForNavigation had a bug
where it passed an effective url, instead of an effective SITE url, to
a function that was expecting the latter.

Add a test that exercises this case. Add a CHECK to content shell browser
client to verify that we're actually getting site urls all the time.

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

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

[commit-bot: I haz the power, 2016-09-09 23:13:22]

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