PlzNavigate: add cancel navigation logic for uncommitted requests

content/browser/frame_host/render_frame_host_manager_unittest.cc

Index: content/browser/frame_host/render_frame_host_manager_unittest.cc
diff --git a/content/browser/frame_host/render_frame_host_manager_unittest.cc b/content/browser/frame_host/render_frame_host_manager_unittest.cc
index acf5f139c4767ecea696694d8bceea8537a41bcd..87d7ab12746c9a7a12260bbbad7cec38af7792be 100644
--- a/content/browser/frame_host/render_frame_host_manager_unittest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -1698,6 +1698,7 @@ TEST_F(RenderFrameHostManagerTest, BrowserSideNavigationBeginNavigation) {
   const GURL kUrl1("http://www.google.com/");
   const GURL kUrl2("http://www.chromium.org/");
   const GURL kUrl3("http://www.gmail.com/");
+  const int64 kFirstNavRequestID = 1;
 
   // TODO(clamy): we should be enabling browser side navigations here
   // when CommitNavigation is properly implemented.
@@ -1722,6 +1723,7 @@ TEST_F(RenderFrameHostManagerTest, BrowserSideNavigationBeginNavigation) {
       kUrl1, subframe_request->info().first_party_for_cookies);
   EXPECT_FALSE(subframe_request->info().is_main_frame);
   EXPECT_TRUE(subframe_request->info().parent_is_main_frame);
+  EXPECT_EQ(kFirstNavRequestID, subframe_request->navigation_request_id());
 
   // Simulate a BeginNavigation IPC on the main frame.
   contents()->GetMainFrame()->SendBeginNavigationWithURL(kUrl3);
@@ -1732,6 +1734,7 @@ TEST_F(RenderFrameHostManagerTest, BrowserSideNavigationBeginNavigation) {
   EXPECT_EQ(kUrl3, main_request->info().first_party_for_cookies);
   EXPECT_TRUE(main_request->info().is_main_frame);
   EXPECT_FALSE(main_request->info().parent_is_main_frame);
+  EXPECT_EQ(kFirstNavRequestID + 1, main_request->navigation_request_id());
 }
 
 // PlzNavigate: Test that RequestNavigation creates a NavigationRequest and that
@@ -1788,9 +1791,57 @@ TEST_F(RenderFrameHostManagerTest,
 
   NavigationBeforeCommitInfo commit_info;
   commit_info.navigation_url = kUrl2;
+  commit_info.navigation_request_id = main_request->navigation_request_id();
   render_manager->CommitNavigation(commit_info);
-  main_request = GetNavigationRequestForRenderFrameManager(render_manager);
   EXPECT_NE(main_test_rfh(), rfh);
 }
 
+// PlzNavigate: Test that a navigation commit is ignored if another request has
+// been issued in the meantime.
+// TODO(carlosk): add URL expectations once those are properly set after the
+// navigation is committed successfully.
+// TODO(carlosk): add checks to assert that the cancel call was sent to
+// ResourceDispatcherHost in the IO thread.
+TEST_F(RenderFrameHostManagerTest,
+       BrowserSideNavigationIgnoreStaleNavigationCommit) {
+  const GURL kUrl0("http://www.wikipedia.org/");
+  const GURL kUrl1("http://www.chromium.org/");
+  const GURL kUrl2("http://www.google.com/");
+
+  // Initialization.
+  contents()->NavigateAndCommit(kUrl0);
+  RenderFrameHostImpl* rfh1 = main_test_rfh();
+  RenderFrameHostManager* render_manager =
+      main_test_rfh()->frame_tree_node()->render_manager();
+  EnableBrowserSideNavigation();
+
+  // Request navigation to the 1st URL and gather data.
+  main_test_rfh()->SendBeginNavigationWithURL(kUrl1);
+  NavigationRequest* request1 =
+      GetNavigationRequestForRenderFrameManager(render_manager);
+  ASSERT_TRUE(request1);
+  int64 request_id1 = request1->navigation_request_id();
+
+  // Request navigation to the 2nd URL and gather more data.
+  main_test_rfh()->SendBeginNavigationWithURL(kUrl2);
+  NavigationRequest* request2 =
+      GetNavigationRequestForRenderFrameManager(render_manager);
+  ASSERT_TRUE(request2);
+  int64 request_id2 = request2->navigation_request_id();
+  EXPECT_NE(request_id1, request_id2);
+
+  // Confirms that a stale commit is ignored by the RHFM.
+  NavigationBeforeCommitInfo nbc_info;
+  nbc_info.navigation_url = kUrl1;
+  nbc_info.navigation_request_id = request_id1;
+  render_manager->CommitNavigation(nbc_info);
+  EXPECT_EQ(main_test_rfh(), rfh1);

[nasko, 2014/09/02 14:10:57]

I think it is better to compare SiteInstances of those, since rfh1 will likely
be deleted if an actual navigation was performed, resulting in use-after-free
bug and invalid test, since noone will invalidate the value of rfh1. 

[carlosk, 2014/09/02 15:30:09]

Done!

When I initially wrote this test the that wasn't still possible as PlzNavigate
wasn't properly setting the SiteInstance yet, hence the (now removed) TODO at
line 1801.

[(Do not use) nasko, 2014/09/02 17:20:28]

You can just capture the site instance of an RFH into a variable and later on do
EXPECT_EQ(). See CreateSwappedOutOpenerRVHs for an example or other tests in
this file.

[carlosk, 2014/09/03 18:27:00]

Executed as discussed with Nasko.

+
+  // Confirms that a valid, request-matching commit is correctly processed.
+  nbc_info.navigation_url = kUrl2;
+  nbc_info.navigation_request_id = request_id2;
+  render_manager->CommitNavigation(nbc_info);
+  EXPECT_NE(main_test_rfh(), rfh1);
+}
+
 }  // namespace content