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

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/logging.h"
 #include "base/metrics/histogram.h"
@@ skipping 783 matching lines @@
   // If we are doing a cross-site reload, we need to replace the existing
   // navigation entry, not add another entry to the history. This has the side
   // effect of removing forward browsing history, if such existed.
   // Or if we are doing a cross-site redirect navigation,
   // we will do a similar thing.
   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);
+  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(),
       params.url, params.was_within_same_page, rfh);
 
   switch (details->type) {
     case NAVIGATION_TYPE_NEW_PAGE:
       RendererDidNavigateToNewPage(rfh, params, details->did_replace_entry);
       break;
     case NAVIGATION_TYPE_EXISTING_PAGE:
@@ skipping 217 matching lines @@
   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;
 }
 
+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,
     bool replace_entry) {
   NavigationEntryImpl* new_entry;
   bool update_virtual_url;
   // Only make a copy of the pending entry if it is appropriate for the new page
   // that was just loaded.  We verify this at a coarse grain by checking that
   // the SiteInstance hasn't been assigned to something else.
   if (pending_entry_ &&
@@ skipping 722 matching lines @@
 int NavigationControllerImpl::GetEntryIndexWithPageID(
     SiteInstance* instance, int32 page_id) const {
   for (int i = static_cast<int>(entries_.size()) - 1; i >= 0; --i) {
     if ((entries_[i]->site_instance() == instance) &&
         (entries_[i]->GetPageID() == page_id))
       return i;
   }
   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;
   return entries_[transient_entry_index_].get();
 }
 
 void NavigationControllerImpl::SetTransientEntry(NavigationEntry* entry) {
   // Discard any current transient entry, we can only have one at a time.
   int index = 0;
   if (last_committed_entry_index_ != -1)
@@ skipping 21 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content