Bugfix: beforeunload starts navigation during webcontent destructor.

Fix https://crbug.com/1147567 and add a regression test.
This is meant to be merged back into M88 beta.

No new navigations should start in a non current frame. There is a
DCHECK and a DumpWithoutCrashing. It was triggered in M88.

What happened:
- Start with nested documents: A(B(C)).
- C adds a beforeunload handler.
- B starts a navigation, waiting for C.
- The WebContents is closed, deleting C, then B, and then A.
By deleting C, the navigations in B can begin, but this happen in the
middle of destructing B.

This is fixed by putting the code responsible for continuing the
navigation in a new task. This is indeed the current behavior for the
second part of the "if" already. This patch just generalize it to both
clauses.

A lot of things can be done to improve the architecture here, but this
patch is meant to be merged into M88, so it should stay short to
minimize the change.

Bug: 1147567
Fixed: 1147567
Change-Id: I980c7da56b9f02b2644ebcb9a9401637848d9c34
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2544967Reviewed-by: Camille Lamy <clamy@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#829166}

content/browser/renderer_host/render_frame_host_impl.cc

@@ -3325,32 +3325,36 @@ void RenderFrameHostImpl::ProcessBeforeUnloadCompletedFromFrame(
     beforeunload_timeout_->Stop();
   send_before_unload_start_time_ = base::TimeTicks();
 
-  // If the ACK is for a navigation, send it to the Navigator to have the
-  // current navigation stop/proceed. Otherwise, send it to the
-  // RenderFrameHostManager which handles closing.
-  if (unload_ack_is_for_navigation_) {
-    frame_tree_node_->navigator().BeforeUnloadCompleted(
-        frame_tree_node_, proceed, before_unload_end_time);
-  } else {
-    // We could reach this from a subframe destructor for |frame| while we're
-    // in the middle of closing the current tab.  In that case, dispatch the
-    // ACK to prevent re-entrancy and a potential nested attempt to free the
-    // current frame.  See https://crbug.com/866382.
-    base::OnceClosure task = base::BindOnce(
-        [](base::WeakPtr<RenderFrameHostImpl> self,
-           const base::TimeTicks& before_unload_end_time, bool proceed) {
-          if (!self)
-            return;
-          self->frame_tree_node()->render_manager()->BeforeUnloadCompleted(
+  // We could reach this from a subframe destructor for |frame| while we're in
+  // the middle of closing the current tab. In that case, dispatch the ACK to
+  // prevent re-entrancy and a potential nested attempt to free the current
+  // frame. See https://crbug.com/866382 and https://crbug.com/1147567.
+  base::OnceClosure task = base::BindOnce(
+      [](base::WeakPtr<RenderFrameHostImpl> self,
+         const base::TimeTicks& before_unload_end_time, bool proceed,
+         bool unload_ack_is_for_navigation) {
+        if (!self)
+          return;
+        FrameTreeNode* frame = self->frame_tree_node();
+        // If the ACK is for a navigation, send it to the Navigator to have the
+        // current navigation stop/proceed. Otherwise, send it to the
+        // RenderFrameHostManager which handles closing.
+        if (unload_ack_is_for_navigation) {
+          frame->navigator().BeforeUnloadCompleted(frame, proceed,
+                                                   before_unload_end_time);
+        } else {
+          frame->render_manager()->BeforeUnloadCompleted(
               proceed, before_unload_end_time);
-        },
-        weak_ptr_factory_.GetWeakPtr(), before_unload_end_time, proceed);
-    if (is_frame_being_destroyed) {
-      DCHECK(proceed);
-      base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, std::move(task));
-    } else {
-      std::move(task).Run();
-    }
+        }
+      },
+      weak_ptr_factory_.GetWeakPtr(), before_unload_end_time, proceed,
+      unload_ack_is_for_navigation_);
+
+  if (is_frame_being_destroyed) {
+    DCHECK(proceed);
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, std::move(task));
+  } else {
+    std::move(task).Run();
   }
 
   // If canceled, notify the delegate to cancel its pending navigation entry.

content/browser/renderer_host/render_frame_host_impl_browsertest.cc

@@ -1178,6 +1178,49 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBeforeUnloadBrowserTest,
   EXPECT_EQ(main_url, web_contents()->GetLastCommittedURL());
 }
 
+// During a complex WebContents destruction, test resuming a navigation, due to
+// of a beforeunloader. This is a regersion test for: https://crbug.com/1147567.
+// - Start from A(B(C))
+// - C adds a beforeunload handler.
+// - B starts a navigation, waiting for C.
+// - The WebContents is closed, which deletes C, then B, then A.
+// When deleting C, the navigations in B can begin, but this happen while B was
+// destructing itself.
+//
+// Note: This needs 3 nested documents instead of 2, because deletion of the
+// main RenderFrameHost is different from normal RenderFrameHost. This is
+// required to reproduce https://crbug.com/1147567.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBeforeUnloadBrowserTest,
+                       CloseWebContent) {
+  // This test exercises a scenario that's only possible with
+  // --site-per-process.
+  if (!AreAllSitesIsolatedForTesting())
+    return;
+
+  // For unknown reasons, it seems required to start from a "live"
+  // RenderFrameHost. Otherwise creating a new Shell below will crash.
+  EXPECT_TRUE(NavigateToURL(shell(), GURL(url::kAboutBlankURL)));
+
+  GURL url = embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b(c))");
+  Shell* new_shell = Shell::CreateNewWindow(
+      web_contents()->GetController().GetBrowserContext(), url, nullptr,
+      gfx::Size());
+  auto* web_contents = static_cast<WebContentsImpl*>(new_shell->web_contents());
+  EXPECT_TRUE(WaitForLoadStop(web_contents));
+  RenderFrameHostImpl* rfh_a = web_contents->GetMainFrame();
+  RenderFrameHostImpl* rfh_b = rfh_a->child_at(0)->current_frame_host();
+  RenderFrameHostImpl* rfh_c = rfh_b->child_at(0)->current_frame_host();
+
+  // C has a beforeunload handler, slow to reply.
+  EXPECT_TRUE(ExecJs(rfh_c, "onbeforeunload = () => {while(1);}"));
+  // B navigate elsewhere. This triggers C's beforeunload handler.
+  EXPECT_TRUE(ExecJs(rfh_b, "location.href = 'about:blank';"));
+  // Closing the Shell, this deletes C and causes the navigation above to start.
+  new_shell->Close();
+  // Test pass if this doesn't reach a CHECK.
+}
+
 namespace {
 
 // A helper to execute some script in a frame just before it is deleted, such