PlzNavigate: don't discard pending entry in DidFailProvisionalLoad

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 <memory>
@@ skipping 397 matching lines @@
 // 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.
+//
+// PlzNavigate: the test case is slightly different, as the events described
+// above cannot happen in that order. Instead, this tests that receiving a
+// DidFailProvisionalLoad from the renderer that is trying to commit an error
+// page won't reset the pending entry of a navigation that just started.

[Charlie Reis, 2016/04/12 23:59:16]

Thanks for adding a test for this!

I feel like it might be better to have separate tests for the default case and
the PlzNavigate case, even if there's a fair amount of copy/paste.  (That's ok
for tests.)  Otherwise it gets hard to follow what's being tested in each mode,
as I mention below.

[clamy, 2016/04/13 12:51:51]

Done. They are now separate, and I skip this one when PlzNavigate is enabled.

 TEST_F(NavigationControllerTest, DontDiscardWrongPendingEntry) {
   NavigationControllerImpl& controller = controller_impl();
   GURL initial_url("http://www.google.com");
-  GURL url_1("http://foo.com");
-  GURL url_2("http://foo2.com");
+  GURL url_1("http://www.google.com/foo");

[clamy, 2016/04/12 14:08:02]

I had to make a few changes to make the test pass on PlzNavigate. First the
failing navigation has to be same-process, otherwise the start of the second
navigation will destroy the speculative RFH that is trying to commit the
navigation.

[Charlie Reis, 2016/04/12 23:59:17]

Seems ok.  (I'm not 100% sure whether the original test cared if this were
cross-process, but I don't think it did.)

+  GURL url_2("http://foo.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(url_1, controller.GetVisibleEntry()->GetURL());
   main_test_rfh()->SimulateNavigationStart(url_1);
   EXPECT_EQ(url_1, controller.GetVisibleEntry()->GetURL());
 
-  // 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.
+  // PlzNavigate: simulate the navigation failing and needing to show an error
+  // page.
+  if (IsBrowserSideNavigationEnabled())
+    main_test_rfh()->SimulateNavigationError(url_1, net::ERR_TIMED_OUT);

[clamy, 2016/04/12 14:08:02]

We need to get the navigation to fail here with PlzNavigate, and the error
cannot be net::ERR_ABORTED, as we would not show an error page in that case.

[Charlie Reis, 2016/04/12 23:59:16]

Acknowledged.

+
+  // Simulate a navigation to url2 starting followed by a DidFailProvsionalLoad
+  // from the navigation to url1.
+  FrameHostMsg_DidFailProvisionalLoadWithError_Params error;
+  error.error_code = net::ERR_TIMED_OUT;

[clamy, 2016/04/12 14:08:02]

Matching error here with the PlzNavigate case.

[Charlie Reis, 2016/04/12 23:59:16]

Shouldn't it be ERR_ABORTED in the non-PlzNavigate case?

+  error.url = url_1;
   controller.LoadURL(url_2, Referrer(), ui::PAGE_TRANSITION_TYPED,
                      std::string());
   EXPECT_EQ(url_2, controller.GetVisibleEntry()->GetURL());
-  main_test_rfh()->SimulateNavigationError(url_1, net::ERR_ABORTED);
+  main_test_rfh()->OnMessageReceived(

[clamy, 2016/04/12 14:08:02]

We just want to simulate getting the DidFailProvisionalLoad IPC in both the
current architecture of navigations and PlzNavigate. So I switched to just
receiving the IPC, in order to not try do more work.

[Charlie Reis, 2016/04/12 23:59:16]

Acknowledged.

+      FrameHostMsg_DidFailProvisionalLoadWithError(
+          main_test_rfh()->GetRoutingID(), error));
   EXPECT_EQ(url_2, controller.GetVisibleEntry()->GetURL());
 
   // Get the DidStartProvisionalLoad message for url_2.
   main_test_rfh()->SimulateNavigationStart(url_2);
 
   EXPECT_EQ(url_2, controller.GetVisibleEntry()->GetURL());
 }
 
 TEST_F(NavigationControllerTest, LoadURL) {
   NavigationControllerImpl& controller = controller_impl();
@@ skipping 4784 matching lines @@
   EXPECT_EQ(default_ssl_status.content_status,
             observer.details().ssl_status.content_status);
   EXPECT_EQ(
       0u,
       observer.details().ssl_status.signed_certificate_timestamp_ids.size());
 
   EXPECT_EQ(1, main_test_rfh()->GetProcess()->bad_msg_count());
 }
 
 }  // namespace content