Ensure that when a new navigation cancels an existing one, the old navigation's DidFinishNavigati...

content/browser/frame_host/frame_tree_node.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/frame_tree_node.h"
 
 #include <queue>
 #include <utility>
 
 #include "base/lazy_instance.h"
@@ skipping 365 matching lines @@
   // state, this matches what Blink is doing: Blink doesn't send throbber
   // notifications for Javascript URLS.
   DCHECK(!navigation_request->common_params().url.SchemeIs(
       url::kJavaScriptScheme));
 
   bool was_previously_loading = frame_tree()->IsLoading();
 
   // There's no need to reset the state: there's still an ongoing load, and the
   // RenderFrameHostManager will take care of updates to the speculative
   // RenderFrameHost in DidCreateNavigationRequest below.
-  if (was_previously_loading)
+  if (was_previously_loading) {
+    if (navigation_request_) {
+      // Mark the old request as aborted.
+      navigation_request_->navigation_handle()->set_net_error_code(

[Charlie Reis, 2017/03/24 20:11:09]

I'm curious if this belongs in ResetNavigationRequest, since several other call
sites do this as well.  Looks like there's 16 call sites, so I wonder how many
of the other ones need it.

Looking closer, I suppose CommitNavigation is one that definitely doesn't need
it, and it might be unfortunate to add yet another bool parameter to
ResetNavigationRequest.  Still, I wouldn't be surprised if there are more places
that need this.

[jam, 2017/03/24 22:08:26]

That's a good question; I didn't see any obvious places in the other callsites,
but I didn't test with and without PlzNavigate to see what happens. Here are the
callsites that don't already have setters, and should probably be audited. I've
filed https://bugs.chromium.org/p/chromium/issues/detail?id=705119

NavigationRequest::OnRequestRedirected
-FilterURL fails

NavigationRequest::OnResponseStarted
-ShouldTransferNavigation returns false
  -this only happens for extensions

NavigatorImpl::OnBeginNavigation
-sends a stop IPC, which when it comes back should set the abort error code 

NavigatorImpl::CancelNavigation
RenderFrameHostImpl::DispatchBeforeUnload
RenderFrameHostManager::CancelPendingIfNecessary
-need to investigate what should happen

RenderFrameHostImpl::FailedNavigation
-should already be setting it today

+          net::ERR_ABORTED);
+    }
     ResetNavigationRequest(true, true);
+  }
 
   navigation_request_ = std::move(navigation_request);
   render_manager()->DidCreateNavigationRequest(navigation_request_.get());
 
   bool to_different_document = !FrameMsg_Navigate_Type::IsSameDocument(
       navigation_request_->common_params().navigation_type);
 
   DidStartLoading(to_different_document, was_previously_loading);
 }
 
@@ skipping 166 matching lines @@
       }
       return parent_->child_at(i + relative_offset);
     }
   }
 
   NOTREACHED() << "FrameTreeNode not found in its parent's children.";
   return nullptr;
 }
 
 }  // namespace content