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 553 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.
   navigation_request_.reset(new NavigationRequest(

[nasko, 2014/08/21 21:53:44]

nit: I'd add a comment here to say that deleting the existing navigation request
will cancel it. This way is non-obvious what is happening.

[carlosk, 2014/08/22 13:43:03]

Comment added. 

But note that right now we're "cancelling" everything when in fact not all NR
destructor calls will indeed cause a cancellation (the request might even have
been committed). See comment at line 44 @
content/browser/frame_host/navigation_request.cc .

[nasko, 2014/08/22 14:00:34]

If the navigation was committed, I would expect the navigation_request_ member
to be cleared, since it should be used only between starting a request and
committing it. So if one does exist, it must be cancelled (through the act of
destructing it).

[carlosk, 2014/08/22 16:54:41]

I did talk with clamy about this specific thing and IIRC she had some reasons
for the request not to be cleared out like that. She's OoO today so I suggest we
postpone this discussion for next week. 

       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 1056 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