Fix RenderFrameHost lifetime and clean up CommitPending.

content/browser/frame_host/render_frame_host_manager.h

Index: content/browser/frame_host/render_frame_host_manager.h
diff --git a/content/browser/frame_host/render_frame_host_manager.h b/content/browser/frame_host/render_frame_host_manager.h
index 058f6a0891f4266954a2d5f6894f825d0ffff5b8..e3e802fd84f06276510564e53a607a04db4f5166 100644
--- a/content/browser/frame_host/render_frame_host_manager.h
+++ b/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"
@@ -303,9 +305,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.
@@ -440,9 +445,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 on the pending delete list after this call.

[nasko, 2014/10/02 16:04:02]

nit: s/deleted or on/deleted or put on/?

[Charlie Reis, 2014/10/02 19:41:19]

Done.  Also s/after/during/.

+  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
@@ -525,10 +536,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_;

[nasko, 2014/10/02 16:04:02]

Why not use ScopedPtrVector? It supports exactly the semantics we need
(hosts_.push_back(param.Pass())) and doesn't have the "complexities" of
linked_ptrs.

[Charlie Reis, 2014/10/02 19:41:19]

It's a good thought, but I don't think it helps in this case.  IsPendingDeletion
and DeleteFromPendingList will end up looking the same (since there's no find()
and erase() still takes an iterator).  Plus it's array based, so it will require
resizing on insert/delete (unlike lists) in order to support indexed access
(which we don't need).

I don't expect this list to get huge (at most 50, if you quickly go back through
all your session history items), but we may as well use lists until we can use
some kind of ownership pointer with sets.

 
   // The intersitial page currently shown if any, not own by this class
   // (the InterstitialPage is self-owned, it deletes itself when hidden).