Fix incorrect creation of duplicate navigation entries for repeated page load failures.

content/browser/frame_host/navigation_controller_impl_browsertest.cc

Index: content/browser/frame_host/navigation_controller_impl_browsertest.cc
diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
index 997a6fdb051ae3ebcf6f6b1de0119286ea08b846..4c6b981e7dfa3d36df746939e32ce7e5a3ea2d23 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -25,6 +25,7 @@
 #include "content/test/content_browser_test_utils_internal.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/url_request/url_request_failed_job.h"
 
 namespace content {
 
@@ -376,6 +377,63 @@ class LoadCommittedCapturer : public WebContentsObserver {
 
 }  // namespace
 
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       ErrorPageReplacement) {
+  NavigationController& controller = shell()->web_contents()->GetController();
+  GURL error_url(
+      net::URLRequestFailedJob::GetMockHttpUrl(net::ERR_CONNECTION_RESET));
+  net::URLRequestFailedJob::AddUrlHandler();
+
+  NavigateToURL(shell(), GURL(url::kAboutBlankURL));
+  EXPECT_EQ(1, controller.GetEntryCount());
+
+  FrameTreeNode* root =
+      static_cast<WebContentsImpl*>(shell()->web_contents())->
+          GetFrameTree()->root();
+
+  // Navigate to a page that fails to load. It must result in an error page, the
+  // NEW_PAGE navigation type, and an addition to the history list.
+  {
+    FrameNavigateParamsCapturer capturer(root);
+    NavigateFrameToURL(root, error_url);
+    capturer.Wait();
+    EXPECT_EQ(NAVIGATION_TYPE_NEW_PAGE, capturer.details().type);
+    NavigationEntry* entry = controller.GetLastCommittedEntry();
+    EXPECT_EQ(PAGE_TYPE_ERROR, entry->GetPageType());
+    EXPECT_EQ(2, controller.GetEntryCount());
+  }
+
+  // Navigate again to the page that fails to load. It must result in an error
+  // page, the EXISTING_PAGE navigation type, and no addition to the history
+  // list. We do not use SAME_PAGE here; that case only differs in that it
+  // clears the pending entry, and there is no pending entry after a load
+  // failure.
+  {
+    FrameNavigateParamsCapturer capturer(root);
+    NavigateFrameToURL(root, error_url);
+    capturer.Wait();
+    EXPECT_EQ(NAVIGATION_TYPE_EXISTING_PAGE, capturer.details().type);
+    NavigationEntry* entry = controller.GetLastCommittedEntry();
+    EXPECT_EQ(PAGE_TYPE_ERROR, entry->GetPageType());
+    EXPECT_EQ(2, controller.GetEntryCount());
+  }
+
+  // Make a new entry ...
+  NavigateToURL(shell(), GURL(url::kAboutBlankURL));
+  EXPECT_EQ(3, controller.GetEntryCount());
+
+  // ... and replace it.
+  {
+    FrameNavigateParamsCapturer capturer(root);
+    NavigateToURLAndReplace(shell(), error_url);
+    capturer.Wait();
+    EXPECT_EQ(NAVIGATION_TYPE_EXISTING_PAGE, capturer.details().type);
+    NavigationEntry* entry = controller.GetLastCommittedEntry();
+    EXPECT_EQ(PAGE_TYPE_ERROR, entry->GetPageType());
+    EXPECT_EQ(3, controller.GetEntryCount());
+  }

[davidben, 2015/03/31 17:26:08]

There's a third case that's probably worth testing: if the
NavigateToURLAndReplace which failed was a cross-process navigation, I recall
things get really weird. Blink and the browser process disagree on the replace
bit in that case.

[Charlie Reis, 2015/03/31 17:29:48]

It looks like that's what NavigateToURLAndReplace essentially does, by setting 
params.should_replace_current_entry.  (Right?)  I agree that actually doing a
location.replace in the renderer process would behave differently, so maybe it's
worth testing both.

+}
+
 // Various tests for navigation type classifications. TODO(avi): It's rather
 // bogus that the same info is in two different enums; http://crbug.com/453555.