From 40bbe59d2dd822e8448047f25c0349bb5c44d938 Mon Sep 17 00:00:00 2001 From: "phajdan.jr@chromium.org" Date: Wed, 6 Apr 2011 12:18:20 +0000 Subject: 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@80608 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/process_singleton_linux_uitest.cc | 15 ++++++++-- chrome/browser/unload_uitest.cc | 35 ++++++++++++------------ 2 files changed, 30 insertions(+), 20 deletions(-) (limited to 'chrome/browser') diff --git a/chrome/browser/process_singleton_linux_uitest.cc b/chrome/browser/process_singleton_linux_uitest.cc index 6606eb0..2129ed0 100644 --- a/chrome/browser/process_singleton_linux_uitest.cc +++ b/chrome/browser/process_singleton_linux_uitest.cc @@ -171,7 +171,10 @@ TEST_F(ProcessSingletonLinuxTest, NotifyOtherProcessFailure) { NotifyOtherProcess(url, TestTimeouts::action_timeout_ms())); // Wait for a while to make sure the browser process is actually killed. - EXPECT_FALSE(CrashAwareSleep(TestTimeouts::action_timeout_ms())); + int exit_code = 0; + ASSERT_TRUE(launcher_->WaitForBrowserProcessToQuit( + TestTimeouts::action_max_timeout_ms(), &exit_code)); + EXPECT_EQ(-1, exit_code); // Expect unclean shutdown. } // Test that we don't kill ourselves by accident if a lockfile with the same pid @@ -225,7 +228,10 @@ TEST_F(ProcessSingletonLinuxTest, NotifyOtherProcessDifferingHost) { // Kill the browser process, so that it does not respond on the socket. kill(pid, SIGKILL); // Wait for a while to make sure the browser process is actually killed. - EXPECT_FALSE(CrashAwareSleep(TestTimeouts::action_timeout_ms())); + int exit_code = 0; + ASSERT_TRUE(launcher_->WaitForBrowserProcessToQuit( + TestTimeouts::action_max_timeout_ms(), &exit_code)); + EXPECT_EQ(-1, exit_code); // Expect unclean shutdown. EXPECT_EQ(0, unlink(lock_path_.value().c_str())); EXPECT_EQ(0, symlink("FAKEFOOHOST-1234", lock_path_.value().c_str())); @@ -247,7 +253,10 @@ TEST_F(ProcessSingletonLinuxTest, NotifyOtherProcessOrCreate_DifferingHost) { // Kill the browser process, so that it does not respond on the socket. kill(pid, SIGKILL); // Wait for a while to make sure the browser process is actually killed. - EXPECT_FALSE(CrashAwareSleep(TestTimeouts::action_timeout_ms())); + int exit_code = 0; + ASSERT_TRUE(launcher_->WaitForBrowserProcessToQuit( + TestTimeouts::action_max_timeout_ms(), &exit_code)); + EXPECT_EQ(-1, exit_code); // Expect unclean shutdown. EXPECT_EQ(0, unlink(lock_path_.value().c_str())); EXPECT_EQ(0, symlink("FAKEFOOHOST-1234", lock_path_.value().c_str())); diff --git a/chrome/browser/unload_uitest.cc b/chrome/browser/unload_uitest.cc index 5ae47bd..002a27d 100644 --- a/chrome/browser/unload_uitest.cc +++ b/chrome/browser/unload_uitest.cc @@ -103,25 +103,13 @@ class UnloadTest : public UITest { UITest::SetUp(); } - void WaitForBrowserClosed() { - const int kCheckDelayMs = 100; - for (int max_wait_time = TestTimeouts::action_max_timeout_ms(); - max_wait_time > 0; max_wait_time -= kCheckDelayMs) { - CrashAwareSleep(kCheckDelayMs); - if (!IsBrowserRunning()) - break; - } - - EXPECT_FALSE(IsBrowserRunning()); - } - void CheckTitle(const std::wstring& expected_title) { const int kCheckDelayMs = 100; for (int max_wait_time = TestTimeouts::action_max_timeout_ms(); max_wait_time > 0; max_wait_time -= kCheckDelayMs) { - CrashAwareSleep(kCheckDelayMs); if (expected_title == GetActiveTabTitle()) break; + base::PlatformThread::Sleep(kCheckDelayMs); } EXPECT_EQ(expected_title, GetActiveTabTitle()); @@ -301,7 +289,11 @@ TEST_F(UnloadTest, BrowserCloseBeforeUnloadOK) { CloseBrowserAsync(browser.get()); ClickModalDialogButton(ui::MessageBoxFlags::DIALOGBUTTON_OK); - WaitForBrowserClosed(); + + int exit_code = -1; + ASSERT_TRUE(launcher_->WaitForBrowserProcessToQuit( + TestTimeouts::action_max_timeout_ms(), &exit_code)); + EXPECT_EQ(0, exit_code); // Expect a clean shutown. } // Tests closing the browser with a beforeunload handler and clicking @@ -313,14 +305,19 @@ TEST_F(UnloadTest, BrowserCloseBeforeUnloadCancel) { CloseBrowserAsync(browser.get()); ClickModalDialogButton(ui::MessageBoxFlags::DIALOGBUTTON_CANCEL); + // There's no real graceful way to wait for something _not_ to happen, so // we just wait a short period. - CrashAwareSleep(500); + base::PlatformThread::Sleep(TestTimeouts::action_timeout_ms()); ASSERT_TRUE(IsBrowserRunning()); CloseBrowserAsync(browser.get()); ClickModalDialogButton(ui::MessageBoxFlags::DIALOGBUTTON_OK); - WaitForBrowserClosed(); + + int exit_code = -1; + ASSERT_TRUE(launcher_->WaitForBrowserProcessToQuit( + TestTimeouts::action_max_timeout_ms(), &exit_code)); + EXPECT_EQ(0, exit_code); // Expect a clean shutdown. } #if defined(OS_LINUX) @@ -342,7 +339,11 @@ TEST_F(UnloadTest, MAYBE_BrowserCloseWithInnerFocusedFrame) { CloseBrowserAsync(browser.get()); ClickModalDialogButton(ui::MessageBoxFlags::DIALOGBUTTON_OK); - WaitForBrowserClosed(); + + int exit_code = -1; + ASSERT_TRUE(launcher_->WaitForBrowserProcessToQuit( + TestTimeouts::action_max_timeout_ms(), &exit_code)); + EXPECT_EQ(0, exit_code); // Expect a clean shutdown. } // Tests closing the browser with a beforeunload handler that takes -- cgit v1.1