Revert of Upgrade the windows specific version of LaunchProcess to av…

…oid raw handles. (patchset #3 id:40001 of https://codereview.chromium.org/759903002/)

Reason for revert:
Looks like it broke the 'Google Chrome Win' builder:

FAILED: ninja -t msvc -e environment.x86 -- c:\b\build\goma/gomacc "c:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo /showIncludes /FC @obj\win8\delegate_execute\delegate_execute.chrome_util.obj.rsp /c ..\..\win8\delegate_execute\chrome_util.cc /Foobj\win8\delegate_execute\delegate_execute.chrome_util.obj /Fdobj\win8\delegate_execute\delegate_execute.cc.pdb
c:\b\build\slave\google-chrome-rel-win\build\src\win8\delegate_execute\chrome_util.cc(119) : error C2665: 'base::LaunchProcess' : none of the 3 overloads could convert all the argument types
        c:\b\build\slave\google-chrome-rel-win\build\src\base\process\launch.h(161): could be 'bool base::LaunchProcess(const base::CommandLine &,const base::LaunchOptions &,base::ProcessHandle *)'
        while trying to match the argument list '(base::string16, base::LaunchOptions, base::win::ScopedHandle *)'

Original issue's description:
> Upgrade the windows specific version of LaunchProcess to avoid raw handles.
>
> This change implies that extensions::LaunchNativeProcess also changes to
> return base::Process, and that requires base::EnsureProcessTerminated to
> deal with base:Process (as it basically claims ownership of the process).
>
> This CL fixes some leaks all around.
>
> BUG=417532
>
> Committed: https://crrev.com/6b687a5e232c80539772dc3dbe35b98095064c38
> Cr-Commit-Position: refs/heads/master@{#306687}

TBR=cpu@chromium.org,finnur@chromium.org,gab@chromium.org,sergeyu@chromium.org,wez@chromium.org,rvargas@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=417532

Review URL: https://codereview.chromium.org/780653003

Cr-Commit-Position: refs/heads/master@{#306712}

base/process/kill.h

@@ -9,7 +9,6 @@
 #define BASE_PROCESS_KILL_H_
 
 #include "base/files/file_path.h"
-#include "base/process/process.h"
 #include "base/process/process_handle.h"
 #include "base/time/time.h"
 
@@ -147,10 +146,10 @@ BASE_EXPORT bool CleanupProcesses(const FilePath::StringType& executable_name,
 // On Linux this method does not block the calling thread.
 // On OS X this method may block for up to 2 seconds.
 //
-// NOTE: The process must have been opened with the PROCESS_TERMINATE and
-// SYNCHRONIZE permissions.
+// NOTE: The process handle must have been opened with the PROCESS_TERMINATE
+// and SYNCHRONIZE permissions.
 //
-BASE_EXPORT void EnsureProcessTerminated(Process process);
+BASE_EXPORT void EnsureProcessTerminated(ProcessHandle process_handle);
 
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
 // The nicer version of EnsureProcessTerminated() that is patient and will

base/process/kill_mac.cc

@@ -165,8 +165,8 @@ void WaitForChildToDie(pid_t child, int timeout) {
 
 }  // namespace
 
-void EnsureProcessTerminated(Process process) {
-  WaitForChildToDie(process.pid(), kWaitBeforeKillSeconds);
+void EnsureProcessTerminated(ProcessHandle process) {
+  WaitForChildToDie(process, kWaitBeforeKillSeconds);
 }
 
 }  // namespace base

base/process/kill_posix.cc

@@ -463,13 +463,13 @@ class BackgroundReaper : public PlatformThread::Delegate {
 
 }  // namespace
 
-void EnsureProcessTerminated(Process process) {
+void EnsureProcessTerminated(ProcessHandle process) {
   // If the child is already dead, then there's nothing to do.
-  if (IsChildDead(process.pid()))
+  if (IsChildDead(process))
     return;
 
   const unsigned timeout = 2;  // seconds
-  BackgroundReaper* reaper = new BackgroundReaper(process.pid(), timeout);
+  BackgroundReaper* reaper = new BackgroundReaper(process, timeout);
   PlatformThread::CreateNonJoinable(0, reaper);
 }
 

base/process/kill_win.cc

@@ -38,7 +38,7 @@ static const int kWaitInterval = 2000;
 
 class TimerExpiredTask : public win::ObjectWatcher::Delegate {
  public:
-  explicit TimerExpiredTask(Process process);
+  explicit TimerExpiredTask(ProcessHandle process);
   ~TimerExpiredTask();
 
   void TimedOut();
@@ -50,23 +50,24 @@ class TimerExpiredTask : public win::ObjectWatcher::Delegate {
   void KillProcess();
 
   // The process that we are watching.
-  Process process_;
+  ProcessHandle process_;
 
   win::ObjectWatcher watcher_;
 
   DISALLOW_COPY_AND_ASSIGN(TimerExpiredTask);
 };
 
-TimerExpiredTask::TimerExpiredTask(Process process) : process_(process.Pass()) {
-  watcher_.StartWatching(process_.Handle(), this);
+TimerExpiredTask::TimerExpiredTask(ProcessHandle process) : process_(process) {
+  watcher_.StartWatching(process_, this);
 }
 
 TimerExpiredTask::~TimerExpiredTask() {
   TimedOut();
+  DCHECK(!process_) << "Make sure to close the handle.";
 }
 
 void TimerExpiredTask::TimedOut() {
-  if (process_.IsValid())
+  if (process_)
     KillProcess();
 }
 
@@ -75,7 +76,8 @@ void TimerExpiredTask::OnObjectSignaled(HANDLE object) {
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION("TimerExpiredTask_OnObjectSignaled"));
 
-  process_.Close();
+  CloseHandle(process_);
+  process_ = NULL;
 }
 
 void TimerExpiredTask::KillProcess() {
@@ -86,10 +88,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_.Handle(), kProcessKilledExitCode, false);
+  base::KillProcess(process_, kProcessKilledExitCode, false);
 
   // Now, just cleanup as if the process exited normally.
-  OnObjectSignaled(process_.Handle());
+  OnObjectSignaled(process_);
 }
 
 }  // namespace
@@ -239,18 +241,19 @@ bool CleanupProcesses(const FilePath::StringType& executable_name,
   return false;
 }
 
-void EnsureProcessTerminated(Process process) {
-  DCHECK(!process.is_current());
+void EnsureProcessTerminated(ProcessHandle process) {
+  DCHECK(process != GetCurrentProcess());
 
   // If already signaled, then we are done!
-  if (WaitForSingleObject(process.Handle(), 0) == WAIT_OBJECT_0) {
+  if (WaitForSingleObject(process, 0) == WAIT_OBJECT_0) {
+    CloseHandle(process);
     return;
   }
 
   MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&TimerExpiredTask::TimedOut,
-                 base::Owned(new TimerExpiredTask(process.Pass()))),
+                 base::Owned(new TimerExpiredTask(process))),
       base::TimeDelta::FromMilliseconds(kWaitInterval));
 }
 

base/process/launch.h

@@ -22,6 +22,7 @@
 #include "base/posix/file_descriptor_shuffle.h"
 #elif defined(OS_WIN)
 #include <windows.h>
+#include "base/win/scoped_handle.h"
 #endif
 
 namespace base {
@@ -173,8 +174,9 @@ BASE_EXPORT bool LaunchProcess(const CommandLine& cmdline,
 //
 // Example (including literal quotes)
 //  cmdline = "c:\windows\explorer.exe" -foo "c:\bar\"
-BASE_EXPORT Process LaunchProcess(const string16& cmdline,
-                                  const LaunchOptions& options);
+BASE_EXPORT bool LaunchProcess(const string16& cmdline,
+                               const LaunchOptions& options,
+                               win::ScopedHandle* process_handle);
 
 // Launches a process with elevated privileges.  This does not behave exactly
 // like LaunchProcess as it uses ShellExecuteEx instead of CreateProcess to

base/process/launch_win.cc

@@ -232,17 +232,6 @@ bool LaunchProcess(const string16& cmdline,
   return true;
 }
 
-// TODO(rvargas) crbug.com/416721: Remove this stub after LaunchProcess is
-// fully migrated to use Process.
-Process LaunchProcess(const string16& cmdline,
-                      const LaunchOptions& options) {
-  win::ScopedHandle process_handle;
-  if (LaunchProcess(cmdline, options, &process_handle))
-    return Process(process_handle.Take());
-
-  return Process();
-}
-
 bool LaunchProcess(const CommandLine& cmdline,
                    const LaunchOptions& options,
                    ProcessHandle* process_handle) {

base/process/process_util_unittest.cc

@@ -888,14 +888,14 @@ bool IsProcessDead(base::ProcessHandle child) {
 }
 
 TEST_F(ProcessUtilTest, DelayedTermination) {
-  base::Process child_process(SpawnChild("process_util_test_never_die"));
-  ASSERT_TRUE(child_process.IsValid());
-  base::EnsureProcessTerminated(child_process.Duplicate());
-  base::WaitForSingleProcess(child_process.Handle(),
-                             base::TimeDelta::FromSeconds(5));
+  base::ProcessHandle child_process = SpawnChild("process_util_test_never_die");
+  ASSERT_TRUE(child_process);
+  base::EnsureProcessTerminated(child_process);
+  base::WaitForSingleProcess(child_process, base::TimeDelta::FromSeconds(5));
 
   // Check that process was really killed.
-  EXPECT_TRUE(IsProcessDead(child_process.Handle()));
+  EXPECT_TRUE(IsProcessDead(child_process));
+  base::CloseProcessHandle(child_process);
 }
 
 MULTIPROCESS_TEST_MAIN(process_util_test_never_die) {
@@ -906,14 +906,16 @@ MULTIPROCESS_TEST_MAIN(process_util_test_never_die) {
 }
 
 TEST_F(ProcessUtilTest, ImmediateTermination) {
-  base::Process child_process(SpawnChild("process_util_test_die_immediately"));
-  ASSERT_TRUE(child_process.IsValid());
+  base::ProcessHandle child_process =
+      SpawnChild("process_util_test_die_immediately");
+  ASSERT_TRUE(child_process);
   // Give it time to die.
   sleep(2);
-  base::EnsureProcessTerminated(child_process.Duplicate());
+  base::EnsureProcessTerminated(child_process);
 
   // Check that process was really killed.
-  EXPECT_TRUE(IsProcessDead(child_process.Handle()));
+  EXPECT_TRUE(IsProcessDead(child_process));
+  base::CloseProcessHandle(child_process);
 }
 
 MULTIPROCESS_TEST_MAIN(process_util_test_die_immediately) {

chrome/browser/extensions/api/messaging/native_message_process_host.cc

@@ -47,6 +47,7 @@ NativeMessageProcessHost::NativeMessageProcessHost(
       native_host_name_(native_host_name),
       launcher_(launcher.Pass()),
       closed_(false),
+      process_handle_(base::kNullProcessHandle),
 #if defined(OS_POSIX)
       read_file_(-1),
 #endif
@@ -102,7 +103,7 @@ void NativeMessageProcessHost::LaunchHostProcess() {
 
 void NativeMessageProcessHost::OnHostProcessLaunched(
     NativeProcessLauncher::LaunchResult result,
-    base::Process process,
+    base::ProcessHandle process_handle,
     base::File read_file,
     base::File write_file) {
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ -124,7 +125,7 @@ void NativeMessageProcessHost::OnHostProcessLaunched(
       break;
   }
 
-  process_ = process.Pass();
+  process_handle_ = process_handle;
 #if defined(OS_POSIX)
   // This object is not the owner of the file so it should not keep an fd.
   read_file_ = read_file.GetPlatformFile();
@@ -348,17 +349,17 @@ void NativeMessageProcessHost::Close(const std::string& error_message) {
     client_->CloseChannel(error_message);
   }
 
-  if (process_.IsValid()) {
+  if (process_handle_ != base::kNullProcessHandle) {
     // Kill the host process if necessary to make sure we don't leave zombies.
     // On OSX base::EnsureProcessTerminated() may block, so we have to post a
     // task on the blocking pool.
 #if defined(OS_MACOSX)
     content::BrowserThread::PostBlockingPoolTask(
-        FROM_HERE,
-        base::Bind(&base::EnsureProcessTerminated, Passed(&process_)));
+        FROM_HERE, base::Bind(&base::EnsureProcessTerminated, process_handle_));
 #else
-    base::EnsureProcessTerminated(process_.Pass());
+    base::EnsureProcessTerminated(process_handle_);
 #endif
+    process_handle_ = base::kNullProcessHandle;
   }
 }
 

chrome/browser/extensions/api/messaging/native_message_process_host.h

@@ -72,7 +72,7 @@ class NativeMessageProcessHost :
 
   // Callback for NativeProcessLauncher::Launch().
   void OnHostProcessLaunched(NativeProcessLauncher::LaunchResult result,
-                             base::Process process,
+                             base::ProcessHandle process_handle,
                              base::File read_file,
                              base::File write_file);
 
@@ -108,7 +108,7 @@ class NativeMessageProcessHost :
   // due to an error.
   bool closed_;
 
-  base::Process process_;
+  base::ProcessHandle process_handle_;
 
   // Input stream reader.
   scoped_ptr<net::FileStream> read_stream_;

chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc

@@ -82,7 +82,8 @@ class FakeLauncher : public NativeProcessLauncher {
               const std::string& native_host_name,
               LaunchedCallback callback) const override {
     callback.Run(NativeProcessLauncher::RESULT_SUCCESS,
-                 base::Process(), read_file_.Pass(), write_file_.Pass());
+                 base::kNullProcessHandle,
+                 read_file_.Pass(), write_file_.Pass());
   }
 
  private:

chrome/browser/extensions/api/messaging/native_process_launcher.cc

@@ -62,12 +62,12 @@ class NativeProcessLauncherImpl : public NativeProcessLauncher {
                               LaunchedCallback callback);
     void PostErrorResult(const LaunchedCallback& callback, LaunchResult error);
     void PostResult(const LaunchedCallback& callback,
-                    base::Process process,
+                    base::ProcessHandle process_handle,
                     base::File read_file,
                     base::File write_file);
     void CallCallbackOnIOThread(LaunchedCallback callback,
                                 LaunchResult result,
-                                base::Process process,
+                                base::ProcessHandle process_handle,
                                 base::File read_file,
                                 base::File write_file);
 
@@ -192,12 +192,12 @@ void NativeProcessLauncherImpl::Core::DoLaunchOnThreadPool(
                                  base::Int64ToString(window_handle_));
 #endif  // !defined(OS_WIN)
 
-  base::Process process;
+  base::ProcessHandle process_handle;
   base::File read_file;
   base::File write_file;
   if (NativeProcessLauncher::LaunchNativeProcess(
-          command_line, &process, &read_file, &write_file)) {
-    PostResult(callback, process.Pass(), read_file.Pass(), write_file.Pass());
+          command_line, &process_handle, &read_file, &write_file)) {
+    PostResult(callback, process_handle, read_file.Pass(), write_file.Pass());
   } else {
     PostErrorResult(callback, RESULT_FAILED_TO_START);
   }
@@ -206,14 +206,14 @@ void NativeProcessLauncherImpl::Core::DoLaunchOnThreadPool(
 void NativeProcessLauncherImpl::Core::CallCallbackOnIOThread(
     LaunchedCallback callback,
     LaunchResult result,
-    base::Process process,
+    base::ProcessHandle process_handle,
     base::File read_file,
     base::File write_file) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   if (detached_)
     return;
 
-  callback.Run(result, process.Pass(), read_file.Pass(), write_file.Pass());
+  callback.Run(result, process_handle, read_file.Pass(), write_file.Pass());
 }
 
 void NativeProcessLauncherImpl::Core::PostErrorResult(
@@ -222,20 +222,20 @@ void NativeProcessLauncherImpl::Core::PostErrorResult(
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&NativeProcessLauncherImpl::Core::CallCallbackOnIOThread, this,
-                 callback, error, Passed(base::Process()),
+                 callback, error, base::kNullProcessHandle,
                  Passed(base::File()), Passed(base::File())));
 }
 
 void NativeProcessLauncherImpl::Core::PostResult(
     const LaunchedCallback& callback,
-    base::Process process,
+    base::ProcessHandle process_handle,
     base::File read_file,
     base::File write_file) {
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&NativeProcessLauncherImpl::Core::CallCallbackOnIOThread, this,
-                 callback, RESULT_SUCCESS, Passed(&process),
-                 Passed(&read_file), Passed(&write_file)));
+                 callback, RESULT_SUCCESS, process_handle,
+                 Passed(read_file.Pass()), Passed(write_file.Pass())));
 }
 
 NativeProcessLauncherImpl::NativeProcessLauncherImpl(

chrome/browser/extensions/api/messaging/native_process_launcher.h

@@ -34,7 +34,7 @@ class NativeProcessLauncher {
   // to false in case of a failure. Handler must take ownership of the IO
   // handles.
   typedef base::Callback<void(LaunchResult result,
-                              base::Process process,
+                              base::ProcessHandle process_handle,
                               base::File read_file,
                               base::File write_file)> LaunchedCallback;
 
@@ -68,7 +68,7 @@ class NativeProcessLauncher {
 
   // Launches native messaging process.
   static bool LaunchNativeProcess(const base::CommandLine& command_line,
-                                  base::Process* process,
+                                  base::ProcessHandle* process_handle,
                                   base::File* read_file,
                                   base::File* write_file);