Make sure new render is used when navigating away from about:blank in WebUI renderer.

Make sure new site instance (and therefore) render is used when navigating away
from blank page (about:blank) that has been loaded in WebUI-bindings-enabled renderer.

This currently reproduces in process models other than default
process-per-site(-instance) as they only enforce new renderer
on webui-vs-regular navigations and fail to recognize that about:blank
might have been loaded in WebUI (as per http://crbug.com/42547).

This CL includes the browser test that reproduces this problem
and verifies the fix.

BUG=424526
Committed: https://crrev.com/2c0c6c9e4f04e76373a884d03a4e09c8fee5d141
Cr-Commit-Position: refs/heads/master@{#300645}

[Krzysztof Olczyk, 2014-10-17 10:20:03]

kolczyk@opera.com changed reviewers:
+ clamy@chromium.org, sky@chromium.org

[Krzysztof Olczyk, 2014-10-17 10:20:04]

Hi sky, clamy,

could you please take a look at this CL
that fixes a CHECK crash on some process models?

[clamy, 2014-10-17 13:17:00]

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

[clamy, 2014-10-17 13:17:43]

+nasko@: I am leaving today for 3 weeks, could you take a look at that one?

[nasko, 2014-10-20 17:15:40]

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

[nasko, 2014-10-20 17:15:40]

Looks fine to me, but creis@ is the process model expert, so including him to
ensure we don't miss some subtlety.

[Charlie Reis, 2014-10-20 19:37:54]

Thanks for tracking this down and including a test!  That helps a ton in
understanding the fix.

A few comments on how I think we should add a more general fix.

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

https://codereview.chromium.org/659293002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:751:
if(current_instance->GetSiteURL().SchemeIs(kGuestScheme))
nit: Space after if.
(git cl format will help catch style errors like this.)

This change looks good-- I like returning early here rather than weaving
is_guest_scheme into everything.

https://codereview.chromium.org/659293002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:773:
current_effective_url.spec() == url::kAboutBlankURL &&
This seems much too specific to me.  The underlying flaw here is that we aren't
checking the process bindings when deciding whether to force a swap.  The same
bug could come up with other URLs like data:, etc.

I suppose we don't usually notice this because GetSiteInstanceForURL normally
catches this case (via HasWrongProcessForURL).  However, I think
ShouldSwapBrowsingInstancesForNavigation should check both the URL and the
bindings when considering WebUI.  

I imagine we should fix this using the {Has/Use}WebUIBindings calls that also
are in RenderProcessHostImpl::IsSuitableHost.  I don't think we want to just
call HasWrongProcessForURL because that might force a swap for hosted apps,
which need to stay in the same BrowsingInstance.

https://codereview.chromium.org/659293002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:782:
navigating_away_from_blank_page_in_webui) {
I'd rather not make this more complicated in this location. 
ShouldSwapBrowsingInstancesForNavigation is the right place to be adding checks
that force a swap, rather than appending it as another condition here.  (And I
do think we should force a BrowsingInstance swap if the bindings situation
requires it.)

https://codereview.chromium.org/659293002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/659293002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1478:
NavigationFromWebUIToRegularPageInterleavedWithBlankPageProcessPerTab) {
Can we come up with a shorter test name, like ForceSwapDueToWebUIBindings?

It also helps to put the bug number in a comment above the test, so that it's
easy to spot the reason this is important (in case the test later fails for
someone).

[Krzysztof Olczyk, 2014-10-21 08:27:05]

Hi Charlie,

thanks for the review and suggestions. I've submitted
a new accordingly updated CL.

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

https://codereview.chromium.org/659293002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:751:
if(current_instance->GetSiteURL().SchemeIs(kGuestScheme))
On 2014/10/20 19:37:53, Charlie Reis wrote:
> nit: Space after if.
> (git cl format will help catch style errors like this.)
> 
> This change looks good-- I like returning early here rather than weaving
> is_guest_scheme into everything.

Done.

https://codereview.chromium.org/659293002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:773:
current_effective_url.spec() == url::kAboutBlankURL &&
On 2014/10/20 19:37:53, Charlie Reis wrote:
> This seems much too specific to me.  The underlying flaw here is that we
aren't
> checking the process bindings when deciding whether to force a swap.  The same
> bug could come up with other URLs like data:, etc.
> 
> I suppose we don't usually notice this because GetSiteInstanceForURL normally
> catches this case (via HasWrongProcessForURL).  However, I think
> ShouldSwapBrowsingInstancesForNavigation should check both the URL and the
> bindings when considering WebUI.  
> 
> I imagine we should fix this using the {Has/Use}WebUIBindings calls that also
> are in RenderProcessHostImpl::IsSuitableHost.  I don't think we want to just
> call HasWrongProcessForURL because that might force a swap for hosted apps,
> which need to stay in the same BrowsingInstance.

Indeed, it looks like it makes more sense to put such a check there.
I modified ShouldSwapBrowsingInstancesForNavigation with HasWebUIBindings check.

https://codereview.chromium.org/659293002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:782:
navigating_away_from_blank_page_in_webui) {
On 2014/10/20 19:37:53, Charlie Reis wrote:
> I'd rather not make this more complicated in this location. 
> ShouldSwapBrowsingInstancesForNavigation is the right place to be adding
checks
> that force a swap, rather than appending it as another condition here.  (And I
> do think we should force a BrowsingInstance swap if the bindings situation
> requires it.)

Done.

https://codereview.chromium.org/659293002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/659293002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1478:
NavigationFromWebUIToRegularPageInterleavedWithBlankPageProcessPerTab) {
On 2014/10/20 19:37:54, Charlie Reis wrote:
> Can we come up with a shorter test name, like ForceSwapDueToWebUIBindings?
> 
> It also helps to put the bug number in a comment above the test, so that it's
> easy to spot the reason this is important (in case the test later fails for
> someone).

Done. I called it ForceSwapAfterWebUIBindings as I would like to
highlight it is about leaving (as opposed to entering) WebUI renderer.

[Charlie Reis, 2014-10-21 19:10:42]

Thanks!  LGTM with nits.

https://codereview.chromium.org/659293002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/659293002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:39: #include
"content/public/common/bindings_policy.h"
Probably don't need this anymore.

https://codereview.chromium.org/659293002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/659293002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1477: // The
test loads WebUI page, then navigates to blank page and to the regular
nit: loads a WebUI page in --process-per-tab mode,
nit: to a blank page and then to a regular page.

https://codereview.chromium.org/659293002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1478: //
page, consecutively. The bug reproduces if blank page is navigated in between
nit: navigated -> visited

[Krzysztof Olczyk, 2014-10-22 06:24:06]

Thanks for the review!

https://codereview.chromium.org/659293002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/659293002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:39: #include
"content/public/common/bindings_policy.h"
On 2014/10/21 19:10:41, Charlie Reis wrote:
> Probably don't need this anymore.

Done.

https://codereview.chromium.org/659293002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/659293002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1477: // The
test loads WebUI page, then navigates to blank page and to the regular
On 2014/10/21 19:10:41, Charlie Reis wrote:
> nit: loads a WebUI page in --process-per-tab mode,
> nit: to a blank page and then to a regular page.

Done.

https://codereview.chromium.org/659293002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1478: //
page, consecutively. The bug reproduces if blank page is navigated in between
On 2014/10/21 19:10:42, Charlie Reis wrote:
> nit: navigated -> visited

Done.

[Krzysztof Olczyk, 2014-10-22 06:24:52]

The CQ bit was checked by kolczyk@opera.com

[commit-bot: I haz the power, 2014-10-22 06:26:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/659293002/40001

[commit-bot: I haz the power, 2014-10-22 07:17:12]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-22 07:18:01]

Patchset 3 (id:??) landed as
https://crrev.com/2c0c6c9e4f04e76373a884d03a4e09c8fee5d141
Cr-Commit-Position: refs/heads/master@{#300645}