Ensure the new navigation classifier works in the real world.

BUG=369661
TEST=No whammies!

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

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

chrome/common/crash_keys.cc

@@ -195,6 +195,19 @@ size_t RegisterChromeCrashKeys() {
 #endif
     { kBug464926CrashKey, kSmallSize },
     { kViewCount, kSmallSize },
+    // Temporary for http://crbug.com/369661.
+    { "369661-navurl", kLargeSize },
+    { "369661-oldtype", kSmallSize },
+    { "369661-newtype", kSmallSize },
+    { "369661-naventryid", kSmallSize },
+    { "369661-oldignore", kSmallSize },
+    { "369661-newignore", kSmallSize },
+    { "369661-didcreatenew", kSmallSize },
+    { "369661-pageid", kSmallSize },
+    { "369661-maxpageid", kSmallSize },
+    { "369661-earlyreturn", kSmallSize },
+    { "369661-doubleignore", kSmallSize },
+    // End http://crbug.com/369661.
   };
 
   // This dynamic set of keys is used for sets of key value pairs when gathering

content/browser/frame_host/navigation_controller_impl.cc

@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/debug/crash_logging.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"  // Temporary
@@ -792,6 +793,11 @@ bool NavigationControllerImpl::RendererDidNavigate(
   // Do navigation-type specific actions. These will make and commit an entry.
   details->type = ClassifyNavigation(rfh, params);
   NavigationType new_type = ClassifyNavigationWithoutPageID(rfh, params);
+  if (details->type == NAVIGATION_TYPE_NAV_IGNORE &&
+      new_type == NAVIGATION_TYPE_NAV_IGNORE) {
+    base::debug::SetCrashKeyValue("369661-doubleignore",
+                                  rfh->CommitCountString());
+  }
   bool ignore_mismatch = false;
   // There are disagreements on some Android bots over SAME_PAGE between the two
   // classifiers so ignore disagreements if that's the case.
@@ -816,7 +822,22 @@ bool NavigationControllerImpl::RendererDidNavigate(
     ignore_mismatch = true;
   }
   if (!ignore_mismatch) {
-    DCHECK_EQ(details->type, new_type);
+    base::debug::SetCrashKeyValue("369661-oldtype",
+                                  base::IntToString(details->type));
+    base::debug::SetCrashKeyValue("369661-newtype",
+                                  base::IntToString(new_type));
+    base::debug::SetCrashKeyValue("369661-navurl", params.url.spec());
+    base::debug::SetCrashKeyValue("369661-naventryid",
+                                  base::IntToString(params.nav_entry_id));
+    base::debug::SetCrashKeyValue("369661-didcreatenew",
+                                  params.did_create_new_entry ? "yes" : "no");
+    base::debug::SetCrashKeyValue("369661-pageid",
+                                  base::IntToString(params.page_id));
+    base::debug::SetCrashKeyValue(
+        "369661-maxpageid",
+        base::IntToString(delegate_->GetMaxPageIDForSiteInstance(
+            rfh->GetSiteInstance())));
+    CHECK_EQ(details->type, new_type);
   }
 
   // is_in_page must be computed before the entry gets committed.
@@ -936,6 +957,9 @@ NavigationType NavigationControllerImpl::ClassifyNavigation(
     //   list.
     //
     // In these cases, there's nothing we can do with them, so ignore.
+    base::debug::SetCrashKeyValue("369661-oldignore",
+                                  rfh->CommitCountString() +
+                                  " no page id");
     return NAVIGATION_TYPE_NAV_IGNORE;
   }
 
@@ -952,8 +976,12 @@ NavigationType NavigationControllerImpl::ClassifyNavigation(
     // navigated on a popup navigated to about:blank (the iframe would be
     // written into the popup by script on the main page). For these cases,
     // there isn't any navigation stuff we can do, so just ignore it.
-    if (!GetLastCommittedEntry())
+    if (!GetLastCommittedEntry()) {
+      base::debug::SetCrashKeyValue("369661-oldignore",
+                                    rfh->CommitCountString() +
+                                    " new subframe no last committed");
       return NAVIGATION_TYPE_NAV_IGNORE;
+    }
 
     // Valid subframe navigation.
     return NAVIGATION_TYPE_NEW_SUBFRAME;
@@ -1004,6 +1032,9 @@ NavigationType NavigationControllerImpl::ClassifyNavigation(
     }
     GURL url(temp);
     rfh->render_view_host()->Send(new ViewMsg_TempCrashWithData(url));
+    base::debug::SetCrashKeyValue("369661-oldignore",
+                                  rfh->CommitCountString() +
+                                  " renderer smoking crack");
     return NAVIGATION_TYPE_NAV_IGNORE;
   }
   NavigationEntryImpl* existing_entry = entries_[existing_entry_index].get();
@@ -1076,8 +1107,12 @@ NavigationType NavigationControllerImpl::ClassifyNavigationWithoutPageID(
     // navigated on a popup navigated to about:blank (the iframe would be
     // written into the popup by script on the main page). For these cases,
     // there isn't any navigation stuff we can do, so just ignore it.
-    if (!GetLastCommittedEntry())
+    if (!GetLastCommittedEntry()) {
+      base::debug::SetCrashKeyValue("369661-newignore",
+                                    rfh->CommitCountString() +
+                                    " new subframe no last committed");
       return NAVIGATION_TYPE_NAV_IGNORE;
+    }
 
     // Valid subframe navigation.
     return NAVIGATION_TYPE_NEW_SUBFRAME;
@@ -1094,6 +1129,9 @@ NavigationType NavigationControllerImpl::ClassifyNavigationWithoutPageID(
     } else {
       // We ignore subframes created in non-committed pages; we'd appreciate if
       // people stopped doing that.
+      base::debug::SetCrashKeyValue("369661-newignore",
+                                    rfh->CommitCountString() +
+                                    " auto subframe no last committed");
       return NAVIGATION_TYPE_NAV_IGNORE;
     }
   }
@@ -1106,8 +1144,12 @@ NavigationType NavigationControllerImpl::ClassifyNavigationWithoutPageID(
     // scribble onto an uncommitted page. Again, there isn't any navigation
     // stuff that we can do, so ignore it here as well.
     NavigationEntry* last_committed = GetLastCommittedEntry();
-    if (!last_committed)
+    if (!last_committed) {
+      base::debug::SetCrashKeyValue("369661-newignore",
+                                    rfh->CommitCountString() +
+                                    " renderer-initiated no last committed");
       return NAVIGATION_TYPE_NAV_IGNORE;
+    }
 
     if (IsURLInPageNavigation(params.url, params.was_within_same_page, rfh)) {
       // This is history.replaceState(), which is renderer-initiated yet within
@@ -1155,6 +1197,9 @@ NavigationType NavigationControllerImpl::ClassifyNavigationWithoutPageID(
     // to such entries). It could also mean that the renderer is smoking crack.
     // TODO(avi): Crash the renderer like we do in the old ClassifyNavigation?
     NOTREACHED() << "Could not find nav entry with id " << params.nav_entry_id;
+    base::debug::SetCrashKeyValue("369661-newignore",
+                                  rfh->CommitCountString() +
+                                  " renderer smoking crack");
     return NAVIGATION_TYPE_NAV_IGNORE;
   }
 

content/browser/frame_host/render_frame_host_impl.cc

@@ -788,11 +788,15 @@ void RenderFrameHostImpl::OnDidFailLoadWithError(
 void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) {
   // Read the parameters out of the IPC message directly to avoid making another
   // copy when we filter the URLs.
+  ++commit_count_;
   base::PickleIterator iter(msg);
   FrameHostMsg_DidCommitProvisionalLoad_Params validated_params;
   if (!IPC::ParamTraits<FrameHostMsg_DidCommitProvisionalLoad_Params>::
-      Read(&msg, &iter, &validated_params))
+      Read(&msg, &iter, &validated_params)) {
+    base::debug::SetCrashKeyValue("369661-earlyreturn",
+                                  CommitCountString() + "/badipc");
     return;
+  }
   TRACE_EVENT1("navigation", "RenderFrameHostImpl::OnDidCommitProvisionalLoad",
                "url", validated_params.url.possibly_invalid_spec());
 
@@ -811,6 +815,8 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) {
       !GetParent()) {
     base::TimeTicks approx_renderer_start_time = send_before_unload_start_time_;
     OnBeforeUnloadACK(true, approx_renderer_start_time, base::TimeTicks::Now());
+    base::debug::SetCrashKeyValue("369661-earlyreturn",
+                                  CommitCountString() + "/beforeunloadwait");
     return;
   }
 
@@ -819,8 +825,11 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) {
   // unload request.  It will either respond to the unload request soon or our
   // timer will expire.  Either way, we should ignore this message, because we
   // have already committed to closing this renderer.
-  if (IsWaitingForUnloadACK())
+  if (IsWaitingForUnloadACK()) {
+    base::debug::SetCrashKeyValue("369661-earlyreturn",
+                                  CommitCountString() + "/unloadwait");
     return;
+  }
 
   if (validated_params.report_type ==
       FrameMsg_UILoadMetricsReportType::REPORT_LINK) {
@@ -872,6 +881,8 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) {
           validated_params.page_state)) {
     bad_message::ReceivedBadMessage(
         GetProcess(), bad_message::RFH_CAN_ACCESS_FILES_OF_PAGE_STATE);
+    base::debug::SetCrashKeyValue("369661-earlyreturn",
+                                  CommitCountString() + "/fileaccess");
     return;
   }
 
@@ -2088,4 +2099,11 @@ void RenderFrameHostImpl::UpdatePermissionsForNavigation(
   }
 }
 
+std::string RenderFrameHostImpl::CommitCountString() {
+  std::string result = base::Int64ToString(reinterpret_cast<int64_t>(this));
+  result += "/";
+  result += base::IntToString(commit_count_);
+  return result;
+}
+
 }  // namespace content

content/browser/frame_host/render_frame_host_impl.h

@@ -10,9 +10,11 @@
 
 #include "base/callback.h"
 #include "base/compiler_specific.h"
+#include "base/debug/crash_logging.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
+#include "base/strings/string_number_conversions.h"  // Temporary
 #include "base/time/time.h"
 #include "content/browser/accessibility/browser_accessibility_manager.h"
 #include "content/browser/site_instance_impl.h"
@@ -439,6 +441,9 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // addition, its associated RenderWidgetHost has to be focused.
   bool IsFocused();
 
+  // Temporary for http://crbug.com/369661.
+  std::string CommitCountString();
+
  protected:
   friend class RenderFrameHostFactory;
 
@@ -728,6 +733,9 @@ class CONTENT_EXPORT RenderFrameHostImpl
   // The frame's Mojo Shell service.
   scoped_ptr<FrameMojoShell> frame_mojo_shell_;
 
+  // Temporary for http://crbug.com/369661
+  int commit_count_ = 0;
+
   // NOTE: This must be the last member.
   base::WeakPtrFactory<RenderFrameHostImpl> weak_ptr_factory_;