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

Now the navigation cancellation logic should conform to what has been described
in the "PlzNavigate: Navigation cancellation" design document [1]. Added some
new tests to exercise the cancellation cases described in the document.

Note: This is being developed on top of http://crrev.com/912833002.

[1] https://docs.google.com/a/chromium.org/document/d/1lO7_fgppFTDd8PSQfIb888z7vODcw2Sc8r7h5Rbiwcg/edit#

BUG=376014

Review URL: https://codereview.chromium.org/914223002

Cr-Commit-Position: refs/heads/master@{#317083}

content/browser/frame_host/navigation_request.h

@@ -78,6 +78,8 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate {
 
   const CommonNavigationParams& common_params() const { return common_params_; }
 
+  const BeginNavigationParams& begin_params() const { return begin_params_; }
+
   const CommitNavigationParams& commit_params() const { return commit_params_; }
 
   NavigationURLLoader* loader_for_testing() const { return loader_.get(); }

content/browser/frame_host/navigator_impl.cc

@@ -703,14 +703,27 @@ void NavigatorImpl::OnBeginNavigation(
     const CommonNavigationParams& common_params,
     const BeginNavigationParams& begin_params,
     scoped_refptr<ResourceRequestBody> body) {
+  // This is a renderer-initiated navigation.
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   DCHECK(frame_tree_node);
 
-  // This is a renderer-initiated navigation, so generate a new
-  // NavigationRequest and store it in the map.
-  // TODO(clamy): Renderer-initiated navigations should not always cancel the
-  // current one.
+  NavigationRequest* ongoing_navigation_request =
+      navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
+
+  // The renderer-initiated navigation request is ignored iff a) there is an
+  // ongoing request b) which is browser or user-initiated and c) the renderer
+  // request is not user-initiated.
+  if (ongoing_navigation_request &&
+      (ongoing_navigation_request->browser_initiated() ||
+       ongoing_navigation_request->begin_params().has_user_gesture) &&
+      !begin_params.has_user_gesture) {
+    return;
+  }
+
+  // In all other cases the current navigation, if any, is canceled and a new
+  // NavigationRequest is created and stored in the map. Actual cancellation
+  // happens when the existing request map entry is replaced and destroyed.
   scoped_ptr<NavigationRequest> navigation_request =
       NavigationRequest::CreateRendererInitiated(
           frame_tree_node, common_params, begin_params, body);

content/browser/frame_host/navigator_impl_unittest.cc

@@ -139,8 +139,8 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   EXPECT_FALSE(GetNavigationRequestForFrameTreeNode(node));
   EXPECT_FALSE(node->render_manager()->pending_frame_host());
 
-  // The main RFH should not have been changed, and the renderer should have
-  // been initialized.
+  // The main RenderFrameHost should not have been changed, and the renderer
+  // should have been initialized.
   EXPECT_EQ(site_instance_id, main_test_rfh()->GetSiteInstance()->GetId());
   EXPECT_TRUE(main_test_rfh()->IsRenderFrameLive());
 
@@ -161,9 +161,9 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   contents()->NavigateAndCommit(kUrl1);
   EXPECT_TRUE(main_test_rfh()->IsRenderFrameLive());
 
-  // Start a renderer-initiated navigation.
+  // Start a renderer-initiated non-user-initiated navigation.
   process()->sink().ClearMessages();
-  main_test_rfh()->SendBeginNavigationWithURL(kUrl2);
+  main_test_rfh()->SendBeginNavigationWithURL(kUrl2, false);
   FrameTreeNode* node = main_test_rfh()->frame_tree_node();
   NavigationRequest* request = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(request);
@@ -171,6 +171,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   // The navigation is immediately started as there's no need to wait for
   // beforeUnload to be executed.
   EXPECT_EQ(NavigationRequest::STARTED, request->state());
+  EXPECT_FALSE(request->begin_params().has_user_gesture);
   EXPECT_EQ(kUrl2, request->common_params().url);
   EXPECT_FALSE(request->browser_initiated());
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
@@ -375,9 +376,12 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, CrossSiteNavigation) {
   // Navigate to a different site.
   process()->sink().ClearMessages();
   RequestNavigation(node, kUrl2);
-  main_test_rfh()->SendBeforeUnloadACK(true);
   NavigationRequest* main_request = GetNavigationRequestForFrameTreeNode(node);
   ASSERT_TRUE(main_request);
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
+
+  // Receive the beforeUnload ACK.
+  main_test_rfh()->SendBeforeUnloadACK(true);
   EXPECT_TRUE(GetSpeculativeRenderFrameHost(node));
 
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
@@ -430,7 +434,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation, RedirectCrossSite) {
   EXPECT_EQ(1, GetLoaderForNavigationRequest(main_request)->redirect_count());
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
-  // Request the RenderFrameHost to commit.
+  // Have the RenderFrameHost commit the navigation.
   response = new ResourceResponse;
   GetLoaderForNavigationRequest(main_request)->CallOnResponseStarted(
       response, MakeEmptyStream());
@@ -475,8 +479,9 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   EXPECT_TRUE(request1->browser_initiated());
   base::WeakPtr<TestNavigationURLLoader> loader1 =
       GetLoaderForNavigationRequest(request1)->AsWeakPtr();
+  EXPECT_TRUE(loader1);
 
-  // Confirm a speculative RFH was created.
+  // Confirm a speculative RenderFrameHost was created.
   TestRenderFrameHost* speculative_rfh = GetSpeculativeRenderFrameHost(node);
   ASSERT_TRUE(speculative_rfh);
   int32 site_instance_id_1 = speculative_rfh->GetSiteInstance()->GetId();
@@ -494,13 +499,13 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   // Confirm that the first loader got destroyed.
   EXPECT_FALSE(loader1);
 
-  // Confirm that a new speculative RFH was created.
+  // Confirm that a new speculative RenderFrameHost 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);
 
-  // Request the RenderFrameHost to commit.
+  // Have the RenderFrameHost commit the navigation.
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
   GetLoaderForNavigationRequest(request2)->CallOnResponseStarted(
       response, MakeEmptyStream());
@@ -515,10 +520,220 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   EXPECT_EQ(kUrl2_site, main_test_rfh()->GetSiteInstance()->GetSiteURL());
   EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL());
 
-  // Confirm that the committed RFH is the latest speculative one.
+  // Confirm that the committed RenderFrameHost is the latest speculative one.
   EXPECT_EQ(site_instance_id_2, main_test_rfh()->GetSiteInstance()->GetId());
 }
 
+// PlzNavigate: Test that a browser-initiated navigation is canceled if a
+// 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
+  // beforeUnload ACK.
+  process()->sink().ClearMessages();
+  RequestNavigation(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();
+  EXPECT_TRUE(loader1);
+
+  // Confirm a speculative RenderFrameHost 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);
+
+  // Confirm that the first loader got destroyed.
+  EXPECT_FALSE(loader1);
+
+  // Confirm that a new speculative RenderFrameHost 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);
+
+  // Have the RenderFrameHost commit the navigation.
+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+  GetLoaderForNavigationRequest(request2)
+      ->CallOnResponseStarted(response, MakeEmptyStream());
+  EXPECT_TRUE(DidRenderFrameHostRequestCommit(speculative_rfh));
+  EXPECT_FALSE(DidRenderFrameHostRequestCommit(main_test_rfh()));
+
+  // Commit the navigation.
+  speculative_rfh->SendNavigate(0, kUrl2);
+
+  // Confirm that the commit corresponds to the new request.
+  ASSERT_TRUE(main_test_rfh());
+  EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL());
+
+  // Confirm that the committed RenderFrameHost is the latest speculative one.
+  EXPECT_EQ(site_instance_id_2, main_test_rfh()->GetSiteInstance()->GetId());
+}
+
+// PlzNavigate: Test that a renderer-initiated user-initiated navigation is NOT
+// canceled if a renderer-initiated non-user-initiated request is issued in the
+// meantime.
+TEST_F(NavigatorTestWithBrowserSideNavigation,
+       RendererNonUserInitiatedNavigationDoesntCancelRendererUserInitiated) {
+  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.
+  process()->sink().ClearMessages();
+  main_test_rfh()->SendBeginNavigationWithURL(kUrl1, true);
+  NavigationRequest* request1 = GetNavigationRequestForFrameTreeNode(node);
+  ASSERT_TRUE(request1);
+  EXPECT_EQ(kUrl1, request1->common_params().url);
+  EXPECT_FALSE(request1->browser_initiated());
+  EXPECT_TRUE(request1->begin_params().has_user_gesture);
+  EXPECT_TRUE(GetSpeculativeRenderFrameHost(node));
+
+  // Now receive a renderer-initiated non-user-initiated request. Nothing should
+  // change.
+  main_test_rfh()->SendBeginNavigationWithURL(kUrl2, false);
+  NavigationRequest* request2 = GetNavigationRequestForFrameTreeNode(node);
+  ASSERT_TRUE(request2);
+  EXPECT_EQ(request1, request2);
+  EXPECT_EQ(kUrl1, request2->common_params().url);
+  EXPECT_FALSE(request2->browser_initiated());
+  EXPECT_TRUE(request2->begin_params().has_user_gesture);
+  TestRenderFrameHost* speculative_rfh = GetSpeculativeRenderFrameHost(node);
+  ASSERT_TRUE(speculative_rfh);
+
+  // Have the RenderFrameHost commit the navigation.
+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+  GetLoaderForNavigationRequest(request2)
+      ->CallOnResponseStarted(response, MakeEmptyStream());
+  EXPECT_TRUE(DidRenderFrameHostRequestCommit(speculative_rfh));
+  EXPECT_FALSE(DidRenderFrameHostRequestCommit(main_test_rfh()));
+
+  // Commit the navigation.
+  speculative_rfh->SendNavigate(0, kUrl1);
+  EXPECT_EQ(kUrl1, contents()->GetLastCommittedURL());
+}
+
+// PlzNavigate: Test that a browser-initiated navigation is NOT canceled if a
+// renderer-initiated non-user-initiated request is issued in the meantime.
+TEST_F(NavigatorTestWithBrowserSideNavigation,
+       RendererNonUserInitiatedNavigationDoesntCancelBrowserInitiated) {
+  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.
+  process()->sink().ClearMessages();
+  RequestNavigation(node, kUrl1);
+  NavigationRequest* request1 = GetNavigationRequestForFrameTreeNode(node);
+  ASSERT_TRUE(request1);
+  EXPECT_EQ(kUrl1, request1->common_params().url);
+  EXPECT_TRUE(request1->browser_initiated());
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
+
+  // Now receive a renderer-initiated non-user-initiated request. Nothing should
+  // change.
+  main_test_rfh()->SendBeginNavigationWithURL(kUrl2, false);
+  NavigationRequest* request2 = GetNavigationRequestForFrameTreeNode(node);
+  ASSERT_TRUE(request2);
+  EXPECT_EQ(request1, request2);
+  EXPECT_EQ(kUrl1, request2->common_params().url);
+  EXPECT_TRUE(request2->browser_initiated());
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
+
+  // Now receive the beforeUnload ACK from the still ongoing navigation.
+  main_test_rfh()->SendBeforeUnloadACK(true);
+  TestRenderFrameHost* speculative_rfh = GetSpeculativeRenderFrameHost(node);
+  ASSERT_TRUE(speculative_rfh);
+
+  // Have the RenderFrameHost commit the navigation.
+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+  GetLoaderForNavigationRequest(request2)
+      ->CallOnResponseStarted(response, MakeEmptyStream());
+  EXPECT_TRUE(DidRenderFrameHostRequestCommit(speculative_rfh));
+  EXPECT_FALSE(DidRenderFrameHostRequestCommit(main_test_rfh()));
+
+  // Commit the navigation.
+  speculative_rfh->SendNavigate(0, kUrl1);
+  EXPECT_EQ(kUrl1, contents()->GetLastCommittedURL());
+}
+
+// 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.
+  process()->sink().ClearMessages();
+  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);
+  EXPECT_TRUE(GetSpeculativeRenderFrameHost(node));
+  base::WeakPtr<TestNavigationURLLoader> loader1 =
+      GetLoaderForNavigationRequest(request1)->AsWeakPtr();
+  EXPECT_TRUE(loader1);
+
+  // 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);
+  TestRenderFrameHost* speculative_rfh = GetSpeculativeRenderFrameHost(node);
+  ASSERT_TRUE(speculative_rfh);
+
+  // Confirm that the first loader got destroyed.
+  EXPECT_FALSE(loader1);
+
+  // Have the RenderFrameHost commit the navigation.
+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+  GetLoaderForNavigationRequest(request2)
+      ->CallOnResponseStarted(response, MakeEmptyStream());
+  EXPECT_TRUE(DidRenderFrameHostRequestCommit(speculative_rfh));
+  EXPECT_FALSE(DidRenderFrameHostRequestCommit(main_test_rfh()));
+
+  // Commit the navigation.
+  speculative_rfh->SendNavigate(0, kUrl2);
+  EXPECT_EQ(kUrl2, contents()->GetLastCommittedURL());
+}
+
 // 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.
@@ -567,6 +782,9 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   const GURL kUrl("http://google.com/");
   process()->sink().ClearMessages();
   RequestNavigation(node, kUrl);
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
+
+  // Receive the beforeUnload ACK.
   main_test_rfh()->SendBeforeUnloadACK(true);
   TestRenderFrameHost* speculative_rfh = GetSpeculativeRenderFrameHost(node);
   ASSERT_TRUE(speculative_rfh);
@@ -608,6 +826,9 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   const GURL kUrl("http://google.com/");
   process()->sink().ClearMessages();
   RequestNavigation(node, kUrl);
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
+
+  // Receive the beforeUnload ACK.
   main_test_rfh()->SendBeforeUnloadACK(true);
   TestRenderFrameHost* speculative_rfh = GetSpeculativeRenderFrameHost(node);
   int32 site_instance_id = speculative_rfh->GetSiteInstance()->GetId();
@@ -634,8 +855,8 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
   speculative_rfh = GetSpeculativeRenderFrameHost(node);
   ASSERT_TRUE(speculative_rfh);
 
-  // For now, ensure that the speculative RFH does not change after the
-  // redirect.
+  // For now, ensure that the speculative RenderFrameHost does not change after
+  // the redirect.
   // TODO(carlosk): once the speculative RenderFrameHost updates with redirects
   // this next check will be changed to verify that it actually happens.
   EXPECT_EQ(site_instance_id, speculative_rfh->GetSiteInstance()->GetId());
@@ -698,6 +919,8 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
       ->sink()
       .ClearMessages();
   RequestNavigation(node, kUrl1);
+  EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
+
   main_test_rfh()->SendBeforeUnloadACK(true);
   EXPECT_EQ(rfh1, GetSpeculativeRenderFrameHost(node));
   EXPECT_NE(RenderFrameHostImpl::STATE_DEFAULT,

content/test/test_render_frame_host.cc

@@ -187,9 +187,10 @@ void TestRenderFrameHost::SendNavigateWithParameters(
   OnDidCommitProvisionalLoad(msg);
 }
 
-void TestRenderFrameHost::SendBeginNavigationWithURL(const GURL& url) {
-  BeginNavigationParams begin_params(
-      "GET", std::string(), net::LOAD_NORMAL, false);
+void TestRenderFrameHost::SendBeginNavigationWithURL(const GURL& url,
+                                                     bool has_user_gesture) {
+  BeginNavigationParams begin_params("GET", std::string(), net::LOAD_NORMAL,
+                                     has_user_gesture);
   CommonNavigationParams common_params;
   common_params.url = url;
   common_params.referrer = Referrer(GURL(), blink::WebReferrerPolicyDefault);
@@ -215,9 +216,9 @@ void TestRenderFrameHost::PrepareForCommit(const GURL& url) {
       static_cast<NavigatorImpl*>(frame_tree_node_->navigator())
           ->GetNavigationRequestForNodeForTesting(frame_tree_node_);
 
-  // We are simulating a renderer-initiated navigation.
+  // We are simulating a renderer-initiated user-initiated navigation.
   if (!request) {
-    SendBeginNavigationWithURL(url);
+    SendBeginNavigationWithURL(url, true);
     request = static_cast<NavigatorImpl*>(frame_tree_node_->navigator())
         ->GetNavigationRequestForNodeForTesting(frame_tree_node_);
   }

content/test/test_render_frame_host.h

@@ -85,7 +85,7 @@ class TestRenderFrameHost : public RenderFrameHostImpl,
       int response_code,
       const base::FilePath* file_path_for_history_item,
       const std::vector<GURL>& redirects);
-  void SendBeginNavigationWithURL(const GURL& url);
+  void SendBeginNavigationWithURL(const GURL& url, bool has_user_gesture);
 
   void DidDisownOpener();