Fix aborted navigates not clearing the pending navigation entry with PlzNavigate.

This fixes BrowserTest.ClearPendingOnFailUnlessNTP.

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2380383004
Cr-Commit-Position: refs/heads/master@{#422587}

content/browser/frame_host/navigation_request.cc

@@ -358,6 +358,8 @@ void NavigationRequest::OnResponseStarted(
   if (response->head.headers.get() &&
       (response->head.headers->response_code() == 204 ||
        response->head.headers->response_code() == 205)) {
+    frame_tree_node_->navigator()->DiscardPendingEntryIfNeeded(
+        navigation_handle_.get());
     frame_tree_node_->ResetNavigationRequest(false);
     return;
   }

content/browser/frame_host/navigator.h

@@ -188,6 +188,10 @@ class CONTENT_EXPORT Navigator : public base::RefCounted<Navigator> {
   virtual NavigationHandleImpl* GetNavigationHandleForFrameHost(
       RenderFrameHostImpl* render_frame_host);
 
+  // Called when a navigation has failed or the response is 204/205 to discard
+  // the pending entry in order to avoid url spoofs.
+  virtual void DiscardPendingEntryIfNeeded(NavigationHandleImpl* handle) {}
+
  protected:
   friend class base::RefCounted<Navigator>;
   virtual ~Navigator() {}

content/browser/frame_host/navigator_impl.cc

@@ -246,7 +246,7 @@ void NavigatorImpl::DidFailProvisionalLoadWithError(
   }
 
   // Discard the pending navigation entry if needed.
-  DiscardPendingEntryOnFailureIfNeeded(render_frame_host->navigation_handle());
+  DiscardPendingEntryIfNeeded(render_frame_host->navigation_handle());
 
   if (delegate_) {
     delegate_->DidFailProvisionalLoadWithError(
@@ -952,7 +952,7 @@ void NavigatorImpl::FailedNavigation(FrameTreeNode* frame_tree_node,
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
   DCHECK(navigation_request);
 
-  DiscardPendingEntryOnFailureIfNeeded(navigation_request->navigation_handle());
+  DiscardPendingEntryIfNeeded(navigation_request->navigation_handle());
 
   // If the request was canceled by the user do not show an error page.
   if (error_code == net::ERR_ABORTED) {
@@ -1010,6 +1010,46 @@ NavigationHandleImpl* NavigatorImpl::GetNavigationHandleForFrameHost(
   return render_frame_host->navigation_handle();
 }
 
+void NavigatorImpl::DiscardPendingEntryIfNeeded(NavigationHandleImpl* handle) {
+  // Racy conditions can cause a fail message to arrive after its corresponding
+  // pending entry has been replaced by another navigation. If
+  // |DiscardPendingEntry| is called in this case, then the completely valid
+  // entry for the new navigation would be discarded. See crbug.com/513742. To
+  // catch this case, the current pending entry is compared against the current
+  // navigation handle's entry id, which should correspond to the failed load.
+  NavigationEntry* pending_entry = controller_->GetPendingEntry();
+  bool pending_matches_fail_msg =
+      handle && pending_entry &&
+      handle->pending_nav_entry_id() == pending_entry->GetUniqueID();
+  if (!pending_matches_fail_msg)
+    return;
+
+  // We usually clear the pending entry when it fails, so that an arbitrary URL
+  // isn't left visible above a committed page. This must be enforced when the
+  // pending entry isn't visible (e.g., renderer-initiated navigations) to
+  // prevent URL spoofs for in-page navigations that don't go through
+  // DidStartProvisionalLoadForFrame.
+  //
+  // However, we do preserve the pending entry in some cases, such as on the
+  // initial navigation of an unmodified blank tab. We also allow the delegate
+  // to say when it's safe to leave aborted URLs in the omnibox, to let the
+  // user edit the URL and try again. This may be useful in cases that the
+  // committed page cannot be attacker-controlled. In these cases, we still
+  // allow the view to clear the pending entry and typed URL if the user
+  // requests (e.g., hitting Escape with focus in the address bar).
+  //
+  // Note: don't touch the transient entry, since an interstitial may exist.
+  bool should_preserve_entry = controller_->IsUnmodifiedBlankTab() ||
+                               delegate_->ShouldPreserveAbortedURLs();
+  if (pending_entry != controller_->GetVisibleEntry() ||
+      !should_preserve_entry) {
+    controller_->DiscardPendingEntry(true);
+
+    // Also force the UI to refresh.
+    controller_->delegate()->NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);
+  }
+}
+
 // PlzNavigate
 void NavigatorImpl::RequestNavigation(FrameTreeNode* frame_tree_node,
                                       const GURL& dest_url,
@@ -1167,45 +1207,4 @@ void NavigatorImpl::DidStartMainFrameNavigation(
   }
 }
 
-void NavigatorImpl::DiscardPendingEntryOnFailureIfNeeded(
-    NavigationHandleImpl* handle) {
-  // Racy conditions can cause a fail message to arrive after its corresponding
-  // pending entry has been replaced by another navigation. If
-  // |DiscardPendingEntry| is called in this case, then the completely valid
-  // entry for the new navigation would be discarded. See crbug.com/513742. To
-  // catch this case, the current pending entry is compared against the current
-  // navigation handle's entry id, which should correspond to the failed load.
-  NavigationEntry* pending_entry = controller_->GetPendingEntry();
-  bool pending_matches_fail_msg =
-      handle && pending_entry &&
-      handle->pending_nav_entry_id() == pending_entry->GetUniqueID();
-  if (!pending_matches_fail_msg)
-    return;
-
-  // We usually clear the pending entry when it fails, so that an arbitrary URL
-  // isn't left visible above a committed page. This must be enforced when the
-  // pending entry isn't visible (e.g., renderer-initiated navigations) to
-  // prevent URL spoofs for in-page navigations that don't go through
-  // DidStartProvisionalLoadForFrame.
-  //
-  // However, we do preserve the pending entry in some cases, such as on the
-  // initial navigation of an unmodified blank tab. We also allow the delegate
-  // to say when it's safe to leave aborted URLs in the omnibox, to let the
-  // user edit the URL and try again. This may be useful in cases that the
-  // committed page cannot be attacker-controlled. In these cases, we still
-  // allow the view to clear the pending entry and typed URL if the user
-  // requests (e.g., hitting Escape with focus in the address bar).
-  //
-  // Note: don't touch the transient entry, since an interstitial may exist.
-  bool should_preserve_entry = controller_->IsUnmodifiedBlankTab() ||
-                               delegate_->ShouldPreserveAbortedURLs();
-  if (pending_entry != controller_->GetVisibleEntry() ||
-      !should_preserve_entry) {
-    controller_->DiscardPendingEntry(true);
-
-    // Also force the UI to refresh.
-    controller_->delegate()->NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);
-  }
-}
-
 }  // namespace content

content/browser/frame_host/navigator_impl.h

@@ -97,6 +97,7 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
   void CancelNavigation(FrameTreeNode* frame_tree_node) override;
   NavigationHandleImpl* GetNavigationHandleForFrameHost(
       RenderFrameHostImpl* render_frame_host) override;
+  void DiscardPendingEntryIfNeeded(NavigationHandleImpl* handle) override;
 
  private:
   // Holds data used to track browser side navigation metrics.
@@ -145,10 +146,6 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
                                    SiteInstanceImpl* site_instance,
                                    NavigationHandleImpl* navigation_handle);
 
-  // Called when a navigation has failed to discard the pending entry in order
-  // to avoid url spoofs.
-  void DiscardPendingEntryOnFailureIfNeeded(NavigationHandleImpl* handle);
-
   // The NavigationController that will keep track of session history for all
   // RenderFrameHost objects using this NavigatorImpl.
   // TODO(nasko): Move ownership of the NavigationController from