Fix RenderView reuse issues after a pending RenderFrameHost dies.

Fix RenderView reuse issues after a pending RenderFrameHost dies.

This CL fixes two issues related to a pending RenderFrameHost's
process dying before commit.

First, the is_active() state of RVH was being reset to true in
ResetWaitingState(), called as part of
RenderFrameHostImpl::Navigate().  This leaves the RVH with the wrong
state if that navigation never commits, which was the case if the
pending RFH's process dies before commit.  If the RVH is reused
later, it will be reinitialized with params.swapped_out being false,
even if there is still a corresponding proxy around.

To fix this, this CL moves the RVH::set_is_active(true) to be done at
commit time, which matches the point in time at which we update the
RVH's main_frame_routing_id_ and also better matches what happens on
the renderer side.

Second, UpdateStateForNavigate attempts to reuse the pending RFH for a
same-site navigation.  This failed when the reused pending RFH was not
live: when trying to reinitialize it in RFHM::Navigate(), we hit a
CHECK in CreateRenderView due to having neither a main frame routing
ID nor a proxy ID.  This is because the RVH doesn't have a main frame
routing ID (since the pending RFH died before commit, and main frame
routing ID is updated at commit); and while it does have a
corresponding proxy (which isn't live), the InitRenderView call in
RenderFrameHostManager::Navigate doesn't pass it in:

  if (!dest_render_frame_host->IsRenderFrameLive()) {
    ...
    if (!InitRenderView(dest_render_frame_host->render_view_host(), nullptr))
    ...
  }

To fix this, this CL destroys the pending RFH when its process dies,
as there is no reason to keep it around.  This way, we can be certain
that when a pending RFH is reused, it will be live.

The InitRenderView call above is still problematic -- namely, it is
not correct for reinitializing a subframe dest_render_frame_host --
but that will be fixed in a follow-up CL and tracked in issue 634368.

BUG=627400, 627893, 544755, 581912, 575245
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/20b99f04d03da2bcf3b7381955040aa9d4fc15a0
Cr-Commit-Position: refs/heads/master@{#410925}

[alexmos, 2016-08-08 23:23:17]

Description was changed from

==========
Fix RenderView reuse issues after a pending RenderFrameHost dies.

This CL fixes two issues related to a pending RenderFrameHost's
process dying before commit.

First, the is_active() state of RVH was being reset to true in
ResetWaitingState(), called as part of
RenderFrameHostImpl::Navigate().  This leaves the RVH with the wrong
state if that navigation never commits, which was the case if the
pending RFH's process dies before commit.  If the RVH is reused
later, it will be reinitialized with params.swapped_out being false,
even if there is still a corresponding proxy around.

To fix this, this CL moves the RVH::set_is_active(true) to be done at
commit time, which matches the point in time at which we update the
RVH's main_frame_routing_id_ and also better matches what happens on
the renderer side.

Second, UpdateStateForNavigate attempts to reuse the pending RFH for a
same-site navigation.  This failed when the reused pending RFH was not
live: when trying to reinitialize it in RFHM::Navigate(), we hit a
CHECK in CreateRenderView due to having neither a main frame routing
ID nor a proxy ID.  This is because the RVH doesn't have a main frame
routing ID (since the pending RFH died before commit, and main frame
routing ID is updated at commit); and while it does have a
corresponding proxy (which isn't live), the InitRenderView call in
RenderFrameHostManager::Navigate doesn't pass it in:

  if (!dest_render_frame_host->IsRenderFrameLive()) {
    ...
    if (!InitRenderView(dest_render_frame_host->render_view_host(), nullptr))
    ...
  }

To fix this, this CL destroys the pending RFH when its process dies,
as there is no reason to keep it around.  This way, we can be certain
that when a pending RFH is reused, it will be live.

The InitRenderView call above is still problematic -- namely, it is
not correct for reinitializing a subframe dest_render_frame_host --
but that will be fixed in a follow-up CL and tracked in issue 634368.

BUG=627400, 627893, 544755, 581912, 575245
==========

to

==========
Fix RenderView reuse issues after a pending RenderFrameHost dies.

This CL fixes two issues related to a pending RenderFrameHost's
process dying before commit.

First, the is_active() state of RVH was being reset to true in
ResetWaitingState(), called as part of
RenderFrameHostImpl::Navigate().  This leaves the RVH with the wrong
state if that navigation never commits, which was the case if the
pending RFH's process dies before commit.  If the RVH is reused
later, it will be reinitialized with params.swapped_out being false,
even if there is still a corresponding proxy around.

To fix this, this CL moves the RVH::set_is_active(true) to be done at
commit time, which matches the point in time at which we update the
RVH's main_frame_routing_id_ and also better matches what happens on
the renderer side.

Second, UpdateStateForNavigate attempts to reuse the pending RFH for a
same-site navigation.  This failed when the reused pending RFH was not
live: when trying to reinitialize it in RFHM::Navigate(), we hit a
CHECK in CreateRenderView due to having neither a main frame routing
ID nor a proxy ID.  This is because the RVH doesn't have a main frame
routing ID (since the pending RFH died before commit, and main frame
routing ID is updated at commit); and while it does have a
corresponding proxy (which isn't live), the InitRenderView call in
RenderFrameHostManager::Navigate doesn't pass it in:

  if (!dest_render_frame_host->IsRenderFrameLive()) {
    ...
    if (!InitRenderView(dest_render_frame_host->render_view_host(), nullptr))
    ...
  }

To fix this, this CL destroys the pending RFH when its process dies,
as there is no reason to keep it around.  This way, we can be certain
that when a pending RFH is reused, it will be live.

The InitRenderView call above is still problematic -- namely, it is
not correct for reinitializing a subframe dest_render_frame_host --
but that will be fixed in a follow-up CL and tracked in issue 634368.

BUG=627400, 627893, 544755, 581912, 575245
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-08-08 23:23:23]

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

[commit-bot: I haz the power, 2016-08-08 23:23:54]

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, 2016-08-08 23:26:44]

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

[commit-bot: I haz the power, 2016-08-08 23:26:44]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alexmos, 2016-08-08 23:31:49]

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

[commit-bot: I haz the power, 2016-08-08 23:32:48]

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

[alexmos, 2016-08-08 23:59:01]

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

[alexmos, 2016-08-08 23:59:02]

Nasko, can you please take a look?

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2146:
render_view_host_->set_is_active(true);
Looking at whether this might affect is_active() uses, one case I'm not sure
about is in RenderFrameHostImpl::UpdatePendingWebUI:

  } else if (render_view_host_->is_active()) {
    // If the ongoing navigation is not to a WebUI or the RenderView is in a
    // guest process, ensure that we don't create an unprivileged RenderView in
    // a WebUI-enabled process unless it's swapped out.
    bool url_acceptable_for_webui =
        WebUIControllerFactoryRegistry::GetInstance()->IsURLAcceptableForWebUI(
            GetSiteInstance()->GetBrowserContext(), dest_url);
    if (!url_acceptable_for_webui) {
      CHECK(!ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
          GetProcess()->GetID()));
    }
  }

This looks like it might skip this check now when the RVH starts out as
swapped-out and later is reused and becomes active, as it seems this check will
be done as part of UpdateStateForNavigate before commit, so before we make the
RVH active per my CL.  Do you know if this intends to enforce the CHECK in that
case?

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

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:244: if
(!InitRenderView(dest_render_frame_host->render_view_host(), nullptr))
This was where we actually crashed in CreateRenderView when we were trying to
reuse the dead pending RFH.  This line is still problematic for subframes, and I
think it will need to pass in a proxy if it exists as well, but I will fix this
in a followup CL, as there are a few more fixes required in that area for issue
634368.  Killing the pending RFH avoids ever going into here, so that fix is
sufficient to prevent the CreateRenderView crash.

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1037:
frame_tree_node_->ResetNavigationRequest(false);
Does this make sense in the PlzNavigate case?  I first tried CleanUpNavigation,
which ran into trouble in my tests due to still having the navigation_request_
around, but ResetNavigationRequest seems to both clear that and the speculative
RFH.  Both new tests now pass with PlzNavigate.

[commit-bot: I haz the power, 2016-08-09 00:34:41]

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

[commit-bot: I haz the power, 2016-08-09 00:34:41]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Charlie Reis, 2016-08-09 16:31:19]

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

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1032: void
RenderFrameHostManager::CancelPendingIfNecessary(
Drive-by: I just made CancelPending() public in
https://codereview.chromium.org/2208933002, but yours is better.  Would you mind
making CancelPending private again and calling this one from
RenderFrameHostImpl::OnCancelInitialHistoryLoad() instead?

[nasko, 2016-08-09 18:32:32]

Looks very good! Few comments.

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2146:
render_view_host_->set_is_active(true);
On 2016/08/08 23:59:01, alexmos wrote:
> Looking at whether this might affect is_active() uses, one case I'm not sure
> about is in RenderFrameHostImpl::UpdatePendingWebUI:
> 
>   } else if (render_view_host_->is_active()) {
>     // If the ongoing navigation is not to a WebUI or the RenderView is in a
>     // guest process, ensure that we don't create an unprivileged RenderView
in
>     // a WebUI-enabled process unless it's swapped out.
>     bool url_acceptable_for_webui =
>        
WebUIControllerFactoryRegistry::GetInstance()->IsURLAcceptableForWebUI(
>             GetSiteInstance()->GetBrowserContext(), dest_url);
>     if (!url_acceptable_for_webui) {
>       CHECK(!ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
>           GetProcess()->GetID()));
>     }
>   }
> 
> This looks like it might skip this check now when the RVH starts out as
> swapped-out and later is reused and becomes active, as it seems this check
will
> be done as part of UpdateStateForNavigate before commit, so before we make the
> RVH active per my CL.  Do you know if this intends to enforce the CHECK in
that
> case?

I don't know and don't recall :(.

However, bindings are granted through an IPC we send to the renderer, so if the
RV was dead and recreated, then the RV wouldn't have the bindings by default
IIRC. They are explicitly granted for WebUI as part of
MojoWebUIControllerBase::RenderViewCreated.

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

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1037:
frame_tree_node_->ResetNavigationRequest(false);
On 2016/08/08 23:59:02, alexmos wrote:
> Does this make sense in the PlzNavigate case?  I first tried
CleanUpNavigation,
> which ran into trouble in my tests due to still having the navigation_request_
> around, but ResetNavigationRequest seems to both clear that and the
speculative
> RFH.  Both new tests now pass with PlzNavigate.

We shouldn't be cancelling the request if the speculative RFH dies. However, I'm
fine landing this CL as is, since there is a DCHECK assuming it is live later on
that gets triggered. Please file a bug to fix this for PlzNavigate and put a
TODO before this call for clamy@ and I to address.

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2213:
CHECK(pending_render_frame_host_->IsRenderFrameLive());
Should this be CHECK or DCHECK?

https://codereview.chromium.org/2226023003/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2226023003/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:7651: // Test for
https://crbug.com/627400, where reusing a top-level pending
nit: Let's say what the test is testing without the bug itself, then later you
can add it as "See https...." for more details.

https://codereview.chromium.org/2226023003/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:7667: // The RenderViewHost for
b.com in the main tab should be swapped-out.
nit: s/swapped-out/not active/, especially since that is the method called in
the expectation macro.

https://codereview.chromium.org/2226023003/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:7675: NavigationStallDelegate
stall_delegate(stall_url);
Let's start using the TestNavigationManager manager class, as it will be the one
to rule them all : ). Just don't call any of the wait methods and it will be
blocked forever ;).

https://codereview.chromium.org/2226023003/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:7718: // pending RFH's process
is killed, rather than the main tab.
Since so much code is common between the two tests, is it worthwhile factoring
it out into a common function and calling it?

[alexmos, 2016-08-09 23:06:44]

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

[commit-bot: I haz the power, 2016-08-09 23:07:24]

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

[alexmos, 2016-08-09 23:09:46]

Thanks for reviewing!  Comments addressed, including Charlie's drive-by.

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2146:
render_view_host_->set_is_active(true);
On 2016/08/09 18:32:32, nasko wrote:
> On 2016/08/08 23:59:01, alexmos wrote:
> > Looking at whether this might affect is_active() uses, one case I'm not sure
> > about is in RenderFrameHostImpl::UpdatePendingWebUI:
> > 
> >   } else if (render_view_host_->is_active()) {
> >     // If the ongoing navigation is not to a WebUI or the RenderView is in a
> >     // guest process, ensure that we don't create an unprivileged RenderView
> in
> >     // a WebUI-enabled process unless it's swapped out.
> >     bool url_acceptable_for_webui =
> >        
> WebUIControllerFactoryRegistry::GetInstance()->IsURLAcceptableForWebUI(
> >             GetSiteInstance()->GetBrowserContext(), dest_url);
> >     if (!url_acceptable_for_webui) {
> >      
CHECK(!ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
> >           GetProcess()->GetID()));
> >     }
> >   }
> > 
> > This looks like it might skip this check now when the RVH starts out as
> > swapped-out and later is reused and becomes active, as it seems this check
> will
> > be done as part of UpdateStateForNavigate before commit, so before we make
the
> > RVH active per my CL.  Do you know if this intends to enforce the CHECK in
> that
> > case?
> 
> I don't know and don't recall :(.
> 
> However, bindings are granted through an IPC we send to the renderer, so if
the
> RV was dead and recreated, then the RV wouldn't have the bindings by default
> IIRC. They are explicitly granted for WebUI as part of
> MojoWebUIControllerBase::RenderViewCreated.

Ah, this is probably fine then.  Thanks!

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

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1032: void
RenderFrameHostManager::CancelPendingIfNecessary(
On 2016/08/09 16:31:18, Charlie Reis wrote:
> Drive-by: I just made CancelPending() public in
> https://codereview.chromium.org/2208933002, but yours is better.  Would you
mind
> making CancelPending private again and calling this one from
> RenderFrameHostImpl::OnCancelInitialHistoryLoad() instead?

Done.

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1037:
frame_tree_node_->ResetNavigationRequest(false);
On 2016/08/09 18:32:32, nasko wrote:
> On 2016/08/08 23:59:02, alexmos wrote:
> > Does this make sense in the PlzNavigate case?  I first tried
> CleanUpNavigation,
> > which ran into trouble in my tests due to still having the
navigation_request_
> > around, but ResetNavigationRequest seems to both clear that and the
> speculative
> > RFH.  Both new tests now pass with PlzNavigate.
> 
> We shouldn't be cancelling the request if the speculative RFH dies. However,
I'm
> fine landing this CL as is, since there is a DCHECK assuming it is live later
on
> that gets triggered. Please file a bug to fix this for PlzNavigate and put a
> TODO before this call for clamy@ and I to address.

Done.  Filed https://crbug.com/636119 and referenced it here.

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2213:
CHECK(pending_render_frame_host_->IsRenderFrameLive());
On 2016/08/09 18:32:32, nasko wrote:
> Should this be CHECK or DCHECK?

It'd be safer with a DCHECK, but I was thinking that if this ever fires, it'll
be easier to catch the CHECK rather than with one of the RenderView crashes that
it might lead to later on.

https://codereview.chromium.org/2226023003/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2226023003/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:7651: // Test for
https://crbug.com/627400, where reusing a top-level pending
On 2016/08/09 18:32:32, nasko wrote:
> nit: Let's say what the test is testing without the bug itself, then later you
> can add it as "See https...." for more details.

Done for both tests.

https://codereview.chromium.org/2226023003/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:7667: // The RenderViewHost for
b.com in the main tab should be swapped-out.
On 2016/08/09 18:32:32, nasko wrote:
> nit: s/swapped-out/not active/, especially since that is the method called in
> the expectation macro.

Done (here and below)

https://codereview.chromium.org/2226023003/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:7675: NavigationStallDelegate
stall_delegate(stall_url);
On 2016/08/09 18:32:32, nasko wrote:
> Let's start using the TestNavigationManager manager class, as it will be the
one
> to rule them all : ). Just don't call any of the wait methods and it will be
> blocked forever ;).

Done.

https://codereview.chromium.org/2226023003/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:7718: // pending RFH's process
is killed, rather than the main tab.
On 2016/08/09 18:32:32, nasko wrote:
> Since so much code is common between the two tests, is it worthwhile factoring
> it out into a common function and calling it?

Leaving this as-is per the discussion on chat.

[nasko, 2016-08-09 23:49:15]

Thanks for tracking this down and fixing it! LGTM

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

https://codereview.chromium.org/2226023003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2213:
CHECK(pending_render_frame_host_->IsRenderFrameLive());
On 2016/08/09 23:09:46, alexmos wrote:
> On 2016/08/09 18:32:32, nasko wrote:
> > Should this be CHECK or DCHECK?
> 
> It'd be safer with a DCHECK, but I was thinking that if this ever fires, it'll
> be easier to catch the CHECK rather than with one of the RenderView crashes
that
> it might lead to later on.

Acknowledged.

[alexmos, 2016-08-09 23:49:59]

The CQ bit was unchecked by alexmos@chromium.org

[alexmos, 2016-08-09 23:50:00]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2016-08-09 23:50:28]

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-08-10 00:46:05]

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

[commit-bot: I haz the power, 2016-08-10 00:46:07]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[alexmos, 2016-08-10 00:47:17]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2016-08-10 00:48:11]

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-08-10 02:20:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-10 02:22:32]

Description was changed from

==========
Fix RenderView reuse issues after a pending RenderFrameHost dies.

This CL fixes two issues related to a pending RenderFrameHost's
process dying before commit.

First, the is_active() state of RVH was being reset to true in
ResetWaitingState(), called as part of
RenderFrameHostImpl::Navigate().  This leaves the RVH with the wrong
state if that navigation never commits, which was the case if the
pending RFH's process dies before commit.  If the RVH is reused
later, it will be reinitialized with params.swapped_out being false,
even if there is still a corresponding proxy around.

To fix this, this CL moves the RVH::set_is_active(true) to be done at
commit time, which matches the point in time at which we update the
RVH's main_frame_routing_id_ and also better matches what happens on
the renderer side.

Second, UpdateStateForNavigate attempts to reuse the pending RFH for a
same-site navigation.  This failed when the reused pending RFH was not
live: when trying to reinitialize it in RFHM::Navigate(), we hit a
CHECK in CreateRenderView due to having neither a main frame routing
ID nor a proxy ID.  This is because the RVH doesn't have a main frame
routing ID (since the pending RFH died before commit, and main frame
routing ID is updated at commit); and while it does have a
corresponding proxy (which isn't live), the InitRenderView call in
RenderFrameHostManager::Navigate doesn't pass it in:

  if (!dest_render_frame_host->IsRenderFrameLive()) {
    ...
    if (!InitRenderView(dest_render_frame_host->render_view_host(), nullptr))
    ...
  }

To fix this, this CL destroys the pending RFH when its process dies,
as there is no reason to keep it around.  This way, we can be certain
that when a pending RFH is reused, it will be live.

The InitRenderView call above is still problematic -- namely, it is
not correct for reinitializing a subframe dest_render_frame_host --
but that will be fixed in a follow-up CL and tracked in issue 634368.

BUG=627400, 627893, 544755, 581912, 575245
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix RenderView reuse issues after a pending RenderFrameHost dies.

This CL fixes two issues related to a pending RenderFrameHost's
process dying before commit.

First, the is_active() state of RVH was being reset to true in
ResetWaitingState(), called as part of
RenderFrameHostImpl::Navigate().  This leaves the RVH with the wrong
state if that navigation never commits, which was the case if the
pending RFH's process dies before commit.  If the RVH is reused
later, it will be reinitialized with params.swapped_out being false,
even if there is still a corresponding proxy around.

To fix this, this CL moves the RVH::set_is_active(true) to be done at
commit time, which matches the point in time at which we update the
RVH's main_frame_routing_id_ and also better matches what happens on
the renderer side.

Second, UpdateStateForNavigate attempts to reuse the pending RFH for a
same-site navigation.  This failed when the reused pending RFH was not
live: when trying to reinitialize it in RFHM::Navigate(), we hit a
CHECK in CreateRenderView due to having neither a main frame routing
ID nor a proxy ID.  This is because the RVH doesn't have a main frame
routing ID (since the pending RFH died before commit, and main frame
routing ID is updated at commit); and while it does have a
corresponding proxy (which isn't live), the InitRenderView call in
RenderFrameHostManager::Navigate doesn't pass it in:

  if (!dest_render_frame_host->IsRenderFrameLive()) {
    ...
    if (!InitRenderView(dest_render_frame_host->render_view_host(), nullptr))
    ...
  }

To fix this, this CL destroys the pending RFH when its process dies,
as there is no reason to keep it around.  This way, we can be certain
that when a pending RFH is reused, it will be live.

The InitRenderView call above is still problematic -- namely, it is
not correct for reinitializing a subframe dest_render_frame_host --
but that will be fixed in a follow-up CL and tracked in issue 634368.

BUG=627400, 627893, 544755, 581912, 575245
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/20b99f04d03da2bcf3b7381955040aa9d4fc15a0
Cr-Commit-Position: refs/heads/master@{#410925}
==========

[commit-bot: I haz the power, 2016-08-10 02:22:33]

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