Fix WCO::RenderFrameCreated and WCO::RenderFrameDeleted notifications.

content/public/test/web_contents_observer_sanity_checker.cc

Index: content/public/test/web_contents_observer_sanity_checker.cc
diff --git a/content/public/test/web_contents_observer_sanity_checker.cc b/content/public/test/web_contents_observer_sanity_checker.cc
index 554992a9716d3ecdfda82103740401d18145759e..a2eccb22b61d94b1bde9c5c6df4195fdc8358ce6 100644
--- a/content/public/test/web_contents_observer_sanity_checker.cc
+++ b/content/public/test/web_contents_observer_sanity_checker.cc
@@ -4,6 +4,7 @@
 
 #include "content/public/test/web_contents_observer_sanity_checker.h"
 
+#include "base/debug/stack_trace.h"
 #include "base/strings/stringprintf.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
@@ -18,6 +19,10 @@ namespace {
 const char kWebContentsObserverSanityCheckerKey[] =
     "WebContentsObserverSanityChecker";
 
+// Set this variable to true to collect stack traces on each RenderFrameCreated
+// and RenderFrameDeleted call, so duplicate calls can be easily debugged.
+static bool g_collect_stack_traces = true;
+
 }  // namespace
 
 // static
@@ -30,27 +35,37 @@ void WebContentsObserverSanityChecker::Enable(WebContents* web_contents) {
 
 void WebContentsObserverSanityChecker::RenderFrameCreated(
     RenderFrameHost* render_frame_host) {
+  LOG(ERROR) << "Checker[" << this << "]::RenderFrameCreated: " << Format(render_frame_host);
+
   CHECK(!web_contents_destroyed_);
   std::pair<int, int> routing_pair =
       std::make_pair(render_frame_host->GetProcess()->GetID(),
                      render_frame_host->GetRoutingID());
   bool frame_exists = !live_routes_.insert(routing_pair).second;
-  bool dead_already = deleted_routes_.count(routing_pair) != 0;
+  deleted_routes_.erase(routing_pair);
 
   if (frame_exists) {
-// TODO(nick): Disabled because of http://crbug.com/425397
-#if 0
+    std::string trace;
+    if (g_collect_stack_traces) {
+      trace = "\nPrevious creation stack trace:\n" +
+          render_frame_created_stacks_[routing_pair];
+    }
     CHECK(false) << "RenderFrameCreated called more than once for routing pair:"
-                 << Format(render_frame_host);
-#endif
-  } else if (dead_already) {
-    CHECK(false) << "RenderFrameCreated called for routing pair that was "
-                 << "previously deleted: " << Format(render_frame_host);
+                 << Format(render_frame_host) << trace;
+  }
+
+  if (g_collect_stack_traces) {
+    base::debug::StackTrace stack;
+    render_frame_created_stacks_.insert(
+        std::make_pair(routing_pair, stack.ToString()));
+    render_frame_deleted_stacks_.erase(routing_pair);
   }
 }
 
 void WebContentsObserverSanityChecker::RenderFrameDeleted(
     RenderFrameHost* render_frame_host) {
+  LOG(ERROR) << "Checker[" << this << "]::RenderFrameDeleted: " << Format(render_frame_host);
+
   CHECK(!web_contents_destroyed_);
   std::pair<int, int> routing_pair =
       std::make_pair(render_frame_host->GetProcess()->GetID(),
@@ -59,8 +74,13 @@ void WebContentsObserverSanityChecker::RenderFrameDeleted(
   bool was_dead_already = !deleted_routes_.insert(routing_pair).second;
 
   if (was_dead_already) {
+    std::string trace;
+    if (g_collect_stack_traces) {
+      trace = "\nPrevious deletion stack trace:\n" +
+          render_frame_deleted_stacks_[routing_pair];
+    }
     CHECK(false) << "RenderFrameDeleted called more than once for routing pair "
-                 << Format(render_frame_host);
+                 << Format(render_frame_host) << trace;
   } else if (!was_live) {
 // TODO(nick): Clients can easily ignore an unrecognized object, but it
 // would be useful from a finding-bugs perspective if we could enable this
@@ -71,6 +91,13 @@ void WebContentsObserverSanityChecker::RenderFrameDeleted(
                  << " for which RenderFrameCreated was never called";
 #endif
   }
+
+  if (g_collect_stack_traces) {
+    base::debug::StackTrace stack;
+    render_frame_deleted_stacks_.insert(
+        std::make_pair(routing_pair, stack.ToString()));
+    render_frame_created_stacks_.erase(routing_pair);
+  }
 }
 
 void WebContentsObserverSanityChecker::RenderFrameForInterstitialPageCreated(
@@ -82,14 +109,42 @@ void WebContentsObserverSanityChecker::RenderFrameHostChanged(
     RenderFrameHost* old_host,
     RenderFrameHost* new_host) {
   CHECK(new_host);
-  if (old_host)
-    AssertFrameExists(old_host);
-  AssertFrameExists(new_host);
+  CHECK_NE(new_host, old_host);
+
+  if (old_host) {
+    LOG(ERROR) << "Checker[" << this << "]::RenderFrameHostChanged: old:" << Format(old_host);
+
+    AssertRenderFrameHostExists(old_host);
+    std::pair<int, int> routing_pair =
+        std::make_pair(old_host->GetProcess()->GetID(),
+                       old_host->GetRoutingID());
+    bool old_did_exist = !!current_hosts_.erase(routing_pair);
+    if (!old_did_exist) {
+      CHECK(false)
+          << "RenderFrameHostChanged called with old host that did not exist:"
+          << Format(old_host);
+    }
+  } else {
+    LOG(ERROR) << "Checker[" << this << "]::RenderFrameHostChanged: old: null";
+  }
+
+  LOG(ERROR) << "Checker[" << this << "]::RenderFrameHostChanged: new:" << Format(new_host);
+
+  std::pair<int, int> routing_pair =
+      std::make_pair(new_host->GetProcess()->GetID(),
+                     new_host->GetRoutingID());
+  bool host_exists = !current_hosts_.insert(routing_pair).second;
+  if (host_exists) {
+    CHECK(false)
+        << "RenderFrameHostChanged called more than once for routing pair:"
+        << Format(new_host);
+  }
 }
 
 void WebContentsObserverSanityChecker::FrameDeleted(
     RenderFrameHost* render_frame_host) {
-  AssertFrameExists(render_frame_host);
+  LOG(ERROR) << "Checker[" << this << "]::FrameDeleted: " << Format(render_frame_host);
+  AssertRenderFrameExists(render_frame_host);
 }
 
 void WebContentsObserverSanityChecker::DidStartProvisionalLoadForFrame(
@@ -97,14 +152,14 @@ void WebContentsObserverSanityChecker::DidStartProvisionalLoadForFrame(
     const GURL& validated_url,
     bool is_error_page,
     bool is_iframe_srcdoc) {
-  AssertFrameExists(render_frame_host);
+  AssertRenderFrameExists(render_frame_host);
 }
 
 void WebContentsObserverSanityChecker::DidCommitProvisionalLoadForFrame(
     RenderFrameHost* render_frame_host,
     const GURL& url,
     ui::PageTransition transition_type) {
-  AssertFrameExists(render_frame_host);
+  AssertRenderFrameExists(render_frame_host);
 }
 
 void WebContentsObserverSanityChecker::DidFailProvisionalLoad(
@@ -112,7 +167,7 @@ void WebContentsObserverSanityChecker::DidFailProvisionalLoad(
     const GURL& validated_url,
     int error_code,
     const base::string16& error_description) {
-  AssertFrameExists(render_frame_host);
+  AssertRenderFrameExists(render_frame_host);
 }
 
 void WebContentsObserverSanityChecker::DidNavigateMainFrame(
@@ -125,7 +180,7 @@ void WebContentsObserverSanityChecker::DidNavigateAnyFrame(
     RenderFrameHost* render_frame_host,
     const LoadCommittedDetails& details,
     const FrameNavigateParams& params) {
-  AssertFrameExists(render_frame_host);
+  AssertRenderFrameExists(render_frame_host);
 }
 
 void WebContentsObserverSanityChecker::DocumentAvailableInMainFrame() {
@@ -138,13 +193,13 @@ void WebContentsObserverSanityChecker::DocumentOnLoadCompletedInMainFrame() {
 
 void WebContentsObserverSanityChecker::DocumentLoadedInFrame(
     RenderFrameHost* render_frame_host) {
-  AssertFrameExists(render_frame_host);
+  AssertRenderFrameExists(render_frame_host);
 }
 
 void WebContentsObserverSanityChecker::DidFinishLoad(
     RenderFrameHost* render_frame_host,
     const GURL& validated_url) {
-  AssertFrameExists(render_frame_host);
+  AssertRenderFrameExists(render_frame_host);
 }
 
 void WebContentsObserverSanityChecker::DidFailLoad(
@@ -152,13 +207,13 @@ void WebContentsObserverSanityChecker::DidFailLoad(
     const GURL& validated_url,
     int error_code,
     const base::string16& error_description) {
-  AssertFrameExists(render_frame_host);
+  AssertRenderFrameExists(render_frame_host);
 }
 
 void WebContentsObserverSanityChecker::DidGetRedirectForResourceRequest(
     RenderFrameHost* render_frame_host,
     const ResourceRedirectDetails& details) {
-  AssertFrameExists(render_frame_host);
+  AssertRenderFrameExists(render_frame_host);
 }
 
 void WebContentsObserverSanityChecker::DidOpenRequestedURL(
@@ -168,7 +223,7 @@ void WebContentsObserverSanityChecker::DidOpenRequestedURL(
     const Referrer& referrer,
     WindowOpenDisposition disposition,
     ui::PageTransition transition) {
-  AssertFrameExists(source_render_frame_host);
+  AssertRenderFrameExists(source_render_frame_host);
 }
 
 bool WebContentsObserverSanityChecker::OnMessageReceived(
@@ -176,7 +231,7 @@ bool WebContentsObserverSanityChecker::OnMessageReceived(
     RenderFrameHost* render_frame_host) {
 #if !defined(OS_MACOSX)
 // TODO(avi): Disabled because of http://crbug.com/445054
-  AssertFrameExists(render_frame_host);
+  AssertRenderFrameExists(render_frame_host);
 #endif
   return false;
 }
@@ -189,21 +244,44 @@ void WebContentsObserverSanityChecker::WebContentsDestroyed() {
 WebContentsObserverSanityChecker::WebContentsObserverSanityChecker(
     WebContents* web_contents)
     : WebContentsObserver(web_contents), web_contents_destroyed_(false) {
+  LOG(ERROR) << "SanityChecker[" << this << "]::ctor";
+
   // Prime the pump with the initial objects.
   RenderViewCreated(web_contents->GetRenderViewHost());
-  RenderFrameCreated(web_contents->GetMainFrame());
 }
 
 WebContentsObserverSanityChecker::~WebContentsObserverSanityChecker() {
+  LOG(ERROR) << "SanityChecker[" << this << "]::dtor";
   CHECK(web_contents_destroyed_);
 }
 
-void WebContentsObserverSanityChecker::AssertFrameExists(
+void WebContentsObserverSanityChecker::AssertRenderFrameHostExists(
     RenderFrameHost* render_frame_host) {
   CHECK(!web_contents_destroyed_);
   std::pair<int, int> routing_pair =
       std::make_pair(render_frame_host->GetProcess()->GetID(),
                      render_frame_host->GetRoutingID());
+
+  bool host_changed_happened = current_hosts_.count(routing_pair) != 0;
+  bool host_deleted_happened = deleted_hosts_.count(routing_pair) != 0;
+
+  CHECK(host_changed_happened)
+      << "A RenderFrameHost pointer was passed to a WebContentsObserver "
+      << "method, but WebContentsObserver::RenderFrameHostChanged was never "
+      << "called for that RenderFrameHost: " << Format(render_frame_host);
+  CHECK(!host_deleted_happened)
+      << "A RenderFrameHost pointer was passed to a WebContentsObserver "
+      << "method, but WebContentsObserver::FrameDeleted was never "
+      << "called for that RenderFrameHost: " << Format(render_frame_host);
+}
+
+void WebContentsObserverSanityChecker::AssertRenderFrameExists(
+    RenderFrameHost* render_frame_host) {
+  CHECK(!web_contents_destroyed_);
+  std::pair<int, int> routing_pair =
+      std::make_pair(render_frame_host->GetProcess()->GetID(),
+                     render_frame_host->GetRoutingID());
+
   bool render_frame_created_happened = live_routes_.count(routing_pair) != 0;
   bool render_frame_deleted_happened = deleted_routes_.count(routing_pair) != 0;
 
@@ -218,13 +296,13 @@ void WebContentsObserverSanityChecker::AssertFrameExists(
 }
 
 void WebContentsObserverSanityChecker::AssertMainFrameExists() {
-  AssertFrameExists(web_contents()->GetMainFrame());
+  AssertRenderFrameExists(web_contents()->GetMainFrame());
 }
 
 std::string WebContentsObserverSanityChecker::Format(
     RenderFrameHost* render_frame_host) {
   return base::StringPrintf(
-      "(%d, %d -> %s )", render_frame_host->GetProcess()->GetID(),
+      "(%d, %d -> %s)", render_frame_host->GetProcess()->GetID(),
       render_frame_host->GetRoutingID(),
       render_frame_host->GetSiteInstance()->GetSiteURL().spec().c_str());
 }