Stop clobbering/leaking a provisional main frame on window close.

The browser can not delete a speculative main frame that has committed
since the renderer takes ownership of it. This is a race since the
renderer makes this decision, and the browser may try delete it before
hearing it has been committed. In this case the renderer ignores the
delete to avoid losing state.

However as part of cancelling speculative navigation during shutdown,
the browser will also replace the RenderFrameProxyHost which makes a
RenderFrameProxy and RemoteFrame as the main frame in the renderer.

This is a problem because it replaces Page's main_frame_ pointer to
the main LocalFrame that was committed and not deleted by the
browser's request. The LocalFrame becomes an orphan that lives
indefinitely, with the LocalFrame and associated RenderFrame and
WebViewFrameWidget holding raw pointers back to the WebViewImpl,
the RenderWidget, and the RenderViewImpl. When these are closed
and destroyed, if the process is not killed entirely, the frame
will crash if it tries to access these.

Since WebContentsImpl drops all RenderFrameProxyHosts before destroying
the FrameTreeNode, we can not go through RenderFrameHostManager's
CleanUpNavigation() or DiscardUnusedFrame() as this path will recreate
the already deleted RenderFrameProxyHost, leading to the leaked frame.

So instead we can simply delete the speculative RenderFrameHostImpl
with UnsetSpeculativeRenderFrameHost(). This skips past recreating the
RenderFrameProxyHost, setting the speculative RenderViewHost as being
swapped out, and notifying in IPC that the nav was cancelled, none of
which should matter to do at shutdown.

R=avi@chromium.org, dcheng@chromium.org

Bug: 964668, 838348, 915179
Change-Id: Icb7f2dfe6752ae745b0ea299fe4ca974b34516a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1648463Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#667139}

content/browser/frame_host/frame_tree_node.cc

@@ -164,17 +164,42 @@ FrameTreeNode::~FrameTreeNode() {
 
   g_frame_tree_node_id_map.Get().erase(frame_tree_node_id_);
 
+  bool did_stop_loading = false;
+
   if (navigation_request_) {
+    navigation_request_.reset();
     // PlzNavigate: if a frame with a pending navigation is detached, make sure
     // the WebContents (and its observers) update their loading state.
-    navigation_request_.reset();
-    DidStopLoading();
+    did_stop_loading = true;
   }
 
-  // ~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();
+  // ~SiteProcessCountTracker DCHECKs in some tests if the speculative
+  // RenderFrameHostImpl is not destroyed last. Ideally this would be closer to
+  // (possible before) the ResetLoadingState() call above.
+  //
+  // There is an inherent race condition causing bugs 838348/915179/et al, where
+  // the renderer may have committed the speculative main frame and the browser
+  // has not heard about it yet. If this is a main frame, then in that case the
+  // speculative RenderFrame was unable to be deleted (it is owned by the
+  // renderer) and we should not be able to cancel the navigation at this point.
+  // CleanUpNavigation() would normally be called here but it will try to undo
+  // the navigation and expose the race condition. When it replaces the main
+  // frame with a RenderFrameProxy, that leaks the committed main frame, leaving
+  // the frame and its friend group with pointers that will become invalid
+  // shortly as we are shutting everything down and deleting the RenderView etc.
+  // We avoid this problematic situation by not calling CleanUpNavigation() or
+  // DiscardUnusedFrame() here. The speculative RenderFrameHost is simply
+  // returned and deleted immediately. This satisfies the requirement that the
+  // speculative RenderFrameHost is removed from the RenderFrameHostManager
+  // before it is destroyed.
+  if (render_manager_.speculative_frame_host()) {
+    did_stop_loading |= render_manager_.speculative_frame_host()->is_loading();
+    render_manager_.UnsetSpeculativeRenderFrameHost();
+  }
+
+  if (did_stop_loading)
+    DidStopLoading();
+
   DCHECK(!IsLoading());
 }