Make running chrome process detection in ui tests more reliable.

Also drops fragile code.

TEST=Covered by ui_tests.
BUG=10840

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

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

12 files changed, 46 insertions, 175 deletions

diff --git a/chrome/browser/process_singleton_linux_uitest.cc b/chrome/browser/process_singleton_linux_uitest.cc
index f325e59..8bef102 100644
--- a/chrome/browser/process_singleton_linux_uitest.cc
+++ b/chrome/browser/process_singleton_linux_uitest.cc
@@ -102,7 +102,7 @@ TEST_F(ProcessSingletonLinuxTest, NotifyOtherProcessSuccess) {
 
 // Test failure case of NotifyOtherProcess().
 TEST_F(ProcessSingletonLinuxTest, NotifyOtherProcessFailure) {
-  base::ProcessId pid = ChromeBrowserProcessId(user_data_dir());
+  base::ProcessId pid = browser_process_id();
 
   ASSERT_GE(pid, 1);
 
@@ -139,7 +139,7 @@ TEST_F(ProcessSingletonLinuxTest, NotifyOtherProcessHostChanged) {
 // Test that we fail when lock says process is on another host and we can't
 // notify it over the socket.
 TEST_F(ProcessSingletonLinuxTest, NotifyOtherProcessDifferingHost) {
-  base::ProcessId pid = ChromeBrowserProcessId(user_data_dir());
+  base::ProcessId pid = browser_process_id();
 
   ASSERT_GE(pid, 1);
 
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index fc55fe3..45f5d7d 100755
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -105,9 +105,7 @@
         'test/automation/window_proxy.h',
         'test/chrome_process_util.cc',
         'test/chrome_process_util.h',
-        'test/chrome_process_util_linux.cc',
         'test/chrome_process_util_mac.cc',
-        'test/chrome_process_util_win.cc',
         'test/test_browser_window.h',
         'test/testing_profile.cc',
         'test/testing_profile.h',
diff --git a/chrome/test/chrome_process_util.cc b/chrome/test/chrome_process_util.cc
index fea1d0e..dd3ee1b 100644
--- a/chrome/test/chrome_process_util.cc
+++ b/chrome/test/chrome_process_util.cc
@@ -15,12 +15,12 @@
 using base::Time;
 using base::TimeDelta;
 
-void TerminateAllChromeProcesses(const FilePath& data_dir) {
+void TerminateAllChromeProcesses(base::ProcessId browser_pid) {
   // Total time the function will wait for chrome processes
   // to terminate after it told them to do so.
   const TimeDelta kExitTimeout = TimeDelta::FromSeconds(30);
 
-  ChromeProcessList process_pids(GetRunningChromeProcesses(data_dir));
+  ChromeProcessList process_pids(GetRunningChromeProcesses(browser_pid));
 
   std::vector<base::ProcessHandle> handles;
   {
@@ -68,11 +68,10 @@ class ChildProcessFilter : public base::ProcessFilter {
   DISALLOW_COPY_AND_ASSIGN(ChildProcessFilter);
 };
 
-ChromeProcessList GetRunningChromeProcesses(const FilePath& data_dir) {
+ChromeProcessList GetRunningChromeProcesses(base::ProcessId browser_pid) {
   ChromeProcessList result;
-
-  base::ProcessId browser_pid = ChromeBrowserProcessId(data_dir);
-  if (browser_pid == (base::ProcessId) -1)
+  
+  if (browser_pid == static_cast<base::ProcessId>(-1))
     return result;
 
   ChildProcessFilter filter(browser_pid);
diff --git a/chrome/test/chrome_process_util.h b/chrome/test/chrome_process_util.h
index 4fb1513..94e0355 100644
--- a/chrome/test/chrome_process_util.h
+++ b/chrome/test/chrome_process_util.h
@@ -13,16 +13,13 @@
 
 typedef std::vector<base::ProcessId> ChromeProcessList;
 
-// Returns PID of browser process running with user data dir |data_dir|.
-// Returns -1 on error.
-base::ProcessId ChromeBrowserProcessId(const FilePath& data_dir);
+// Returns a vector of PIDs of all chrome processes (main and renderers etc)
+// based on |browser_pid|, the PID of the main browser process.
+ChromeProcessList GetRunningChromeProcesses(base::ProcessId browser_pid);
 
-// Returns a vector of PIDs of chrome processes (main and renderers etc)
-// associated with user data dir |data_dir|. On error returns empty vector.
-ChromeProcessList GetRunningChromeProcesses(const FilePath& data_dir);
-
-// Attempts to terminate all chrome processes associated with |data_dir|.
-void TerminateAllChromeProcesses(const FilePath& data_dir);
+// Attempts to terminate all chrome processes launched by (and including)
+// |browser_pid|.
+void TerminateAllChromeProcesses(base::ProcessId browser_pid);
 
 // A wrapper class for tests to use in fetching process metrics.
 // Delegates everything we need to base::ProcessMetrics, except
diff --git a/chrome/test/chrome_process_util_linux.cc b/chrome/test/chrome_process_util_linux.cc
deleted file mode 100644
index 8905c8c..0000000
--- a/chrome/test/chrome_process_util_linux.cc
+++ /dev/null
@@ -1,44 +0,0 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/test/chrome_process_util.h"
-
-#include <stdio.h>
-
-#include <string>
-#include <vector>
-
-#include "base/command_line.h"
-#include "base/logging.h"
-#include "base/process_util.h"
-#include "base/string_util.h"
-#include "chrome/common/chrome_constants.h"
-
-base::ProcessId ChromeBrowserProcessId(const FilePath& data_dir) {
-  FilePath socket_name = data_dir.Append(chrome::kSingletonSocketFilename);
-
-  std::vector<std::string> argv;
-  argv.push_back("fuser");
-  argv.push_back(socket_name.value());
-
-  std::string fuser_output;
-  if (!base::GetAppOutput(CommandLine(argv), &fuser_output))
-    return -1;
-
-  std::string trimmed_output;
-  TrimWhitespaceASCII(fuser_output, TRIM_ALL, &trimmed_output);
-
-  if (trimmed_output.find(' ') != std::string::npos) {
-    LOG(FATAL) << "Expected exactly 1 process to have socket open: " <<
-                  fuser_output;
-    return -1;
-  }
-
-  int pid;
-  if (!StringToInt(trimmed_output, &pid)) {
-    LOG(FATAL) << "Unexpected fuser output: " << fuser_output;
-    return -1;
-  }
-  return pid;
-}
diff --git a/chrome/test/chrome_process_util_mac.cc b/chrome/test/chrome_process_util_mac.cc
index 4c60ad3..d920735 100644
--- a/chrome/test/chrome_process_util_mac.cc
+++ b/chrome/test/chrome_process_util_mac.cc
@@ -11,51 +11,6 @@
 #include "base/process_util.h"
 #include "base/string_util.h"
 
-// Yes, this is impossibly lame. This horrible hack is Good Enough, though,
-// because it's not used in production code, but just for testing.
-//
-// We could make this better by creating a system through which all instances of
-// Chromium can communicate. ProcessSingleton does that for Windows and Linux,
-// but the Mac doesn't implement it as its system services handle it. It's not
-// worth implementing just for this.
-//
-// We could do something similar to what Linux does, and use |fuser| to find a
-// file that the app ordinarily opens within the data dir. However, fuser is
-// broken on Leopard, and does not detect files that lsof shows are open.
-//
-// What's going on here is that during ui_tests, the Chromium application is
-// launched using the --user-data-dir command line option. By examining the
-// output of ps, we can find the appropriately-launched Chromium process. Note
-// that this _does_ work for paths with spaces. The command line that ps gives
-// is just the argv separated with spaces. There's no escaping spaces as a shell
-// would do, so a straight string comparison will work just fine.
-//
-// TODO(avi):see if there is a better way
-
-base::ProcessId ChromeBrowserProcessId(const FilePath& data_dir) {
-  std::vector<std::string> argv;
-  argv.push_back("ps");
-  argv.push_back("-xw");
-
-  std::string ps_output;
-  if (!base::GetAppOutput(CommandLine(argv), &ps_output))
-    return -1;
-
-  std::vector<std::string> lines;
-  SplitString(ps_output, '\n', &lines);
-
-  for (size_t i=0; i<lines.size(); ++i) {
-    const std::string& line = lines[i];
-    if (line.find(data_dir.value()) != std::string::npos &&
-        line.find("type=renderer") == std::string::npos) {
-      int pid = StringToInt(line);  // pid is at beginning of line
-      return pid==0 ? -1 : pid;
-    }
-  }
-
-  return -1;
-}
-
 MacChromeProcessInfoList GetRunningMacProcessInfo(
     const ChromeProcessList &process_list) {
   MacChromeProcessInfoList result;
diff --git a/chrome/test/chrome_process_util_uitest.cc b/chrome/test/chrome_process_util_uitest.cc
index 78f3fa9..92865c0 100644
--- a/chrome/test/chrome_process_util_uitest.cc
+++ b/chrome/test/chrome_process_util_uitest.cc
@@ -11,10 +11,11 @@ class ChromeProcessUtilTest : public UITest {
 
 TEST_F(ChromeProcessUtilTest, SanityTest) {
   EXPECT_TRUE(IsBrowserRunning());
-  EXPECT_NE(-1, ChromeBrowserProcessId(user_data_dir()));
-  EXPECT_GE(GetRunningChromeProcesses(user_data_dir()).size(), 1U);
+  ChromeProcessList processes = GetRunningChromeProcesses(browser_process_id());
+  EXPECT_FALSE(processes.empty());
   QuitBrowser();
   EXPECT_FALSE(IsBrowserRunning());
-  EXPECT_EQ(-1, ChromeBrowserProcessId(user_data_dir()));
-  EXPECT_EQ(0U, GetRunningChromeProcesses(user_data_dir()).size());
+  EXPECT_EQ(-1, browser_process_id());
+  processes = GetRunningChromeProcesses(browser_process_id());
+  EXPECT_TRUE(processes.empty());
 }
diff --git a/chrome/test/chrome_process_util_win.cc b/chrome/test/chrome_process_util_win.cc
deleted file mode 100644
index 97fbc19..0000000
--- a/chrome/test/chrome_process_util_win.cc
+++ /dev/null
@@ -1,26 +0,0 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/test/chrome_process_util.h"
-
-#include <windows.h>
-
-#include <vector>
-
-#include "base/logging.h"
-#include "base/process_util.h"
-#include "chrome/common/chrome_constants.h"
-
-base::ProcessId ChromeBrowserProcessId(const FilePath& data_dir) {
-  HWND message_window = FindWindowEx(HWND_MESSAGE, NULL,
-                                     chrome::kMessageWindowClass,
-                                     data_dir.value().c_str());
-  if (!message_window)
-    return -1;
-
-  DWORD browser_pid = -1;
-  GetWindowThreadProcessId(message_window, &browser_pid);
-
-  return browser_pid;
-}
diff --git a/chrome/test/memory_test/memory_test.cc b/chrome/test/memory_test/memory_test.cc
index 5284bb8..9c22c1b 100644
--- a/chrome/test/memory_test/memory_test.cc
+++ b/chrome/test/memory_test/memory_test.cc
@@ -225,8 +225,8 @@ class MemoryTest : public UITest {
     }
 
     size_t stop_size = base::GetSystemCommitCharge();
-    PrintIOPerfInfo(test_name, user_data_dir_);
-    PrintMemoryUsageInfo(test_name, user_data_dir_);
+    PrintIOPerfInfo(test_name);
+    PrintMemoryUsageInfo(test_name);
     PrintSystemCommitCharge(test_name, stop_size - start_size,
                             true /* important */);
   }
diff --git a/chrome/test/page_cycler/page_cycler_test.cc b/chrome/test/page_cycler/page_cycler_test.cc
index 172655d..471a581 100644
--- a/chrome/test/page_cycler/page_cycler_test.cc
+++ b/chrome/test/page_cycler/page_cycler_test.cc
@@ -246,11 +246,8 @@ class PageCyclerTest : public UITest {
       return;
     size_t stop_size = base::GetSystemCommitCharge();
 
-    FilePath data_dir;
-    PathService::Get(chrome::DIR_USER_DATA, &data_dir);
-
-    PrintMemoryUsageInfo(suffix, data_dir);
-    PrintIOPerfInfo(suffix, data_dir);
+    PrintMemoryUsageInfo(suffix);
+    PrintIOPerfInfo(suffix);
     PrintSystemCommitCharge(suffix, stop_size - start_size,
                             false /* not important */);
 
@@ -297,11 +294,8 @@ class PageCyclerReferenceTest : public PageCyclerTest {
       return;
     size_t stop_size = base::GetSystemCommitCharge();
 
-    FilePath data_dir;
-    PathService::Get(chrome::DIR_USER_DATA, &data_dir);
-
-    PrintMemoryUsageInfo("_ref", data_dir);
-    PrintIOPerfInfo("_ref", data_dir);
+    PrintMemoryUsageInfo("_ref");
+    PrintIOPerfInfo("_ref");
     PrintSystemCommitCharge("_ref", stop_size - start_size,
                             false /* not important */);
 
diff --git a/chrome/test/ui/ui_test.cc b/chrome/test/ui/ui_test.cc
index 4c2f887..2eb163f 100644
--- a/chrome/test/ui/ui_test.cc
+++ b/chrome/test/ui/ui_test.cc
@@ -472,14 +472,14 @@ void UITest::QuitBrowser() {
   // Don't forget to close the handle
   base::CloseProcessHandle(process_);
   process_ = base::kNullProcessHandle;
+  process_id_ = -1;
 }
 
 void UITest::AssertAppNotRunning(const std::wstring& error_message) {
   std::wstring final_error_message(error_message);
 
-  int process_count = GetBrowserProcessCount();
-  if (process_count > 0) {
-    ChromeProcessList processes = GetRunningChromeProcesses(user_data_dir());
+  ChromeProcessList processes = GetRunningChromeProcesses(process_id_);
+  if (!processes.empty()) {
     final_error_message += L" Leftover PIDs: [";
     for (ChromeProcessList::const_iterator it = processes.begin();
          it != processes.end(); ++it) {
@@ -487,11 +487,11 @@ void UITest::AssertAppNotRunning(const std::wstring& error_message) {
     }
     final_error_message += L" ]";
   }
-  ASSERT_EQ(0, process_count) << final_error_message;
+  ASSERT_TRUE(processes.empty()) << final_error_message;
 }
 
 void UITest::CleanupAppProcesses() {
-  TerminateAllChromeProcesses(user_data_dir());
+  TerminateAllChromeProcesses(process_id_);
 }
 
 scoped_refptr<TabProxy> UITest::GetActiveTab(int window_index) {
@@ -657,11 +657,8 @@ bool UITest::CrashAwareSleep(int time_out_ms) {
   return base::CrashAwareSleep(process_, time_out_ms);
 }
 
-// static
 int UITest::GetBrowserProcessCount() {
-  FilePath data_dir;
-  PathService::Get(chrome::DIR_USER_DATA, &data_dir);
-  return GetRunningChromeProcesses(data_dir).size();
+  return GetRunningChromeProcesses(process_id_).size();
 }
 
 static DictionaryValue* LoadDictionaryValueFromPath(const FilePath& path) {
@@ -1219,9 +1216,8 @@ void UITest::UpdateHistoryDates() {
   file_util::EvictFileFromSystemCache(history);
 }
 
-void UITest::PrintIOPerfInfo(const char* test_name, FilePath data_dir) {
-  int browser_process_pid = ChromeBrowserProcessId(data_dir);
-  ChromeProcessList chrome_processes(GetRunningChromeProcesses(data_dir));
+void UITest::PrintIOPerfInfo(const char* test_name) {
+  ChromeProcessList chrome_processes(GetRunningChromeProcesses(process_id_));
 
   size_t read_op_b = 0;
   size_t read_op_r = 0;
@@ -1260,7 +1256,7 @@ void UITest::PrintIOPerfInfo(const char* test_name, FilePath data_dir) {
     if (process_metrics.get()->GetIOCounters(&io_counters)) {
       // Print out IO performance.  We assume that the values can be
       // converted to size_t (they're reported as ULONGLONG, 64-bit numbers).
-      std::string chrome_name = (*it == browser_process_pid) ? "_b" : "_r";
+      std::string chrome_name = (*it == browser_process_id()) ? "_b" : "_r";
 
       size_t read_op = static_cast<size_t>(io_counters.ReadOperationCount);
       size_t write_op = static_cast<size_t>(io_counters.WriteOperationCount);
@@ -1280,7 +1276,7 @@ void UITest::PrintIOPerfInfo(const char* test_name, FilePath data_dir) {
                                                io_counters.OtherTransferCount)
                                               / 1024);
 
-      if (*it == browser_process_pid) {
+      if (*it == browser_process_id()) {
         read_op_b = read_op;
         write_op_b = write_op;
         other_op_b = other_op;
@@ -1326,9 +1322,8 @@ void UITest::PrintIOPerfInfo(const char* test_name, FilePath data_dir) {
   PrintResult("total_byte_r", "", "IO_r" + t_name, total_byte_r, "kb", true);
 }
 
-void UITest::PrintMemoryUsageInfo(const char* test_name, FilePath data_dir) {
-  int browser_process_pid = ChromeBrowserProcessId(data_dir);
-  ChromeProcessList chrome_processes(GetRunningChromeProcesses(data_dir));
+void UITest::PrintMemoryUsageInfo(const char* test_name) {
+  ChromeProcessList chrome_processes(GetRunningChromeProcesses(process_id_));
 
   size_t browser_virtual_size = 0;
   size_t browser_working_set_size = 0;
@@ -1362,7 +1357,7 @@ void UITest::PrintMemoryUsageInfo(const char* test_name, FilePath data_dir) {
     size_t current_virtual_size = process_metrics->GetPagefileUsage();
     size_t current_working_set_size = process_metrics->GetWorkingSetSize();
 
-    if (*it == browser_process_pid) {
+    if (*it == browser_process_id()) {
       browser_virtual_size = current_virtual_size;
       browser_working_set_size = current_working_set_size;
     } else {
@@ -1375,7 +1370,7 @@ void UITest::PrintMemoryUsageInfo(const char* test_name, FilePath data_dir) {
 #if defined(OS_WIN)
     size_t peak_virtual_size = process_metrics->GetPeakPagefileUsage();
     size_t peak_working_set_size = process_metrics->GetPeakWorkingSetSize();
-    if (*it == browser_process_pid) {
+    if (*it == browser_process_id()) {
       browser_peak_virtual_size = peak_virtual_size;
       browser_peak_working_set_size = peak_working_set_size;
     } else {
diff --git a/chrome/test/ui/ui_test.h b/chrome/test/ui/ui_test.h
index 64677bc..367d8b9a 100644
--- a/chrome/test/ui/ui_test.h
+++ b/chrome/test/ui/ui_test.h
@@ -388,6 +388,9 @@ class UITest : public PlatformTest {
   // UITest::SetUp().
   FilePath user_data_dir() const { return user_data_dir_; }
 
+  // Return the process id of the browser process (-1 on error).
+  base::ProcessId browser_process_id() const { return process_id_; }
+
   // Timeout accessors.
   int command_execution_timeout_ms() const {
     return command_execution_timeout_ms_;
@@ -401,10 +404,9 @@ class UITest : public PlatformTest {
 
   std::wstring ui_test_name() const { return ui_test_name_; }
 
-  // Count the number of active browser processes.  This function only counts
-  // browser processes that share the same profile directory as the current
-  // process.  The count includes browser sub-processes.
-  static int GetBrowserProcessCount();
+  // Count the number of active browser processes launched by this test.
+  // The count includes browser sub-processes.
+  int GetBrowserProcessCount();
 
   // Returns a copy of local state preferences. The caller is responsible for
   // deleting the returned object. Returns NULL if there is an error.
@@ -458,10 +460,10 @@ class UITest : public PlatformTest {
   void StopWebSocketServer();
 
   // Prints IO performance data for use by perf graphs.
-  void PrintIOPerfInfo(const char* test_name, FilePath data_dir);
+  void PrintIOPerfInfo(const char* test_name);
 
   // Prints memory usage data for use by perf graphs.
-  void PrintMemoryUsageInfo(const char* test_name, FilePath data_dir);
+  void PrintMemoryUsageInfo(const char* test_name);
 
   // Prints memory commit charge stats for use by perf graphs.
   void PrintSystemCommitCharge(const char* test_name,