Ensure a FrameTreeNode is no longer loading  at the end of its destructor

Currently, the FrameTreeNode destructor ensures that its
navigation_request_ is cleared, but does not ensure that navigation
has been cancelled for any of the other criteria it uses to determine
that it is loading. Ensure that the current and speculative
RenderFrameHostImpls are reset as well.

Many tests tear down a WebContentsImpl without ensuring that it is
done loading, and that is benign. However, it is possible to get a
situation where a subframe FrameTreeNode is detached while still
loading. If it is the last frame loading, this will cause the
FrameTree to transition from loading to not loading without an
associated DidStopLoading() callback.
WebContentsObserverSanityChecker DCHECKs on the rare case where
that happens.

Bug: 916413, 795479
Change-Id: I5abed2648dba2eb99db66a222b4fb5a22d594612
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1455110
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Auto-Submit: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637846}
diff --git a/content/browser/frame_host/frame_tree_node.cc b/content/browser/frame_host/frame_tree_node.cc
index d6bbdb7..e8201d4 100644
--- a/content/browser/frame_host/frame_tree_node.cc
+++ b/content/browser/frame_host/frame_tree_node.cc
@@ -138,6 +138,8 @@
   // Remove the children.
+  current_frame_host()->ResetLoadingState();
   // If the removed frame was created by a script, then its history entry will
   // never be reused - we can save some memory by removing the history entry.
   // See also https://crbug.com/784356.
@@ -167,6 +169,12 @@
+  // ~SiteProcessCountTracker DCHECKs in some tests if CleanUpNavigation is not
+  // called last. Ideally this would be closer to (possible before) the
+  // ResetLoadingState() call above.
+  render_manager_.CleanUpNavigation();
+  DCHECK(!IsLoading());
 void FrameTreeNode::AddObserver(Observer* observer) {
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 9d8a7f4..cc7c7b4 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -82,8 +82,7 @@
 RenderFrameHostManager::~RenderFrameHostManager() {
-  if (speculative_render_frame_host_)
-    UnsetSpeculativeRenderFrameHost();
+  DCHECK(!speculative_render_frame_host_);
   // Delete any RenderFrameProxyHosts. It is important to delete those prior to
   // deleting the current RenderFrameHost, since the CrossProcessFrameConnector
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 09c35a2..28984b3 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -5067,6 +5067,8 @@
 void WebContentsImpl::LoadingStateChanged(bool to_different_document,
                                           bool due_to_interstitial,
                                           LoadNotificationDetails* details) {
+  if (is_being_destroyed_)
+    return;
   // Do not send notifications about loading changes in the FrameTree while the
   // interstitial page is pausing the throbber.
   if (ShowingInterstitialPage() && interstitial_page_->pause_throbber() &&
@@ -5792,6 +5794,8 @@
 void WebContentsImpl::DidChangeLoadProgress() {
+  if (is_being_destroyed_)
+    return;
   double load_progress = frame_tree_.load_progress();
   // The delegate is notified immediately for the first and last updates. Also,