Revert "CHECK that CommitNavigation/CommitFailedNavigation IPCs always commit."

This reverts commit 95f797d6.

Reason for revert:
It broke some tests in official builds, see bug.

Original change's description:
> CHECK that CommitNavigation/CommitFailedNavigation IPCs always commit.
> 
> Currently, state synchronization between the browser and the renderer
> process is complicated and buggy for cross-process navigations. The
> renderer process is responsible for processing the commit IPC and then
> reporting success back up to the browser process, which then updates its
> state. However, it's unclear if this can lead to races.
> 
> Instead, if it is possible to assume that one CommitNavigation() IPC
> from the browser always maps to one committed navigation in the
> renderer, the code can be simplified to remove multiphase navigation
> commits. This means the browser would be able to mark a provisional
> local frame as committed as soon as it sends a commit IPC to the
> renderer for that frame.
> 
> Bug: 999255
> Change-Id: I7d58293e6c2ec6ca0d09b614ae5a41340f4afd8e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1775663
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: danakj <danakj@chromium.org>
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Reviewed-by: Camille Lamy <clamy@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#705414}

TBR=danakj@chromium.org,dcheng@chromium.org,nasko@chromium.org,clamy@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 999255,1014903
Change-Id: I9b5429c66a47e1a180ba2ff82059fe596d7f0b79
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1874575Reviewed-by: Hans Wennborg <hans@chromium.org>
Commit-Queue: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708414}

content/renderer/render_frame_impl.cc

@@ -1079,26 +1079,6 @@ void FillNavigationParamsOriginPolicy(
 
 }  // namespace
 
-class RenderFrameImpl::AssertNavigationCommits {
- public:
-  explicit AssertNavigationCommits(RenderFrameImpl* frame)
-      : frame_(frame->weak_factory_.GetWeakPtr()) {
-    CHECK_EQ(NavigationCommitState::kNone, frame->navigation_commit_state_);
-    frame->navigation_commit_state_ = NavigationCommitState::kWillCommitFromIPC;
-  }
-  ~AssertNavigationCommits() {
-    // Frame might have been synchronously detached when dispatching JS events.
-    if (frame_) {
-      CHECK_EQ(NavigationCommitState::kDidCommitFromIPC,
-               frame_->navigation_commit_state_);
-      frame_->navigation_commit_state_ = NavigationCommitState::kNone;
-    }
-  }
-
- private:
-  const base::WeakPtr<RenderFrameImpl> frame_;
-};
-
 // This class uses existing WebNavigationBodyLoader to read the whole response
 // body into in-memory buffer, and then creates another body loader with static
 // data so that we can parse mhtml archive synchronously. This is a workaround
@@ -1110,45 +1090,17 @@ class RenderFrameImpl::MHTMLBodyLoaderClient
   // Once the body is read, fills |navigation_params| with the new body loader
   // and calls |done_callbcak|.
   MHTMLBodyLoaderClient(
-      RenderFrameImpl* frame,
       std::unique_ptr<blink::WebNavigationParams> navigation_params,
       base::OnceCallback<void(std::unique_ptr<blink::WebNavigationParams>)>
           done_callback)
-      : frame_(frame),
-        navigation_params_(std::move(navigation_params)),
-        body_loader_(std::move(navigation_params_->body_loader)),
+      : navigation_params_(std::move(navigation_params)),
         done_callback_(std::move(done_callback)) {
+    body_loader_ = std::move(navigation_params_->body_loader);
     body_loader_->StartLoadingBody(this, false /* use_isolated_code_cache */);
   }
 
-  ~MHTMLBodyLoaderClient() override {
-    // MHTMLBodyLoaderClient is reset in several different places. Either:
-    CHECK(
-        // - the body load finished and the result is being committed, so
-        //   |BodyLoadingFinished| (see below) should still be on the stack, or
-        committing_ ||
-        // - MHTMLBodyLoaderClient is abandoned, either because:
-        //   - the browser requested a different navigation be committed in this
-        //     frame, i.e. the navigation commit state should be
-        //     kWillCommitFromIPC, or
-        NavigationCommitState::kWillCommitFromIPC ==
-            frame_->navigation_commit_state_ ||
-        //   - a new renderer-initiated navigation began, which explicitly
-        //     detaches any existing MHTMLBodyLoaderClient
-        //   - this renderer began the navigation and cancelled it with
-        //     |AbortClientNavigation|, e.g. JS called windows.stop(), which
-        //     explicitly detaches any existing MHTMLBodyLoaderClient
-        //   - the frame is detached and self-deleted, which also explicitly
-        //     detaches any existing MHTMLBodyLoaderClient
-        !frame_);
-  }
-
-  void Detach() {
-    CHECK(frame_->in_frame_tree_);
-    frame_ = nullptr;
-  }
+  ~MHTMLBodyLoaderClient() override {}
 
-  // blink::WebNavigationBodyLoader::Client overrides:
   void BodyCodeCacheReceived(mojo_base::BigBuffer data) override {}
 
   void BodyDataReceived(base::span<const char> data) override {
@@ -1161,8 +1113,6 @@ class RenderFrameImpl::MHTMLBodyLoaderClient
                            int64_t total_decoded_body_length,
                            bool should_report_corb_blocking,
                            const base::Optional<WebURLError>& error) override {
-    committing_ = true;
-    AssertNavigationCommits assert_navigation_commits(frame_);
     if (!error.has_value()) {
       WebNavigationParams::FillBodyLoader(navigation_params_.get(), data_);
       // Clear |is_static_data| flag to avoid the special behavior it triggers,
@@ -1174,9 +1124,6 @@ class RenderFrameImpl::MHTMLBodyLoaderClient
   }
 
  private:
-  // |RenderFrameImpl| owns |this|, so |frame_| is guaranteed to be valid.
-  RenderFrameImpl* frame_;
-  bool committing_ = false;
   WebData data_;
   std::unique_ptr<blink::WebNavigationParams> navigation_params_;
   std::unique_ptr<blink::WebNavigationBodyLoader> body_loader_;
@@ -3441,8 +3388,6 @@ void RenderFrameImpl::CommitNavigationInternal(
     return;
   }
 
-  AssertNavigationCommits assert_navigation_commits(this);
-
   bool was_initiated_in_this_frame = false;
   if (IsPerNavigationMojoInterfaceEnabled()) {
     was_initiated_in_this_frame =
@@ -3543,31 +3488,7 @@ void RenderFrameImpl::CommitNavigationInternal(
     // We need this to retrieve the document mime type prior to committing.
     mhtml_body_loader_client_ =
         std::make_unique<RenderFrameImpl::MHTMLBodyLoaderClient>(
-            this, std::move(navigation_params), std::move(commit_with_params));
-    // The navigation didn't really commit, but lie about it anyway. Why? MHTML
-    // is a bit special: the renderer process is responsible for parsing the
-    // archive, but at this point, the response body isn't fully loaded yet.
-    // Instead, MHTMLBodyLoaderClient will read the entire response body and
-    // parse the archive to extract the main resource to be committed.
-    //
-    // There are two possibilities from this point:
-    // - MHTMLBodyLoaderClient::BodyLoadingFinished() is called. At that point,
-    //   the main resource can be extracted, and the navigation should be
-    //   synchronously committed. If this is a provisional frame, it will be
-    //   swapped in and committed. AssertNavigationCommits is used to ensure
-    //   that this always happens. ✔️
-    // - Alternatively, the pending archive load may be cancelled. This can only
-    //   happen if the renderer initiates a new navigation, or if the browser
-    //   requests that this frame commit a different navigation.
-    //   - If |this| is already swapped in, the reason for cancelling the
-    //     pending archive load does not matter. There will be no state skew
-    //     between the browser and the renderer, since |this| has already
-    //     committed a navigation. ✔️
-    //   - If |this| is provisional, the pending archive load may only be
-    //     cancelled by the browser requesting |this| to commit a different
-    //     navigation. AssertNavigationCommits ensures the new request ends up
-    //     committing and swapping in |this|, so this is OK. ✔️
-    navigation_commit_state_ = NavigationCommitState::kDidCommitFromIPC;
+            std::move(navigation_params), std::move(commit_with_params));
     return;
   }
 
@@ -3584,14 +3505,6 @@ bool RenderFrameImpl::ShouldIgnoreCommitNavigation(
       !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;
@@ -3771,9 +3684,6 @@ void RenderFrameImpl::CommitFailedNavigationInternal(
   TRACE_EVENT1("navigation,benchmark,rail",
                "RenderFrameImpl::CommitFailedNavigation", "id", routing_id_);
   DCHECK(!NavigationTypeUtils::IsSameDocument(common_params->navigation_type));
-
-  AssertNavigationCommits assert_navigation_commits(this);
-
   RenderFrameImpl::PrepareRenderViewForNavigation(common_params->url,
                                                   *commit_params);
   sync_navigation_callback_.Cancel();
@@ -3815,16 +3725,6 @@ void RenderFrameImpl::CommitFailedNavigationInternal(
     Send(new FrameHostMsg_DidStopLoading(routing_id_));
     browser_side_navigation_pending_ = false;
     browser_side_navigation_pending_url_ = GURL();
-
-    // 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|...
-    // TODO(dcheng): This is potentially buggy. This means that the browser
-    // asked the renderer to commit a failed navigation, but it declined. In the
-    // future, when speculative RFH management is refactored, this will likely
-    // have to self-discard if |this| is provisional.
-    navigation_commit_state_ = NavigationCommitState::kDidCommitFromIPC;
-
     return;
   }
 
@@ -3873,14 +3773,6 @@ void RenderFrameImpl::CommitFailedNavigationInternal(
     }
     browser_side_navigation_pending_ = false;
     browser_side_navigation_pending_url_ = GURL();
-
-    // Disarm AssertNavigationCommits and pretend that the commit actually
-    // happened. Hopefully, either:
-    // - the browser process is signalled to stop loading, which /should/ signal
-    //   the browser to tear down the speculative RFH associated with |this| or
-    // - fallback content is triggered, which /should/ eventually discard |this|
-    //   as well.
-    navigation_commit_state_ = NavigationCommitState::kDidCommitFromIPC;
     return;
   }
 
@@ -4557,11 +4449,6 @@ void RenderFrameImpl::FrameDetached(DetachType type) {
     proxy->set_provisional_frame_routing_id(MSG_ROUTING_NONE);
   }
 
-  if (mhtml_body_loader_client_) {
-    mhtml_body_loader_client_->Detach();
-    mhtml_body_loader_client_.reset();
-  }
-
   delete this;
   // Object is invalid after this point.
 }
@@ -4755,9 +4642,6 @@ void RenderFrameImpl::DidCommitProvisionalLoad(
                "id", routing_id_,
                "url", GetLoadingUrl().possibly_invalid_spec());
 
-  if (navigation_commit_state_ == NavigationCommitState::kWillCommitFromIPC)
-    navigation_commit_state_ = NavigationCommitState::kDidCommitFromIPC;
-
   InternalDocumentStateData* internal_data =
       InternalDocumentStateData::FromDocumentLoader(
           frame_->GetDocumentLoader());
@@ -5195,13 +5079,9 @@ void RenderFrameImpl::RenderFallbackContentInParentProcess() {
 }
 
 void RenderFrameImpl::AbortClientNavigation() {
-  CHECK(in_frame_tree_);
   browser_side_navigation_pending_ = false;
   sync_navigation_callback_.Cancel();
-  if (mhtml_body_loader_client_) {
-    mhtml_body_loader_client_->Detach();
   mhtml_body_loader_client_.reset();
-  }
   NotifyObserversOfFailedProvisionalLoad();
   if (!IsPerNavigationMojoInterfaceEnabled()) {
     Send(new FrameHostMsg_AbortNavigation(routing_id_));
@@ -6270,11 +6150,6 @@ void RenderFrameImpl::OnReportContentSecurityPolicyViolation(
 
 void RenderFrameImpl::BeginNavigation(
     std::unique_ptr<blink::WebNavigationInfo> info) {
-  // A provisional frame should never make a renderer-initiated navigation: no
-  // JS should be running in |this|, and no other frame should have a reference
-  // to |this|.
-  CHECK(in_frame_tree_);
-
   // This method is only called for renderer initiated navigations, which
   // may have originated from a link-click, script, drag-n-drop operation, etc.
 
@@ -6418,10 +6293,7 @@ void RenderFrameImpl::BeginNavigation(
     }
 
     sync_navigation_callback_.Cancel();
-    if (mhtml_body_loader_client_) {
-      mhtml_body_loader_client_->Detach();
     mhtml_body_loader_client_.reset();
-    }
 
     // First navigation in a frame to an empty document must be handled
     // synchronously.

content/renderer/render_frame_impl.h

@@ -1749,26 +1749,6 @@ class CONTENT_EXPORT RenderFrameImpl
   std::unique_ptr<blink::WebURLLoaderFactoryForTest>
       web_url_loader_factory_override_for_test_;
 
-  // When the browser asks the renderer to commit a navigation, it should always
-  // result in a committed navigation reported via DidCommitProvisionalLoad().
-  // This is important because DidCommitProvisionalLoad() is responsible for
-  // swapping in the provisional local frame during a cross-process navigation.
-  // Since this involves updating state in both the browser process and the
-  // renderer process, this assert ensures that the state remains synchronized
-  // between the two processes.
-  //
-  // Note: there is one exception that can result in no commit happening.
-  // Committing a navigation runs unload handlers, which can detach |this|. In
-  // that case, it doesn't matter that the navigation never commits, since the
-  // logical node for |this| has been removed from the DOM.
-  enum class NavigationCommitState {
-    kNone,
-    kWillCommitFromIPC,
-    kDidCommitFromIPC,
-  };
-  class AssertNavigationCommits;
-  NavigationCommitState navigation_commit_state_ = NavigationCommitState::kNone;
-
   base::WeakPtrFactory<RenderFrameImpl> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(RenderFrameImpl);