PlzNavigate: Updated navigation cancel policy for renderer-initiated requests.

content/browser/frame_host/navigator_impl_unittest.cc

Index: content/browser/frame_host/navigator_impl_unittest.cc
diff --git a/content/browser/frame_host/navigator_impl_unittest.cc b/content/browser/frame_host/navigator_impl_unittest.cc
index 142d6d756c487a06bc0d734b1b555111cb6aa223..baa91f411b8bae4c58245655e69e7d32145ff173 100644
--- a/content/browser/frame_host/navigator_impl_unittest.cc
+++ b/content/browser/frame_host/navigator_impl_unittest.cc
@@ -161,13 +161,14 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
 
   EXPECT_FALSE(main_test_rfh()->IsRenderFrameLive());
 
-  // Start a renderer-initiated navigation.
-  main_test_rfh()->SendBeginNavigationWithURL(kUrl);
+  // Start a renderer-initiated non-user-initiated navigation.
+  main_test_rfh()->SendBeginNavigationWithURL(kUrl, false);
   FrameTreeNode* node = main_test_rfh()->frame_tree_node();
   NavigationRequest* request = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(request);
   EXPECT_EQ(kUrl, request->common_params().url);
   EXPECT_FALSE(request->browser_initiated());
+  EXPECT_FALSE(request->begin_params().has_user_gesture);
   EXPECT_EQ(NavigationRequest::STARTED, request->state());
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
@@ -515,6 +516,107 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   EXPECT_EQ(site_instance_id_2, main_test_rfh()->GetSiteInstance()->GetId());
 }
 
+// PlzNavigate: Test that a navigation is canceled if another renderer-initiated
+// user-initiated request has been issued in the meantime.
+TEST_F(NavigatorTestWithBrowserSideNavigation,
+       RendererUserInitiatedNavigationCancel) {
+  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);
+  FrameTreeNode* node = main_test_rfh()->frame_tree_node();
+
+  // Start a browser-initiated navigation to the 1st URL and receive its
+  // beoforeUnload ACK.

[clamy, 2015/02/11 17:22:30]

nit: s/beoforeUnload/beforeUnload

[carlosk, 2015/02/12 12:21:06]

Done.

+  SendRequestNavigation(node, kUrl1);
+  main_test_rfh()->SendBeforeUnloadACK(true);
+  NavigationRequest* request1 = GetNavigationRequestForFrameTreeNode(node);
+  ASSERT_TRUE(request1);
+  EXPECT_EQ(kUrl1, request1->common_params().url);
+  EXPECT_TRUE(request1->browser_initiated());
+  base::WeakPtr<TestNavigationURLLoader> loader1 =
+      GetLoaderForNavigationRequest(request1)->AsWeakPtr();
+
+  // Confirm a speculative RFH was created.
+  TestRenderFrameHost* speculative_rfh = GetSpeculativeRenderFrameHost(node);
+  ASSERT_TRUE(speculative_rfh);
+  int32 site_instance_id_1 = speculative_rfh->GetSiteInstance()->GetId();
+
+  // Now receive a renderer-initiated user-initiated request. It should replace
+  // the current NavigationRequest.
+  main_test_rfh()->SendBeginNavigationWithURL(kUrl2, true);
+  NavigationRequest* request2 = GetNavigationRequestForFrameTreeNode(node);
+  ASSERT_TRUE(request2);
+  EXPECT_EQ(kUrl2, request2->common_params().url);
+  EXPECT_FALSE(request2->browser_initiated());
+  EXPECT_TRUE(request2->begin_params().has_user_gesture);

[clamy, 2015/02/11 17:22:30]

Maybe add a check that loader1 was properly destroyed? I think this is why you
declared it as a WeakPtr.

[carlosk, 2015/02/12 12:21:06]

Yes and done.

+
+  // Confirm that a new speculative RFH was created.
+  speculative_rfh = GetSpeculativeRenderFrameHost(node);
+  ASSERT_TRUE(speculative_rfh);
+  int32 site_instance_id_2 = speculative_rfh->GetSiteInstance()->GetId();
+  EXPECT_NE(site_instance_id_1, site_instance_id_2);
+}
+
+// PlzNavigate: Test that a user-initiated navigation is NOT canceled if a
+// renderer-initiated non-user-initiated request is issued in the meantime.
+TEST_F(NavigatorTestWithBrowserSideNavigation,
+       RendererNonUserInitiatedNavigationDoesntCancel) {
+  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);
+  FrameTreeNode* node = main_test_rfh()->frame_tree_node();
+
+  // Start a renderer-initiated user-initiated navigation to the 1st URL.
+  main_test_rfh()->SendBeginNavigationWithURL(kUrl1, true);
+  NavigationRequest* request = GetNavigationRequestForFrameTreeNode(node);
+  ASSERT_TRUE(request);
+  EXPECT_EQ(kUrl1, request->common_params().url);
+  EXPECT_FALSE(request->browser_initiated());
+  EXPECT_TRUE(request->begin_params().has_user_gesture);
+
+  // Now receive a renderer-initiated non-user-initiated request. Nothing should
+  // change.
+  main_test_rfh()->SendBeginNavigationWithURL(kUrl2, false);
+  ASSERT_EQ(request, GetNavigationRequestForFrameTreeNode(node));
+  EXPECT_EQ(kUrl1, request->common_params().url);
+  EXPECT_FALSE(request->browser_initiated());
+  EXPECT_TRUE(request->begin_params().has_user_gesture);

[clamy, 2015/02/11 17:22:30]

Could you also expend this test to check that a browser-initiated request does
not get canceled by a non user-initiated renderer-initiated navigation? Spoiler:
this is not currently the case as far as I can read the code.

[carlosk, 2015/02/12 12:21:06]

Done. As a new test.

+}
+
+// PlzNavigate: Test that a renderer-initiated non-user-initiated navigation is
+// canceled if a another similar request is issued in the meantime.
+TEST_F(NavigatorTestWithBrowserSideNavigation,
+       RendererNonUserInitiatedNavigationCancelSimilarNavigation) {
+  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);
+  FrameTreeNode* node = main_test_rfh()->frame_tree_node();
+
+  // Start a renderer-initiated non-user-initiated navigation to the 1st URL.
+  main_test_rfh()->SendBeginNavigationWithURL(kUrl1, false);
+  NavigationRequest* request1 = GetNavigationRequestForFrameTreeNode(node);
+  ASSERT_TRUE(request1);
+  EXPECT_EQ(kUrl1, request1->common_params().url);
+  EXPECT_FALSE(request1->browser_initiated());
+  EXPECT_FALSE(request1->begin_params().has_user_gesture);
+
+  // Now receive a 2nd similar request that should replace the current one.
+  main_test_rfh()->SendBeginNavigationWithURL(kUrl2, false);
+  NavigationRequest* request2 = GetNavigationRequestForFrameTreeNode(node);
+  EXPECT_EQ(kUrl2, request2->common_params().url);
+  EXPECT_FALSE(request2->browser_initiated());
+  EXPECT_FALSE(request2->begin_params().has_user_gesture);
+}
+
 // PlzNavigate: Test that a reload navigation is properly signaled to the
 // RenderFrame when the navigation can commit. A speculative RenderFrameHost
 // should not be created at any step.