Create RenderFrameProxyHost at time of swap-out instead of commit.

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 <utility>
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 75 matching lines @@
 RenderFrameHostManager::~RenderFrameHostManager() {
   if (pending_render_frame_host_)
     CancelPending();
 
   if (cross_process_frame_connector_)
     delete cross_process_frame_connector_;
 
   // We should always have a current RenderFrameHost except in some tests.
   render_frame_host_.reset();
 
-  // Delete any swapped out RenderFrameHosts.
+  // Delete any swapped out RenderFrameHosts after sending a message to delete
+  // their counterparts in the renderer process.
+  for (RenderFrameProxyHostMap::iterator iter = proxy_hosts_.begin();
+       iter != proxy_hosts_.end();
+       ++iter) {
+    iter->second->DeleteRendererProxy();
+  }
   STLDeleteValues(&proxy_hosts_);
 }
 
 void RenderFrameHostManager::Init(BrowserContext* browser_context,
                                   SiteInstance* site_instance,
                                   int view_routing_id,
                                   int frame_routing_id) {
   // Create a RenderViewHost and RenderFrameHost, once we have an instance.  It
   // is important to immediately give this SiteInstance to a RenderViewHost so
   // that the SiteInstance is ref counted.
@@ skipping 61 matching lines @@
     return NULL;  // We weren't able to create a pending render frame host.
 
   // If the current render_frame_host_ isn't live, we should create it so
   // that we don't show a sad tab while the dest_render_frame_host fetches
   // its first page.  (Bug 1145340)
   if (dest_render_frame_host != render_frame_host_ &&
       !render_frame_host_->render_view_host()->IsRenderViewLive()) {
     // Note: we don't call InitRenderView here because we are navigating away
     // soon anyway, and we don't have the NavigationEntry for this host.
     delegate_->CreateRenderViewForRenderManager(
-        render_frame_host_->render_view_host(), MSG_ROUTING_NONE, NULL);
+        render_frame_host_->render_view_host(), MSG_ROUTING_NONE,
+        MSG_ROUTING_NONE, NULL);
   }
 
   // If the renderer crashed, then try to create a new one to satisfy this
   // navigation request.
   if (!dest_render_frame_host->render_view_host()->IsRenderViewLive()) {
     // Recreate the opener chain.
     int opener_route_id = delegate_->CreateOpenerRenderViewsForRenderManager(
         dest_render_frame_host->GetSiteInstance());
     if (!InitRenderView(dest_render_frame_host->render_view_host(),
-                        opener_route_id))
+                        opener_route_id, MSG_ROUTING_NONE))
       return NULL;
 
     // 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_ &&
         dest_render_frame_host->render_view_host()->GetView()) {
       dest_render_frame_host->render_view_host()->GetView()->Hide();
     } else if (frame_tree_node_->IsMainFrame()) {
       // This is our primary renderer, notify here as we won't be calling
@@ skipping 300 matching lines @@
     // The same CrossProcessFrameConnector is used for subsequent cross-
     // process navigations, but it will be destroyed if the Frame is
     // navigated back to the same site instance as its parent.
     // TODO(kenrb): This will change when RenderFrameProxyHost is created.
     if (!cross_process_frame_connector_) {
       cross_process_frame_connector_ =
           new CrossProcessFrameConnector(render_frame_host_.get());
     }
   }
 
+  // Create the RenderFrameProxyHost that will replace the RenderFrameHost
+  // which is swapping out.
+  RenderFrameProxyHost* proxy = new RenderFrameProxyHost(
+      render_frame_host_->GetSiteInstance(), frame_tree_node_);
+  proxy_hosts_[render_frame_host_->GetSiteInstance()->GetId()] = proxy;

[Charlie Reis, 2014/05/15 00:32:50]

To be clear, render_frame_host_ (and its SiteInstance) now exist both as the
current RFH and on the proxy list.  Is that going to violate any assumptions in
the time period before commit?

[ncarter (slow), 2014/05/15 00:42:23]

What guarantees that there's not already something in the map at
proxy_hosts_[id]? It was clear in RenderFrameHostManager::CreateRenderFrame that
this was the case at the time of the assignment, but I get confused why it's not
a problem here.

[nasko, 2014/05/15 18:47:13]

It actually won't exist in two places. Here only a proxy is created, as we can't
have the RFH be in both places at the same time, this will be nasty.
Only at commit time we know that we will no longer use the RFH, so it is at that
point that we pass the ownership of the RFH from RFHM to the proxy.

[nasko, 2014/05/15 18:47:13]

I don't think there is a guarantee and I found one test that hits this case -
CrossSiteTransferTest.ReplaceEntryInProcessThenTranfers. I've changed the code
to ensure it deletes the existing one before creating the new one.

[Charlie Reis, 2014/05/15 22:55:00]

In that case, should IsOnSwappedOutList(RFH) will return false if the proxy is
on the list but the RFH is the current one?  Looks like it will current return
true because it just compares against the proxy's SiteInstance.

Also, what about APIs that look up a host given a SiteInstance, like
GetSwappedOutRenderViewHost and GetRenderFrameProxyHost?  A given SiteInstance
can now be the current RFH and have a proxy on the list.  If we want to hide
that fact, we should probably update those and add a unit test for it.

[nasko, 2014/05/15 23:36:49]

The only way for IsOnSwappedOutList(RFH) to return true is for
(iter->second->render_frame_host() == rfh) to be true. The call to
render_frame_host() will return NULL prior to the proxy taking ownership, which
happens at commit time or when creating swapped out RFH. In both of these cases
it is indeed swapped out. What am I missing?
 
I'm not sure how we would hide the fact that there is a proxy and a frame for
short period of time. I'm open to ideas, but here is my analysis why I think it
is currently ok.

GetSwappedOutRenderViewHost will behave similarly to IsOnSwappedOutList(RFH),
though I had to restore one of my previous changes. See
RFPH::render_view_host().

For GetRenderFrameProxyHost, it is used in 3 locations:
* IsRVHOnSwappedOutList - already safe usage
* GetSwappedOutRenderViewHost - again, safe usage
* CreateRenderFrame - this one is a bit trickier, but there is already a
CHECK_NE ensuring that the current host and the new frame don't have the same
SiteInstance.

[Charlie Reis, 2014/05/16 18:54:07]

I think I confused myself-- I must have been looking at
GetRenderFrameProxyHost(SiteInstance), which won't notice that the RFH is
missing during this time period.  We could detect whether the proxy has a null
RFH and not return it, but I think I agree with your comments below that it's
not necessary to hide it.

+
   // Tell the old frame it is being swapped out.  This will fire the unload
   // handler in the background (without firing the beforeunload handler a second
   // time).  When the navigation completes, we will send a message to the
   // ResourceDispatcherHost, allowing the pending RVH's response to resume.
-  render_frame_host_->SwapOut();
+  render_frame_host_->SwapOut(proxy);
 
   // ResourceDispatcherHost has told us to run the onunload handler, which
   // means it is not a download or unsafe page, and we are going to perform the
   // navigation.  Thus, we no longer need to remember that the RenderFrameHost
   // is part of a pending cross-site request.
   if (pending_render_frame_host_) {
     pending_render_frame_host_->render_view_host()->
         SetHasPendingCrossSiteRequest(false);
   }
 }
@@ skipping 27 matching lines @@
     int32 site_instance_id,
     FrameTreeNode* node) {
   RenderFrameProxyHostMap::iterator iter =
       node->render_manager()->proxy_hosts_.find(site_instance_id);
   if (iter != node->render_manager()->proxy_hosts_.end()) {
     RenderFrameProxyHost* proxy = iter->second;
     // If the RVH is pending swap out, it needs to switch state to
     // pending shutdown. Otherwise it is deleted.
     if (proxy->render_view_host()->rvh_state() ==
         RenderViewHostImpl::STATE_PENDING_SWAP_OUT) {
-      scoped_ptr<RenderFrameHostImpl> swapped_out_rfh = proxy->PassFrameHost();
+      scoped_ptr<RenderFrameHostImpl> swapped_out_rfh =
+          proxy->PassFrameHostOwnership();
 
       swapped_out_rfh->SetPendingShutdown(base::Bind(
           &RenderFrameHostManager::ClearPendingShutdownRFHForSiteInstance,
           node->render_manager()->weak_factory_.GetWeakPtr(),
           site_instance_id,
           swapped_out_rfh.get()));
       RFHPendingDeleteMap::iterator pending_delete_iter =
           node->render_manager()->pending_delete_hosts_.find(site_instance_id);
       if (pending_delete_iter ==
               node->render_manager()->pending_delete_hosts_.end() ||
           pending_delete_iter->second.get() != swapped_out_rfh) {
         node->render_manager()->pending_delete_hosts_[site_instance_id] =
             linked_ptr<RenderFrameHostImpl>(swapped_out_rfh.release());
       }
     } else {
+      proxy->DeleteRendererProxy();
       delete proxy;
     }
     node->render_manager()->proxy_hosts_.erase(site_instance_id);
   }
 
   return true;
 }
 
 bool RenderFrameHostManager::ShouldTransitionCrossSite() {
   // False in the single-process mode, as it makes RVHs to accumulate
@@ skipping 304 matching lines @@
 
   // Check if we've already created an RFH for this SiteInstance.  If so, try
   // to re-use the existing one, which has already been initialized.  We'll
   // remove it from the list of swapped out hosts if it commits.
   RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(instance);
 
   FrameTreeNode* parent_node = frame_tree_node_->parent();
 
   if (proxy) {
     routing_id = proxy->render_view_host()->GetRoutingID();
+
     // Delete the existing RenderFrameProxyHost, but reuse the RenderFrameHost.
     // Prevent the process from exiting while we're trying to use it.
     if (!swapped_out) {
-      new_render_frame_host = proxy->PassFrameHost();
+      new_render_frame_host = proxy->PassFrameHostOwnership();
+      new_render_frame_host->set_render_frame_proxy(NULL);

[Charlie Reis, 2014/05/15 00:32:50]

Would it make sense to do this step inside PassFrameHostOwnership, or are we
intentionally skipping that in some call sites?

[nasko, 2014/05/15 18:47:13]

Done.

       new_render_frame_host->GetProcess()->AddPendingView();
 
       proxy_hosts_.erase(instance->GetId());
       delete proxy;
     } else {
       // Detect if this is a cross-process child frame that is navigating
       // back to the same SiteInstance as its parent.
       if (parent_node && cross_process_frame_connector_ &&
           render_frame_host_->GetSiteInstance() == parent_node->
               render_manager()->current_frame_host()->GetSiteInstance()) {
         delete cross_process_frame_connector_;
         cross_process_frame_connector_ = NULL;
       }
     }
   } else {
     // Create a new RenderFrameHost if we don't find an existing one.
     new_render_frame_host = CreateRenderFrameHost(
         instance, MSG_ROUTING_NONE, MSG_ROUTING_NONE, swapped_out, hidden);
     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_hosts_[instance->GetId()] = new RenderFrameProxyHost(
-          new_render_frame_host.Pass());
+      if (!proxy) {

[Charlie Reis, 2014/05/15 00:32:50]

We already know there's no proxy, since we're in the else block of the if on
line 914.

[nasko, 2014/05/15 18:47:13]

Done.

+        proxy = new RenderFrameProxyHost(
+            new_render_frame_host->GetSiteInstance(), frame_tree_node_);
+      }
+      proxy->TakeFrameHostOwnership(new_render_frame_host.Pass());
+      proxy_routing_id = proxy->GetRoutingID();
+      proxy_hosts_[instance->GetId()] = proxy;
     }
 
-    bool success = InitRenderView(render_view_host, opener_route_id);
+    bool success = InitRenderView(
+        render_view_host, opener_route_id, proxy_routing_id);
     if (success && frame_tree_node_->IsMainFrame()) {
       // Don't show the main frame's view until we get a DidNavigate from it.
       render_view_host->GetView()->Hide();
     } else if (!swapped_out && pending_render_frame_host_) {
       CancelPending();
     }
     routing_id = render_view_host->GetRoutingID();
   }
 
   // Use this as our new pending RFH if it isn't swapped out.
   if (!swapped_out)
     pending_render_frame_host_ = new_render_frame_host.Pass();
 
   return routing_id;
 }
 
 bool RenderFrameHostManager::InitRenderView(RenderViewHost* render_view_host,
-                                            int opener_route_id) {
+                                            int opener_route_id,
+                                            int proxy_routing_id) {
   // We may have initialized this RenderViewHost for another RenderFrameHost.
   if (render_view_host->IsRenderViewLive())
     return true;
 
   // If the pending navigation is to a WebUI and the RenderView is not in a
   // guest process, tell the RenderViewHost about any bindings it will need
   // enabled.
   if (pending_web_ui() && !render_view_host->GetProcess()->IsGuest()) {
     render_view_host->AllowBindings(pending_web_ui()->GetBindings());
   } else {
     // Ensure that we don't create an unprivileged RenderView in a WebUI-enabled
     // process unless it's swapped out.
     RenderViewHostImpl* rvh_impl =
         static_cast<RenderViewHostImpl*>(render_view_host);
     if (!rvh_impl->IsSwappedOut()) {
       CHECK(!ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
                 render_view_host->GetProcess()->GetID()));
     }
   }
 
   return delegate_->CreateRenderViewForRenderManager(
-      render_view_host, opener_route_id, cross_process_frame_connector_);
+      render_view_host, opener_route_id, proxy_routing_id,
+      cross_process_frame_connector_);
 }
 
 void RenderFrameHostManager::CommitPending() {
   // First check whether we're going to want to focus the location bar after
   // this commit.  We do this now because the navigation hasn't formally
   // committed yet, so if we've already cleared |pending_web_ui_| the call chain
   // this triggers won't be able to figure out what's going on.
   bool will_focus_location_bar = delegate_->FocusLocationBarByDefault();
 
   // We expect SwapOutOldPage to have canceled any modal dialogs and told the
@@ skipping 100 matching lines @@
   }
 
   // If the old RFH is live, we are swapping it out and should keep track of
   // it in case we navigate back to it, or it is waiting for the unload event
   // to execute in the background.
   // TODO(creis): Swap out the subframe in --site-per-process.
   if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess))
     DCHECK(old_render_frame_host->is_swapped_out() ||
            !RenderViewHostImpl::IsRVHStateActive(
                old_render_frame_host->render_view_host()->rvh_state()));
-  // Temp fix for http://crbug.com/90867 until we do a better cleanup to make
-  // sure we don't get different rvh instances for the same site instance
-  // in the same rvhmgr.
-  // TODO(creis): Clean this up.
-  RenderFrameProxyHostMap::iterator iter =
-      proxy_hosts_.find(old_site_instance_id);
-  if (iter != proxy_hosts_.end() &&
-      iter->second->render_frame_host() != old_render_frame_host) {
-    // Delete the proxy that will be replaced in the map to avoid a leak.
-    delete iter->second;
-  }
 
   // If the RenderViewHost backing the RenderFrameHost is pending shutdown,
   // the RenderFrameHost should be put in the map of RenderFrameHosts pending
   // shutdown. Otherwise, it is stored in the map of proxy hosts.
   if (old_render_frame_host->render_view_host()->rvh_state() ==
           RenderViewHostImpl::STATE_PENDING_SHUTDOWN) {
-    proxy_hosts_.erase(old_site_instance_id);
+    // The proxy for this RenderFrameHost is created when sending the
+    // SwapOut message, so check if it already exists and delete it.
+    RenderFrameProxyHostMap::iterator iter =
+      proxy_hosts_.find(old_site_instance_id);

[Charlie Reis, 2014/05/15 00:32:50]

nit: Wrong indent.

[nasko, 2014/05/15 18:47:13]

Done.

+    if (iter != proxy_hosts_.end()) {
+      iter->second->DeleteRendererProxy();
+      delete iter->second;
+      proxy_hosts_.erase(iter);
+    }
     RFHPendingDeleteMap::iterator pending_delete_iter =
         pending_delete_hosts_.find(old_site_instance_id);
     if (pending_delete_iter == pending_delete_hosts_.end() ||
         pending_delete_iter->second.get() != old_render_frame_host) {
       pending_delete_hosts_[old_site_instance_id] =
           linked_ptr<RenderFrameHostImpl>(old_render_frame_host.release());
     }
   } else {
+    // Capture the active view count on the old RFH SiteInstance, since the
+    // ownership will be passed into the proxy and the pointer will be invalid.
+    int active_view_count = static_cast<SiteInstanceImpl*>(
+            old_render_frame_host->GetSiteInstance())->active_view_count();

[Charlie Reis, 2014/05/15 00:32:50]

nit: Wrong indent.

[nasko, 2014/05/15 18:47:13]

Done.

+
+    RenderFrameProxyHostMap::iterator iter =
+        proxy_hosts_.find(old_site_instance_id);
+    CHECK(iter != proxy_hosts_.end());
+    iter->second->TakeFrameHostOwnership(old_render_frame_host.Pass());
+
     // If there are no active views in this SiteInstance, it means that
     // this RFH was the last active one in the SiteInstance. Now that we
     // know that all RFHs are swapped out, we can delete all the RFHs and RVHs
-    // in this SiteInstance.  We do this after ensuring the RFH is on the
-    // swapped out list to simplify the deletion.
-    if (!static_cast<SiteInstanceImpl*>(
-            old_render_frame_host->GetSiteInstance())->active_view_count()) {
-      old_render_frame_host.reset();
+    // in this SiteInstance.
+    if (!active_view_count) {

[Charlie Reis, 2014/05/15 00:32:50]

nit: No braces needed.

[nasko, 2014/05/15 18:47:13]

Done.

       ShutdownRenderFrameHostsInSiteInstance(old_site_instance_id);
-    } else {
-      proxy_hosts_[old_site_instance_id] = new RenderFrameProxyHost(
-          old_render_frame_host.Pass());
     }
   }
 }
 
 void RenderFrameHostManager::ShutdownRenderFrameHostsInSiteInstance(
     int32 site_instance_id) {
   // First remove any swapped out RFH for this SiteInstance from our own list.
   ClearProxiesInSiteInstance(site_instance_id, frame_tree_node_);
 
   // Use the safe RenderWidgetHost iterator for now to find all RenderViewHosts
@@ skipping 192 matching lines @@
   pending_render_frame_host->GetProcess()->RemovePendingView();
 
   // If the SiteInstance for the pending RFH is being used by others, don't
   // delete the RFH, just swap it out and it can be reused at a later point.
   SiteInstanceImpl* site_instance = static_cast<SiteInstanceImpl*>(
       pending_render_frame_host->GetSiteInstance());
   if (site_instance->active_view_count() > 1) {
     // Any currently suspended navigations are no longer needed.
     pending_render_frame_host->render_view_host()->CancelSuspendedNavigations();
 
-    pending_render_frame_host->SwapOut();
-
-    proxy_hosts_[site_instance->GetId()] = new RenderFrameProxyHost(
-        pending_render_frame_host.Pass());
+    RenderFrameProxyHost* proxy =
+        new RenderFrameProxyHost(site_instance, frame_tree_node_);
+    pending_render_frame_host->SwapOut(proxy);
+    proxy->TakeFrameHostOwnership(pending_render_frame_host.Pass());
+    proxy_hosts_[site_instance->GetId()] = proxy;

[Charlie Reis, 2014/05/15 00:32:50]

I'm a little uncomfortable that we put the proxy into proxy_hosts_ before
calling SwapOut in one case and after calling it in the other.  SwapOut
synchronously calls back into SwappedOut, which might call RequestTransferURL,
so I think we should be consistent about the state of proxy_hosts_ during this
call if possible.

[nasko, 2014/05/15 18:47:13]

This was just a honest omission, not intentional. I've made all creations for
RFPH to use the same sequence.

   } else {
     // We won't be coming back, so delete this one.
     pending_render_frame_host.reset();
   }
 
   pending_web_ui_.reset();
   pending_and_current_web_ui_.reset();
 }
 
 bool RenderFrameHostManager::IsRVHOnSwappedOutList(
@@ skipping 30 matching lines @@
     SiteInstance* instance) const {
   RenderFrameProxyHostMap::const_iterator iter =
       proxy_hosts_.find(instance->GetId());
   if (iter != proxy_hosts_.end())
     return iter->second;
 
   return NULL;
 }
 
 }  // namespace content