Always call WCO::RenderFrameCreated and WCO::RenderFrameDeleted.

Call WebContentsObserver::RenderFrameCreated for frames created
via RenderFrameHostManager::CreateRenderFrame.

During WebContentsImpl destruction, make sure that RenderFrameDeleted
is always called, by destroying the frame tree and all proxies before
clearing the observer list.

BUG=364974,374491

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@272586 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/frame_host/render_frame_host_manager.cc

@@ -553,6 +553,10 @@ void RenderFrameHostManager::ClearPendingShutdownRFHForSiteInstance(
     pending_delete_hosts_.erase(site_instance_id);
 }
 
+void RenderFrameHostManager::ResetProxyHosts() {
+  STLDeleteValues(&proxy_hosts_);
+}
+
 void RenderFrameHostManager::Observe(
     int type,
     const NotificationSource& source,
@@ -905,6 +909,7 @@ int RenderFrameHostManager::CreateRenderFrame(
   DCHECK(!swapped_out || hidden); // Swapped out views should always be hidden.
 
   scoped_ptr<RenderFrameHostImpl> new_render_frame_host;
+  RenderFrameHostImpl* frame_to_announce = NULL;
   int routing_id = MSG_ROUTING_NONE;
 
   // We are creating a pending or swapped out RFH here.  We should never create
@@ -958,6 +963,7 @@ int RenderFrameHostManager::CreateRenderFrame(
     RenderViewHostImpl* render_view_host =
         new_render_frame_host->render_view_host();
     int proxy_routing_id = MSG_ROUTING_NONE;
+    frame_to_announce = new_render_frame_host.get();
 
     // Prevent the process from exiting while we're trying to navigate in it.
     // Otherwise, if the new RFH is swapped out already, store it.
@@ -987,6 +993,10 @@ int RenderFrameHostManager::CreateRenderFrame(
   if (!swapped_out)
     pending_render_frame_host_ = new_render_frame_host.Pass();
 
+  // If a brand new RFH was created, announce it to observers.
+  if (frame_to_announce)
+    render_frame_delegate_->RenderFrameCreated(frame_to_announce);
+
   return routing_id;
 }
 

content/browser/frame_host/render_frame_host_manager.h

@@ -231,6 +231,7 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   // Helper method to create and initialize a RenderFrameHost.  If |swapped_out|
   // is true, it will be initially placed on the swapped out hosts list.
   // Otherwise, it will be used for a pending cross-site navigation.
+  // Returns the routing id of the *view* associated with the frame.
   int CreateRenderFrame(SiteInstance* instance,
                         int opener_route_id,
                         bool swapped_out,
@@ -281,6 +282,10 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   void ClearPendingShutdownRFHForSiteInstance(int32 site_instance_id,
                                               RenderFrameHostImpl* rfh);
 
+  // Deletes any proxy hosts associated with this node. Used during destruction
+  // of WebContentsImpl.
+  void ResetProxyHosts();
+
  private:
   friend class RenderFrameHostManagerTest;
   friend class TestWebContents;

content/browser/frame_host/render_frame_host_manager_unittest.cc

@@ -124,8 +124,7 @@ class RenderViewHostDeletedObserver : public WebContentsObserver {
   DISALLOW_COPY_AND_ASSIGN(RenderViewHostDeletedObserver);
 };
 
-
-// This observer keeps track of the last deleted RenderViewHost to avoid
+// This observer keeps track of the last deleted RenderFrameHost to avoid
 // accessing it and causing use-after-free condition.
 class RenderFrameHostDeletedObserver : public WebContentsObserver {
  public:
@@ -193,6 +192,60 @@ class PluginFaviconMessageObserver : public WebContentsObserver {
   DISALLOW_COPY_AND_ASSIGN(PluginFaviconMessageObserver);
 };
 
+// Ensures that RenderFrameDeleted and RenderFrameCreated are called in a
+// consistent manner.
+class FrameLifetimeConsistencyChecker : public WebContentsObserver {
+ public:
+  explicit FrameLifetimeConsistencyChecker(WebContentsImpl* web_contents)
+      : WebContentsObserver(web_contents) {
+    RenderViewCreated(web_contents->GetRenderViewHost());
+    RenderFrameCreated(web_contents->GetMainFrame());
+  }
+
+  virtual void RenderFrameCreated(RenderFrameHost* render_frame_host) OVERRIDE {
+    std::pair<int, int> routing_pair =
+        std::make_pair(render_frame_host->GetProcess()->GetID(),
+                       render_frame_host->GetRoutingID());
+    bool was_live_already = !live_routes_.insert(routing_pair).second;
+    bool was_used_before = deleted_routes_.count(routing_pair) != 0;
+
+    if (was_live_already) {
+      FAIL() << "RenderFrameCreated called more than once for routing pair: "
+             << Format(render_frame_host);
+    } else if (was_used_before) {
+      FAIL() << "RenderFrameCreated called for routing pair "
+             << Format(render_frame_host) << " that was previously deleted.";
+    }
+  }
+
+  virtual void RenderFrameDeleted(RenderFrameHost* render_frame_host) OVERRIDE {
+    std::pair<int, int> routing_pair =
+        std::make_pair(render_frame_host->GetProcess()->GetID(),
+                       render_frame_host->GetRoutingID());
+    bool was_live = live_routes_.erase(routing_pair);
+    bool was_dead_already = !deleted_routes_.insert(routing_pair).second;
+
+    if (was_dead_already) {
+      FAIL() << "RenderFrameDeleted called more than once for routing pair "
+             << Format(render_frame_host);
+    } else if (!was_live) {
+      FAIL() << "RenderFrameDeleted called for routing pair "
+             << Format(render_frame_host)
+             << " for which RenderFrameCreated was never called";
+    }
+  }
+
+ private:
+  std::string Format(RenderFrameHost* render_frame_host) {
+    return base::StringPrintf(
+        "(%d, %d -> %s )",
+        render_frame_host->GetProcess()->GetID(),
+        render_frame_host->GetRoutingID(),
+        render_frame_host->GetSiteInstance()->GetSiteURL().spec().c_str());
+  }
+  std::set<std::pair<int, int> > live_routes_;
+  std::set<std::pair<int, int> > deleted_routes_;
+};
 
 }  // namespace
 
@@ -202,9 +255,11 @@ class RenderFrameHostManagerTest
   virtual void SetUp() OVERRIDE {
     RenderViewHostImplTestHarness::SetUp();
     WebUIControllerFactory::RegisterFactory(&factory_);
+    lifetime_checker_.reset(new FrameLifetimeConsistencyChecker(contents()));
   }
 
   virtual void TearDown() OVERRIDE {
+    lifetime_checker_.reset();
     RenderViewHostImplTestHarness::TearDown();
     WebUIControllerFactory::UnregisterFactoryForTesting(&factory_);
   }
@@ -310,6 +365,7 @@ class RenderFrameHostManagerTest
 
  private:
   RenderFrameHostManagerTestWebUIControllerFactory factory_;
+  scoped_ptr<FrameLifetimeConsistencyChecker> lifetime_checker_;
 };
 
 // Tests that when you navigate from a chrome:// url to another page, and

content/browser/web_contents/web_contents_impl.cc

@@ -211,30 +211,12 @@ bool ForEachFrameInternal(
   return true;
 }
 
-bool ForEachPendingFrameInternal(
-    const base::Callback<void(RenderFrameHost*)>& on_frame,
-    FrameTreeNode* node) {
-  RenderFrameHost* pending_frame_host =
-      node->render_manager()->pending_frame_host();
-  if (pending_frame_host)
-    on_frame.Run(pending_frame_host);
-  return true;
-}
-
 void SendToAllFramesInternal(IPC::Message* message, RenderFrameHost* rfh) {
   IPC::Message* message_copy = new IPC::Message(*message);
   message_copy->set_routing_id(rfh->GetRoutingID());
   rfh->Send(message_copy);
 }
 
-void RunRenderFrameDeleted(
-    ObserverList<WebContentsObserver>* observer_list,
-    RenderFrameHost* render_frame_host) {
-  FOR_EACH_OBSERVER(WebContentsObserver,
-                    *observer_list,
-                    RenderFrameDeleted(render_frame_host));
-}
-
 }  // namespace
 
 WebContents* WebContents::Create(const WebContents::CreateParams& params) {
@@ -401,23 +383,30 @@ WebContentsImpl::~WebContentsImpl() {
       Source<WebContents>(this),
       NotificationService::NoDetails());
 
-  base::Callback<void(RenderFrameHost*)> run_render_frame_deleted_callback =
-      base::Bind(&RunRenderFrameDeleted, base::Unretained(&observers_));
-  frame_tree_.ForEach(base::Bind(&ForEachPendingFrameInternal,
-                                 run_render_frame_deleted_callback));
+  // Destroy all frame tree nodes except for the root; this notifies observers.
+  frame_tree_.ResetForMainFrameSwap();
+  GetRenderManager()->ResetProxyHosts();
 
-  RenderViewHost* pending_rvh = GetRenderManager()->pending_render_view_host();
-  if (pending_rvh) {
+  // Manually call the observer methods for the root frame tree node.
+  RenderFrameHostManager* root = GetRenderManager();
+  if (root->pending_frame_host()) {
     FOR_EACH_OBSERVER(WebContentsObserver,
                       observers_,
-                      RenderViewDeleted(pending_rvh));
+                      RenderFrameDeleted(root->pending_frame_host()));
   }
+  FOR_EACH_OBSERVER(WebContentsObserver,
+                    observers_,
+                    RenderFrameDeleted(root->current_frame_host()));
 
-  ForEachFrame(run_render_frame_deleted_callback);
+  if (root->pending_render_view_host()) {
+    FOR_EACH_OBSERVER(WebContentsObserver,
+                      observers_,
+                      RenderViewDeleted(root->pending_render_view_host()));
+  }
 
   FOR_EACH_OBSERVER(WebContentsObserver,
                     observers_,
-                    RenderViewDeleted(GetRenderManager()->current_host()));
+                    RenderViewDeleted(root->current_host()));
 
   FOR_EACH_OBSERVER(WebContentsObserver,
                     observers_,