Fix RenderFrameCreated and RenderFrameDeleted WebContentsObserver methods

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..1b2c90dd7924b5cedb33c8710a7763c2ec1cf3e9 100644
--- a/content/public/test/web_contents_observer_sanity_checker.cc
+++ b/content/public/test/web_contents_observer_sanity_checker.cc
@@ -4,7 +4,9 @@
 
 #include "content/public/test/web_contents_observer_sanity_checker.h"
 
+#include "base/debug/stack_trace.h"
 #include "base/strings/stringprintf.h"
+#include "content/common/frame_messages.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/site_instance.h"
@@ -18,6 +20,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 = false;
+
 }  // namespace
 
 // static
@@ -35,17 +41,23 @@ void WebContentsObserverSanityChecker::RenderFrameCreated(
       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);
   }
 }
 
@@ -59,8 +71,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 +88,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 +106,15 @@ 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);
+
+  // TODO(nasko): Implement consistency checking for RenderFrameHostChanged
+  // in follow up CL.
 }
 
 void WebContentsObserverSanityChecker::FrameDeleted(
     RenderFrameHost* render_frame_host) {
-  AssertFrameExists(render_frame_host);
+  AssertRenderFrameExists(render_frame_host);

[Charlie Reis, 2015/02/12 00:29:31]

Interesting.  Does this mean we don't call WCO::FrameDeleted if a subframe
crashes and then we later remove the frame from the frame tree?  In that case,
RenderFrameDeleted would be called at the time of the crash, and this assert
would fail if we later removed the FTN.

[nasko, 2015/02/12 17:52:26]

I think you are right that we don't do the right thing. I think we need explicit
test for it and change of behavior in the sanity checker, though I think it
should go into a separate CL, as this one is big as it is.

[Charlie Reis, 2015/02/12 23:51:25]

Agreed.

 }
 
 void WebContentsObserverSanityChecker::DidStartProvisionalLoadForFrame(
@@ -97,14 +122,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 +137,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 +150,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 +163,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 +177,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,15 +193,21 @@ void WebContentsObserverSanityChecker::DidOpenRequestedURL(
     const Referrer& referrer,
     WindowOpenDisposition disposition,
     ui::PageTransition transition) {
-  AssertFrameExists(source_render_frame_host);
+  AssertRenderFrameExists(source_render_frame_host);
 }
 
 bool WebContentsObserverSanityChecker::OnMessageReceived(
     const IPC::Message& message,
     RenderFrameHost* render_frame_host) {
+  // TODO(nasko): FrameHostMsg_RenderProcessGone is delivered to
+  // WebContentsObserver since RenderFrameHost allows the delegate to handle
+  // the message first. This shouldn't happen, but for now handle it here.

[Charlie Reis, 2015/02/12 00:29:31]

Let's list a bug number here.

Any ideas on what the right behavior is?

[nasko, 2015/02/12 17:52:26]

Done
I don't quite know yet, need to think about it, but probably at least suppress
this specific message, as we already have an WCO method for it.

+  if (message.type() == FrameHostMsg_RenderProcessGone::ID)
+    return false;
+
 #if !defined(OS_MACOSX)
 // TODO(avi): Disabled because of http://crbug.com/445054
-  AssertFrameExists(render_frame_host);
+  AssertRenderFrameExists(render_frame_host);
 #endif
   return false;
 }
@@ -190,20 +221,21 @@ WebContentsObserverSanityChecker::WebContentsObserverSanityChecker(
     WebContents* web_contents)
     : WebContentsObserver(web_contents), web_contents_destroyed_(false) {
   // Prime the pump with the initial objects.
+  // TODO(nasko): Investigate why this is needed.
   RenderViewCreated(web_contents->GetRenderViewHost());
-  RenderFrameCreated(web_contents->GetMainFrame());
 }
 
 WebContentsObserverSanityChecker::~WebContentsObserverSanityChecker() {
   CHECK(web_contents_destroyed_);
 }
 
-void WebContentsObserverSanityChecker::AssertFrameExists(
+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 +250,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());
 }