Ensure the new navigation classifier works in the real world.

content/browser/frame_host/navigation_controller_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/navigation_controller_impl.h"
 
 #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
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
 #include "cc/base/switches.h"
 #include "components/mime_util/mime_util.h"
@@ skipping 766 matching lines @@
   if (params.url_is_unreachable && failed_pending_entry_id_ != 0) {
     details->did_replace_entry = failed_pending_entry_should_replace_;
   } else {
     details->did_replace_entry = pending_entry_ &&
                                  pending_entry_->should_replace_entry();
   }
 
   // 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.
   ignore_mismatch |= details->type == NAVIGATION_TYPE_EXISTING_PAGE &&
                      new_type == NAVIGATION_TYPE_SAME_PAGE;
   ignore_mismatch |= details->type == NAVIGATION_TYPE_SAME_PAGE &&
                      new_type == NAVIGATION_TYPE_EXISTING_PAGE;
   // There are mismatches in the field where the new classifier thinks it's
   // AUTO_SUBFRAME and the old classifier somehow thinks it's NEW or IGNORE. For
   // IGNORE we know of at least one repro (https://crbug.com/492875), and for
   // NEW it's not entirely clear what's going on, but we're pretty sure the new
   // classifier is correct for both cases, so we're letting these mismatches go.
   ignore_mismatch |= details->type == NAVIGATION_TYPE_NEW_SUBFRAME &&
                      new_type == NAVIGATION_TYPE_AUTO_SUBFRAME;
   ignore_mismatch |= details->type == NAVIGATION_TYPE_NAV_IGNORE &&
                      new_type == NAVIGATION_TYPE_AUTO_SUBFRAME;
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
         switches::kSitePerProcess)) {
     // We know that the old classifier is wrong for OOPIFs, so use the new one
     // in --site-per-process mode.
     details->type = new_type;
     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));
+    if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kSitePerProcess)) {
+      base::debug::SetCrashKeyValue("369661-spp", "yes");

[Charlie Reis, 2015/06/09 23:32:13]

This is unreachable, given line 822.

[Avi (use Gerrit), 2015/06/10 16:34:52]

Good point. (I just copied the old set of keys.)

+    } else {
+      base::debug::SetCrashKeyValue("369661-spp", "no");
+    }
+    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.
   details->is_in_page = IsURLInPageNavigation(
       params.url, params.was_within_same_page, rfh);
 
   switch (details->type) {
     case NAVIGATION_TYPE_NEW_PAGE:
       RendererDidNavigateToNewPage(rfh, params, details->did_replace_entry);
       break;
@@ skipping 99 matching lines @@
     // - We were also getting these for failed loads (for example, bug 21849).
     //   The guess is that we get a "load commit" for the alternate error page,
     //   but that doesn't affect the page ID, so we get the "old" one, which
     //   could be invalid. This can also happen for a cross-site transition
     //   that causes us to swap processes. Then the error page load will be in
     //   a new process with no page IDs ever assigned (and hence a -1 value),
     //   yet the navigation controller still might have previous pages in its
     //   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;
   }
 
   if (params.page_id > delegate_->GetMaxPageIDForSiteInstance(
           rfh->GetSiteInstance())) {
     // Greater page IDs than we've ever seen before are new pages. We may or may
     // not have a pending entry for the page, and this may or may not be the
     // main frame.
     if (!rfh->GetParent())
       return NAVIGATION_TYPE_NEW_PAGE;
 
     // When this is a new subframe navigation, we should have a committed page
     // for which it's a suframe in. This may not be the case when an iframe is
     // 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;
   }
 
   // We only clear the session history when navigating to a new page.
   DCHECK(!params.history_list_was_cleared);
 
   // Now we know that the notification is for an existing page. Find that entry.
   int existing_entry_index = GetEntryIndexWithPageID(
@@ skipping 30 matching lines @@
       if (entries_[i]->site_instance())
         temp.append(base::IntToString(entries_[i]->site_instance()->GetId()));
       else
         temp.append("N");
       if (entries_[i]->site_instance() != rfh->GetSiteInstance())
         temp.append("x");
       temp.append(",");
     }
     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();
 
   if (rfh->GetParent()) {
     // All manual subframes would get new IDs and were handled above, so we
     // know this is auto. Since the current page was found in the navigation
     // entry list, we're guaranteed to have a last committed entry.
     DCHECK(GetLastCommittedEntry());
     return NAVIGATION_TYPE_AUTO_SUBFRAME;
@@ skipping 52 matching lines @@
     // this may or may not be the main frame.
     if (!rfh->GetParent()) {
       return NAVIGATION_TYPE_NEW_PAGE;
     }
 
     // When this is a new subframe navigation, we should have a committed page
     // in which it's a subframe. This may not be the case when an iframe is
     // 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;
   }
 
   // We only clear the session history when navigating to a new page.
   DCHECK(!params.history_list_was_cleared);
 
   if (rfh->GetParent()) {
     // All manual subframes would be did_create_new_entry and handled above, so
     // we know this is auto.
     if (GetLastCommittedEntry()) {
       return NAVIGATION_TYPE_AUTO_SUBFRAME;
     } 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;
     }
   }
 
   if (params.nav_entry_id == 0) {
     // This is a renderer-initiated navigation (nav_entry_id == 0), but didn't
     // create a new page.
 
     // Just like above in the did_create_new_entry case, it's possible to
     // 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
       // the same page.
       return NAVIGATION_TYPE_IN_PAGE;
     } else {
       // This is history.reload() or a client-side redirect.
       return NAVIGATION_TYPE_EXISTING_PAGE;
     }
   }
@@ skipping 27 matching lines @@
   }
 
   // Now we know that the notification is for an existing page. Find that entry.
   int existing_entry_index = GetEntryIndexWithUniqueID(params.nav_entry_id);
   if (existing_entry_index == -1) {
     // The page was not found. It could have been pruned because of the limit on
     // back/forward entries (not likely since we'll usually tell it to navigate
     // 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;
   }
 
   // Any top-level navigations with the same base (minus the reference fragment)
   // are in-page navigations. (We weeded out subframe navigations above.) Most
   // of the time this doesn't matter since Blink doesn't tell us about subframe
   // navigations that don't actually navigate, but it can happen when there is
   // an encoding override (it always sends a navigation request).
   if (IsURLInPageNavigation(params.url, params.was_within_same_page, rfh))
     return NAVIGATION_TYPE_IN_PAGE;
@@ skipping 868 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content