PlzNavigate: add cancel navigation logic for uncommitted requests

Allows navigation requests to be canceled before they are committed when they become stale.

BUG=397998

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

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

content/browser/frame_host/navigation_before_commit_info.cc

+// Copyright 2014 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_before_commit_info.h"
+
+namespace content {
+
+NavigationBeforeCommitInfo::NavigationBeforeCommitInfo()
+    : navigation_request_id(-1) {}
+
+}  // namespace content

content/browser/frame_host/navigation_before_commit_info.h

@@ -8,6 +8,8 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/time/time.h"
+#include "content/common/content_export.h"
 #include "content/public/common/page_transition_types.h"
 #include "content/public/common/referrer.h"
 #include "url/gurl.h"
@@ -20,7 +22,7 @@ namespace content {
 // thread where it should be used to send a FrameMsg_CommitNavigation message to
 // the renderer.
 struct NavigationBeforeCommitInfo {
-  NavigationBeforeCommitInfo() {};
+  CONTENT_EXPORT NavigationBeforeCommitInfo();
 
   // The url that is actually being loaded.
   GURL navigation_url;
@@ -35,6 +37,9 @@ struct NavigationBeforeCommitInfo {
   // The navigationStart time to expose to JS for this navigation.
   // TODO(clamy): Add the other values that matter to the Navigation Timing API.
   base::TimeTicks browser_navigation_start;
+
+  // The unique ID of this navigation request.
+  int64 navigation_request_id;
 };
 
 }  // namespace content

content/browser/frame_host/navigation_request.cc

@@ -13,25 +13,35 @@ namespace content {
 
 namespace {
 
+// The next available browser-global navigation request ID.
+static int64 next_navigation_request_id_ = 0;
+
 void OnBeginNavigation(const NavigationRequestInfo& info,
                        scoped_refptr<ResourceRequestBody> request_body,
-                       int64 frame_node_id) {
+                       int64 navigation_request_id,
+                       int64 frame_tree_node_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  ResourceDispatcherHostImpl::Get()->NavigationRequest(
-      info, request_body, frame_node_id);
+  ResourceDispatcherHostImpl::Get()->StartNavigationRequest(
+      info, request_body, navigation_request_id, frame_tree_node_id);
+}
+
+void CancelNavigationRequest(int64 navigation_request_id,
+                             int64 frame_tree_node_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  ResourceDispatcherHostImpl::Get()->CancelNavigationRequest(
+      navigation_request_id, frame_tree_node_id);
 }
 
 }  // namespace
 
 NavigationRequest::NavigationRequest(const NavigationRequestInfo& info,
-                                     int64 frame_node_id)
-  : info_(info),
-    frame_node_id_(frame_node_id) {
+                                     int64 frame_tree_node_id)
+  : navigation_request_id_(++next_navigation_request_id_),
+    info_(info),
+    frame_tree_node_id_(frame_tree_node_id) {
 }
 
 NavigationRequest::~NavigationRequest() {
-  // TODO(clamy): Cancel the corresponding request in ResourceDispatcherHost if
-  // it has not commited yet.
 }
 
 void NavigationRequest::BeginNavigation(
@@ -40,7 +50,20 @@ void NavigationRequest::BeginNavigation(
   BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
-      base::Bind(&OnBeginNavigation, info_, request_body, frame_node_id_));
+      base::Bind(&OnBeginNavigation,
+                 info_,
+                 request_body,
+                 navigation_request_id_,
+                 frame_tree_node_id_));
+}
+
+void NavigationRequest::CancelNavigation() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&CancelNavigationRequest,
+                 navigation_request_id_, frame_tree_node_id_));
 }
 
 }  // namespace content

content/browser/frame_host/navigation_request.h

@@ -8,6 +8,7 @@
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "content/browser/frame_host/navigation_request_info.h"
+#include "content/common/content_export.h"
 
 namespace content {
 class ResourceRequestBody;
@@ -20,21 +21,30 @@ class ResourceRequestBody;
 // the navigation following its refactoring.
 class NavigationRequest {
  public:
-  NavigationRequest(const NavigationRequestInfo& info, int64 frame_node_id);
+  NavigationRequest(const NavigationRequestInfo& info,
+                    int64 frame_tree_node_id);
 
   ~NavigationRequest();
 
-  const NavigationRequestInfo& info() const { return info_; }
-  int64 frame_node_id() const { return frame_node_id_; }
-
   // Called on the UI thread by the RenderFrameHostManager which owns the
   // NavigationRequest. After calling this function, |body| can no longer be
   // manipulated on the UI thread.
   void BeginNavigation(scoped_refptr<ResourceRequestBody> body);
 
+  // Called on the UI thread by the RenderFrameHostManager which owns the
+  // NavigationRequest whenever this navigation request should be canceled.
+  void CancelNavigation();
+
+  const NavigationRequestInfo& info() const { return info_; }
+
+  int64 frame_tree_node_id() const { return frame_tree_node_id_; }
+
+  int64 navigation_request_id() const { return navigation_request_id_; }
+
  private:
+  const int64 navigation_request_id_;
   const NavigationRequestInfo info_;
-  const int64 frame_node_id_;
+  const int64 frame_tree_node_id_;
 
   DISALLOW_COPY_AND_ASSIGN(NavigationRequest);
 };

content/browser/frame_host/render_frame_host_manager.cc

@@ -106,6 +106,16 @@ RenderFrameHostManager::~RenderFrameHostManager() {
 
   // Delete any swapped out RenderFrameHosts.
   STLDeleteValues(&proxy_hosts_);
+
+  // PlzNavigate
+  // There is an active navigation request for this RFHM so it needs to be
+  // canceled.
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation)) {
+    if (navigation_request_.get())
+      navigation_request_->CancelNavigation();
+  }
+
 }
 
 void RenderFrameHostManager::Init(BrowserContext* browser_context,
@@ -428,6 +438,14 @@ void RenderFrameHostManager::ResumeResponseDeferredAtStart() {
 
 void RenderFrameHostManager::DidNavigateFrame(
     RenderFrameHostImpl* render_frame_host) {
+  // PlzNavigate
+  // The navigation request has been committed so the browser process doesn't
+  // need to care about it anymore.
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation)) {
+    navigation_request_.reset();
+  }
+
   if (!cross_navigation_pending_) {
     DCHECK(!pending_render_frame_host_);
 
@@ -582,6 +600,11 @@ void RenderFrameHostManager::OnBeginNavigation(
   // crashed) not to display a sad tab while navigating.
   // TODO(clamy): Spawn a speculative renderer process if we do not have one to
   // use for the navigation.
+
+  // If there is an ongoing request it must be canceled.
+  if (navigation_request_.get())
+    navigation_request_->CancelNavigation();
+
   navigation_request_.reset(new NavigationRequest(
       info, frame_tree_node_->frame_tree_node_id()));
   navigation_request_->BeginNavigation(params.request_body);
@@ -592,6 +615,14 @@ void RenderFrameHostManager::CommitNavigation(
     const NavigationBeforeCommitInfo& info) {
   CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
+  DCHECK(navigation_request_.get());
+  // Ignores navigation commits if the request ID doesn't match the current
+  // active request.
+  if (navigation_request_->navigation_request_id() !=
+          info.navigation_request_id) {
+    return;
+  }
+
   // Pick the right RenderFrameHost to commit the navigation.
   SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
   // TODO(clamy): Replace the default values by the right ones. This may require

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
@@ -1755,6 +1758,7 @@ TEST_F(RenderFrameHostManagerTest,
   // Now commit the same url.
   NavigationBeforeCommitInfo commit_info;
   commit_info.navigation_url = kUrl;
+  commit_info.navigation_request_id = main_request->navigation_request_id();
   render_manager->CommitNavigation(commit_info);
   main_request = GetNavigationRequestForRenderFrameManager(render_manager);
 
@@ -1790,10 +1794,60 @@ 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);
   EXPECT_TRUE(main_test_rfh()->render_view_host()->IsRenderViewLive());
 }
 
+// PlzNavigate: Test that a navigation commit is ignored if another request has
+// been issued in the meantime.
+// TODO(carlosk): add checks to assert that the cancel call was sent to
+// ResourceDispatcherHost in the IO thread by extending
+// ResourceDispatcherHostDelegate (like in cross_site_transfer_browsertest.cc
+// and plugin_browsertest.cc).
+TEST_F(RenderFrameHostManagerTest,
+       BrowserSideNavigationIgnoreStaleNavigationCommit) {
+  const GURL kUrl0("http://www.wikipedia.org/");
+  const GURL kUrl0_site = SiteInstance::GetSiteForURL(browser_context(), kUrl0);
+  const GURL kUrl1("http://www.chromium.org/");
+  const GURL kUrl2("http://www.google.com/");
+  const GURL kUrl2_site = SiteInstance::GetSiteForURL(browser_context(), kUrl2);
+
+  // Initialization.
+  contents()->NavigateAndCommit(kUrl0);
+  RenderFrameHostManager* render_manager =
+      main_test_rfh()->frame_tree_node()->render_manager();
+  EnableBrowserSideNavigation();
+  EXPECT_EQ(kUrl0_site, main_test_rfh()->GetSiteInstance()->GetSiteURL());
+
+  // 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(kUrl0_site, main_test_rfh()->GetSiteInstance()->GetSiteURL());
+
+  // 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_EQ(kUrl2_site, main_test_rfh()->GetSiteInstance()->GetSiteURL());
+}
+
 }  // namespace content

content/browser/loader/resource_dispatcher_host_impl.cc

@@ -1660,9 +1660,16 @@ void ResourceDispatcherHostImpl::FinishedWithResourcesForRequest(
   IncrementOutstandingRequestsCount(-1, *info);
 }
 
-void ResourceDispatcherHostImpl::NavigationRequest(
+void ResourceDispatcherHostImpl::StartNavigationRequest(
     const NavigationRequestInfo& info,
     scoped_refptr<ResourceRequestBody> request_body,
+    int64 navigation_request_id,
+    int64 frame_node_id) {
+  NOTIMPLEMENTED();
+}
+
+void ResourceDispatcherHostImpl::CancelNavigationRequest(
+    int64 navigation_request_id,
     int64 frame_node_id) {
   NOTIMPLEMENTED();
 }

content/browser/loader/resource_dispatcher_host_impl.h

@@ -227,7 +227,7 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl
 
   // Must be called after the ResourceRequestInfo has been created
   // and associated with the request.  If |payload| is set to a non-empty value,
-  // the value will be sent to the old resource handler instead of cancelling
+  // the value will be sent to the old resource handler instead of canceling
   // it, except on HTTP errors.
   scoped_ptr<ResourceHandler> MaybeInterceptAsStream(
       net::URLRequest* request,
@@ -249,11 +249,20 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl
   // elsewhere.
   void FinishedWithResourcesForRequest(const net::URLRequest* request_);
 
+  // PlzNavigate
   // Called by NavigationRequest to start a navigation request in the node
   // identified by |frame_node_id|.
-  void NavigationRequest(const NavigationRequestInfo& info,
-                         scoped_refptr<ResourceRequestBody> request_body,
-                         int64 frame_node_id);
+  void StartNavigationRequest(const NavigationRequestInfo& info,
+                              scoped_refptr<ResourceRequestBody> request_body,
+                              int64 navigation_request_id,
+                              int64 frame_node_id);
+
+  // PlzNavigate
+  // Called by NavigationRequest to cancel a navigation request with the
+  // provided |navigation_request_id| in the node identified by
+  // |frame_node_id|.
+  void CancelNavigationRequest(int64 navigation_request_id,
+                               int64 frame_node_id);
 
  private:
   friend class ResourceDispatcherHostTest;

content/content_browser.gypi

@@ -587,6 +587,7 @@
       'browser/frame_host/interstitial_page_impl.h',
       'browser/frame_host/interstitial_page_navigator_impl.cc',
       'browser/frame_host/interstitial_page_navigator_impl.h',
+      'browser/frame_host/navigation_before_commit_info.cc',
       'browser/frame_host/navigation_before_commit_info.h',
       'browser/frame_host/navigation_controller_android.cc',
       'browser/frame_host/navigation_controller_android.h',