Fix RenderView reuse issues after a pending RenderFrameHost dies.

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/render_frame_host_manager.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <utility>
@@ skipping 223 matching lines @@
     // occur before initializing the RenderView; the flow of creating the
     // RenderView can cause browser-side code to execute that expects the this
     // RFH's shell::InterfaceRegistry to be initialized (e.g., if the site is a
     // WebUI site that is handled via Mojo, then Mojo WebUI code in //chrome
     // will add an interface to this RFH's InterfaceRegistry).
     dest_render_frame_host->SetUpMojoIfNeeded();
 
     // Recreate the opener chain.
     CreateOpenerProxies(dest_render_frame_host->GetSiteInstance(),
                         frame_tree_node_);
     if (!InitRenderView(dest_render_frame_host->render_view_host(), nullptr))

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

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.

       return nullptr;
 
     if (GetNavigatingWebUI()) {
       // A new RenderView was created and there is a navigating WebUI which
       // never interacted with it. So notify the WebUI using RenderViewCreated.
       GetNavigatingWebUI()->RenderViewCreated(
           dest_render_frame_host->render_view_host());
     }
 
     // Now that we've created a new renderer, be sure to hide it if it isn't
@@ skipping 767 matching lines @@
     GURL dest_url,
     SiteInstanceRelation relation_to_current)
     : existing_site_instance(nullptr), relation(relation_to_current) {
   new_site_url = SiteInstance::GetSiteForURL(browser_context, dest_url);
 }
 
 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
   // need to maintain a proxy there anymore.
   RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(site_instance);
   CHECK(proxy);
 
   DeleteRenderFrameProxyHost(site_instance);
 }
 
@@ skipping 1087 matching lines @@
   delegate_->NotifySwappedFromRenderManager(
       old_render_frame_host.get(), render_frame_host_.get(), is_main_frame);
 
   // The RenderViewHost keeps track of the main RenderFrameHost routing id.
   // If this is committing a main frame navigation, update it and set the
   // routing id in the RenderViewHost associated with the old RenderFrameHost
   // to MSG_ROUTING_NONE.
   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);
   }
 
   // Swap out the old frame now that the new one is visible.
   // This will swap it out and schedule it for deletion when the swap out ack
   // arrives (or immediately if the process isn't live).
   SwapOutOldFrame(std::move(old_render_frame_host));
 
   // Since the new RenderFrameHost is now committed, there must be no proxies
@@ skipping 37 matching lines @@
 
   SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
   scoped_refptr<SiteInstance> new_instance = GetSiteInstanceForNavigation(
       dest_url, source_instance, dest_instance, nullptr, transition,
       dest_is_restore, dest_is_view_source_mode);
 
   // If we are currently navigating cross-process to a pending RFH for a
   // 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(
         "navigation",
         "RenderFrameHostManager::UpdateStateForNavigate:New SiteInstance",
         TRACE_EVENT_SCOPE_THREAD,
         "current_instance id", current_instance->GetId(),
         "new_instance id", new_instance->GetId());
 
     // New SiteInstance: create a pending RFH to navigate.
@@ skipping 396 matching lines @@
                                              resolved_url)) {
     DCHECK(!dest_instance ||
            dest_instance == render_frame_host_->GetSiteInstance());
     return false;
   }
 
   return true;
 }
 
 }  // namespace content