Properly recreate swapped out RenderView.

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 1d36b037795c5cd216fadeb4f67851c11f9e1101..6233a3f57b7da2385ed82d5c9e5be66e11df86a6 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -418,8 +418,7 @@ RenderFrameHostImpl* RenderFrameHostManager::Navigate(
     // Recreate the opener chain.
     CreateOpenerProxies(dest_render_frame_host->GetSiteInstance(),
                         frame_tree_node_);
-    if (!InitRenderView(dest_render_frame_host->render_view_host(),
-                        MSG_ROUTING_NONE))
+    if (!InitRenderView(dest_render_frame_host->render_view_host(), nullptr))
       return nullptr;
 
     // Now that we've created a new renderer, be sure to hide it if it isn't
@@ -1077,7 +1076,7 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
   if (!navigation_rfh->IsRenderFrameLive()) {
     // Recreate the opener chain.
     CreateOpenerProxies(navigation_rfh->GetSiteInstance(), frame_tree_node_);
-    if (!InitRenderView(navigation_rfh->render_view_host(), MSG_ROUTING_NONE)) {
+    if (!InitRenderView(navigation_rfh->render_view_host(), nullptr)) {
       return nullptr;
     }
 
@@ -1794,7 +1793,8 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
 
       // If we are reusing the RenderViewHost and it doesn't already have a
       // RenderWidgetHostView, we need to create one if this is the main frame.
-      if (!render_view_host->GetWidget()->GetView() &&
+      if (render_view_host->IsRenderViewLive() &&
+          !render_view_host->GetWidget()->GetView() &&
           frame_tree_node_->IsMainFrame()) {
         delegate_->CreateRenderWidgetHostViewForRenderManager(render_view_host);
       }
@@ -1816,7 +1816,6 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
         instance, MSG_ROUTING_NONE, MSG_ROUTING_NONE, widget_routing_id, flags);
     RenderViewHostImpl* render_view_host =
         new_render_frame_host->render_view_host();
-    int proxy_routing_id = MSG_ROUTING_NONE;
 
     // Prevent the process from exiting while we're trying to navigate in it.
     // Otherwise, if the new RFH is swapped out already, store it.
@@ -1827,12 +1826,11 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
           new_render_frame_host->GetSiteInstance(),
           new_render_frame_host->render_view_host(), frame_tree_node_);
       proxy_hosts_->Add(instance->GetId(), make_scoped_ptr(proxy));
-      proxy_routing_id = proxy->GetRoutingID();

[Charlie Reis, 2015/10/16 23:09:57]

For posterity: this is the key part of the fix.  We had a proxy but we're no
longer swapped_out, so we weren't passing the proxy_routing_id.  We need to
create both the RenderView and RenderFrameProxy when recreating the process,
since the swap hasn't happened yet.

       proxy->TakeFrameHostOwnership(new_render_frame_host.Pass());
     }
 
     if (frame_tree_node_->IsMainFrame()) {
-      success = InitRenderView(render_view_host, proxy_routing_id);
+      success = InitRenderView(render_view_host, proxy);
 
       // If we are reusing the RenderViewHost and it doesn't already have a
       // RenderWidgetHostView, we need to create one if this is the main frame.
@@ -1843,9 +1841,6 @@ scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
     }
 
     if (success) {
-      // Remember that InitRenderView also created the RenderFrameProxy.
-      if (swapped_out)
-        proxy->set_render_frame_proxy_created(true);
       if (frame_tree_node_->IsMainFrame()) {
         // Don't show the main frame's view until we get a DidNavigate from it.
         // Only the RenderViewHost for the top-level RenderFrameHost has a
@@ -1926,8 +1921,7 @@ int RenderFrameHostManager::CreateRenderFrameProxy(SiteInstance* instance) {
 
   if (SiteIsolationPolicy::IsSwappedOutStateForbidden() &&
       frame_tree_node_->IsMainFrame()) {
-    InitRenderView(render_view_host, proxy->GetRoutingID());
-    proxy->set_render_frame_proxy_created(true);
+    InitRenderView(render_view_host, proxy);
   } else {
     proxy->InitRenderFrameProxy();
   }
@@ -1961,8 +1955,7 @@ void RenderFrameHostManager::EnsureRenderViewInitialized(
   if (!proxy)
     return;
 
-  InitRenderView(render_view_host, proxy->GetRoutingID());
-  proxy->set_render_frame_proxy_created(true);
+  InitRenderView(render_view_host, proxy);
 }
 
 void RenderFrameHostManager::CreateOuterDelegateProxy(
@@ -1996,7 +1989,7 @@ void RenderFrameHostManager::SetRWHViewForInnerContents(
 
 bool RenderFrameHostManager::InitRenderView(
     RenderViewHostImpl* render_view_host,
-    int proxy_routing_id) {
+    RenderFrameProxyHost* proxy) {
   // Ensure the renderer process is initialized before creating the
   // RenderView.
   if (!render_view_host->GetProcess()->Init())
@@ -2011,9 +2004,8 @@ bool RenderFrameHostManager::InitRenderView(
   // about any bindings it will need enabled.
   // TODO(carlosk): Move WebUI to RenderFrameHost in https://crbug.com/508850.
   WebUIImpl* dest_web_ui = nullptr;
-  DCHECK_EQ(render_view_host->is_active(),
-            proxy_routing_id == MSG_ROUTING_NONE);
-  if (proxy_routing_id == MSG_ROUTING_NONE) {
+  DCHECK_EQ(render_view_host->is_active(), !proxy);

[Charlie Reis, 2015/10/16 23:09:57]

These are no longer equal, as your test shows.

Suppose that b.com were a WebUI page.  I think we would want to grant the
bindings based on whether the RVH is active (and thus for the main frame),
regardless of whether there's a leftover proxy around from before.

You can see my discussion with Alex here:
https://codereview.chromium.org/1403343002/

[nasko, 2015/10/16 23:42:09]

I think this is correct. I've removed the DCHECK.

+  if (!proxy) {
     if (base::CommandLine::ForCurrentProcess()->HasSwitch(
             switches::kEnableBrowserSideNavigation)) {
       dest_web_ui =
@@ -2036,9 +2028,15 @@ bool RenderFrameHostManager::InitRenderView(
   int opener_frame_routing_id =
       GetOpenerRoutingID(render_view_host->GetSiteInstance());
 
-  return delegate_->CreateRenderViewForRenderManager(
-      render_view_host, opener_frame_routing_id, proxy_routing_id,
+  bool created = delegate_->CreateRenderViewForRenderManager(
+      render_view_host, opener_frame_routing_id,
+      proxy ? proxy->GetRoutingID() : MSG_ROUTING_NONE,
       frame_tree_node_->current_replication_state());
+
+  if (proxy)

[nasko, 2015/10/16 23:42:09]

I also noticed that the bool is set unconditionally, which seems wrong. It used
to be the case before, so we probably had a bug even before I moved the code
here.

Now it is set only if the creation above succeeded.

[alexmos, 2015/10/16 23:54:17]

Acknowledged.

+    proxy->set_render_frame_proxy_created(true);
+
+  return created;
 }
 
 bool RenderFrameHostManager::InitRenderFrame(