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..d218208e847381662c45ef00ba701621c3db8de1 100644
--- a/chrome/browser/lifetime/browser_close_manager_browsertest.cc
+++ b/chrome/browser/lifetime/browser_close_manager_browsertest.cc
@@ -479,6 +479,78 @@ IN_PROC_BROWSER_TEST_P(BrowserCloseManagerBrowserTest,
   EXPECT_TRUE(BrowserList::GetInstance()->empty());
 }
 
+// Test that tabs that slow to respond are not closed prematurely.
+IN_PROC_BROWSER_TEST_P(BrowserCloseManagerBrowserTest,
+                       TestUnloadMultipleSlowTabs) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  for (int i = 0; i < 7; i++) {

[Charlie Reis, 2016/05/13 22:24:01]

Can you add a comment to this block saying why you're loading one instance of
beforeunload.html vs lots of beforeunload_slow.html?

[Mikhail Goncharov, 2016/05/16 06:10:30]

I have added comment and uploaded next patchset.

+    if (i)
+      AddBlankTabAndShow(browsers_[0]);
+    if (i == 3) {
+      ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(
+          browsers_[0], embedded_test_server()->GetURL("/beforeunload.html")));
+    } else {
+      ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(
+          browsers_[0],
+          embedded_test_server()->GetURL("/beforeunload_slow.html")));
+    }
+  }
+  RepeatedNotificationObserver cancel_observer(
+      chrome::NOTIFICATION_BROWSER_CLOSE_CANCELLED, 1);
+  chrome::CloseAllBrowsersAndQuit();
+  ASSERT_NO_FATAL_FAILURE(CancelClose());
+  cancel_observer.Wait();
+  EXPECT_FALSE(browser_shutdown::IsTryingToQuit());
+  // All tabs should still be open.
+  EXPECT_EQ(7, browsers_[0]->tab_strip_model()->count());
+
+  RepeatedNotificationObserver close_observer(
+      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.
+IN_PROC_BROWSER_TEST_P(BrowserCloseManagerBrowserTest,
+                       TestBeforeUnloadMultipleSlowWindows) {

[Charlie Reis, 2016/05/13 22:24:01]

Is there a meaningful difference between slow windows and slow tabs?  I would
imagine that's the same code, but I don't know the UnloadController.

[Mikhail Goncharov, 2016/05/16 04:50:40]

There is a subtle difference on how unload controller iterates over windows vs
tabs and this test fails in a different way with no fix (on (d)check that tab
closed on another tabsstrip)

+  ASSERT_TRUE(embedded_test_server()->Start());
+  for (int i = 0; i < 5; i++) {
+    if (i)
+      browsers_.push_back(CreateBrowser(browser()->profile()));
+    if (i == 2) {
+      ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(
+          browsers_[i], embedded_test_server()->GetURL("/beforeunload.html")));
+    } else {
+      ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(
+          browsers_[i],
+          embedded_test_server()->GetURL("/beforeunload_slow.html")));
+    }
+  }
+
+  RepeatedNotificationObserver cancel_observer(
+      chrome::NOTIFICATION_BROWSER_CLOSE_CANCELLED, 3);
+  chrome::CloseAllBrowsersAndQuit();
+  ASSERT_NO_FATAL_FAILURE(CancelClose());
+  cancel_observer.Wait();
+  EXPECT_FALSE(browser_shutdown::IsTryingToQuit());
+  // All windows should still be open.
+  for (int i = 0; i < 5; i++)
+    EXPECT_EQ(1, browsers_[i]->tab_strip_model()->count());
+
+  RepeatedNotificationObserver close_observer(
+      chrome::NOTIFICATION_BROWSER_CLOSED, 5);
+  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) {