Solidify Entry discarding logic (NavigationHandle keeps its ID)

content/browser/frame_host/navigation_controller_impl_unittest.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 <stddef.h>
 #include <stdint.h>
 #include <utility>
 
@@ skipping 366 matching lines @@
     navigation_entry_committed_counter_ = 0;
     // Check that we can go to any valid offset into the history.
     for (size_t j = 0; j < urls.size(); ++j)
       EXPECT_TRUE(controller.CanGoToOffset(j - url_index));
     // Check that we can't go beyond the beginning or end of the history.
     EXPECT_FALSE(controller.CanGoToOffset(-(url_index + 1)));
     EXPECT_FALSE(controller.CanGoToOffset(urls.size() - url_index));
   }
 }
 
+TEST_F(NavigationControllerTest, DontDiscardWrongPendingEntry) {
+  // This test case was added to reproduce crbug.com/513742. The repro steps are
+  // as follows:
+  // 1. Pending entry for A is created.
+  // 2. DidStartProvisionalLoad message for A arrives.
+  // 3. Pending entry for B is created.
+  // 4. DidFailProvisionalLoad message for A arrives. The logic here discards.
+  // 5. DidStartProvisionalLoad message for B arrives.
+  //
+  // At step (4), the pending entry for B is discarded, when A is the one that
+  // is being aborted. This caused the last committed entry to be displayed in
+  // the omnibox, which is the entry before A was created.

[Charlie Reis, 2016/02/06 00:54:58]

Perfect.  Tiny nit: We usually put these comments above the TEST_F line.

[Charlie Harrison, 2016/02/08 15:06:41]

Done.

+  NavigationControllerImpl& controller = controller_impl();
+  GURL initial_url("http://www.google.com");
+  GURL url_1("http://foo.com");
+  GURL url_2("http://foo2.com");
+
+  // Navigate inititally. This is the url that could erroneously be the visible
+  // entry when url_1 fails.
+  NavigateAndCommit(initial_url);
+
+  // Set the pending entry as url_1 and receive the DidStartProvisionalLoad
+  // message, creating the NavigationHandle.
+  controller.LoadURL(url_1, Referrer(), ui::PAGE_TRANSITION_TYPED,
+                     std::string());
+  EXPECT_EQ(controller.GetVisibleEntry()->GetURL(), url_1);
+  main_test_rfh()->SimulateNavigationStart(url_1);
+  EXPECT_EQ(controller.GetVisibleEntry()->GetURL(), url_1);
+
+  // Navigate to url_2, aborting url_1 before the DidStartProvisionalLoad
+  // message is received for url_2. Do not discard the pending entry for url_2
+  // here.
+  controller.LoadURL(url_2, Referrer(), ui::PAGE_TRANSITION_TYPED,
+                     std::string());
+  EXPECT_EQ(controller.GetVisibleEntry()->GetURL(), url_2);
+  main_test_rfh()->SimulateNavigationError(url_1, net::ERR_ABORTED);
+  EXPECT_EQ(controller.GetVisibleEntry()->GetURL(), url_2);
+
+  // Get the DidStartProvisionalLoad message for url_2.
+  main_test_rfh()->SimulateNavigationStart(url_2);
+
+  EXPECT_EQ(controller.GetVisibleEntry()->GetURL(), url_2);
+}
+
 TEST_F(NavigationControllerTest, LoadURL) {
   NavigationControllerImpl& controller = controller_impl();
   TestNotificationTracker notifications;
   RegisterForAllNavNotifications(&notifications, &controller);
 
   const GURL url1("http://foo1");
   const GURL url2("http://foo2");
 
   controller.LoadURL(
       url1, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
@@ skipping 4721 matching lines @@
   EXPECT_EQ(default_ssl_status.connection_status,
             details.ssl_status.connection_status);
   EXPECT_EQ(default_ssl_status.content_status,
             details.ssl_status.content_status);
   EXPECT_EQ(0u, details.ssl_status.signed_certificate_timestamp_ids.size());
 
   EXPECT_EQ(1, main_test_rfh()->GetProcess()->bad_msg_count());
 }
 
 }  // namespace content