Introduce RenderFrameProxyHost object and use it in RFHM.

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"
 #include "base/logging.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/devtools/render_view_devtools_agent_host.h"
 #include "content/browser/frame_host/cross_process_frame_connector.h"
 #include "content/browser/frame_host/cross_site_transferring_request.h"
 #include "content/browser/frame_host/debug_urls.h"
 #include "content/browser/frame_host/interstitial_page_impl.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/render_frame_host_factory.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
+#include "content/browser/frame_host/render_frame_proxy_host.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_factory.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/browser/webui/web_ui_controller_factory_registry.h"
 #include "content/browser/webui/web_ui_impl.h"
 #include "content/common/view_messages.h"
 #include "content/port/browser/render_widget_host_view_port.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/notification_service.h"
@@ skipping 49 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();
 
-  // TODO(creis): Now that we aren't using Shutdown, make RenderFrameHostMap
-  // use scoped_ptrs.
   // Delete any swapped out RenderFrameHosts.
-  for (RenderFrameHostMap::iterator iter = swapped_out_hosts_.begin();
-       iter != swapped_out_hosts_.end();
+  for (RenderFrameHostMap::iterator iter = proxy_hosts_.begin();
+       iter != proxy_hosts_.end();
        ++iter) {
     delete iter->second;
   }
 }
 
 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.
   if (!site_instance)
     site_instance = SiteInstance::Create(browser_context);
 
-  render_frame_host_ = make_scoped_ptr(
-      CreateRenderFrameHost(site_instance, view_routing_id, frame_routing_id,
-                            false, delegate_->IsHidden()));
+  render_frame_host_ = CreateRenderFrameHost(site_instance,
+                                             view_routing_id,
+                                             frame_routing_id,
+                                             false,
+                                             delegate_->IsHidden());
 
   // Keep track of renderer processes as they start to shut down or are
   // crashed/killed.
   registrar_.Add(this, NOTIFICATION_RENDERER_PROCESS_CLOSED,
                  NotificationService::AllSources());
   registrar_.Add(this, NOTIFICATION_RENDERER_PROCESS_CLOSING,
                  NotificationService::AllSources());
 }
 
 RenderViewHostImpl* RenderFrameHostManager::current_host() const {
@@ skipping 315 matching lines @@
     cross_navigation_pending_ = false;
   } else {
     // No one else should be sending us DidNavigate in this state.
     DCHECK(false);
   }
 }
 
 // TODO(creis): Take in RenderFrameHost instead, since frames can have openers.
 void RenderFrameHostManager::DidDisownOpener(RenderViewHost* render_view_host) {
   // Notify all swapped out hosts, including the pending RVH.
-  for (RenderFrameHostMap::iterator iter = swapped_out_hosts_.begin();
-       iter != swapped_out_hosts_.end();
+  for (RenderFrameHostMap::iterator iter = proxy_hosts_.begin();
+       iter != proxy_hosts_.end();
        ++iter) {
-    DCHECK_NE(iter->second->GetSiteInstance(),
+    DCHECK_NE(iter->second->render_frame_host()->GetSiteInstance(),

[Charlie Reis, 2014/04/09 21:48:30]

I think it would make sense to put a GetSiteInstance() on RenderFrameProxyHost,
since it'll have one of its own long term.

[nasko, 2014/04/10 20:37:36]

Done.

               current_frame_host()->GetSiteInstance());
     iter->second->render_view_host()->DisownOpener();
   }
 }
 
 void RenderFrameHostManager::RendererProcessClosing(
     RenderProcessHost* render_process_host) {
   // Remove any swapped out RVHs from this process, so that we don't try to
   // swap them back in while the process is exiting.  Start by finding them,
   // since there could be more than one.
   std::list<int> ids_to_remove;
-  for (RenderFrameHostMap::iterator iter = swapped_out_hosts_.begin();
-       iter != swapped_out_hosts_.end();
+  for (RenderFrameHostMap::iterator iter = proxy_hosts_.begin();
+       iter != proxy_hosts_.end();
        ++iter) {
     if (iter->second->GetProcess() == render_process_host)
       ids_to_remove.push_back(iter->first);
   }
 
   // Now delete them.
   while (!ids_to_remove.empty()) {
-    delete swapped_out_hosts_[ids_to_remove.back()];
-    swapped_out_hosts_.erase(ids_to_remove.back());
+    delete proxy_hosts_[ids_to_remove.back()];
+    proxy_hosts_.erase(ids_to_remove.back());
     ids_to_remove.pop_back();
   }
 }
 
 void RenderFrameHostManager::SwapOutOldPage() {
   // Should only see this while we have a pending renderer or transfer.
   CHECK(cross_navigation_pending_ || pending_nav_params_.get());
 
   // Tell the renderer to suppress any further modal dialogs so that we can swap
   // it out.  This must be done before canceling any current dialog, in case
@@ skipping 55 matching lines @@
     case NOTIFICATION_RENDERER_PROCESS_CLOSING:
       RendererProcessClosing(
           Source<RenderProcessHost>(source).ptr());
       break;
 
     default:
       NOTREACHED();
   }
 }
 
-bool RenderFrameHostManager::ClearSwappedOutRFHsInSiteInstance(
+bool RenderFrameHostManager::ClearProxiesInSiteInstance(
     int32 site_instance_id,
     FrameTreeNode* node) {
   RenderFrameHostMap::iterator iter =
-      node->render_manager()->swapped_out_hosts_.find(site_instance_id);
-  if (iter != node->render_manager()->swapped_out_hosts_.end()) {
-    RenderFrameHostImpl* swapped_out_rfh = iter->second;
+      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 (swapped_out_rfh->render_view_host()->rvh_state() ==
+    if (proxy->render_view_host()->rvh_state() ==
         RenderViewHostImpl::STATE_PENDING_SWAP_OUT) {
-      swapped_out_rfh->SetPendingShutdown(base::Bind(
+      scoped_ptr<RenderFrameHostImpl> rfh = proxy->PassFrameHost();

[Charlie Reis, 2014/04/09 21:48:30]

nit: I liked the swapped_out_rfh name from before.  It's a bit more accurate and
easy to grep for as we remove this long term.

[nasko, 2014/04/10 20:37:36]

Done.

+
+      rfh->SetPendingShutdown(base::Bind(
           &RenderFrameHostManager::ClearPendingShutdownRFHForSiteInstance,
           node->render_manager()->weak_factory_.GetWeakPtr(),
           site_instance_id,
-          swapped_out_rfh));
+          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() != iter->second) {
+          pending_delete_iter->second.get() != rfh) {
         node->render_manager()->pending_delete_hosts_[site_instance_id] =
-            linked_ptr<RenderFrameHostImpl>(swapped_out_rfh);
+            linked_ptr<RenderFrameHostImpl>(rfh.release());
       }
     } else {
-      delete swapped_out_rfh;
+      delete proxy;
     }
-    node->render_manager()->swapped_out_hosts_.erase(site_instance_id);
+    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
   // in swapped_out_hosts_.
   // True if we are using process-per-site-instance (default) or
   // process-per-site (kProcessPerSite).
@@ skipping 237 matching lines @@
   }
 
   // Start the new renderer in a new SiteInstance, but in the current
   // BrowsingInstance.  It is important to immediately give this new
   // SiteInstance to a RenderViewHost (if it is different than our current
   // SiteInstance), so that it is ref counted.  This will happen in
   // CreateRenderView.
   return current_instance->GetRelatedSiteInstance(dest_url);
 }
 
-RenderFrameHostImpl* RenderFrameHostManager::CreateRenderFrameHost(
+scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrameHost(
     SiteInstance* site_instance,
     int view_routing_id,
     int frame_routing_id,
     bool swapped_out,
     bool hidden) {
   if (frame_routing_id == MSG_ROUTING_NONE)
     frame_routing_id = site_instance->GetProcess()->GetNextRoutingID();
 
   // Create a RVH for main frames, or find the existing one for subframes.
   FrameTree* frame_tree = frame_tree_node_->frame_tree();
   RenderViewHostImpl* render_view_host = NULL;
   if (frame_tree_node_->IsMainFrame()) {
     render_view_host = frame_tree->CreateRenderViewHostForMainFrame(
         site_instance, view_routing_id, frame_routing_id, swapped_out, hidden);
   } else {
     render_view_host = frame_tree->GetRenderViewHostForSubFrame(site_instance);
 
     // If we haven't found a RVH for a subframe RFH, it's because we currently
     // do not create top-level RFHs for pending subframe navigations.  Create
     // the RVH here for now.
     // TODO(creis): Mirror the frame tree so this check isn't necessary.
     if (!render_view_host) {
       render_view_host = frame_tree->CreateRenderViewHostForMainFrame(
           site_instance, view_routing_id, frame_routing_id, swapped_out,
           hidden);
     }
   }
 
-  // TODO(creis): Make render_frame_host a scoped_ptr.
   // TODO(creis): Pass hidden to RFH.
-  RenderFrameHostImpl* render_frame_host =
-      RenderFrameHostFactory::Create(render_view_host,
-                                     render_frame_delegate_,
-                                     frame_tree,
-                                     frame_tree_node_,
-                                     frame_routing_id,
-                                     swapped_out).release();
-  return render_frame_host;
+  scoped_ptr<RenderFrameHostImpl> render_frame_host =
+      make_scoped_ptr(RenderFrameHostFactory::Create(render_view_host,
+                                                     render_frame_delegate_,
+                                                     frame_tree,
+                                                     frame_tree_node_,
+                                                     frame_routing_id,
+                                                     swapped_out).release());
+  return render_frame_host.Pass();
 }
 
 int RenderFrameHostManager::CreateRenderFrame(
     SiteInstance* instance,
     int opener_route_id,
     bool swapped_out,
     bool hidden) {
   CHECK(instance);
   DCHECK(!swapped_out || hidden); // Swapped out views should always be hidden.
 
+  scoped_ptr<RenderFrameHostImpl> new_render_frame_host;
+  int routing_id = MSG_ROUTING_NONE;
+
   // We are creating a pending or swapped out RFH here.  We should never create
   // it in the same SiteInstance as our current RFH.
   CHECK_NE(render_frame_host_->GetSiteInstance(), instance);
 
   // 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.
-  RenderFrameHostImpl* new_render_frame_host =
-      GetSwappedOutRenderFrameHost(instance);
+  RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(instance);
 
   FrameTreeNode* parent_node = NULL;
   if (frame_tree_node_)
     parent_node = frame_tree_node_->parent();
 
-  if (new_render_frame_host) {
+  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->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.
-    // TODO(creis): Make new_render_frame_host a scoped_ptr.
-    new_render_frame_host = CreateRenderFrameHost(instance, MSG_ROUTING_NONE,
-                                                  MSG_ROUTING_NONE, swapped_out,
-                                                  hidden);
+    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();
 
     // If the new RFH is swapped out already, store it.  Otherwise prevent the
     // process from exiting while we're trying to navigate in it.
-    if (swapped_out) {
-      swapped_out_hosts_[instance->GetId()] = new_render_frame_host;
+    if (!swapped_out) {

[Charlie Reis, 2014/04/09 21:48:30]

Why reverse this block?  I'm not opposed, but we should update the comment as
well if there's a reason to do it.

[nasko, 2014/04/10 20:37:36]

Done.

+      new_render_frame_host->GetProcess()->AddPendingView();
     } else {
-      new_render_frame_host->GetProcess()->AddPendingView();
+      proxy_hosts_[instance->GetId()] = new RenderFrameProxyHost(
+          new_render_frame_host.Pass());
     }
 
-    RenderViewHostImpl* render_view_host =
-        new_render_frame_host->render_view_host();
     bool success = InitRenderView(render_view_host, opener_route_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_.reset(new_render_frame_host);
+    pending_render_frame_host_ = new_render_frame_host.Pass();
 
-  return new_render_frame_host->render_view_host()->GetRoutingID();
+  return routing_id;
 }
 
 bool RenderFrameHostManager::InitRenderView(RenderViewHost* render_view_host,
                                             int opener_route_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
@@ skipping 56 matching lines @@
   bool focus_render_view = !will_focus_location_bar &&
       render_frame_host_->render_view_host()->GetView() &&
       render_frame_host_->render_view_host()->GetView()->HasFocus();
 
   // TODO(creis): As long as show/hide are on RVH, we don't want to do them for
   // subframe navigations or they'll interfere with the top-level page.
   bool is_main_frame = frame_tree_node_->IsMainFrame();
 
   // Swap in the pending frame and make it active. Also ensure the FrameTree
   // stays in sync.
-  RenderFrameHostImpl* old_render_frame_host = render_frame_host_.release();
+  scoped_ptr<RenderFrameHostImpl> old_render_frame_host =
+      render_frame_host_.Pass();
   render_frame_host_ = pending_render_frame_host_.Pass();
   if (is_main_frame)
     render_frame_host_->render_view_host()->AttachToFrameTree();
 
   // The process will no longer try to exit, so we can decrement the count.
   render_frame_host_->GetProcess()->RemovePendingView();
 
   // If the view is gone, then this RenderViewHost died while it was hidden.
   // We ignored the RenderProcessGone call at the time, so we should send it now
   // to make sure the sad tab shows up, etc.
   if (!render_frame_host_->render_view_host()->GetView()) {
     delegate_->RenderProcessGoneFromRenderManager(
         render_frame_host_->render_view_host());
   } else if (!delegate_->IsHidden()) {
     render_frame_host_->render_view_host()->GetView()->Show();
   }
 
   // If the old view is live and top-level, hide it now that the new one is
   // visible.
   int32 old_site_instance_id =
       old_render_frame_host->GetSiteInstance()->GetId();
   if (old_render_frame_host->render_view_host()->GetView()) {
     if (is_main_frame) {
       old_render_frame_host->render_view_host()->GetView()->Hide();
       old_render_frame_host->render_view_host()->WasSwappedOut(base::Bind(
           &RenderFrameHostManager::ClearPendingShutdownRFHForSiteInstance,
           weak_factory_.GetWeakPtr(),
           old_site_instance_id,
-          old_render_frame_host));
-    } else {
-      // TODO(creis): We'll need to set this back to false if we navigate back.
-      old_render_frame_host->set_swapped_out(true);
+          old_render_frame_host.get()));
     }
+    // TODO(creis): We'll need to set this back to false if we navigate back.
+    old_render_frame_host->set_swapped_out(true);

[Charlie Reis, 2014/04/09 21:48:30]

I don't think we want to do this for main frames, do we?  I thought
RFH::is_swapped_out_ is kind of broken and only used for subframes (i.e., behind
the flag).

[nasko, 2014/04/10 20:37:36]

Done.

   }
 
   // Make sure the size is up to date.  (Fix for bug 1079768.)
   delegate_->UpdateRenderViewSizeForRenderManager();
 
   if (will_focus_location_bar) {
     delegate_->SetFocusToLocationBar(false);
   } else if (focus_render_view &&
              render_frame_host_->render_view_host()->GetView()) {
     RenderWidgetHostViewPort::FromRWHV(
         render_frame_host_->render_view_host()->GetView())->Focus();
   }
 
   // Notify that we've swapped RenderFrameHosts. We do this before shutting down
   // the RFH so that we can clean up RendererResources related to the RFH first.
   // TODO(creis): Only do this on top-level RFHs for now, and later update it to
   // pass the RFHs.
   if (is_main_frame) {
     delegate_->NotifySwappedFromRenderManager(
         old_render_frame_host->render_view_host(),
         render_frame_host_->render_view_host());
   }
 
   // If the pending frame was on the swapped out list, we can remove it.

[Charlie Reis, 2014/04/09 21:48:30]

This suggests that the pending can still be on the proxy list, but we've removed
the comment about that in the .h file.  Which is right?

(nit: If we keep this, s/swapped out list/proxy list/)

[nasko, 2014/04/10 20:37:36]

Done.

-  swapped_out_hosts_.erase(render_frame_host_->GetSiteInstance()->GetId());
+  proxy_hosts_.erase(render_frame_host_->GetSiteInstance()->GetId());
 
   if (old_render_frame_host->render_view_host()->IsRenderViewLive()) {

[Charlie Reis, 2014/04/09 21:48:30]

One suggestion to make this block of nested ifs slightly easier to parse-- let's
invert this so that it becomes:
if (!old_render_frame_host->render_view_host()->IsRenderViewLive()) {
  old_render_frame_host.reset();
  return;
}

// If the old...

[nasko, 2014/04/10 20:37:36]

Done.

     // 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.
-    RenderFrameHostMap::iterator iter =
-        swapped_out_hosts_.find(old_site_instance_id);
-    if (iter != swapped_out_hosts_.end() &&
-        iter->second != old_render_frame_host) {
-      // Delete the RFH that will be replaced in the map to avoid a leak.
+    RenderFrameHostMap::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;
     }
+
+    SiteInstanceImpl* old_site_instance = static_cast<SiteInstanceImpl*>(

[Charlie Reis, 2014/04/09 21:48:30]

This makes me nervous.  Deleting old_render_frame_host can cause the
SiteInstance's refcount to drop to 0 and be deleted.  We probably shouldn't keep
it in a pointer, which might encourage others to use it unsafely at the end of
the function.

[nasko, 2014/04/10 20:37:36]

Done.

+        old_render_frame_host->GetSiteInstance());
+
     // 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 swapped out
-    // RenderFrameHosts.
+    // 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) {
-      swapped_out_hosts_.erase(old_site_instance_id);
+      proxy_hosts_.erase(old_site_instance_id);
       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);
+            linked_ptr<RenderFrameHostImpl>(old_render_frame_host.release());
       }
     } else {
-      swapped_out_hosts_[old_site_instance_id] = old_render_frame_host;
-    }
-
-    // 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()) {
-      ShutdownRenderFrameHostsInSiteInstance(old_site_instance_id);
-      // This is deleted while cleaning up the SiteInstance's views.
-      old_render_frame_host = NULL;
+      // 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.

[Charlie Reis, 2014/04/09 21:48:30]

I'm having trouble wrapping my head around this change (lines 1127-1138).  It
seems like a big semantic change, but I can't figure out why it's needed. 
(Also, the last sentence of the comment doesn't seem correct anymore.)

Can you help me understand it?

[nasko, 2014/04/10 20:37:36]

As we discussed, I've moved it around to preserve sane scoped_ptr semantics. The
old_render_frame_host isn't added in the list, just deleted. The shutdown then
will not delete it, since it will never be in the list.

+      if (!old_site_instance->active_view_count()) {
+        old_render_frame_host.reset(NULL);
+        ShutdownRenderFrameHostsInSiteInstance(old_site_instance_id);
+      } else {
+        proxy_hosts_[old_site_instance_id] = new RenderFrameProxyHost(
+            old_render_frame_host.Pass());
+      }
     }
   } else {
-    delete old_render_frame_host;
+    old_render_frame_host.reset(NULL);

[Charlie Reis, 2014/04/09 21:48:30]

The default argument is NULL, so you can just say reset() (here and above).

[nasko, 2014/04/10 20:37:36]

Done.

   }
 }
 
 void RenderFrameHostManager::ShutdownRenderFrameHostsInSiteInstance(
     int32 site_instance_id) {
   // First remove any swapped out RFH for this SiteInstance from our own list.
-  ClearSwappedOutRFHsInSiteInstance(site_instance_id, frame_tree_node_);
+  ClearProxiesInSiteInstance(site_instance_id, frame_tree_node_);
 
   // Use the safe RenderWidgetHost iterator for now to find all RenderViewHosts
   // in the SiteInstance, then tell their respective FrameTrees to remove all
-  // swapped out RenderFrameHosts corresponding to them.
+  // RenderFrameProxyHosts corresponding to them.
   // TODO(creis): Replace this with a RenderFrameHostIterator that protects
   // against use-after-frees if a later element is deleted before getting to it.
   scoped_ptr<RenderWidgetHostIterator> widgets(
       RenderWidgetHostImpl::GetAllRenderWidgetHosts());
   while (RenderWidgetHost* widget = widgets->GetNextHost()) {
     if (!widget->IsRenderView())
       continue;
     RenderViewHostImpl* rvh =
         static_cast<RenderViewHostImpl*>(RenderViewHost::From(widget));
     if (site_instance_id == rvh->GetSiteInstance()->GetId()) {
       // This deletes all RenderFrameHosts using the |rvh|, which then causes
       // |rvh| to Shutdown.
       FrameTree* tree = rvh->GetDelegate()->GetFrameTree();
       tree->ForEach(base::Bind(
-          &RenderFrameHostManager::ClearSwappedOutRFHsInSiteInstance,
+          &RenderFrameHostManager::ClearProxiesInSiteInstance,
           site_instance_id));
     }
   }
 }
 
 RenderFrameHostImpl* RenderFrameHostManager::UpdateRendererStateForNavigate(
     const NavigationEntryImpl& entry) {
   // If we are currently navigating cross-process, we want to get back to normal
   // and then navigate as usual.
   if (cross_navigation_pending_) {
@@ skipping 157 matching lines @@
   RenderFrameHostImpl* pending_render_frame_host =
       pending_render_frame_host_.release();
 
   RenderViewDevToolsAgentHost::OnCancelPendingNavigation(
       pending_render_frame_host->render_view_host(),
       render_frame_host_->render_view_host());
 
   // We no longer need to prevent the process from exiting.
   pending_render_frame_host->GetProcess()->RemovePendingView();
 
-  // The pending RFH may already be on the swapped out list if we started to
-  // swap it back in and then canceled.  If so, make sure it gets swapped out
-  // again.  If it's not on the swapped out list (e.g., aborting a pending
-  // load), then it's safe to shut down.
-  if (IsOnSwappedOutList(pending_render_frame_host)) {
+  // 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()) {

[Charlie Reis, 2014/04/09 21:48:30]

Isn't this backwards?  If the view count is 0, we can delete it.

Otherwise I think I agree that we can use the view count here rather than
checking whether it was swapped out.  (In fact, I think there are cases that's
an improvement, like a new pending navigation to an existing SiteInstance which
then gets canceled.)

[nasko, 2014/04/10 20:37:36]

Done.

     // Any currently suspended navigations are no longer needed.
     pending_render_frame_host->render_view_host()->CancelSuspendedNavigations();
 
     pending_render_frame_host->SwapOut();
   } else {
     // We won't be coming back, so shut this one down.
     delete pending_render_frame_host;
   }
 
   pending_web_ui_.reset();
@@ skipping 13 matching lines @@
     NOTREACHED();
     pending_render_frame_host_.reset();
   }
 
   // Make sure deleted RVHs are not kept in the swapped out list while we are
   // still alive.  (If render_frame_host_ is null, we're already being deleted.)
   if (!render_frame_host_)
     return;
 
   // We can't look it up by SiteInstance ID, which may no longer be valid.
-  for (RenderFrameHostMap::iterator iter = swapped_out_hosts_.begin();
-       iter != swapped_out_hosts_.end();
+  for (RenderFrameHostMap::iterator iter = proxy_hosts_.begin();
+       iter != proxy_hosts_.end();
        ++iter) {
     if (iter->second->render_view_host() == rvh) {
-      swapped_out_hosts_.erase(iter);
+      proxy_hosts_.erase(iter);
       break;
     }
   }
 }
 
 bool RenderFrameHostManager::IsRVHOnSwappedOutList(
     RenderViewHostImpl* rvh) const {
-  RenderFrameHostImpl* render_frame_host = GetSwappedOutRenderFrameHost(
+  RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(
       rvh->GetSiteInstance());
-  if (!render_frame_host)
+  if (!proxy)
     return false;
-  return IsOnSwappedOutList(render_frame_host);
+  return IsOnSwappedOutList(proxy->render_frame_host());
 }
 
 bool RenderFrameHostManager::IsOnSwappedOutList(
     RenderFrameHostImpl* rfh) const {
   if (!rfh->GetSiteInstance())
     return false;
 
-  RenderFrameHostMap::const_iterator iter = swapped_out_hosts_.find(
+  RenderFrameHostMap::const_iterator iter = proxy_hosts_.find(
       rfh->GetSiteInstance()->GetId());
-  if (iter == swapped_out_hosts_.end())
+  if (iter == proxy_hosts_.end())
     return false;
 
-  return iter->second == rfh;
+  return iter->second->render_frame_host() == rfh;
 }
 
 RenderViewHostImpl* RenderFrameHostManager::GetSwappedOutRenderViewHost(
    SiteInstance* instance) const {
-  RenderFrameHostImpl* render_frame_host =
-      GetSwappedOutRenderFrameHost(instance);
-  if (render_frame_host)
-    return render_frame_host->render_view_host();
+  RenderFrameProxyHost* proxy = GetRenderFrameProxyHost(instance);
+  if (proxy)
+    return proxy->render_view_host();
   return NULL;
 }
 
-RenderFrameHostImpl* RenderFrameHostManager::GetSwappedOutRenderFrameHost(
+RenderFrameProxyHost* RenderFrameHostManager::GetRenderFrameProxyHost(
     SiteInstance* instance) const {
   RenderFrameHostMap::const_iterator iter =
-      swapped_out_hosts_.find(instance->GetId());
-  if (iter != swapped_out_hosts_.end())
+      proxy_hosts_.find(instance->GetId());
+  if (iter != proxy_hosts_.end())
     return iter->second;
 
   return NULL;
 }
 
 }  // namespace content