Fix tab/browser close to consider beforeunload handlers in same-site iframes.

When the user closes a tab (or browser), we first determine whether we
can take a "fast shutdown" path and simply shut down the main frame
process.  That can be done if nothing in the current tab (or all tabs
if closing the browser) needs to run beforeunload/unload handlers.
Otherwise, we take the slow path, actually try to run those handlers
in all the frames, and wait for the results (including beforeunload
dialog dismissals) before proceeding with the close.

Historically, this was done by checking SuddenTerminationAllowed() on
the main frame's process, which would be set to true when *any* frame
in that process adds a beforeunload or unload handler.  When site
isolation shipped, we also added code to consult beforeunload handlers
in cross-site iframes (see issue https://crbug.com/853021) in M68.

But SuddenTerminationAllowed() is actually too coarse-grained for
this: it not only becomes false due to (before)unload handlers, but
other things as well (such as BlobBytesProvider).  So
https://chromium-review.googlesource.com/c/chromium/src/+/1504130
changed things to explicitly consult the main frame's
beforeunload/unload handler status, as tracked by
RFH::GetSuddenTerminationDisablerState(), instead of
SuddenTerminationAllowed().  Unfortunately, it seems that in that CL
we forgot about the process-wide nature of SuddenTerminationAllowed(),
specifically that it also covered handlers from any subframes in the
same process.  And/or, we missed that the call in
WebContentsImpl::NeedToFireBeforeUnload() to check subframes via
GetMainFrame()->ShouldDispatchBeforeUnload( true /*
check_subframes_only */); was in fact only checking cross-site
iframes, skipping not only the main frame but also its same-site
descendants.

That means we would incorrectly fast-close the tab (or browser) even
if a same-site iframe had a beforeunload handler.  This CL fixes this
by walking the full frame tree to check for these handlers and adds
two regression tests for the tab close and browser close cases.

Along the way, the CL also fixes a TODO to check not only beforeunload
but also unload handlers in subframes.  This is a bit more correct and
will also make it easier to fix issue 1014550, though note that
subframe unload handlers weren't broken before -- they actually still
ran due to the subframe unload process timeout that we still have in
RenderProcessHostImpl::DelayProcessShutdownForUnload().

Bug: 1010456, 1014550
Change-Id: I2b2c3e2ee546a766e5838e14f293266def4aeeed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1881948
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711413}

chrome/browser/site_isolation/chrome_site_per_process_browsertest.cc

@@ -1657,6 +1657,57 @@ IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessTest,
   EXPECT_EQ(first_web_contents, tab_strip_model->GetActiveWebContents());
 }
 
+// Tests that a same-site iframe runs its beforeunload handler when closing a
+// tab.  Same as the test above, but for a same-site rather than cross-site
+// iframe.  See https://crbug.com/1010456.
+IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessTest,
+                       TabCloseWithSameSiteBeforeUnloadIframe) {
+  TabStripModel* tab_strip_model = browser()->tab_strip_model();
+  content::WebContents* first_web_contents =
+      tab_strip_model->GetActiveWebContents();
+
+  // Add a second tab and load a page with a same-site iframe.
+  GURL main_url(embedded_test_server()->GetURL("a.com", "/iframe.html"));
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), main_url, WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  content::WebContents* second_web_contents =
+      tab_strip_model->GetActiveWebContents();
+  EXPECT_NE(first_web_contents, second_web_contents);
+  content::RenderFrameHost* child =
+      ChildFrameAt(second_web_contents->GetMainFrame(), 0);
+  EXPECT_EQ(child->GetSiteInstance(),
+            second_web_contents->GetMainFrame()->GetSiteInstance());
+
+  // Install a dialog-showing beforeunload handler in the iframe.
+  EXPECT_TRUE(
+      ExecuteScript(child, "window.onbeforeunload = () => { return 'x' };"));
+  content::PrepContentsForBeforeUnloadTest(second_web_contents);
+
+  // Close the second tab.  This should return false to indicate that we're
+  // waiting for the beforeunload dialog.
+  EXPECT_FALSE(
+      tab_strip_model->CloseWebContentsAt(tab_strip_model->active_index(), 0));
+
+  // Cancel the dialog and make sure the tab stays alive.
+  auto* dialog = ui_test_utils::WaitForAppModalDialog();
+  dialog->native_dialog()->CancelAppModalDialog();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(second_web_contents, tab_strip_model->GetActiveWebContents());
+  EXPECT_EQ(2, browser()->tab_strip_model()->count());
+
+  // Try closing the tab again.
+  EXPECT_FALSE(
+      tab_strip_model->CloseWebContentsAt(tab_strip_model->active_index(), 0));
+
+  // Accept the dialog and wait for tab close to complete.
+  content::WebContentsDestroyedWatcher destroyed_watcher(second_web_contents);
+  dialog = ui_test_utils::WaitForAppModalDialog();
+  dialog->native_dialog()->AcceptAppModalDialog();
+  destroyed_watcher.Wait();
+  EXPECT_EQ(first_web_contents, tab_strip_model->GetActiveWebContents());
+}
+
 // Test that there's no crash in the following scenario:
 // 1. a page opens a cross-site popup, where the popup has a cross-site iframe
 //    with a beforeunload handler.

chrome/browser/unload_browsertest.cc

@@ -648,5 +648,30 @@ IN_PROC_BROWSER_TEST_F(UnloadTest, BrowserCloseWithCrossSiteIframe) {
   ManuallyCloseWindow();
 }
 
+// Tests that a same-site iframe runs its beforeunload handler when closing the
+// browser.  See https://crbug.com/1010456.
+IN_PROC_BROWSER_TEST_F(UnloadTest, BrowserCloseWithSameSiteIframe) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Navigate to a page with a same-site iframe.
+  GURL main_url(embedded_test_server()->GetURL("a.com", "/iframe.html"));
+  ui_test_utils::NavigateToURL(browser(), main_url);
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::RenderFrameHost* child =
+      ChildFrameAt(web_contents->GetMainFrame(), 0);
+  EXPECT_EQ(child->GetSiteInstance(),
+            web_contents->GetMainFrame()->GetSiteInstance());
+
+  // Install a dialog-showing beforeunload handler in the iframe.
+  EXPECT_TRUE(
+      ExecuteScript(child, "window.onbeforeunload = () => { return 'x' };"));
+
+  // Close the browser and make sure the beforeunload dialog is shown and can
+  // be clicked.
+  PrepareForDialog(browser());
+  ManuallyCloseWindow();
+}
+
 // TODO(ojan): Add tests for unload/beforeunload that have multiple tabs
 // and multiple windows.

content/browser/web_contents/web_contents_impl.cc

@@ -1825,18 +1825,22 @@ bool WebContentsImpl::NeedToFireBeforeUnload() {
   if (GetRenderViewHost()->SuddenTerminationAllowed())
     return false;
 
-  // Check whether the main frame needs to run beforeunload or unload handlers.
-  if (GetMainFrame()->GetSuddenTerminationDisablerState(
-          blink::kBeforeUnloadHandler | blink::kUnloadHandler)) {
+  // Check whether any frame in the frame tree needs to run beforeunload or
+  // unload handlers.
+  for (FrameTreeNode* node : frame_tree_.Nodes()) {
+    RenderFrameHostImpl* rfh = node->current_frame_host();
+
+    // No need to run beforeunload/unload if the RenderFrame isn't live.
+    if (!rfh->IsRenderFrameLive())
+      continue;
+
+    if (rfh->GetSuddenTerminationDisablerState(blink::kBeforeUnloadHandler |
+                                               blink::kUnloadHandler)) {
       return true;
     }
+  }
 
-  // Check whether any subframes need to run beforeunload handlers.
-  //
-  // TODO(alexmos): Also check whether subframes need to run unload handlers in
-  // addition to beforeunload.
-  return GetMainFrame()->ShouldDispatchBeforeUnload(
-      true /* check_subframes_only */);
+  return false;
 }
 
 void WebContentsImpl::DispatchBeforeUnload(bool auto_cancel) {