Fix RenderView reuse issues after a pending RenderFrameHost dies.

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 6e33a629383190e33f19f24dcca72a011c0106e7..b9d06d59cf7508453eff9aa460ab50a7aa0df303 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -1029,6 +1029,15 @@ void RenderFrameHostManager::RenderProcessGone(SiteInstanceImpl* instance) {
   GetRenderFrameProxyHost(instance)->set_render_frame_proxy_created(false);
 }
 
+void RenderFrameHostManager::CancelPendingIfNecessary(

[Charlie Reis, 2016/08/09 16:31:18]

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?

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

Done.

+    RenderFrameHostImpl* render_frame_host) {
+  if (render_frame_host == pending_render_frame_host_.get())
+    CancelPending();
+  else if (render_frame_host == speculative_render_frame_host_.get()) {
+    frame_tree_node_->ResetNavigationRequest(false);

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

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.

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

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.

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

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

+  }
+}
+
 void RenderFrameHostManager::ActiveFrameCountIsZero(
     SiteInstanceImpl* site_instance) {
   // |site_instance| no longer contains any active RenderFrameHosts, so we don't
@@ -2136,6 +2145,8 @@ void RenderFrameHostManager::CommitPending() {
   if (is_main_frame) {
     render_frame_host_->render_view_host()->set_main_frame_routing_id(
         render_frame_host_->routing_id());
+    render_frame_host_->render_view_host()->set_is_active(true);
+    render_frame_host_->render_view_host()->set_is_swapped_out(false);
     old_render_frame_host->render_view_host()->set_main_frame_routing_id(
         MSG_ROUTING_NONE);
   }
@@ -2193,9 +2204,15 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
   // different SiteInstance, we want to get back to normal and then navigate as
   // usual.  We will reuse the pending RFH below if it matches the destination
   // SiteInstance.
-  if (pending_render_frame_host_ &&
-      pending_render_frame_host_->GetSiteInstance() != new_instance)
-    CancelPending();
+  if (pending_render_frame_host_) {
+    if (pending_render_frame_host_->GetSiteInstance() != new_instance) {
+      CancelPending();
+    } else {
+      // When a pending RFH is reused, it should always be live, since it is
+      // cleared whenever a process dies.
+      CHECK(pending_render_frame_host_->IsRenderFrameLive());

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

Should this be CHECK or DCHECK?

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

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.

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

Acknowledged.

+    }
+  }
 
   if (new_instance.get() != current_instance) {
     TRACE_EVENT_INSTANT2(