Fix RenderFrameHost lifetime and clean up CommitPending.

This updates the CommitPending logic now that swap out happens at commit
time, ensuring that the old RFH is kept alive until the SwapOut ACK is
received.

It is now also possible to swap out without creating a proxy.

BUG=407160
TEST=Cross-site iframe is visible in --site-per-process mode.

Review URL: https://codereview.chromium.org/525583002

Cr-Commit-Position: refs/heads/master@{#297902}

content/browser/frame_host/frame_tree.cc

@@ -237,12 +237,13 @@ RenderViewHostImpl* FrameTree::CreateRenderViewHost(SiteInstance* site_instance,
   RenderViewHostMap::iterator iter =
       render_view_host_map_.find(site_instance->GetId());
   if (iter != render_view_host_map_.end()) {
-    // If a RenderViewHost's main frame is pending shutdown for this
+    // If a RenderViewHost's main frame is pending deletion for this
     // |site_instance|, put it in the map of RenderViewHosts pending shutdown.
     // Otherwise return the existing RenderViewHost for the SiteInstance.
-    RenderFrameHost* main_frame = iter->second->GetMainFrame();
-    if (static_cast<RenderFrameHostImpl*>(main_frame)->rfh_state() ==
-        RenderFrameHostImpl::STATE_PENDING_SHUTDOWN) {
+    RenderFrameHostImpl* main_frame = static_cast<RenderFrameHostImpl*>(
+        iter->second->GetMainFrame());
+    if (main_frame->frame_tree_node()->render_manager()->IsPendingDeletion(
+            main_frame)) {
       render_view_host_pending_shutdown_map_.insert(
           std::pair<int, RenderViewHostImpl*>(site_instance->GetId(),
                                               iter->second));

content/browser/frame_host/render_frame_host_impl.cc

@@ -407,6 +407,7 @@ bool RenderFrameHostImpl::OnMessageReceived(const IPC::Message &msg) {
 #endif
   IPC_END_MESSAGE_MAP()
 
+  // No further actions here, since we may have been deleted.
   return handled;
 }
 
@@ -785,17 +786,24 @@ void RenderFrameHostImpl::SwapOut(RenderFrameProxyHost* proxy) {
 
   // If this RenderFrameHost is not in the default state, it must have already
   // gone through this, therefore just return.
-  if (rfh_state_ != RenderFrameHostImpl::STATE_DEFAULT)
+  if (rfh_state_ != RenderFrameHostImpl::STATE_DEFAULT) {
+    NOTREACHED() << "RFH should be in default state when calling SwapOut.";
     return;
+  }
 
   SetState(RenderFrameHostImpl::STATE_PENDING_SWAP_OUT);
   swapout_event_monitor_timeout_->Start(
       base::TimeDelta::FromMilliseconds(RenderViewHostImpl::kUnloadTimeoutMS));
 
-  set_render_frame_proxy_host(proxy);
+  // There may be no proxy if there are no active views in the process.
+  int proxy_routing_id = MSG_ROUTING_NONE;
+  if (proxy) {
+    set_render_frame_proxy_host(proxy);
+    proxy_routing_id = proxy->GetRoutingID();
+  }
 
   if (IsRenderFrameLive())
-    Send(new FrameMsg_SwapOut(routing_id_, proxy->GetRoutingID()));
+    Send(new FrameMsg_SwapOut(routing_id_, proxy_routing_id));
 
   if (!GetParent())
     delegate_->SwappedOut(this);
@@ -879,7 +887,6 @@ void RenderFrameHostImpl::OnBeforeUnloadACK(
 
 bool RenderFrameHostImpl::IsWaitingForUnloadACK() const {
   return render_view_host_->is_waiting_for_close_ack_ ||
-      rfh_state_ == STATE_PENDING_SHUTDOWN ||
       rfh_state_ == STATE_PENDING_SWAP_OUT;
 }
 
@@ -889,24 +896,19 @@ void RenderFrameHostImpl::OnSwapOutACK() {
 
 void RenderFrameHostImpl::OnSwappedOut() {
   // Ignore spurious swap out ack.
-  if (rfh_state_ != STATE_PENDING_SWAP_OUT &&
-      rfh_state_ != STATE_PENDING_SHUTDOWN)
+  if (rfh_state_ != STATE_PENDING_SWAP_OUT)
     return;
 
   TRACE_EVENT_ASYNC_END0("navigation", "RenderFrameHostImpl::SwapOut", this);
   swapout_event_monitor_timeout_->Stop();
 
-  switch (rfh_state_) {
-    case STATE_PENDING_SWAP_OUT:
-      SetState(STATE_SWAPPED_OUT);
-      break;
-    case STATE_PENDING_SHUTDOWN:
-      DCHECK(!pending_shutdown_on_swap_out_.is_null());
-      pending_shutdown_on_swap_out_.Run();
-      break;
-    default:
-      NOTREACHED();
+  if (frame_tree_node_->render_manager()->DeleteFromPendingList(this)) {
+    // We are now deleted.
+    return;
   }
+
+  // If this RFH wasn't pending deletion, then it is now swapped out.
+  SetState(RenderFrameHostImpl::STATE_SWAPPED_OUT);
 }
 
 void RenderFrameHostImpl::OnContextMenu(const ContextMenuParams& params) {
@@ -1179,6 +1181,10 @@ void RenderFrameHostImpl::OnHidePopup() {
 #endif
 
 void RenderFrameHostImpl::SetState(RenderFrameHostImplState rfh_state) {
+  // Only main frames should be swapped out and retained inside a proxy host.
+  if (rfh_state == STATE_SWAPPED_OUT)
+    CHECK(!GetParent());
+
   // We update the number of RenderFrameHosts in a SiteInstance when the swapped
   // out status of a RenderFrameHost gets flipped to/from active.
   if (!IsRFHStateActive(rfh_state_) && IsRFHStateActive(rfh_state))
@@ -1208,11 +1214,6 @@ void RenderFrameHostImpl::SetState(RenderFrameHostImplState rfh_state) {
   rfh_state_ = rfh_state;
 }
 
-void RenderFrameHostImpl::SetPendingShutdown(const base::Closure& on_swap_out) {
-  pending_shutdown_on_swap_out_ = on_swap_out;
-  SetState(STATE_PENDING_SHUTDOWN);
-}
-
 bool RenderFrameHostImpl::CanCommitURL(const GURL& url) {
   // TODO(creis): We should also check for WebUI pages here.  Also, when the
   // out-of-process iframes implementation is ready, we should check for

content/browser/frame_host/render_frame_host_impl.h

@@ -75,17 +75,13 @@ class CONTENT_EXPORT RenderFrameHostImpl
     // RenderFrame.
     STATE_DEFAULT = 0,
     // The RFH has not received the SwapOutACK yet, but the new page has
-    // committed in a different RFH. The number of active frames of the RFH
-    // SiteInstanceImpl is not zero. Upon reception of the SwapOutACK, the RFH
-    // will be swapped out.
+    // committed in a different RFH.  Upon reception of the SwapOutACK, the RFH
+    // will either enter STATE_SWAPPED_OUT (if it is a main frame and there are
+    // other active frames in its SiteInstance) or it will be deleted.
     STATE_PENDING_SWAP_OUT,
-    // The RFH has not received the SwapOutACK yet, but the new page has
-    // committed in a different RFH. The number of active frames of the RFH
-    // SiteInstanceImpl is zero. Upon reception of the SwapOutACK, the RFH will
-    // be shutdown.
-    STATE_PENDING_SHUTDOWN,
     // The RFH is swapped out and stored inside a RenderFrameProxyHost, being
-    // used as a placeholder to allow cross-process communication.
+    // used as a placeholder to allow cross-process communication.  Only main
+    // frames can enter this state.
     STATE_SWAPPED_OUT,
   };
   // Helper function to determine whether the RFH state should contribute to the
@@ -214,11 +210,12 @@ class CONTENT_EXPORT RenderFrameHostImpl
       const TransitionLayerData& transition_data);
 
   // Tells the renderer that this RenderFrame is being swapped out for one in a
-  // different renderer process.  It should run its unload handler, move to
-  // a blank document and create a RenderFrameProxy to replace the RenderFrame.
-  // The renderer should preserve the Proxy object until it exits, in case we
-  // come back.  The renderer can exit if it has no other active RenderFrames,
-  // but not until WasSwappedOut is called (when it is no longer visible).
+  // different renderer process.  It should run its unload handler and move to
+  // a blank document.  If |proxy| is not null, it should also create a
+  // RenderFrameProxy to replace the RenderFrame. The renderer should preserve
+  // the RenderFrameProxy object until it exits, in case we come back.  The
+  // renderer can exit if it has no other active RenderFrames, but not until
+  // WasSwappedOut is called.
   void SwapOut(RenderFrameProxyHost* proxy);
 
   bool is_waiting_for_beforeunload_ack() const {
@@ -239,10 +236,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // The current state of this RFH.
   RenderFrameHostImplState rfh_state() const { return rfh_state_; }
 
-  // Set |this| as pending shutdown. |on_swap_out| will be called
-  // when the SwapOutACK is received, or when the unload timer times out.
-  void SetPendingShutdown(const base::Closure& on_swap_out);
-
   // Sends the given navigation message. Use this rather than sending it
   // yourself since this does the internal bookkeeping described below. This
   // function takes ownership of the provided message pointer.
@@ -552,10 +545,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // SiteInstance.
   scoped_ptr<TimeoutMonitor> swapout_event_monitor_timeout_;
 
-  // Called after receiving the SwapOutACK when the RFH is in the pending
-  // shutdown state. Also called if the unload timer times out.
-  base::Closure pending_shutdown_on_swap_out_;
-
   ServiceRegistryImpl service_registry_;
 
   scoped_ptr<BrowserAccessibilityManager> browser_accessibility_manager_;

content/browser/frame_host/render_frame_host_manager.h

@@ -5,6 +5,8 @@
 #ifndef CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_HOST_MANAGER_H_
 #define CONTENT_BROWSER_FRAME_HOST_RENDER_FRAME_HOST_MANAGER_H_
 
+#include <list>
+
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
@@ -298,9 +300,12 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   RenderFrameProxyHost* GetRenderFrameProxyHost(
       SiteInstance* instance) const;
 
-  // Deletes a RenderFrameHost that was pending shutdown.
-  void ClearPendingShutdownRFHForSiteInstance(int32 site_instance_id,
-                                              RenderFrameHostImpl* rfh);
+  // Returns whether |render_frame_host| is on the pending deletion list.
+  bool IsPendingDeletion(RenderFrameHostImpl* render_frame_host);
+
+  // If |render_frame_host| is on the pending deletion list, this deletes it.
+  // Returns whether it was deleted.
+  bool DeleteFromPendingList(RenderFrameHostImpl* render_frame_host);
 
   // Deletes any proxy hosts associated with this node. Used during destruction
   // of WebContentsImpl.
@@ -416,9 +421,15 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   // since there could be Web UI switching as well. Call this for every commit.
   void CommitPending();
 
-  // Runs the unload handler in the current page, after the new page has
-  // committed.
-  void SwapOutOldPage(RenderFrameHostImpl* old_render_frame_host);
+  // Runs the unload handler in the old RenderFrameHost, after the new
+  // RenderFrameHost has committed.  |old_render_frame_host| will either be
+  // deleted or put on the pending delete list during this call.
+  void SwapOutOldFrame(scoped_ptr<RenderFrameHostImpl> old_render_frame_host);
+
+  // Holds |render_frame_host| until it can be deleted when its swap out ACK
+  // arrives.
+  void MoveToPendingDeleteHosts(
+      scoped_ptr<RenderFrameHostImpl> render_frame_host);
 
   // Shutdown all RenderFrameProxyHosts in a SiteInstance. This is called to
   // shutdown frames when all the frames in a SiteInstance are confirmed to be
@@ -507,10 +518,11 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   typedef base::hash_map<int32, RenderFrameProxyHost*> RenderFrameProxyHostMap;
   RenderFrameProxyHostMap proxy_hosts_;
 
-  // A map of RenderFrameHosts pending shutdown.
-  typedef base::hash_map<int32, linked_ptr<RenderFrameHostImpl> >
-      RFHPendingDeleteMap;
-  RFHPendingDeleteMap pending_delete_hosts_;
+  // A list of RenderFrameHosts waiting to shut down after swapping out.  We use
+  // a linked list since we expect frequent deletes and no indexed access, and
+  // because sets don't appear to support linked_ptrs.
+  typedef std::list<linked_ptr<RenderFrameHostImpl> > RFHPendingDeleteList;
+  RFHPendingDeleteList pending_delete_hosts_;
 
   // The intersitial page currently shown if any, not own by this class
   // (the InterstitialPage is self-owned, it deletes itself when hidden).

content/browser/frame_host/render_frame_host_manager_unittest.cc

@@ -307,13 +307,13 @@ class RenderFrameHostManagerTest
     // Make sure that we start to run the unload handler at the time of commit.
     bool expecting_rfh_shutdown = false;
     if (old_rfh != active_rfh && !rfh_observer.deleted()) {
+      EXPECT_EQ(RenderFrameHostImpl::STATE_PENDING_SWAP_OUT,
+                old_rfh->rfh_state());
       if (!old_rfh->GetSiteInstance()->active_frame_count()) {
         expecting_rfh_shutdown = true;
-        EXPECT_EQ(RenderFrameHostImpl::STATE_PENDING_SHUTDOWN,
-                  old_rfh->rfh_state());
-      } else {
-        EXPECT_EQ(RenderFrameHostImpl::STATE_PENDING_SWAP_OUT,
-                  old_rfh->rfh_state());
+        EXPECT_TRUE(
+            old_rfh->frame_tree_node()->render_manager()->IsPendingDeletion(
+                old_rfh));
       }
     }
 
@@ -1549,7 +1549,9 @@ TEST_F(RenderFrameHostManagerTest, DeleteFrameAfterSwapOutACK) {
   EXPECT_EQ(rfh2, contents()->GetMainFrame());
   EXPECT_TRUE(contents()->GetPendingMainFrame() == NULL);
   EXPECT_EQ(RenderFrameHostImpl::STATE_DEFAULT, rfh2->rfh_state());
-  EXPECT_EQ(RenderFrameHostImpl::STATE_PENDING_SHUTDOWN, rfh1->rfh_state());
+  EXPECT_EQ(RenderFrameHostImpl::STATE_PENDING_SWAP_OUT, rfh1->rfh_state());
+  EXPECT_TRUE(
+      rfh1->frame_tree_node()->render_manager()->IsPendingDeletion(rfh1));
 
   // Simulate the swap out ack.
   rfh1->OnSwappedOut();

content/renderer/render_frame_impl.cc

@@ -1051,12 +1051,15 @@ void RenderFrameImpl::OnSwapOut(int proxy_routing_id) {
     // clearing the page.  We also allow this process to exit if there are no
     // other active RenderFrames in it.
 
-    // Send an UpdateState message before we get swapped out. Create the
-    // RenderFrameProxy as well so its routing id is registered for receiving
-    // IPC messages.
+    // Send an UpdateState message before we get swapped out.
     render_view_->SyncNavigationState();
-    proxy = RenderFrameProxy::CreateProxyToReplaceFrame(this,
-                                                        proxy_routing_id);
+
+    // If we need a proxy to replace this, create it now so its routing id is
+    // registered for receiving IPC messages.
+    if (proxy_routing_id != MSG_ROUTING_NONE) {
+      proxy = RenderFrameProxy::CreateProxyToReplaceFrame(this,
+                                                          proxy_routing_id);
+    }
 
     // Synchronously run the unload handler before sending the ACK.
     // TODO(creis): Call dispatchUnloadEvent unconditionally here to support