Classify navigations without page id in parallel to the existing classifier.

content/browser/frame_host/navigation_controller_impl.cc

Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 6610cb1202c04ab2360021ba4a1df4f4aba3f4ea..49e1fe76128631be09c4731efd837273ee8e918e 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -801,6 +801,7 @@ bool NavigationControllerImpl::RendererDidNavigate(
 
   // Do navigation-type specific actions. These will make and commit an entry.
   details->type = ClassifyNavigation(rfh, params);
+  DCHECK_EQ(details->type, ClassifyNavigationWithoutPageID(rfh, params));
 
   // is_in_page must be computed before the entry gets committed.
   details->is_in_page = AreURLsInPageNavigation(rfh->GetLastCommittedURL(),
@@ -1038,6 +1039,119 @@ NavigationType NavigationControllerImpl::ClassifyNavigation(
   return NAVIGATION_TYPE_EXISTING_PAGE;
 }
 
+NavigationType NavigationControllerImpl::ClassifyNavigationWithoutPageID(
+    RenderFrameHostImpl* rfh,
+    const FrameHostMsg_DidCommitProvisionalLoad_Params& params) const {
+  if (params.commit_type == blink::WebStandardCommit) {
+    // Standard commits 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()) {
+      // TODO(avi): Everyone does ui::PageTransitionIsMainFrame to determine
+      // main-frame-ness. I can get that consumers of page transitions would
+      // want to do that, but for code inside RenderFrameHost and NavController?
+      // Please.
+      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())
+      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 WebStandardCommit and handled above, so we
+    // know this is auto.
+    DCHECK(GetLastCommittedEntry());
+    return NAVIGATION_TYPE_AUTO_SUBFRAME;
+  }
+
+  if (params.nav_entry_id == 0) {
+    // This is a renderer-initiated navigation, but didn't create a new page.
+    // This must be an inert commit (standard commits are handled above,

[Charlie Reis, 2015/03/12 18:28:17]

Can you explain what inert commit means?  That's a Blink term that I'm not
familiar with.

[Avi (use Gerrit), 2015/03/12 19:21:17]

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

There are four Blink commit types: standard, back/forward, initial in child
frame, inert. That is what I'm referring to here.

+    // back/forward commits are always browser-initiated, and initial child

[Charlie Reis, 2015/03/12 18:28:17]

Minor wording issue, though I'm not sure if it changes your decisions:
Back/forward commits are not always browser-initiated.  They do always go
through the browser process, but they continue to track whether they were
renderer-initiated (e.g., history.back()) or not (e.g., Back button).

[Avi (use Gerrit), 2015/03/12 19:21:17]

That doesn't change the code.

Because the request comes via the browser (whether or not it's initiated by it)
it will have a nav entry id.

I will clarify the comment.

+    // frame commits are only seen with subframes, also handled above).
+    DCHECK_EQ(blink::WebHistoryInertCommit, params.commit_type);
+    if (params.was_within_same_page) {
+      // This is history.replaceState(), which is renderer-initiated yet within
+      // the same page.
+      return NAVIGATION_TYPE_IN_PAGE;
+    } else {
+      // This is history.reload() and client-side redirection.
+      return NAVIGATION_TYPE_EXISTING_PAGE;
+    }
+  }
+
+  if (pending_entry_ && pending_entry_index_ == -1 &&
+      pending_entry_->GetUniqueID() == params.nav_entry_id) {
+    DCHECK_EQ(blink::WebHistoryInertCommit, params.commit_type);
+    // In this case, we have a pending entry for a load of a new URL but Blink
+    // didn't do a new navigation (aka WebStandardCommit). This happens when you
+    // press enter in the URL bar to reload. We will create a pending entry, but
+    // Blink will convert it to a reload since it's the same page and not create
+    // a new entry for it (the user doesn't want to have a new back/forward
+    // entry when they do this). Therefore we want to just ignore the pending
+    // entry and go back to where we were (the "existing entry").
+    return NAVIGATION_TYPE_SAME_PAGE;
+  }
+
+  // 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.
+    NOTREACHED() << "Could not find nav entry with id " << params.nav_entry_id;
+
+    // Because the unknown entry has committed, we risk showing the wrong URL in
+    // release builds. Instead, we'll kill the renderer process to be safe.
+    LOG(ERROR) << "terminating renderer for bad navigation: " << params.url;
+    RecordAction(base::UserMetricsAction("BadMessageTerminate_NC"));

[Charlie Reis, 2015/03/12 18:28:17]

We probably don't want to kill the renderer in both versions of this method
(i.e., it's not idempotent).  That said, we probably want to keep some version
of the kill when we move to the new approach.  It'd be nice to be able to
distinguish between the new reports and the old reports, since I expect they
will differ (and maybe even go away?).

[Avi (use Gerrit), 2015/03/12 19:21:17]

Done.

+
+    // Temporary code so we can get more information.  Format:
+    //  http://url/foo.html#uniqueid5#frame1#ids2,3,4,
+    std::string temp = params.url.spec();
+    temp.append("#uniqueid");
+    temp.append(base::IntToString(params.nav_entry_id));
+    temp.append("#frame");
+    temp.append(base::IntToString(rfh->GetRoutingID()));
+    temp.append("#ids");
+    for (int i = 0; i < static_cast<int>(entries_.size()); ++i) {
+      // Append entry metadata (e.g., 3):
+      //  3: nav_entry_id
+      temp.append(base::IntToString(entries_[i]->GetUniqueID()));
+      temp.append(",");
+    }
+    GURL url(temp);
+    rfh->render_view_host()->Send(new ViewMsg_TempCrashWithData(url));
+    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).
+  NavigationEntryImpl* existing_entry = entries_[existing_entry_index].get();
+  if (AreURLsInPageNavigation(existing_entry->GetURL(), params.url,
+                              params.was_within_same_page, rfh)) {
+    return NAVIGATION_TYPE_IN_PAGE;
+  }
+
+  // Since we weeded out "new" navigations above, we know this is an existing
+  // (back/forward) navigation.
+  return NAVIGATION_TYPE_EXISTING_PAGE;
+}
+
 void NavigationControllerImpl::RendererDidNavigateToNewPage(
     RenderFrameHostImpl* rfh,
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params,
@@ -1780,6 +1894,15 @@ int NavigationControllerImpl::GetEntryIndexWithPageID(
   return -1;
 }
 
+int NavigationControllerImpl::GetEntryIndexWithUniqueID(
+    int nav_entry_id) const {
+  for (int i = static_cast<int>(entries_.size()) - 1; i >= 0; --i) {
+    if (entries_[i]->GetUniqueID() == nav_entry_id)
+      return i;
+  }
+  return -1;
+}
+
 NavigationEntryImpl* NavigationControllerImpl::GetTransientEntry() const {
   if (transient_entry_index_ == -1)
     return NULL;