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

base/process/kill_win.cc

Index: base/process/kill_win.cc
diff --git a/base/process/kill_win.cc b/base/process/kill_win.cc
index b102a8781d074337841b794c94267232aa718735..311bdd7c4f714c38fa3a4b44b832a1a3b88d368d 100644
--- a/base/process/kill_win.cc
+++ b/base/process/kill_win.cc
@@ -38,7 +38,7 @@ static const int kWaitInterval = 2000;
 
 class TimerExpiredTask : public win::ObjectWatcher::Delegate {
  public:
-  explicit TimerExpiredTask(ProcessHandle process);
+  explicit TimerExpiredTask(Process process);
   ~TimerExpiredTask();
 
   void TimedOut();
@@ -50,24 +50,23 @@ class TimerExpiredTask : public win::ObjectWatcher::Delegate {
   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();
 }
 
@@ -75,9 +74,6 @@ 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() {
@@ -88,10 +84,10 @@ void TimerExpiredTask::KillProcess() {
   // 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
@@ -241,19 +237,18 @@ bool CleanupProcesses(const FilePath::StringType& executable_name,
   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));
 }