Remove pre-NavigationClient code for ignoring CommitNavigation().

AbortClientNavigation() resets the navigation client message pipe, so it
is impossible to be in a state where the navigation client message pipe
is still connected but the commit should be ignored.

Unfortunately, this doesn't mean that the race previously fixed in
https://crbug.com/763106 is gone. It just means that the browser process
can send a CommitNavigation() that the renderer will never hear about
because the renderer's endpoint is already gone.

There is one tricky case with CommitNavigationWithParams(), which
previously also checked if the commit should be ignored. This method is
asynchronously called by MHTMLBodyLoaderClient when the MHTML archive
body is fully loaded. However, since MHTMLBodyLoaderClient is reset with
the same timing as the navigation client message pipe, this turns out
not to be a problem in practice.

Bug: 1020175
Change-Id: I3a8dc816030071a30f315ebc62a819ef3cde143a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2493669
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820307}

content/renderer/render_frame_impl.cc

@@ -3100,18 +3100,6 @@ void RenderFrameImpl::CommitNavigation(
   AssertNavigationCommits assert_navigation_commits(
       this, kMayReplaceInitialEmptyDocument);
 
-  if (ShouldIgnoreCommitNavigation(*commit_params)) {
-    browser_side_navigation_pending_url_ = GURL();
-    AbortCommitNavigation();
-
-    // Disarm AssertNavigationCommits and pretend that the commit actually
-    // happened. Signalling that the load stopped /should/ signal the browser to
-    // tear down the speculative RFH associated with |this|...
-    navigation_commit_state_ = NavigationCommitState::kDidCommit;
-
-    return;
-  }
-
   SetOldPageLifecycleStateFromNewPageCommitIfNeeded(
       commit_params->old_page_info.get());
 
@@ -3239,28 +3227,6 @@ void RenderFrameImpl::CommitNavigation(
   std::move(commit_with_params).Run(std::move(navigation_params));
 }
 
-bool RenderFrameImpl::ShouldIgnoreCommitNavigation(
-    const mojom::CommitNavigationParams& commit_params) {
-  // We can ignore renderer-initiated navigations (nav_entry_id == 0) which
-  // have been canceled in the renderer, but browser was not aware yet at the
-  // moment of issuing a CommitNavigation call.
-  if (!browser_side_navigation_pending_ &&
-      !browser_side_navigation_pending_url_.is_empty() &&
-      browser_side_navigation_pending_url_ == commit_params.original_url &&
-      commit_params.nav_entry_id == 0) {
-    // If a navigation has been cancelled in the renderer, the renderer must
-    // have already committed and swapped this frame into the frame tree:
-    // - a provisional frame cannot run JS, so it cannot self-cancel its own
-    //   navigation.
-    // - a provisional frame is only partially linked into the frame tree, so
-    //   other frames should not be able to reach into it to cancel the
-    //   navigation either.
-    CHECK(in_frame_tree_);
-    return true;
-  }
-  return false;
-}
-
 void RenderFrameImpl::CommitNavigationWithParams(
     mojom::CommonNavigationParamsPtr common_params,
     mojom::CommitNavigationParamsPtr commit_params,
@@ -3274,10 +3240,6 @@ void RenderFrameImpl::CommitNavigationWithParams(
         prefetch_loader_factory,
     std::unique_ptr<DocumentState> document_state,
     std::unique_ptr<WebNavigationParams> navigation_params) {
-  if (ShouldIgnoreCommitNavigation(*commit_params)) {
-    browser_side_navigation_pending_url_ = GURL();
-    return;
-  }
 
   // TODO(738611): This is temporary switch to have chrome WebUI use the old web
   // APIs. After completion of the migration, we should remove this.
@@ -3487,7 +3449,6 @@ void RenderFrameImpl::CommitFailedNavigation(
       GetFrameHost()->DidStopLoading();
     }
     browser_side_navigation_pending_ = false;
-    browser_side_navigation_pending_url_ = GURL();
 
     // Disarm AssertNavigationCommits and pretend that the commit actually
     // happened. Hopefully, either:
@@ -3563,7 +3524,6 @@ void RenderFrameImpl::CommitFailedNavigation(
 
   pending_loader_factories_ = nullptr;
   browser_side_navigation_pending_ = false;
-  browser_side_navigation_pending_url_ = GURL();
 }
 
 // mojom::FrameNavigationControl implementation --------------------------------
@@ -5051,7 +5011,6 @@ void RenderFrameImpl::RemoveObserver(RenderFrameObserver* observer) {
 
 void RenderFrameImpl::OnDroppedNavigation() {
   browser_side_navigation_pending_ = false;
-  browser_side_navigation_pending_url_ = GURL();
   frame_->DidDropNavigation();
 }
 
@@ -5415,7 +5374,6 @@ void RenderFrameImpl::PrepareFrameForCommit(
     const GURL& url,
     const mojom::CommitNavigationParams& commit_params) {
   browser_side_navigation_pending_ = false;
-  browser_side_navigation_pending_url_ = GURL();
   GetContentClient()->SetActiveURL(
       url, frame_->Top()->GetSecurityOrigin().ToString().Utf8());
 
@@ -6158,7 +6116,6 @@ void RenderFrameImpl::BeginNavigationInternal(
   for (auto& observer : observers_)
     observer.DidStartNavigation(info->url_request.Url(), info->navigation_type);
   browser_side_navigation_pending_ = true;
-  browser_side_navigation_pending_url_ = info->url_request.Url();
 
   blink::WebURLRequest& request = info->url_request;
 
@@ -6621,6 +6578,9 @@ RenderFrameImpl::CreateWebSocketHandshakeThrottle() {
 
 void RenderFrameImpl::AbortCommitNavigation() {
   DCHECK(navigation_client_impl_);
+  // This is only called by CommitFailedNavigation(), which should have already
+  // reset any extant MHTMLBodyLoaderClient.
+  DCHECK(!mhtml_body_loader_client_);
   // Interface disconnection will trigger
   // RenderFrameHostImpl::OnCrossDocumentCommitProcessed().
   navigation_client_impl_.reset();

content/renderer/render_frame_impl.h

@@ -1041,12 +1041,6 @@ class CONTENT_EXPORT RenderFrameImpl
       std::unique_ptr<DocumentState> document_state,
       std::unique_ptr<blink::WebNavigationParams> navigation_params);
 
-  // We can ignore renderer-initiated navigations which have been canceled
-  // in the renderer, but browser was not aware yet at the moment of issuing
-  // a CommitNavigation call.
-  bool ShouldIgnoreCommitNavigation(
-      const mojom::CommitNavigationParams& commit_params);
-
   // Decodes a data url for navigation commit.
   void DecodeDataURL(const mojom::CommonNavigationParams& common_params,
                      const mojom::CommitNavigationParams& commit_params,
@@ -1373,7 +1367,6 @@ class CONTENT_EXPORT RenderFrameImpl
   // as a result of mojom::FrameHost::BeginNavigation call. It is reset when the
   // navigation is either committed or cancelled.
   bool browser_side_navigation_pending_ = false;
-  GURL browser_side_navigation_pending_url_;
 
   // A bitwise OR of bindings types that have been enabled for this RenderFrame.
   // See BindingsPolicy for details.