Try to shutdown the browser more peacefully before using more brutal methods.

Also, do proper cleanup before browser shutdown in MetricsService UI test.

I hope this will reduce the flakiness.

TEST=none
BUG=none

Review URL: http://codereview.chromium.org/173023

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

2 files changed, 33 insertions, 33 deletions

diff --git a/chrome/browser/metrics/metrics_service_uitest.cc b/chrome/browser/metrics/metrics_service_uitest.cc
index cf453a5..da25d8e 100644
--- a/chrome/browser/metrics/metrics_service_uitest.cc
+++ b/chrome/browser/metrics/metrics_service_uitest.cc
@@ -31,18 +31,18 @@ class MetricsServiceTest : public UITest {
 
   // Open a few tabs of random content
   void OpenTabs() {
-    window_ = automation()->GetBrowserWindow(0);
-    ASSERT_TRUE(window_.get());
+    scoped_refptr<BrowserProxy> window = automation()->GetBrowserWindow(0);
+    ASSERT_TRUE(window.get());
 
     FilePath page1_path;
     ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &page1_path));
     page1_path = page1_path.AppendASCII("title2.html");
-    ASSERT_TRUE(window_->AppendTab(net::FilePathToFileURL(page1_path)));
+    ASSERT_TRUE(window->AppendTab(net::FilePathToFileURL(page1_path)));
 
     FilePath page2_path;
     ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &page2_path));
     page2_path = page2_path.AppendASCII("iframe.html");
-    ASSERT_TRUE(window_->AppendTab(net::FilePathToFileURL(page2_path)));
+    ASSERT_TRUE(window->AppendTab(net::FilePathToFileURL(page2_path)));
   }
 
   // Get a PrefService whose contents correspond to the Local State file
@@ -55,14 +55,6 @@ class MetricsServiceTest : public UITest {
     PrefService* local_state(new PrefService(local_state_path, NULL));
     return local_state;
   }
-
-  virtual void TearDown() {
-    window_ = NULL;
-    UITest::TearDown();
-  }
-
- protected:
-  scoped_refptr<BrowserProxy> window_;
 };
 
 TEST_F(MetricsServiceTest, CloseRenderersNormally) {
@@ -87,15 +79,23 @@ TEST_F(MetricsServiceTest, CrashRenderers) {
 
   OpenTabs();
 
-  // kill the process for one of the tabs
-  scoped_refptr<TabProxy> tab(window_->GetTab(1));
-  ASSERT_TRUE(tab.get());
+  {
+    // Limit the lifetime of various automation proxies used here. We must
+    // destroy them before calling QuitBrowser.
+
+    scoped_refptr<BrowserProxy> window = automation()->GetBrowserWindow(0);
+    ASSERT_TRUE(window.get());
+
+    // Kill the process for one of the tabs.
+    scoped_refptr<TabProxy> tab(window->GetTab(1));
+    ASSERT_TRUE(tab.get());
 
 // Only windows implements the crash service for now.
 #if defined(OS_WIN)
-  expected_crashes_ = 1;
+    expected_crashes_ = 1;
 #endif
-  tab->NavigateToURLAsync(GURL("about:crash"));
+    tab->NavigateToURLAsync(GURL("about:crash"));
+  }
 
   // Give the browser a chance to notice the crashed tab.
   PlatformThread::Sleep(1000);
diff --git a/chrome/test/ui/ui_test.cc b/chrome/test/ui/ui_test.cc
index 8ddf22a..9f744f2 100644
--- a/chrome/test/ui/ui_test.cc
+++ b/chrome/test/ui/ui_test.cc
@@ -41,8 +41,8 @@ static const int kWaitForActionMsec = 2000;
 static const int kWaitForActionMaxMsec = 10000;
 // Delay to let the browser complete the test.
 static const int kMaxTestExecutionTime = 30000;
-// Delay to let the browser shut down.
-static const int kWaitForTerminateMsec = 5000;
+// Delay to let the browser shut down before trying more brutal methods.
+static const int kWaitForTerminateMsec = 30000;
 
 const wchar_t UITest::kFailedNoCrashService[] =
     L"NOTE: This test is expected to fail if crash_service.exe is not "
@@ -478,32 +478,32 @@ void UITest::LaunchBrowser(const CommandLine& arguments, bool clear_profile) {
 }
 
 void UITest::QuitBrowser() {
-  typedef std::vector<scoped_refptr<BrowserProxy> > BrowserVector;
-
   // There's nothing to do here if the browser is not running.
   if (IsBrowserRunning()) {
     automation()->SetFilteredInet(false);
-    BrowserVector browsers;
 
-    // Build up a list of HWNDs; we do this as a separate step so that closing
-    // the windows doesn't mess up the iteration.
     int window_count = 0;
     EXPECT_TRUE(automation()->GetBrowserWindowCount(&window_count));
 
-    for (int i = 0; i < window_count; ++i) {
+    // Synchronously close all but the last browser window. Closing them
+    // one-by-one may help with stability.
+    while (window_count > 1) {
       scoped_refptr<BrowserProxy> browser_proxy =
-          automation()->GetBrowserWindow(i);
-      browsers.push_back(browser_proxy);
+          automation()->GetBrowserWindow(0);
+      EXPECT_TRUE(browser_proxy->RunCommand(IDC_CLOSE_WINDOW));
+      EXPECT_TRUE(automation()->GetBrowserWindowCount(&window_count));
     }
 
-    for (BrowserVector::iterator iter = browsers.begin();
-      iter != browsers.end(); ++iter) {
-      // Use ApplyAccelerator since it doesn't wait
-      (*iter)->ApplyAccelerator(IDC_CLOSE_WINDOW);
+    // Close the last window asynchronously, because the browser may shutdown
+    // faster than it will be able to send a synchronous response to our
+    // message.
+    scoped_refptr<BrowserProxy> browser_proxy =
+        automation()->GetBrowserWindow(0);
+    if (browser_proxy.get()) {
+      browser_proxy->ApplyAccelerator(IDC_CLOSE_WINDOW);
+      browser_proxy = NULL;
     }
 
-    browsers.clear();
-
     // Now, drop the automation IPC channel so that the automation provider in
     // the browser notices and drops its reference to the browser process.
     server_->Disconnect();