GTTF: Detect browser crashes on shutdown in UI tests.

Previously the automation framework could miss a browser
crash during shutdown on POSIX (on Windows there is
crash_service.exe that should catch all crashes).

This change makes the automation framework avoid losing
information about the browser process' exit status
(CrashAwareSleep), and fixes a bug in base::WaitForExitCodeWithTimeout
(which on POSIX never reported the process has been signaled).

Finally, it makes the automation framework use WaitForExitCodeWithTimeout
instead of WaitForSingleProcess. This way we can get the exit status
information in an accurate and cross-platform way.

To avoid trying to close the same process handle twice (it's only an issue on Windows) I've changed WaitForExitCodeWithTimeout not to close the passed handle. It's only used in few places and I think this CL fixes all of them.

I've tested this change locally on Mac with a UI test that SIGKILLs the browser.
Before this change the test passed (it shouldn't), and after this change
the test failed with an information that the browser has not exited cleanly.

BUG=56644
Review URL: http://codereview.chromium.org/6689014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@80472 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 59 insertions, 95 deletions

diff --git a/chrome/test/automation/proxy_launcher.cc b/chrome/test/automation/proxy_launcher.cc
index 90a62b2..1928902 100644
--- a/chrome/test/automation/proxy_launcher.cc
+++ b/chrome/test/automation/proxy_launcher.cc
@@ -206,13 +206,14 @@ void ProxyLauncher::QuitBrowser() {
     return;
   }
 
+  base::TimeTicks quit_start = base::TimeTicks::Now();
+
   // There's nothing to do here if the browser is not running.
   // WARNING: There is a race condition here where the browser may shut down
   // after this check but before some later automation call. Your test should
   // use WaitForBrowserProcessToQuit() if it intentionally
   // causes the browser to shut down.
   if (IsBrowserRunning()) {
-    base::TimeTicks quit_start = base::TimeTicks::Now();
     EXPECT_TRUE(automation()->SetFilteredInet(false));
 
     if (WINDOW_CLOSE == shutdown_type_) {
@@ -253,21 +254,24 @@ void ProxyLauncher::QuitBrowser() {
     } else {
       NOTREACHED() << "Invalid shutdown type " << shutdown_type_;
     }
+  }
 
-    // Now, drop the automation IPC channel so that the automation provider in
-    // the browser notices and drops its reference to the browser process.
-    automation()->Disconnect();
-
-    // Wait for the browser process to quit. It should quit once all tabs have
-    // been closed.
-    if (!WaitForBrowserProcessToQuit(
-        TestTimeouts::wait_for_terminate_timeout_ms())) {
-      // We need to force the browser to quit because it didn't quit fast
-      // enough. Take no chance and kill every chrome processes.
-      CleanupAppProcesses();
-    }
-    browser_quit_time_ = base::TimeTicks::Now() - quit_start;
+  // Now, drop the automation IPC channel so that the automation provider in
+  // the browser notices and drops its reference to the browser process.
+  automation()->Disconnect();
+
+  // Wait for the browser process to quit. It should quit once all tabs have
+  // been closed.
+  int exit_code = -1;
+  if (WaitForBrowserProcessToQuit(
+          TestTimeouts::wait_for_terminate_timeout_ms(), &exit_code)) {
+    EXPECT_EQ(0, exit_code);  // Expect a clean shutdown.
+  } else {
+    // We need to force the browser to quit because it didn't quit fast
+    // enough. Take no chance and kill every chrome processes.
+    CleanupAppProcesses();
   }
+  browser_quit_time_ = base::TimeTicks::Now() - quit_start;
 
   // Don't forget to close the handle
   base::CloseProcessHandle(process_);
@@ -276,8 +280,9 @@ void ProxyLauncher::QuitBrowser() {
 }
 
 void ProxyLauncher::TerminateBrowser() {
+  base::TimeTicks quit_start = base::TimeTicks::Now();
+
   if (IsBrowserRunning()) {
-    base::TimeTicks quit_start = base::TimeTicks::Now();
     EXPECT_TRUE(automation()->SetFilteredInet(false));
 #if defined(OS_WIN)
     scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
@@ -292,15 +297,18 @@ void ProxyLauncher::TerminateBrowser() {
 #if defined(OS_POSIX)
     EXPECT_EQ(kill(process_, SIGTERM), 0);
 #endif  // OS_POSIX
+  }
 
-    if (!WaitForBrowserProcessToQuit(
-        TestTimeouts::wait_for_terminate_timeout_ms())) {
-      // We need to force the browser to quit because it didn't quit fast
-      // enough. Take no chance and kill every chrome processes.
-      CleanupAppProcesses();
-    }
-    browser_quit_time_ = base::TimeTicks::Now() - quit_start;
+  int exit_code = 0;
+  if (WaitForBrowserProcessToQuit(
+          TestTimeouts::wait_for_terminate_timeout_ms(), &exit_code)) {
+    EXPECT_EQ(0, exit_code);  // Expect a clean shutdown.
+  } else {
+    // We need to force the browser to quit because it didn't quit fast
+    // enough. Take no chance and kill every chrome processes.
+    CleanupAppProcesses();
   }
+  browser_quit_time_ = base::TimeTicks::Now() - quit_start;
 
   // Don't forget to close the handle
   base::CloseProcessHandle(process_);
@@ -327,19 +335,18 @@ void ProxyLauncher::CleanupAppProcesses() {
   TerminateAllChromeProcesses(process_id_);
 }
 
-bool ProxyLauncher::WaitForBrowserProcessToQuit(int timeout) {
+bool ProxyLauncher::WaitForBrowserProcessToQuit(int timeout, int* exit_code) {
 #ifdef WAIT_FOR_DEBUGGER_ON_OPEN
   timeout = 500000;
 #endif
-  return base::WaitForSingleProcess(process_, timeout);
+  return base::WaitForExitCodeWithTimeout(process_, exit_code, timeout);
 }
 
 bool ProxyLauncher::IsBrowserRunning() {
-  return CrashAwareSleep(0);
-}
-
-bool ProxyLauncher::CrashAwareSleep(int timeout_ms) {
-  return base::CrashAwareSleep(process_, timeout_ms);
+  // Send a simple message to the browser. If it comes back, the browser
+  // must be alive.
+  int window_count;
+  return automation_proxy_->GetBrowserWindowCount(&window_count);
 }
 
 void ProxyLauncher::PrepareTestCommandline(CommandLine* command_line,
diff --git a/chrome/test/automation/proxy_launcher.h b/chrome/test/automation/proxy_launcher.h
index 642c539..505c65d 100644
--- a/chrome/test/automation/proxy_launcher.h
+++ b/chrome/test/automation/proxy_launcher.h
@@ -122,13 +122,10 @@ class ProxyLauncher {
   // window or if the browser process died by itself.
   bool IsBrowserRunning();
 
-  // Returns true when timeout_ms milliseconds have elapsed.
-  // Returns false if the browser process died while waiting.
-  bool CrashAwareSleep(int timeout_ms);
-
   // Wait for the browser process to shut down on its own (i.e. as a result of
-  // some action that your test has taken).
-  bool WaitForBrowserProcessToQuit(int timeout);
+  // some action that your test has taken). If it has exited within |timeout|,
+  // puts the exit code in |exit_code| and returns true.
+  bool WaitForBrowserProcessToQuit(int timeout, int* exit_code);
 
   AutomationProxy* automation() const;
 
diff --git a/chrome/test/interactive_ui/fast_shutdown_interactive_uitest.cc b/chrome/test/interactive_ui/fast_shutdown_interactive_uitest.cc
index 15a0ab8..a783f3c 100644
--- a/chrome/test/interactive_ui/fast_shutdown_interactive_uitest.cc
+++ b/chrome/test/interactive_ui/fast_shutdown_interactive_uitest.cc
@@ -54,6 +54,9 @@ TEST_F(FastShutdown, MAYBE_SlowTermination) {
   ASSERT_TRUE(automation()->WaitForAppModalDialog());
   ASSERT_TRUE(automation()->ClickAppModalDialogButton(
                   ui::MessageBoxFlags::DIALOGBUTTON_OK));
-  ASSERT_TRUE(WaitForBrowserProcessToQuit(
-      TestTimeouts::wait_for_terminate_timeout_ms()));
+
+  int exit_code = -1;
+  ASSERT_TRUE(launcher_->WaitForBrowserProcessToQuit(
+                  TestTimeouts::wait_for_terminate_timeout_ms(), &exit_code));
+  EXPECT_EQ(0, exit_code);  // Expect a clean shutdown.
 }
diff --git a/chrome/test/out_of_proc_test_runner.cc b/chrome/test/out_of_proc_test_runner.cc
index db5e54c..6e4bc3b 100644
--- a/chrome/test/out_of_proc_test_runner.cc
+++ b/chrome/test/out_of_proc_test_runner.cc
@@ -401,6 +401,8 @@ int RunTest(const std::string& test_name, int default_timeout_ms) {
   }
 #endif
 
+  base::CloseProcessHandle(process_handle);
+
   return exit_code;
 }
 
diff --git a/chrome/test/ui/ui_test.cc b/chrome/test/ui/ui_test.cc
index 2e47340..95378d7 100644
--- a/chrome/test/ui/ui_test.cc
+++ b/chrome/test/ui/ui_test.cc
@@ -297,10 +297,6 @@ void UITestBase::NavigateToURLBlockUntilNavigationsComplete(
                 url, number_of_navigations)) << url.spec();
 }
 
-bool UITestBase::WaitForBrowserProcessToQuit(int timeout) {
-  return launcher_->WaitForBrowserProcessToQuit(timeout);
-}
-
 bool UITestBase::WaitForBookmarkBarVisibilityChange(BrowserProxy* browser,
                                                     bool wait_for_open) {
   const int kCycles = 10;
@@ -313,11 +309,7 @@ bool UITestBase::WaitForBookmarkBarVisibilityChange(BrowserProxy* browser,
       return true;  // Bookmark bar visibility change complete.
 
     // Give it a chance to catch up.
-    bool browser_survived = CrashAwareSleep(
-        TestTimeouts::action_timeout_ms() / kCycles);
-    EXPECT_TRUE(browser_survived);
-    if (!browser_survived)
-      return false;
+    base::PlatformThread::Sleep(TestTimeouts::action_timeout_ms() / kCycles);
   }
 
   ADD_FAILURE() << "Timeout reached in WaitForBookmarkBarVisibilityChange";
@@ -365,10 +357,6 @@ bool UITestBase::IsBrowserRunning() {
   return launcher_->IsBrowserRunning();
 }
 
-bool UITestBase::CrashAwareSleep(int timeout_ms) {
-  return launcher_->CrashAwareSleep(timeout_ms);
-}
-
 int UITestBase::GetTabCount() {
   return GetTabCount(0);
 }
@@ -391,12 +379,10 @@ void UITestBase::WaitUntilTabCount(int tab_count) {
   const int kIntervalMs = TestTimeouts::action_timeout_ms() / kMaxIntervals;
 
   for (int i = 0; i < kMaxIntervals; ++i) {
-    bool browser_survived = CrashAwareSleep(kIntervalMs);
-    EXPECT_TRUE(browser_survived);
-    if (!browser_survived)
-      return;
     if (GetTabCount() == tab_count)
       return;
+
+    base::PlatformThread::Sleep(kIntervalMs);
   }
 
   ADD_FAILURE() << "Timeout reached in WaitUntilTabCount";
@@ -727,11 +713,6 @@ bool UITest::WaitUntilJavaScriptCondition(TabProxy* tab,
 
   // Wait until the test signals it has completed.
   for (int i = 0; i < kMaxIntervals; ++i) {
-    bool browser_survived = CrashAwareSleep(kIntervalMs);
-    EXPECT_TRUE(browser_survived);
-    if (!browser_survived)
-      return false;
-
     bool done_value = false;
     bool success = tab->ExecuteAndExtractBool(frame_xpath, jscript,
                                               &done_value);
@@ -740,6 +721,8 @@ bool UITest::WaitUntilJavaScriptCondition(TabProxy* tab,
       return false;
     if (done_value)
       return true;
+
+    base::PlatformThread::Sleep(kIntervalMs);
   }
 
   ADD_FAILURE() << "Timeout reached in WaitUntilJavaScriptCondition";
@@ -756,14 +739,11 @@ bool UITest::WaitUntilCookieValue(TabProxy* tab,
 
   std::string cookie_value;
   for (int i = 0; i < kMaxIntervals; ++i) {
-    bool browser_survived = CrashAwareSleep(kIntervalMs);
-    EXPECT_TRUE(browser_survived);
-    if (!browser_survived)
-      return false;
-
     EXPECT_TRUE(tab->GetCookieByName(url, cookie_name, &cookie_value));
     if (cookie_value == expected_value)
       return true;
+
+    base::PlatformThread::Sleep(kIntervalMs);
   }
 
   ADD_FAILURE() << "Timeout reached in WaitUntilCookieValue";
@@ -778,15 +758,12 @@ std::string UITest::WaitUntilCookieNonEmpty(TabProxy* tab,
   const int kMaxIntervals = timeout_ms / kIntervalMs;
 
   for (int i = 0; i < kMaxIntervals; ++i) {
-    bool browser_survived = CrashAwareSleep(kIntervalMs);
-    EXPECT_TRUE(browser_survived);
-    if (!browser_survived)
-      return std::string();
-
     std::string cookie_value;
     EXPECT_TRUE(tab->GetCookieByName(url, cookie_name, &cookie_value));
     if (!cookie_value.empty())
       return cookie_value;
+
+    base::PlatformThread::Sleep(kIntervalMs);
   }
 
   ADD_FAILURE() << "Timeout reached in WaitUntilCookieNonEmpty";
@@ -812,11 +789,7 @@ bool UITest::WaitForFindWindowVisibilityChange(BrowserProxy* browser,
       return true;  // Find window visibility change complete.
 
     // Give it a chance to catch up.
-    bool browser_survived = CrashAwareSleep(
-        TestTimeouts::action_timeout_ms() / kCycles);
-    EXPECT_TRUE(browser_survived);
-    if (!browser_survived)
-      return false;
+    base::PlatformThread::Sleep(TestTimeouts::action_timeout_ms() / kCycles);
   }
 
   ADD_FAILURE() << "Timeout reached in WaitForFindWindowVisibilityChange";
@@ -840,19 +813,6 @@ bool UITest::WaitForDownloadShelfVisibilityChange(BrowserProxy* browser,
   int incorrect_state_count = 0;
   base::Time start = base::Time::Now();
   for (int i = 0; i < kCycles; i++) {
-    // Give it a chance to catch up.
-    bool browser_survived = CrashAwareSleep(
-        TestTimeouts::action_timeout_ms() / kCycles);
-    EXPECT_TRUE(browser_survived);
-    if (!browser_survived) {
-      LOG(INFO) << "Elapsed time: " << (base::Time::Now() - start).InSecondsF()
-                << " seconds"
-                << " call failed " << fail_count << " times"
-                << " state was incorrect " << incorrect_state_count << " times";
-      ADD_FAILURE() << "Browser failed in " << __FUNCTION__;
-      return false;
-    }
-
     bool visible = !wait_for_open;
     if (!browser->IsShelfVisible(&visible)) {
       fail_count++;
@@ -866,6 +826,9 @@ bool UITest::WaitForDownloadShelfVisibilityChange(BrowserProxy* browser,
       return true;  // Got the download shelf.
     }
     incorrect_state_count++;
+
+    // Give it a chance to catch up.
+    base::PlatformThread::Sleep(TestTimeouts::action_timeout_ms() / kCycles);
   }
 
   LOG(INFO) << "Elapsed time: " << (base::Time::Now() - start).InSecondsF()
diff --git a/chrome/test/ui/ui_test.h b/chrome/test/ui/ui_test.h
index f446c6a..5ec4039 100644
--- a/chrome/test/ui/ui_test.h
+++ b/chrome/test/ui/ui_test.h
@@ -139,10 +139,6 @@ class UITestBase {
   // window or if the browser process died by itself.
   bool IsBrowserRunning();
 
-  // Returns true when timeout_ms milliseconds have elapsed.
-  // Returns false if the browser process died while waiting.
-  bool CrashAwareSleep(int timeout_ms);
-
   // Returns the number of tabs in the first window.  If no windows exist,
   // causes a test failure and returns 0.
   int GetTabCount();
@@ -154,10 +150,6 @@ class UITestBase {
   // assert that the tab count is valid at the end of the wait.
   void WaitUntilTabCount(int tab_count);
 
-  // Wait for the browser process to shut down on its own (i.e. as a result of
-  // some action that your test has taken).
-  bool WaitForBrowserProcessToQuit(int timeout);
-
   // Waits until the Bookmark bar has stopped animating and become fully visible
   // (if |wait_for_open| is true) or fully hidden (if |wait_for_open| is false).
   // This function can time out (in which case it returns false).