PlzNavigate: add cancel navigation logic for uncommitted requests

content/browser/frame_host/render_frame_host_manager_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 "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/cross_site_transferring_request.h"
 #include "content/browser/frame_host/navigation_before_commit_info.h"
@@ skipping 1680 matching lines @@
     EXPECT_FALSE(rvh_deleted_observer.deleted());
   }
 }
 
 // PlzNavigate: Test that a proper NavigationRequest is created by
 // BeginNavigation.
 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.
   // Navigate to the first page.
   contents()->NavigateAndCommit(kUrl1);
 
   EnableBrowserSideNavigation();
   // Add a subframe.
   TestRenderFrameHost* subframe_rfh = static_cast<TestRenderFrameHost*>(
       contents()->GetFrameTree()->AddFrame(
           contents()->GetFrameTree()->root(), 14, "Child"));
 
   // Simulate a BeginNavigation IPC on the subframe.
   subframe_rfh->SendBeginNavigationWithURL(kUrl2);
   NavigationRequest* subframe_request =
       GetNavigationRequestForRenderFrameManager(
           subframe_rfh->frame_tree_node()->render_manager());
   ASSERT_TRUE(subframe_request);
   EXPECT_EQ(kUrl2, subframe_request->info().navigation_params.url);
   // First party for cookies url should be that of the main frame.
   EXPECT_EQ(
       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);
   NavigationRequest* main_request = GetNavigationRequestForRenderFrameManager(
       contents()->GetMainFrame()->frame_tree_node()->render_manager());
   ASSERT_TRUE(main_request);
   EXPECT_EQ(kUrl3, main_request->info().navigation_params.url);
   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
 // RenderFrameHost is not modified when the navigation commits.
 TEST_F(RenderFrameHostManagerTest,
        BrowserSideNavigationRequestNavigationNoLiveRenderer) {
   const GURL kUrl("http://www.google.com/");
 
   EnableBrowserSideNavigation();
   EXPECT_FALSE(main_test_rfh()->render_view_host()->IsRenderViewLive());
@@ skipping 36 matching lines @@
 
   EnableBrowserSideNavigation();
   // Navigate to a different site.
   main_test_rfh()->SendBeginNavigationWithURL(kUrl2);
   NavigationRequest* main_request =
       GetNavigationRequestForRenderFrameManager(render_manager);
   ASSERT_TRUE(main_request);
 
   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);
 }
 
+class TestNavigationRequestObserver : public NavigationRequest::Observer {
+ public:
+  explicit TestNavigationRequestObserver(NavigationRequest* nv)
+    : NavigationRequest::Observer(nv), cancelCounter(0), destroyCounter(0) {}
+
+  virtual void RequestCanceled(NavigationRequest* nv) OVERRIDE {
+    ++cancelCounter;
+  }
+
+  virtual void RequestDestroyed(NavigationRequest* nv) OVERRIDE {
+    ++destroyCounter;
+  }
+
+  int cancelCounter;

[clamy, 2014/08/28 12:17:01]

I would rather have a boolean than a counter here.

[carlosk, 2014/08/29 15:54:43]

In this case I prefer counters instead of booleans because I more easily can
verify they are not called anymore when they shouldn't. WDYT?

+

[clamy, 2014/08/28 12:17:01]

nit: unnecessary line.

[carlosk, 2014/08/29 15:54:43]

Done.

+  int destroyCounter;
+

[clamy, 2014/08/28 12:17:01]

nit: unnecessary line.

[carlosk, 2014/08/29 15:54:43]

Done.

+};
+
+// 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.
+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);
+  TestNavigationRequestObserver observer1(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);
+  TestNavigationRequestObserver observer2(request2);
+
+  // Confirms the 1st observer is at the expected state
+  EXPECT_EQ(1, observer1.cancelCounter);
+  EXPECT_EQ(1, observer1.destroyCounter);
+
+  // 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);
+
+  // 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);
+
+  // Confirms both 1st and 2nd observers are at the expected states

[clamy, 2014/08/28 12:17:01]

I would replace this comment with something a bit more explicit:
The first request should have been canceled and destroyed.
The second request should have not been canceled neither destroyed.

[carlosk, 2014/08/29 15:54:43]

Done.

+  EXPECT_EQ(1, observer1.cancelCounter);
+  EXPECT_EQ(1, observer1.destroyCounter);
+  EXPECT_EQ(0, observer2.cancelCounter);
+  EXPECT_EQ(0, observer2.destroyCounter);
+
+  // Finishes the navigation process
+  int32 new_page_id = 1 + contents()->GetMaxPageIDForSiteInstance(
+      active_rvh()->GetSiteInstance());
+  active_test_rvh()->SendNavigate(new_page_id, kUrl2);
+
+  // Confirms both 1st and 2nd observers are at the expected states

[clamy, 2014/08/28 12:17:01]

I would replace the comment with something like:
The second request should have been destroyed but not canceled.

[carlosk, 2014/08/29 15:54:43]

Done.

+  EXPECT_EQ(1, observer1.cancelCounter);
+  EXPECT_EQ(1, observer1.destroyCounter);
+  EXPECT_EQ(0, observer2.cancelCounter);
+  EXPECT_EQ(1, observer2.destroyCounter);
+}
+
 }  // namespace content