Fix: tabs slow to response to beforeunload are closed prematurely.

chrome/browser/lifetime/browser_close_manager_browsertest.cc

Index: chrome/browser/lifetime/browser_close_manager_browsertest.cc
diff --git a/chrome/browser/lifetime/browser_close_manager_browsertest.cc b/chrome/browser/lifetime/browser_close_manager_browsertest.cc
index fa9d7b997d77c3aa502e0c18a46295a6eebeab50..ff650e8074a9f210a4a5ca8c34227c41d6495afa 100644
--- a/chrome/browser/lifetime/browser_close_manager_browsertest.cc
+++ b/chrome/browser/lifetime/browser_close_manager_browsertest.cc
@@ -479,6 +479,88 @@ IN_PROC_BROWSER_TEST_P(BrowserCloseManagerBrowserTest,
   EXPECT_TRUE(BrowserList::GetInstance()->empty());
 }
 
+// Test that tabs that are slow to respond are not closed prematurely.
+// Regression for chrbug.com/365052 caused some of tabs to be closed even if

[Charlie Reis, 2016/05/19 22:11:56]

nit: No h in crbug.com.

+// user chose to cancel browser close.
+IN_PROC_BROWSER_TEST_P(BrowserCloseManagerBrowserTest,
+                       TestUnloadMultipleSlowTabs) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const int kTabCount = 5;
+  const int kResposiveTabIndex = 2;
+  // Create tab strip with all tabs except one responding after
+  // RenderViewHostImpl::kUnloadTimeoutMS .

[Charlie Reis, 2016/05/19 22:11:55]

nit: No space before period.

+  // Minimum configuration is two slow tabs and then responsive tab.
+  // But we also want to check how slow tabs behave in tail.
+  for (int i = 0; i < kTabCount; i++) {
+    if (i)
+      AddBlankTabAndShow(browsers_[0]);
+    ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(
+        browsers_[0],
+        embedded_test_server()->GetURL((i == kResposiveTabIndex)
+                                           ? "/beforeunload.html"
+                                           : "/beforeunload_slow.html")));
+  }
+  RepeatedNotificationObserver cancel_observer(
+      chrome::NOTIFICATION_BROWSER_CLOSE_CANCELLED, 1);
+  chrome::CloseAllBrowsersAndQuit();
+  ASSERT_NO_FATAL_FAILURE(CancelClose());

[Charlie Reis, 2016/05/19 22:11:55]

Yep, this is where the test will be flaky, as you mentioned.  If the bot is
overloaded and it takes more than 1 second to get the dialog from the responsive
beforeunload.html tab, this CancelClose call will never see a dialog to cancel,
and we'll quit (or rather, crash in AppModalDialogWaiter::Wait).  You can repro
this by setting RenderViewHostImpl::kUnloadTimeoutMS to 0.

That exactly matches the flakiness in other tests in this file (e.g., the report
in https://crbug.com/276366).  Any test that uses CancelClose() or AcceptClose()
will have this problem.

I'm not really sure how to fix this.  We need the timeout to fire to exercise
the bug in this test, so we can't disable it entirely, but we also can't
realistically guarantee that the "fast" page will respond before the timeout.

One option might be gracefully handling the case that the dialog is never
displayed, such that the test passes.  That means that the test would always
pass with the bug fixed, and it would usually fail but occasionally pass (on
slow bots) if the bug regressed.  I'd be ok with that outcome.

Another option would be exposing some interface to the underlying timer on each
tab, so that we can selectively tell them to time out or not.  That seems like
it would be pretty nice.

That said, I don't think we want that to be a prerequisite for landing this CL,
since it's a very valuable fix, and I've already verified locally that the test
correctly exercises the bug and the fix.  I'm inclined to say that we should
land this CL with these new tests disabled, and follow up with a way to fix and
enable them (perhaps with a timeout interface for testing).

[Mikhail Goncharov, 2016/05/20 04:20:01]

That's right. I have experimented with different timeout values locally and it
turns out that plain increase of timeout is not a clear cut.
Under the same load, --gtest_repeat=100 for TestHangInBeforeUnloadMultipleTabs:
kUnloadTimeoutMS = 100  => 15% failed
kUnloadTimeoutMS = 1000 => 10% failed
kUnloadTimeoutMS = 2000 => 9% failed
kUnloadTimeoutMS = 3000 => 12% failed
(lager timeout increases a chance that test will not be able to finish within 30
seconds).

It is interesting that test with multiple windows doesn't seem to flack as much
as one with single window and multiple tabs. One of possible explanations is
that process composition and os scheduling differs.

I am going to disable tests on the same platforms for now and implement second
approach you've described below.

Is it OK to upload "flak-less" patchset to this CL or better to create new one?

Do you have means to bulk run particular test with load similar to normal on
bots to check flakiness before / after fix?
I like that 

+  cancel_observer.Wait();
+  EXPECT_FALSE(browser_shutdown::IsTryingToQuit());
+  // All tabs should still be open.

[Charlie Reis, 2016/05/19 22:11:56]

nit: Add blank line before.

+  EXPECT_EQ(kTabCount, browsers_[0]->tab_strip_model()->count());
+  RepeatedNotificationObserver close_observer(

[Charlie Reis, 2016/05/19 22:11:56]

nit: Add blank line and comment before (e.g., Quit again and close all tabs).

+      chrome::NOTIFICATION_BROWSER_CLOSED, 1);
+  chrome::CloseAllBrowsersAndQuit();
+  ASSERT_NO_FATAL_FAILURE(AcceptClose());
+  close_observer.Wait();
+  EXPECT_TRUE(browser_shutdown::IsTryingToQuit());
+  EXPECT_TRUE(BrowserList::GetInstance()->empty());
+}
+
+// Test that tabs in different windows with a slow beforeunload event response
+// are treated the same as the user accepting the close, but do not close the
+// tab early.
+// Regression for chrbug.com/365052 caused CHECK in tabstrip.

[Charlie Reis, 2016/05/19 22:11:55]

nit: No h in crbug.com.

+IN_PROC_BROWSER_TEST_P(BrowserCloseManagerBrowserTest,
+                       TestBeforeUnloadMultipleSlowWindows) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const int kBrowserCount = 5;
+  const int kResposiveBrowserIndex = 2;
+  // Create multiple browsers with all tabs except one responding after
+  // RenderViewHostImpl::kUnloadTimeoutMS .
+  // Minimum configuration is just one browser with slow tab and then
+  // browser with responsive tab.
+  // But we also want to check how slow tabs behave in tail and make test
+  // more robust.
+  for (int i = 0; i < kBrowserCount; i++) {
+    if (i)
+      browsers_.push_back(CreateBrowser(browser()->profile()));
+    ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(
+        browsers_[i],
+        embedded_test_server()->GetURL((i == kResposiveBrowserIndex)
+                                           ? "/beforeunload.html"
+                                           : "/beforeunload_slow.html")));
+  }
+
+  RepeatedNotificationObserver cancel_observer(
+      chrome::NOTIFICATION_BROWSER_CLOSE_CANCELLED, kResposiveBrowserIndex + 1);
+  chrome::CloseAllBrowsersAndQuit();
+  ASSERT_NO_FATAL_FAILURE(CancelClose());
+  cancel_observer.Wait();
+  EXPECT_FALSE(browser_shutdown::IsTryingToQuit());
+  // All windows should still be open.

[Charlie Reis, 2016/05/19 22:11:55]

nit: Blank line before.

+  for (int i = 0; i < kBrowserCount; i++)
+    EXPECT_EQ(1, browsers_[i]->tab_strip_model()->count());
+
+  RepeatedNotificationObserver close_observer(

[Charlie Reis, 2016/05/19 22:11:55]

nit: Comment before.

+      chrome::NOTIFICATION_BROWSER_CLOSED, kBrowserCount);
+  chrome::CloseAllBrowsersAndQuit();
+  ASSERT_NO_FATAL_FAILURE(AcceptClose());
+  close_observer.Wait();
+  EXPECT_TRUE(browser_shutdown::IsTryingToQuit());
+  EXPECT_TRUE(BrowserList::GetInstance()->empty());
+}
+
 // Test that a window created during shutdown is closed.
 IN_PROC_BROWSER_TEST_P(BrowserCloseManagerBrowserTest,
                        TestAddWindowDuringShutdown) {