Don't handle in-page navigations if it's for a subframe. Previously we would

compare the URL with the main frame to see if it was in page. In very rare
cases, these can actually be the same, which will confuse us.

http://crbug.com/5585
Review URL: http://codereview.chromium.org/14824

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@7220 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 39 insertions, 3 deletions

diff --git a/chrome/browser/navigation_controller.cc b/chrome/browser/navigation_controller.cc
index 2b3f96e..deee023 100644
--- a/chrome/browser/navigation_controller.cc
+++ b/chrome/browser/navigation_controller.cc
@@ -683,9 +683,6 @@ NavigationType::Type NavigationController::ClassifyNavigation(
     return NavigationType::SAME_PAGE;
   }
 
-  if (AreURLsInPageNavigation(existing_entry->url(), params.url))
-    return NavigationType::IN_PAGE;
-
   if (!PageTransition::IsMainFrame(params.transition)) {
     // 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
@@ -694,6 +691,14 @@ NavigationType::Type NavigationController::ClassifyNavigation(
     return NavigationType::AUTO_SUBFRAME;
   }
 
+  // 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->url(), params.url))
+    return NavigationType::IN_PAGE;
+
   // Since we weeded out "new" navigations above, we know this is an existing
   // (back/forward) navigation.
   return NavigationType::EXISTING_PAGE;
diff --git a/chrome/browser/navigation_controller_unittest.cc b/chrome/browser/navigation_controller_unittest.cc
index 08b43a0..7938877 100644
--- a/chrome/browser/navigation_controller_unittest.cc
+++ b/chrome/browser/navigation_controller_unittest.cc
@@ -1497,6 +1497,37 @@ TEST_F(NavigationControllerTest, IsInPageNavigation) {
       other_url_with_ref));
 }
 
+// Some pages can have subframes with the same base URL (minus the reference) as
+// the main page. Even though this is hard, it can happen, and we don't want
+// these subframe navigations to affect the toplevel document. They should
+// instead be ignored.  http://crbug.com/5585
+TEST_F(NavigationControllerTest, SameSubframe) {
+  // Navigate the main frame.
+  const GURL url("http://www.google.com/");
+  contents->CompleteNavigationAsRenderer(0, url);
+
+  // We should be at the first navigation entry.
+  EXPECT_EQ(contents->controller()->GetEntryCount(), 1);
+  EXPECT_EQ(contents->controller()->GetLastCommittedEntryIndex(), 0);
+
+  // Navigate a subframe that would normally count as in-page.
+  const GURL subframe("http://www.google.com/#");
+  ViewHostMsg_FrameNavigate_Params params;
+  params.page_id = 0;
+  params.url = subframe;
+  params.transition = PageTransition::AUTO_SUBFRAME;
+  params.should_update_history = false;
+  params.gesture = NavigationGestureAuto;
+  params.is_post = false;
+  NavigationController::LoadCommittedDetails details;
+  EXPECT_FALSE(contents->controller()->RendererDidNavigate(params, false,
+                                                           &details));
+
+  // Nothing should have changed.
+  EXPECT_EQ(contents->controller()->GetEntryCount(), 1);
+  EXPECT_EQ(contents->controller()->GetLastCommittedEntryIndex(), 0);
+}
+
 // A basic test case. Navigates to a single url, and make sure the history
 // db matches.
 TEST_F(NavigationControllerHistoryTest, Basic) {