Upgrade the windows specific version of LaunchProcess to avoid raw handles.

base/process/kill_win.cc

 // Copyright (c) 2013 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 "base/process/kill.h"
 
 #include <io.h>
 #include <windows.h>
 
 #include "base/bind.h"
@@ skipping 20 matching lines @@
 // seems to be common practice on Windows to test for it as an
 // indication that the task manager has killed something if the
 // process goes away.
 const DWORD kProcessKilledExitCode = 1;
 
 // Maximum amount of time (in milliseconds) to wait for the process to exit.
 static const int kWaitInterval = 2000;
 
 class TimerExpiredTask : public win::ObjectWatcher::Delegate {
  public:
-  explicit TimerExpiredTask(ProcessHandle process);
+  explicit TimerExpiredTask(Process process);
   ~TimerExpiredTask();
 
   void TimedOut();
 
   // MessageLoop::Watcher -----------------------------------------------------
   virtual void OnObjectSignaled(HANDLE object);
 
  private:
   void KillProcess();
 
   // The process that we are watching.
-  ProcessHandle process_;
+  Process process_;

[gab, 2014/11/27 13:28:44]

If I'm reading this correctly, the changes in this file now make it such that
the HANDLE won't be closed until TimerExpiredTask::TimedOut() kicks in as it is
that task that owns the TimerExpiredTask object [1] (which now in turn owns the
HANDLE through process_).

Should we have some kind of explicit process_.release() which would both release
its handle and ensure that |!process_.IsValid()|?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/process/kill_...

[rvargas (doing something else), 2014/12/01 20:50:36]

You're right, there is no need to extend the handle lifetime beyond
OnObjectSignaled, so I could call process_.Close() there. I don't think there's
any danger in keeping the process object around for a while after it the process
dies as all resources should be released at exit time (other than the
handle/object).

I've been resisting to add a release method to Process (I'm assuming you mean a
way to grab the handle from the object) as that's easy to misuse.

I'm adding Close() back, let me know if that doesn't address your concern.

[gab, 2014/12/01 21:31:05]

Indeed, I meant reset(), not release() (my bad) -- i.e. a way to clear the
state, not to obtain it.

But Handle() already provides a way to obtain the raw state, so isn't your
concern already in place? Or do you think it's fine in that case because
ownership of the handle is maintained and as long as the caller respects the
implicit contract it works fine (just like scoped_ptr::get().
Close() fits this perfectly.

[rvargas (doing something else), 2014/12/02 21:07:47]

It would be better to not have a Handle method at all, but that's kind of hard
to do. At least the interface there implies that there is no transfer of
ownership and storing that handle is either dangerous or plain wrong.

 
   win::ObjectWatcher watcher_;
 
   DISALLOW_COPY_AND_ASSIGN(TimerExpiredTask);
 };
 
-TimerExpiredTask::TimerExpiredTask(ProcessHandle process) : process_(process) {
-  watcher_.StartWatching(process_, this);
+TimerExpiredTask::TimerExpiredTask(Process process) : process_(process.Pass()) {
+  watcher_.StartWatching(process_.Handle(), this);
 }
 
 TimerExpiredTask::~TimerExpiredTask() {
   TimedOut();
-  DCHECK(!process_) << "Make sure to close the handle.";
 }
 
 void TimerExpiredTask::TimedOut() {
-  if (process_)
+  if (process_.IsValid())
     KillProcess();
 }
 
 void TimerExpiredTask::OnObjectSignaled(HANDLE object) {
   // TODO(vadimt): Remove ScopedTracker below once crbug.com/418183 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION("TimerExpiredTask_OnObjectSignaled"));
-
-  CloseHandle(process_);
-  process_ = NULL;
 }
 
 void TimerExpiredTask::KillProcess() {
   // Stop watching the process handle since we're killing it.
   watcher_.StopWatching();
 
   // OK, time to get frisky.  We don't actually care when the process
   // terminates.  We just care that it eventually terminates, and that's what
   // TerminateProcess should do for us. Don't check for the result code since
   // it fails quite often. This should be investigated eventually.
-  base::KillProcess(process_, kProcessKilledExitCode, false);
+  base::KillProcess(process_.Handle(), kProcessKilledExitCode, false);
 
   // Now, just cleanup as if the process exited normally.
-  OnObjectSignaled(process_);
+  OnObjectSignaled(process_.Handle());
 }
 
 }  // namespace
 
 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))
       DPLOG(ERROR) << "Error waiting for process exit";
@@ skipping 129 matching lines @@
 bool CleanupProcesses(const FilePath::StringType& executable_name,
                       base::TimeDelta wait,
                       int exit_code,
                       const ProcessFilter* filter) {
   if (WaitForProcessesToExit(executable_name, wait, filter))
     return true;
   KillProcesses(executable_name, exit_code, filter);
   return false;
 }
 
-void EnsureProcessTerminated(ProcessHandle process) {
-  DCHECK(process != GetCurrentProcess());
+void EnsureProcessTerminated(Process process) {
+  DCHECK(!process.is_current());
 
   // If already signaled, then we are done!
-  if (WaitForSingleObject(process, 0) == WAIT_OBJECT_0) {
-    CloseHandle(process);
+  if (WaitForSingleObject(process.Handle(), 0) == WAIT_OBJECT_0) {
     return;
   }
 
   MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&TimerExpiredTask::TimedOut,
-                 base::Owned(new TimerExpiredTask(process))),
+                 base::Owned(new TimerExpiredTask(process.Pass()))),
       base::TimeDelta::FromMilliseconds(kWaitInterval));
 }
 
 }  // namespace base