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 2ebdd261005e2f91e2f6cecadceeec6a5102ef6b..8d5d6ecd2de2c670dbcaebbe86d41b240c8bd81b 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -813,6 +813,13 @@ bool NavigationControllerImpl::RendererDidNavigate(
 
   // Do navigation-type specific actions. These will make and commit an entry.
   details->type = ClassifyNavigation(rfh, params);

[clamy, 2015/04/10 12:05:44]

With PlzNavigate enabled, ClassifyNavigation fails in the
NavigationControllerTest.CopyRestoredStateAndNavigate test because of a hack
with SiteInstances. Do you think it is possible for us to use
ClassifyNavigationWithoutPageID instead? (I can do it in a follow-up patch to
this).

[Avi (use Gerrit), 2015/04/13 22:42:48]

My current plan is to commit this with the DCHECK (ensuring they behave the same
in debug builds), then switch to CHECK, then switch over.

I want to get some time for it to bake and for us to be confident in it. I
understand that this CL unblocks you, but I want to be sure it doesn't break
things before unleashing it onto the world.

[clamy, 2015/04/14 15:44:47]

Acknowledged.

+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kSitePerProcess)) {
+    // For site-per-process, both ClassifyNavigation methods get it wrong (see
+    // http://crbug.com/464014) so don't worry about a mismatch if that's the
+    // case.
+    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(),
@@ -912,12 +919,12 @@ NavigationType NavigationControllerImpl::ClassifyNavigation(
     RenderFrameHostImpl* rfh,
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params) const {
   if (params.page_id == -1) {
-    // TODO(nasko, creis):  An out-of-process child frame has no way of
-    // knowing the page_id of its parent, so it is passing back -1. The
-    // semantics here should be re-evaluated during session history refactor
-    // (see http://crbug.com/236848). For now, we assume this means the
-    // child frame loaded and proceed. Note that this may do the wrong thing
-    // for cross-process AUTO_SUBFRAME navigations.
+    // TODO(nasko, creis): An out-of-process child frame has no way of knowing
+    // the page_id of its parent, so it is passing back -1. The semantics here
+    // should be re-evaluated during session history refactor (see
+    // http://crbug.com/236848 and in particular http://crbug.com/464014). For
+    // now, we assume this means the child frame loaded and proceed. Note that
+    // this may do the wrong thing for cross-process AUTO_SUBFRAME navigations.
     if (rfh->IsCrossProcessSubframe())
       return NAVIGATION_TYPE_NEW_SUBFRAME;
 
@@ -1025,14 +1032,22 @@ NavigationType NavigationControllerImpl::ClassifyNavigation(
       existing_entry != pending_entry_ &&
       pending_entry_->GetPageID() == -1 &&
       existing_entry == GetLastCommittedEntry()) {
-    // In this case, we have a pending entry for a URL but WebCore didn't do a
-    // new navigation. This happens when you press enter in the URL bar to
-    // reload. We will create a pending entry, but WebKit 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). If this matches the last committed entry, we want to just ignore
-    // the pending entry and go back to where we were (the "existing entry").
-    return NAVIGATION_TYPE_SAME_PAGE;
+    const std::vector<GURL>& existing_redirect_chain =
+        existing_entry->GetRedirectChain();
+
+    if (existing_entry->GetURL() == pending_entry_->GetURL() ||
+        (existing_redirect_chain.size() &&
+         existing_redirect_chain[0] == pending_entry_->GetURL()) ||
+        existing_entry->GetURL() == pending_entry_->GetVirtualURL()) {

[Charlie Reis, 2015/04/10 23:54:21]

We probably shouldn't be changing ClassifyNavigation in the same CL that we're
introducing ClassifyNavigationWithoutPageID.  Can we pull this part out into a
preliminary CL?

[Avi (use Gerrit), 2015/04/13 22:42:48]

https://codereview.chromium.org/1082083002

It should pass all the tests. Fingers crossed.

+      // In this case, we have a pending entry for a URL but WebCore didn't do a
+      // new navigation. This happens when you press enter in the URL bar to
+      // reload. We will create a pending entry, but WebKit 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). If this matches the last committed entry, we want to just ignore
+      // the pending entry and go back to where we were (the "existing entry").
+      return NAVIGATION_TYPE_SAME_PAGE;
+    }
   }
 
   // Any toplevel navigations with the same base (minus the reference fragment)
@@ -1050,6 +1065,111 @@ NavigationType NavigationControllerImpl::ClassifyNavigation(
   return NAVIGATION_TYPE_EXISTING_PAGE;
 }
 
+NavigationType NavigationControllerImpl::ClassifyNavigationWithoutPageID(
+    RenderFrameHostImpl* rfh,
+    const FrameHostMsg_DidCommitProvisionalLoad_Params& params) const {
+  if (params.did_create_new_entry) {
+    // A new entry. We may or may not have a pending entry for the page, and
+    // this may or may not be the main frame.
+    if (ui::PageTransitionIsMainFrame(params.transition)) {
+      // TODO(avi): I want to use |if (!rfh->GetParent())| here but lots of unit
+      // tests fake auto subframe commits by sending the main frame a
+      // PAGE_TRANSITION_AUTO_SUBFRAME transition. Fix those, and adjust here.

[Charlie Reis, 2015/04/10 23:54:20]

I like this plan.

[Avi (use Gerrit), 2015/04/13 22:42:48]

Acknowledged.

PageTransitionIsMainFrame is used in a *lot* of places in NavController, not
just in ClassifyNavigation. I would prefer to switch them all over.

+      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 (!ui::PageTransitionIsMainFrame(params.transition)) {
+    // 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.
+      return NAVIGATION_TYPE_NAV_IGNORE;
+    }
+  }
+
+  if (params.nav_entry_id == 0) {

[Charlie Reis, 2015/04/10 23:54:20]

Can you add a comment saying what nav_entry_id == 0 means?  (I'm guessing from
context below that it equates to renderer-initiated navigations?  Or is it just
a subset of those?)

[Avi (use Gerrit), 2015/04/13 22:42:48]

Done.

+    // 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.
+    if (!GetLastCommittedEntry())
+      return NAVIGATION_TYPE_NAV_IGNORE;
+
+    // This is a renderer-initiated navigation, but didn't create a new page.
+    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) {
+    // In this case, we have a pending entry for a load of a new URL but Blink
+    // didn't do a new navigation (params.did_create_new_entry). 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;
+  }
+
+  if (params.url_is_unreachable && failed_pending_entry_id_ != 0 &&
+      params.nav_entry_id == failed_pending_entry_id_) {
+    // If the renderer was going to a new pending entry that got cleared because
+    // of an error, this is the case of the user trying to retry a failed load
+    // by pressing return.
+    return NAVIGATION_TYPE_EXISTING_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.
+    // TODO(avi): Crash the renderer like we do in the old ClassifyNavigation?
+    NOTREACHED() << "Could not find nav entry with id " << params.nav_entry_id;
+    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,
@@ -1811,6 +1931,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;