Fix the windows implementation of KillProcess and WaitForSingleProcess to not close the process handle that they do not own.

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

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

10 files changed, 35 insertions, 22 deletions

diff --git a/base/process_util.h b/base/process_util.h
index c921d7a..029f307 100644
--- a/base/process_util.h
+++ b/base/process_util.h
@@ -151,9 +151,9 @@ bool KillProcesses(const std::wstring& executable_name, int exit_code,
 // entry structure, giving it the specified exit code. If |wait| is true, wait
 // for the process to be actually terminated before returning.
 // Returns true if this is successful, false otherwise.
-bool KillProcess(int process_id, int exit_code, bool wait);
+bool KillProcess(ProcessHandle process, int exit_code, bool wait);
 #if defined(OS_WIN)
-bool KillProcess(HANDLE process, int exit_code, bool wait);
+bool KillProcessById(DWORD process_id, int exit_code, bool wait);
 #endif
 
 // Get the termination status (exit code) of the process and return true if the
diff --git a/base/process_util_posix.cc b/base/process_util_posix.cc
index 7cabb3a..e8845fb 100644
--- a/base/process_util_posix.cc
+++ b/base/process_util_posix.cc
@@ -52,7 +52,7 @@ int GetProcId(ProcessHandle process) {
 // Attempts to kill the process identified by the given process
 // entry structure.  Ignores specified exit_code; posix can't force that.
 // Returns true if this is successful, false otherwise.
-bool KillProcess(int process_id, int exit_code, bool wait) {
+bool KillProcess(ProcessHandle process_id, int exit_code, bool wait) {
   bool result = false;
 
   int status = kill(process_id, SIGTERM);
diff --git a/base/process_util_unittest.cc b/base/process_util_unittest.cc
index 09922cc..97ba756 100644
--- a/base/process_util_unittest.cc
+++ b/base/process_util_unittest.cc
@@ -34,6 +34,7 @@ TEST_F(ProcessUtilTest, SpawnChild) {
 
   ASSERT_NE(static_cast<ProcessHandle>(NULL), handle);
   EXPECT_TRUE(WaitForSingleProcess(handle, 5000));
+  base::CloseProcessHandle(handle);
 }
 
 MULTIPROCESS_TEST_MAIN(SlowChildProcess) {
@@ -65,6 +66,7 @@ TEST_F(ProcessUtilTest, KillSlowChild) {
   fclose(fp);
   EXPECT_TRUE(base::WaitForSingleProcess(handle, 5000));
   EXPECT_EQ(oldcount, GetProcessCount(L"base_unittests" EXE_SUFFIX, 0));
+  base::CloseProcessHandle(handle);
 }
 
 // TODO(estade): if possible, port these 2 tests.
@@ -216,6 +218,7 @@ TEST_F(ProcessUtilTest, FDRemapping) {
   ASSERT_EQ(0, num_open_files);
 
   EXPECT_TRUE(WaitForSingleProcess(handle, 1000));
+  base::CloseProcessHandle(handle);
   close(fds[0]);
   close(sockets[0]);
   close(sockets[1]);
diff --git a/base/process_util_win.cc b/base/process_util_win.cc
index bb0e626..2edf5ec 100644
--- a/base/process_util_win.cc
+++ b/base/process_util_win.cc
@@ -160,17 +160,20 @@ bool LaunchApp(const CommandLine& cl,
 // Attempts to kill the process identified by the given process
 // entry structure, giving it the specified exit code.
 // Returns true if this is successful, false otherwise.
-bool KillProcess(int process_id, int exit_code, bool wait) {
+bool KillProcessById(DWORD process_id, int exit_code, bool wait) {
   HANDLE process = OpenProcess(PROCESS_TERMINATE | SYNCHRONIZE,
                                FALSE,  // Don't inherit handle
                                process_id);
-  if (process)
-    return KillProcess(process, exit_code, wait);
-  return false;
+  if (!process)
+    return false;
+
+  bool ret = KillProcess(process, exit_code, wait);
+  CloseHandle(process);
+  return ret;
 }
 
-bool KillProcess(HANDLE process, int exit_code, bool wait) {
-  bool result = !!TerminateProcess(process, exit_code);
+bool KillProcess(ProcessHandle process, int exit_code, bool wait) {
+  bool result = (TerminateProcess(process, exit_code) != FALSE);
   if (result && wait) {
     // The process may not end immediately due to pending I/O
     if (WAIT_OBJECT_0 != WaitForSingleObject(process, 60 * 1000))
@@ -178,7 +181,6 @@ bool KillProcess(HANDLE process, int exit_code, bool wait) {
   } else {
     DLOG(ERROR) << "Unable to terminate process: " << GetLastError();
   }
-  CloseHandle(process);
   return result;
 }
 
@@ -252,12 +254,12 @@ bool WaitForExitCode(ProcessHandle handle, int* exit_code) {
 }
 
 NamedProcessIterator::NamedProcessIterator(const std::wstring& executable_name,
-                                           const ProcessFilter* filter) :
-  started_iteration_(false),
-  executable_name_(executable_name),
-  filter_(filter) {
-    snapshot_ = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
-  }
+                                           const ProcessFilter* filter)
+    : started_iteration_(false),
+      executable_name_(executable_name),
+      filter_(filter) {
+  snapshot_ = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
+}
 
 NamedProcessIterator::~NamedProcessIterator() {
   CloseHandle(snapshot_);
@@ -315,8 +317,10 @@ bool KillProcesses(const std::wstring& executable_name, int exit_code,
   const ProcessEntry* entry;
 
   NamedProcessIterator iter(executable_name, filter);
-  while (entry = iter.NextProcessEntry())
-    result = KillProcess((*entry).th32ProcessID, exit_code, true) && result;
+  while (entry = iter.NextProcessEntry()) {
+    if (!KillProcessById((*entry).th32ProcessID, exit_code, true))
+      result = false;
+  }
 
   return result;
 }
@@ -346,7 +350,6 @@ bool WaitForProcessesToExit(const std::wstring& executable_name,
 
 bool WaitForSingleProcess(ProcessHandle handle, int wait_milliseconds) {
   bool retval = WaitForSingleObject(handle, wait_milliseconds) == WAIT_OBJECT_0;
-  CloseHandle(handle);
   return retval;
 }
 
diff --git a/base/stats_table_unittest.cc b/base/stats_table_unittest.cc
index 7c4c4f6..5d1c841 100644
--- a/base/stats_table_unittest.cc
+++ b/base/stats_table_unittest.cc
@@ -210,6 +210,7 @@ TEST_F(StatsTableTest, MultipleProcesses) {
   // Wait for the processes to finish.
   for (int index = 0; index < kMaxProcs; index++) {
     EXPECT_TRUE(WaitForSingleProcess(procs[index], 60 * 1000));
+    base::CloseProcessHandle(procs[index]);
   }
 
   StatsCounter zero_counter(kCounterZero);
diff --git a/chrome/browser/message_window.cc b/chrome/browser/message_window.cc
index 3bfced7..d198254 100644
--- a/chrome/browser/message_window.cc
+++ b/chrome/browser/message_window.cc
@@ -106,7 +106,7 @@ bool MessageWindow::NotifyOtherProcess() {
   }
 
   // Time to take action. Kill the browser process.
-  base::KillProcess(process_id, ResultCodes::HUNG, true);
+  base::KillProcessById(process_id, ResultCodes::HUNG, true);
   remote_window_ = NULL;
   return false;
 }
diff --git a/chrome/browser/metrics/metrics_service_uitest.cc b/chrome/browser/metrics/metrics_service_uitest.cc
index db35f1c..e782128 100644
--- a/chrome/browser/metrics/metrics_service_uitest.cc
+++ b/chrome/browser/metrics/metrics_service_uitest.cc
@@ -91,7 +91,8 @@ TEST_F(MetricsServiceTest, CrashRenderers) {
   int process_id = 0;
   ASSERT_TRUE(tab->GetProcessID(&process_id));
   ASSERT_NE(0, process_id);
-  base::KillProcess(process_id, 0xc0000005, true);  // Fake Access Violation.
+  // Fake Access Violation.
+  base::KillProcessById(process_id, 0xc0000005, true);
 
   // Give the browser a chance to notice the crashed tab.
   Sleep(1000);
diff --git a/chrome/common/ipc_fuzzing_tests.cc b/chrome/common/ipc_fuzzing_tests.cc
index 0638bb4..b9a9563 100644
--- a/chrome/common/ipc_fuzzing_tests.cc
+++ b/chrome/common/ipc_fuzzing_tests.cc
@@ -302,6 +302,7 @@ TEST_F(IPCFuzzingTest, SanityTest) {
   EXPECT_TRUE(listener.ExpectMessage(value, MsgClassSI::ID));
 
   EXPECT_TRUE(base::WaitForSingleProcess(server_process, 5000));
+  base::CloseProcessHandle(server_process);
 }
 
 // This test uses a payload that is smaller than expected.
@@ -331,6 +332,7 @@ TEST_F(IPCFuzzingTest, MsgBadPayloadShort) {
   EXPECT_TRUE(listener.ExpectMessage(1, MsgClassSI::ID));
 
   EXPECT_TRUE(base::WaitForSingleProcess(server_process, 5000));
+  base::CloseProcessHandle(server_process);
 }
 #endif  // NDEBUG
 
@@ -365,6 +367,7 @@ TEST_F(IPCFuzzingTest, MsgBadPayloadArgs) {
   EXPECT_TRUE(listener.ExpectMessage(3, MsgClassIS::ID));
 
   EXPECT_TRUE(base::WaitForSingleProcess(server_process, 5000));
+  base::CloseProcessHandle(server_process);
 }
 
 // This class is for testing the IPC_BEGIN_MESSAGE_MAP_EX macros.
diff --git a/chrome/common/ipc_tests.cc b/chrome/common/ipc_tests.cc
index e2b5416..01873e7 100644
--- a/chrome/common/ipc_tests.cc
+++ b/chrome/common/ipc_tests.cc
@@ -231,6 +231,7 @@ TEST_F(IPCChannelTest, ChannelTest) {
 
   // Cleanup child process.
   EXPECT_TRUE(base::WaitForSingleProcess(process_handle, 5000));
+  base::CloseProcessHandle(process_handle);
 }
 
 #if defined(OS_POSIX)
@@ -357,6 +358,7 @@ TEST_F(IPCChannelTest, ChannelProxyTest) {
 
     // cleanup child process
     EXPECT_TRUE(base::WaitForSingleProcess(process_handle, 5000));
+    base::CloseProcessHandle(process_handle);
   }
   thread.Stop();
 }
diff --git a/net/disk_cache/stress_cache.cc b/net/disk_cache/stress_cache.cc
index 8ad94e2..a7fb144 100644
--- a/net/disk_cache/stress_cache.cc
+++ b/net/disk_cache/stress_cache.cc
@@ -144,7 +144,7 @@ class CrashTask : public Task {
       printf("sweet death...\n");
 #if defined(OS_WIN)
       // Windows does more work on _exit() that we would like, so we use Kill.
-      base::KillProcess(base::GetCurrentProcId(), kExpectedCrash, false);
+      base::KillProcessById(base::GetCurrentProcId(), kExpectedCrash, false);
 #elif defined(OS_POSIX)
       // On POSIX, _exit() will terminate the process with minimal cleanup,
       // and it is cleaner than killing.