Properly recreate swapped out RenderView.

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 <algorithm>
 #include <utility>
 
 #include "base/command_line.h"
@@ skipping 400 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 ServiceRegistry 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 a service to this RFH's ServiceRegistry).
     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(),
-                        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
     // our primary one.  Otherwise, we might crash if we try to call Show()
     // on it later.
     if (dest_render_frame_host != render_frame_host_) {
       if (dest_render_frame_host->GetView())
         dest_render_frame_host->GetView()->Hide();
     } else {
       // After a renderer crash we'd have marked the host as invisible, so we
@@ skipping 637 matching lines @@
   }
   DCHECK(navigation_rfh &&
          (navigation_rfh == render_frame_host_.get() ||
           navigation_rfh == speculative_render_frame_host_.get()));
 
   // If the RenderFrame that needs to navigate is not live (its process was just
   // created or has crashed), initialize it.
   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;
     }
 
     if (navigation_rfh == render_frame_host_) {
       // TODO(nasko): This is a very ugly hack. The Chrome extensions process
       // manager still uses NotificationService and expects to see a
       // RenderViewHost changed notification after WebContents and
       // RenderFrameHostManager are completely initialized. This should be
       // removed once the process manager moves away from NotificationService.
       // See https://crbug.com/462682.
@@ skipping 696 matching lines @@
     // Prevent the process from exiting while we're trying to use it.
     if (!swapped_out) {
       new_render_frame_host = proxy->PassFrameHostOwnership();
       new_render_frame_host->GetProcess()->AddPendingView();
 
       proxy_hosts_->Remove(instance->GetId());
       // NB |proxy| is deleted at this point.
 
       // 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);
       }
     }
   } else {
     // Create a new RenderFrameHost if we don't find an existing one.
 
     int32 widget_routing_id = MSG_ROUTING_NONE;
     // A RenderFrame in a different process from its parent RenderFrame
     // requires a RenderWidget for input/layout/painting.
     if (frame_tree_node_->parent() &&
         frame_tree_node_->parent()->current_frame_host()->GetSiteInstance() !=
             instance) {
       CHECK(SiteIsolationPolicy::AreCrossProcessFramesPossible());
       widget_routing_id = instance->GetProcess()->GetNextRoutingID();
     }
 
     new_render_frame_host = CreateRenderFrameHost(
         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.
     if (!swapped_out) {
       new_render_frame_host->GetProcess()->AddPendingView();
     } else {
       proxy = new RenderFrameProxyHost(
           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.
       if (!swapped_out && !render_view_host->GetWidget()->GetView())
         delegate_->CreateRenderWidgetHostViewForRenderManager(render_view_host);
     } else {
       DCHECK(render_view_host->IsRenderViewLive());
     }
 
     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
         // RenderWidgetHostView; RenderWidgetHosts for out-of-process iframes
         // will be created later and hidden.
         if (render_view_host->GetWidget()->GetView())
           render_view_host->GetWidget()->GetView()->Hide();
       }
       // RenderViewHost for |instance| might exist prior to calling
       // CreateRenderFrame. In such a case, InitRenderView will not create the
@@ skipping 60 matching lines @@
     return proxy->GetRoutingID();
 
   if (!proxy) {
     proxy =
         new RenderFrameProxyHost(instance, render_view_host, frame_tree_node_);
     proxy_hosts_->Add(instance->GetId(), make_scoped_ptr(proxy));
   }
 
   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();
   }
 
   return proxy->GetRoutingID();
 }
 
 void RenderFrameHostManager::CreateProxiesForChildFrame(FrameTreeNode* child) {
   for (const auto& pair : *proxy_hosts_) {
     // Do not create proxies for subframes in the outer delegate's process,
@@ skipping 13 matching lines @@
 
   if (render_view_host->IsRenderViewLive())
     return;
 
   // If the proxy in |instance| doesn't exist, this RenderView is not swapped
   // out and shouldn't be reinitialized here.
   RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(instance);
   if (!proxy)
     return;
 
-  InitRenderView(render_view_host, proxy->GetRoutingID());
-  proxy->set_render_frame_proxy_created(true);
+  InitRenderView(render_view_host, proxy);
 }
 
 void RenderFrameHostManager::CreateOuterDelegateProxy(
     SiteInstance* outer_contents_site_instance,
     RenderFrameHostImpl* render_frame_host) {
   CHECK(BrowserPluginGuestMode::UseCrossProcessFramesForGuests());
   RenderFrameProxyHost* proxy = new RenderFrameProxyHost(
       outer_contents_site_instance, nullptr, frame_tree_node_);
   proxy_hosts_->Add(outer_contents_site_instance->GetId(),
                     make_scoped_ptr(proxy));
@@ skipping 13 matching lines @@
 }
 
 void RenderFrameHostManager::SetRWHViewForInnerContents(
     RenderWidgetHostView* child_rwhv) {
   DCHECK(ForInnerDelegate() && frame_tree_node_->IsMainFrame());
   GetProxyToOuterDelegate()->SetChildRWHView(child_rwhv);
 }
 
 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())
     return false;
 
   // We may have initialized this RenderViewHost for another RenderFrameHost.
   if (render_view_host->IsRenderViewLive())
     return true;
 
   // If |render_view_host| is not for a proxy and the navigation is to a WebUI,
   // and if the RenderView is not in a guest process, tell |render_view_host|
   // 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 =
           should_reuse_web_ui_ ? web_ui_.get() : speculative_web_ui_.get();
     } else {
       dest_web_ui = pending_web_ui();
     }
   }
   if (dest_web_ui && !render_view_host->GetProcess()->IsForGuestsOnly()) {
     render_view_host->AllowBindings(dest_web_ui->GetBindings());
   } else {
     // Ensure that we don't create an unprivileged RenderView in a WebUI-enabled
     // process unless it's swapped out.
     if (render_view_host->is_active()) {
       CHECK(!ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
                 render_view_host->GetProcess()->GetID()));
     }
   }
 
   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(
     RenderFrameHostImpl* render_frame_host) {
   if (render_frame_host->IsRenderFrameLive())
     return true;
 
   SiteInstance* site_instance = render_frame_host->GetSiteInstance();
 
   int opener_routing_id = MSG_ROUTING_NONE;
@@ skipping 598 matching lines @@
 int RenderFrameHostManager::GetOpenerRoutingID(SiteInstance* instance) {
   if (!frame_tree_node_->opener())
     return MSG_ROUTING_NONE;
 
   return frame_tree_node_->opener()
       ->render_manager()
       ->GetRoutingIdForSiteInstance(instance);
 }
 
 }  // namespace content