Change BrowsingInstance when changing WebUI type.

Change BrowsingInstance when changing WebUI type.

In the current implementation of --process-per-tab process model, navigation
from one type of WebUI to another type of WebUI does not result in a change
of SiteInstance. Since different WebUI types are unrelated, the
navigation should result in changing of SiteInstance.

Since the current code doesn't support keeping the same BrowsingInstance
and only changing the SiteInstance, this CL achieves the desired behavior
by forcing a BrowsingInstance change.

BUG=508850
Committed: https://crrev.com/cf3a66c97c6f3b22b6adb31daf52bce767a1ecbf
Cr-Commit-Position: refs/heads/master@{#355951}

[nasko, 2015-10-23 22:35:45]

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

[nasko, 2015-10-23 22:35:46]

Hey Charlie,
This is the change we have been discussing for ensuring we do SiteInstance swap
on navigation between different WebUI types, even in --process-per-tab mode.
Nasko

[Charlie Reis, 2015-10-23 23:09:50]

Yes, I think we should force a swap for different WebUI types.  That has the
huge benefit that we'll never swap WebUI objects for a given RenderFrameHost,
which will vastly simplify the code.  (It's a bit strange that we allowed
different WebUIs to share the same RFH before, but it was largely from a
misunderstanding on my part when writing this check.)

Can you add a test similar to ForceSwapAfterWebUIBindings showing that two
different WebUI pages get a process swap in --process-per-tab?  kChromeUIGpuHost
to kChromeUIHistogramsHost would probably work.

One concern below.  We should also CC mostynb@opera.com.

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

https://codereview.chromium.org/1413863006/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1245: if
(IsRendererDebugURL(new_effective_url))
What if we also returned false for about:blank?  That seems safe to load in any
BI, and it eliminates the concern below.

https://codereview.chromium.org/1413863006/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1249: // page and one
isn't.
nit: , or if the Web UI types differ.

https://codereview.chromium.org/1413863006/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1256: if
(!WebUIControllerFactoryRegistry::GetInstance()->IsURLAcceptableForWebUI(
I'm concerned about the about:blank case.  Before, we said it was allowed to
stay (per https://crbug.com/42547#c13, which found that about:blank could be
loaded in WebUI processes).

Now we'll probably find that the WebUIType of about:blank is kWebUINone, so
we'll say it requires a swap.

Do we want to preserve that behavior?  I'm not sure if WebUI pages actually
navigate to it anymore in the main frame, but it could definitely come up in
subframes.

If so, let's add a check to the ForceSwapAfterWebUIBindings test to verify that
the about:blank page stays in process.

[nasko, 2015-10-23 23:46:53]

Fixes, no test yet.

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

https://codereview.chromium.org/1413863006/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1245: if
(IsRendererDebugURL(new_effective_url))
On 2015/10/23 23:09:50, Charlie Reis wrote:
> What if we also returned false for about:blank?  That seems safe to load in
any
> BI, and it eliminates the concern below.

Agree!

https://codereview.chromium.org/1413863006/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1249: // page and one
isn't.
On 2015/10/23 23:09:50, Charlie Reis wrote:
> nit: , or if the Web UI types differ.

Done.

https://codereview.chromium.org/1413863006/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1256: if
(!WebUIControllerFactoryRegistry::GetInstance()->IsURLAcceptableForWebUI(
On 2015/10/23 23:09:50, Charlie Reis wrote:
> I'm concerned about the about:blank case.  Before, we said it was allowed to
> stay (per https://crbug.com/42547#c13, which found that about:blank could be
> loaded in WebUI processes).
> 
> Now we'll probably find that the WebUIType of about:blank is kWebUINone, so
> we'll say it requires a swap.
> 
> Do we want to preserve that behavior?  I'm not sure if WebUI pages actually
> navigate to it anymore in the main frame, but it could definitely come up in
> subframes.
> 
> If so, let's add a check to the ForceSwapAfterWebUIBindings test to verify
that
> the about:blank page stays in process.

Acknowledged.

[nasko, 2015-10-24 00:16:09]

Now it has a test too : )

[nasko, 2015-10-24 00:17:41]

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

[nasko, 2015-10-24 00:17:41]

Hey Lei,
Can you do an OWNERS review of chrome/? I had to add a test at the chrome/
layer, as WebUI type in content/ is the same for all WebUI URLs :(.
Thanks in advance!
Nasko

[nasko, 2015-10-24 00:19:21]

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

[nasko, 2015-10-24 00:19:22]

Adding mostynb@opera.com for FYI.

Carlos, this will make your refactor much simpler. No need for us to do switch
of WebUI type on the same RenderFrameHost! Let's get rid of that complexity in
your patch!

[Charlie Reis, 2015-10-24 00:19:22]

Thanks!  LGTM with nits.

Don't forget to give mostynb@opera.com a heads up.

https://codereview.chromium.org/1413863006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/webui_browsertest.cc (right):

https://codereview.chromium.org/1413863006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/webui_browsertest.cc:37: // Capture the SiteInstance
befor navigating for later comparison.
nit: before

https://codereview.chromium.org/1413863006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/webui_browsertest.cc:42: // has changed and the process
still has WebUI bindings.
s/process still/new process also/

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

https://codereview.chromium.org/1413863006/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1661: //
Capture the SiteInstance befor navigating to about:blank to ensure
nit: before

[Lei Zhang, 2015-10-24 00:21:23]

lgtm stamp

https://codereview.chromium.org/1413863006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/webui_browsertest.cc (right):

https://codereview.chromium.org/1413863006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/webui_browsertest.cc:18: typedef InProcessBrowserTest
WebUIBrowserTest;
nit: using foo = bar instead of typedef bar foo

[nasko, 2015-10-24 00:30:49]

Fixed nits.

https://codereview.chromium.org/1413863006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/webui_browsertest.cc (right):

https://codereview.chromium.org/1413863006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/webui_browsertest.cc:18: typedef InProcessBrowserTest
WebUIBrowserTest;
On 2015/10/24 00:21:23, Lei Zhang wrote:
> nit: using foo = bar instead of typedef bar foo

Done.

https://codereview.chromium.org/1413863006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/webui_browsertest.cc:37: // Capture the SiteInstance
befor navigating for later comparison.
On 2015/10/24 00:19:22, Charlie Reis wrote:
> nit: before

Done.

https://codereview.chromium.org/1413863006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/webui_browsertest.cc:42: // has changed and the process
still has WebUI bindings.
On 2015/10/24 00:19:22, Charlie Reis wrote:
> s/process still/new process also/

Done.

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

https://codereview.chromium.org/1413863006/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1661: //
Capture the SiteInstance befor navigating to about:blank to ensure
On 2015/10/24 00:19:22, Charlie Reis wrote:
> nit: before

Done.

[nasko, 2015-10-24 00:31:08]

The CQ bit was checked by nasko@chromium.org

[nasko, 2015-10-24 00:31:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1413863006/#ps60001
(title: "Fix nits.")

[commit-bot: I haz the power, 2015-10-24 00:32:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413863006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413863006/60001

[commit-bot: I haz the power, 2015-10-24 01:43:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-24 01:44:08]

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