Fix ClassifyNavigation.

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 894 matching lines @@
   details->http_status_code = params.http_status_code;
   NotifyNavigationEntryCommitted(details);
 
   return true;
 }
 
 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;
 
     // The renderer generates the page IDs, and so if it gives us the invalid
     // page ID (-1) we know it didn't actually navigate. This happens in a few
     // cases:
     //
     // - If a page makes a popup navigated to about blank, and then writes
     //   stuff like a subframe navigated to a real page. We'll get the commit
     //   for the subframe, but there won't be any commit for the outer page.
@@ skipping 87 matching lines @@
     DCHECK(GetLastCommittedEntry());
     return NAVIGATION_TYPE_AUTO_SUBFRAME;
   }
 
   // Anything below here we know is a main frame navigation.
   if (pending_entry_ &&
       !pending_entry_->is_renderer_initiated() &&
       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()) ||

[Charlie Reis, 2015/04/13 23:45:41]

It would help to have a test showing why this is the right thing to check.  I'm
having trouble reasoning about why it's needed.

[Avi (use Gerrit), 2015/04/14 02:00:43]

There's a test where if you navigate to url1 that redirects to url2, and then
you navigate to url1 again and again it redirects to url2, that's a SAME_PAGE.

[Charlie Reis, 2015/04/15 17:43:08]

Acknowledged.

+        existing_entry->GetURL() == pending_entry_->GetVirtualURL()) {

[Charlie Reis, 2015/04/13 23:45:40]

Why are we comparing GetURL() and GetVirtualURL() here?

[Avi (use Gerrit), 2015/04/14 02:00:43]

To cover SAME_PAGE for view-source:

[Charlie Reis, 2015/04/15 17:43:08]

Sorry, I still don't understand this one.  I just tried hitting enter on an
already committed view-source URL, and both the existing and pending entry have
http://www.chromium.org for GetURL() and view-source:http://www.chromium.org for
GetVirtualURL().

Why would the pending entry have a virtual URL that matches the existing entry's
actual URL?  We could compare GetVirtualURL() instead if there's a case where
that matters, but I think comparing GetURL() might be equivalent.

We should either remove this line or have a test showing why it matters.

[Avi (use Gerrit), 2015/04/15 19:14:16]

I remember putting this line in for a test, but I can't make things fail without
this, so I'll put it for a new patchset.

[Charlie Reis, 2015/04/15 19:57:36]

Ok, thanks.  Let me know if you find the case it matters for.

+      // 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)
   // are in-page navigations. We weeded out subframe navigations above. Most of
   // the time this doesn't matter since WebKit 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 (AreURLsInPageNavigation(existing_entry->GetURL(), params.url,
                               params.was_within_same_page, rfh)) {
     return NAVIGATION_TYPE_IN_PAGE;
@@ skipping 801 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content