PlzNavigate: add cancel navigation logic for uncommitted requests

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 0468f581228542d5a92bb9b4acc0fc7e9aead0de..63431a4f86a6d96690617b026cdb83bf1a50ee85 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/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,
@@ -427,6 +437,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();

[(Do not use) nasko, 2014/08/28 15:55:05]

This seems a bit fragile to me. Once we tell the renderer that the navigation
has committed, we don't need to hold the nav request in RFHM, do we? Since this
is in response to renderer telling us it committed something, there can be race
conditions between quick navigations.

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

I think this was what was discussed between you Nasko and Camille in this week's
meeting which I wasn't able to attend(*). From what Camille told me I understood
you two agreed the correct place to destroy the NavigationRequest instance was
at the DidNavigateFrame call. Did I get something wrong? 

(*): Really sorry about that but it was due to an urgent matter regarding the
relocation of my belongings. :/

+  }
+
   if (!cross_navigation_pending_) {
     DCHECK(!pending_render_frame_host_);
 
@@ -580,6 +598,10 @@ 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 (navigation_request_.get())

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

Please add a comment explaining that if we have a non-commited navigation
request in flight, it should be canceled.

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

Done.

+    navigation_request_->CancelNavigation();
+
   navigation_request_.reset(new NavigationRequest(
       info, frame_tree_node_->frame_tree_node_id()));
   navigation_request_->BeginNavigation(params.request_body);
@@ -590,6 +612,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