PlzNavigate: add cancel navigation logic for uncommitted requests

content/browser/frame_host/render_frame_host_manager.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 "content/browser/frame_host/render_frame_host_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 88 matching lines @@
 
 RenderFrameHostManager::~RenderFrameHostManager() {
   if (pending_render_frame_host_)
     CancelPending();
 
   // We should always have a current RenderFrameHost except in some tests.
   SetRenderFrameHost(scoped_ptr<RenderFrameHostImpl>());
 
   // 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,
                                   SiteInstance* site_instance,
                                   int view_routing_id,
                                   int frame_routing_id) {
   // Create a RenderViewHost and RenderFrameHost, once we have an instance.  It
   // is important to immediately give this SiteInstance to a RenderViewHost so
   // that the SiteInstance is ref counted.
   if (!site_instance)
@@ skipping 301 matching lines @@
       static_cast<RenderProcessHostImpl*>(render_frame_host_->GetProcess());
   process->ResumeResponseDeferredAtStart(*response_started_id_);
 
   render_frame_host_->ClearPendingTransitionRequestData();
 
   response_started_id_.reset();
 }
 
 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_);
 
     // We should only hear this from our current renderer.
     DCHECK_EQ(render_frame_host_, render_frame_host);
 
     // Even when there is no pending RVH, there may be a pending Web UI.
     if (pending_web_ui())
       CommitPending();
     return;
@@ skipping 133 matching lines @@
       params.url : frame_tree_node_->frame_tree()->root()->current_url();
   info.is_main_frame = frame_tree_node_->IsMainFrame();
   info.parent_is_main_frame = !frame_tree_node_->parent() ?
       false : frame_tree_node_->parent()->IsMainFrame();
   info.is_showing = GetRenderWidgetHostView()->IsShowing();
 
   // TODO(clamy): Check if the current RFH should be initialized (in case it has
   // 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);
 }
 
 // PlzNavigate
 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
   // some storing in RequestNavigation.
   SiteInstance* new_instance = GetSiteInstanceForNavigation(
       info.navigation_url,
       NULL,
       navigation_request_->info().navigation_params.transition_type,
       false,
       false);
@@ skipping 1055 matching lines @@
 void RenderFrameHostManager::DeleteRenderFrameProxyHost(
     SiteInstance* instance) {
   RenderFrameProxyHostMap::iterator iter = proxy_hosts_.find(instance->GetId());
   if (iter != proxy_hosts_.end()) {
     delete iter->second;
     proxy_hosts_.erase(iter);
   }
 }
 
 }  // namespace content