[Chromoting] Reimplemented the worker process launcher to take into account the encountered issues:

remoting/host/win/wts_session_process_delegate.cc

Index: remoting/host/win/wts_session_process_delegate.cc
diff --git a/remoting/host/win/wts_session_process_delegate.cc b/remoting/host/win/wts_session_process_delegate.cc
new file mode 100644
index 0000000000000000000000000000000000000000..023856cbf962c7f3143ef276b31553d0b6f494d7
--- /dev/null
+++ b/remoting/host/win/wts_session_process_delegate.cc
@@ -0,0 +1,446 @@
+// Copyright (c) 2012 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.
+//
+// This file implements the Windows service controlling Me2Me host processes
+// running within user sessions.
+
+#include "remoting/host/win/wts_session_process_delegate.h"
+
+#include <sddl.h>
+#include <limits>
+
+#include "base/base_switches.h"
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/command_line.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/single_thread_task_runner.h"

[Wez, 2012/10/09 03:40:39]

nit: Re-order this and the next include.

[alexeypa (please no reviews), 2012/10/09 19:42:04]

Done.

+#include "base/path_service.h"
+#include "base/time.h"
+#include "base/timer.h"
+#include "base/utf_string_conversions.h"
+#include "base/win/scoped_handle.h"
+#include "base/win/windows_version.h"
+#include "ipc/ipc_channel.h"
+#include "ipc/ipc_channel_proxy.h"
+#include "ipc/ipc_message.h"
+#include "remoting/host/host_exit_codes.h"
+#include "remoting/host/win/launch_process_with_token.h"
+#include "remoting/host/win/worker_process_launcher.h"
+#include "remoting/host/win/wts_console_monitor.h"
+#include "remoting/host/worker_process_ipc_delegate.h"
+
+using base::win::ScopedHandle;
+using base::TimeDelta;

[Wez, 2012/10/09 03:40:39]

nit: Swap the order of these so they're alphabetical.

[alexeypa (please no reviews), 2012/10/09 19:42:04]

Done.

+
+namespace {
+
+const FilePath::CharType kDaemonBinaryName[] =
+    FILE_PATH_LITERAL("remoting_daemon.exe");
+
+// The command line switch specifying the name of the daemon IPC endpoint.
+const char kDaemonIpcSwitchName[] = "daemon-pipe";
+
+const char kElevateSwitchName[] = "elevate";
+
+// The command line parameters that should be copied from the service's command
+// line to the host process.
+const char* kCopiedSwitchNames[] = {
+    "host-config", switches::kV, switches::kVModule };
+
+} // namespace

[Wez, 2012/10/09 03:40:39]

nit: IIRC, const implies static, so you don't actually need the anon namespace
here.

[alexeypa (please no reviews), 2012/10/09 19:42:04]

Done.

+
+namespace remoting {
+
+// A private class actually implementing the functionality provided by
+// |WtsSessionProcessDelegate|. This class is ref-counted and implements
+// asynchronous fire-and-forget shutdown.
+class WtsSessionProcessDelegateImpl
+    : public base::RefCountedThreadSafe<WtsSessionProcessDelegateImpl>,
+      public base::MessagePumpForIO::IOHandler,
+      public WorkerProcessLauncher::Delegate {
+ public:
+  // The caller must ensure that |delegate| remains valid at least until
+  // Stop() method has been called.
+  WtsSessionProcessDelegateImpl(
+      scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
+      const FilePath& binary_path,
+      uint32 session_id,
+      bool launch_elevated);
+
+  // base::MessagePumpForIO::IOHandler implementation.
+  virtual void OnIOCompleted(base::MessagePumpForIO::IOContext* context,
+                             DWORD bytes_transferred,
+                             DWORD error) OVERRIDE;
+
+  // WorkerProcessLauncher::Delegate implementation.
+  virtual DWORD GetExitCode() OVERRIDE;
+  virtual void KillProcess(DWORD exit_code) OVERRIDE;
+  virtual bool LaunchProcess(
+      const std::string& channel_name,
+      base::win::ScopedHandle* process_exit_event_out) OVERRIDE;
+
+  // Stops the object asynchronously.
+  void Stop();
+
+ private:
+  friend class base::RefCountedThreadSafe<WtsSessionProcessDelegateImpl>;
+  virtual ~WtsSessionProcessDelegateImpl();
+
+  // Drains the completion port queue to make sure that all job object
+  // notifications have been received.
+  void DrainJobNotifications();
+
+  // Notified that the completion port queue has been drained.
+  void DrainJobNotificationsCompleted();
+
+  // Creates and initializes the job object that will sandbox the launched child
+  // processes.
+  void InitializeJob();
+
+  // Notified that the job object initialization is complete.
+  void InitializeJobCompleted(scoped_ptr<base::win::ScopedHandle> job);
+
+  // Called to process incoming job object notifications.
+  void OnJobNotification(DWORD message, DWORD pid);
+
+  // The task runner all public methods of this class should be called on.
+  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
+
+  // The task runner serving job object notifications.
+  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
+
+  // Path to the worker process binary.
+  FilePath binary_path_;
+
+  // The job object used to control the lifetime of child processes.
+  base::win::ScopedHandle job_;
+
+  // True if the worker process should be launched elevated.
+  bool launch_elevated_;
+
+  // A waiting handle that becomes signalled once all process associated with

[Wez, 2012/10/09 03:40:39]

typo: processes

[Wez, 2012/10/09 03:40:39]

nit: Drop "A waiting"

[alexeypa (please no reviews), 2012/10/09 19:42:04]

Done.

[alexeypa (please no reviews), 2012/10/09 19:42:04]

Done.

+  // the job have been terminated.
+  base::win::ScopedHandle process_exit_event_;
+
+  // The token to be used to launch a process in a different session.
+  base::win::ScopedHandle session_token_;
+
+  // True if Stop() has been called.
+  bool stopping_;
+
+  // The handle of the worker process, if launched.
+  base::win::ScopedHandle worker_process_;
+
+  DISALLOW_COPY_AND_ASSIGN(WtsSessionProcessDelegateImpl);
+};
+
+WtsSessionProcessDelegateImpl::WtsSessionProcessDelegateImpl(
+    scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
+    const FilePath& binary_path,
+    uint32 session_id,
+    bool launch_elevated)
+    : main_task_runner_(main_task_runner),
+      io_task_runner_(io_task_runner),
+      binary_path_(binary_path),
+      launch_elevated_(launch_elevated),
+      stopping_(false) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  if (base::win::GetVersion() == base::win::VERSION_XP)
+    launch_elevated_ = false;
+
+  if (launch_elevated_) {
+    process_exit_event_.Set(CreateEvent(NULL, TRUE, FALSE, NULL));
+    CHECK(process_exit_event_.IsValid());
+
+    // To receive job object notifications the job object is registered with
+    // the completion port represented by |io_task_runner|. The registration has
+    // to be done on the I/O thread because
+    // MessageLoopForIO::RegisterJobObject() can only be called via
+    // MessageLoopForIO::current().
+    io_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&WtsSessionProcessDelegateImpl::InitializeJob, this));
+  }
+
+  // Create a session token for the launched process.
+  CHECK(CreateSessionToken(session_id, &session_token_));
+}
+
+void WtsSessionProcessDelegateImpl::OnIOCompleted(
+    base::MessagePumpForIO::IOContext* context,
+    DWORD bytes_transferred,
+    DWORD error) {
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
+
+  // |bytes_transferred| is used in job object notifications to supply
+  // the message ID; |context| carries process ID.
+  main_task_runner_->PostTask(FROM_HERE, base::Bind(
+      &WtsSessionProcessDelegateImpl::OnJobNotification, this,
+      bytes_transferred, reinterpret_cast<DWORD>(context)));
+}
+
+DWORD WtsSessionProcessDelegateImpl::GetExitCode() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  DWORD exit_code = CONTROL_C_EXIT;
+  if (worker_process_.IsValid()) {
+    if (!::GetExitCodeProcess(worker_process_, &exit_code)) {
+      LOG_GETLASTERROR(INFO)
+          << "Failed to query the exit code of the worker process";
+      exit_code = CONTROL_C_EXIT;
+    }
+  }
+
+  return exit_code;
+}
+
+void WtsSessionProcessDelegateImpl::KillProcess(DWORD exit_code) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  if (launch_elevated_) {
+    if (job_.IsValid()) {
+      TerminateJobObject(job_, exit_code);
+    }
+  } else {
+    if (worker_process_.IsValid()) {
+      TerminateProcess(worker_process_, exit_code);
+    }
+  }
+}
+
+bool WtsSessionProcessDelegateImpl::LaunchProcess(
+    const std::string& channel_name,
+    ScopedHandle* process_exit_event_out) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  CommandLine command_line(CommandLine::NO_PROGRAM);
+  if (launch_elevated_) {
+    // The job object is not ready. Retry starting the host process later.
+    if (!job_.IsValid()) {
+      return false;
+    }
+
+    // Construct the helper binary name.
+    FilePath dir_path;
+    if (!PathService::Get(base::DIR_EXE, &dir_path)) {
+      LOG(ERROR) << "Failed to get the executable file name.";
+      return false;
+    }
+    FilePath daemon_binary = dir_path.Append(kDaemonBinaryName);
+
+    // Create the command line passing the name of the IPC channel to use and
+    // copying known switches from the caller's command line.
+    command_line.SetProgram(daemon_binary);
+    command_line.AppendSwitchPath(kElevateSwitchName, binary_path_);
+
+    CHECK(ResetEvent(process_exit_event_));
+  } else {
+    command_line.SetProgram(binary_path_);
+  }
+
+  // Create the command line passing the name of the IPC channel to use and
+  // copying known switches from the caller's command line.
+  command_line.AppendSwitchNative(kDaemonIpcSwitchName,
+                                  UTF8ToWide(channel_name));
+  command_line.CopySwitchesFrom(*CommandLine::ForCurrentProcess(),
+                                kCopiedSwitchNames,
+                                arraysize(kCopiedSwitchNames));
+
+  // Try to launch the process and attach an object watcher to the returned
+  // handle so that we get notified when the process terminates.

[Wez, 2012/10/09 03:40:39]

nit: You're not watching the handle, you're duping it for the caller to watch.

[alexeypa (please no reviews), 2012/10/09 19:42:04]

Done.

+  ScopedHandle worker_process;
+  ScopedHandle worker_thread;
+  if (!LaunchProcessWithToken(command_line.GetProgram(),
+                              command_line.GetCommandLineString(),
+                              session_token_,
+                              CREATE_SUSPENDED | CREATE_BREAKAWAY_FROM_JOB,
+                              &worker_process,
+                              &worker_thread)) {
+    return false;
+  }
+
+  HANDLE local_process_exit_event;
+  if (launch_elevated_) {
+    if (!AssignProcessToJobObject(job_, worker_process)) {
+      LOG_GETLASTERROR(ERROR)
+          << "Failed to assign the worker to the job object";
+      TerminateProcess(worker_process, CONTROL_C_EXIT);
+      return false;

[Wez, 2012/10/09 03:40:39]

Do you want to exit here, before you've set |process_exit_event|; the caller
won't know when the broken child process has exited, will it?

[alexeypa (please no reviews), 2012/10/09 19:42:04]

It will not. It checks the return value and handles it properly.

+    }
+
+    local_process_exit_event = process_exit_event_;
+  } else {
+    worker_process_ = worker_process.Pass();
+    local_process_exit_event = worker_process_;
+  }
+
+  if (!ResumeThread(worker_thread)) {
+    LOG_GETLASTERROR(ERROR) << "Failed to resume the worker thread";
+    KillProcess(CONTROL_C_EXIT);
+    return false;
+  }
+
+  ScopedHandle process_exit_event;
+  if (!DuplicateHandle(GetCurrentProcess(),
+                       local_process_exit_event,
+                       GetCurrentProcess(),
+                       process_exit_event.Receive(),
+                       SYNCHRONIZE,
+                       FALSE,
+                       0)) {
+    LOG_GETLASTERROR(ERROR) << "Failed to duplicate a handle";
+    KillProcess(CONTROL_C_EXIT);
+    return false;
+  }
+
+  *process_exit_event_out = process_exit_event.Pass();
+  return true;
+}
+
+void WtsSessionProcessDelegateImpl::Stop() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  if (!stopping_) {
+    stopping_ = true;
+
+    // Drain the completion queue to make sure all job object notification have

[Wez, 2012/10/09 03:40:39]

typo: notifications

[alexeypa (please no reviews), 2012/10/09 19:42:04]

Done.

+    // been received.
+    DrainJobNotificationsCompleted();

[Wez, 2012/10/09 03:40:39]

It's weird to call this DrainJobNotificationsCompleted; close the job here and
move the DrainJobNotifications task most from *Completed here too?

[alexeypa (please no reviews), 2012/10/09 19:42:04]

I cannot avoid checking for hob_ being valid in
DrainJobNotificationsCompleted(). There will be a race between Stop() and
InitializeJob() otherwise. Imagine the sequence:

1. InitializeJob();
2. Stop(); // close the handle here.
3. InitializeJobCompleted();
4. DrainJobNotificationsCompleted(). // oops, the handle is valid because of #3.

+  }
+}
+
+WtsSessionProcessDelegateImpl::~WtsSessionProcessDelegateImpl() {
+}
+
+void WtsSessionProcessDelegateImpl::DrainJobNotifications() {
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
+
+  // DrainJobNotifications() is posted after the job object is destroyed, so
+  // by this time all notifications from the job object have been processed
+  // already. Let the main thread know that the queue has been drained.
+  main_task_runner_->PostTask(FROM_HERE, base::Bind(
+      &WtsSessionProcessDelegateImpl::DrainJobNotificationsCompleted, this));
+}
+
+void WtsSessionProcessDelegateImpl::DrainJobNotificationsCompleted() {

[Wez, 2012/10/09 03:40:39]

You're not doing anything in this method in the case that the job notifications
are drained; can't you just let the *Impl be torn down as soon as any remaining
references from pending tasks have been dropped?

[alexeypa (please no reviews), 2012/10/09 19:42:04]

Right now it is derived from Stoppable because the caller requires that. I guess
it can be rewritten, but I don't want to do it in this CL.

+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  if (job_.IsValid()) {
+    job_.Close();
+
+    // Drain the completion queue to make sure all job object notification have
+    // been received.
+    io_task_runner_->PostTask(FROM_HERE, base::Bind(
+        &WtsSessionProcessDelegateImpl::DrainJobNotifications, this));
+  }
+}
+
+void WtsSessionProcessDelegateImpl::InitializeJob() {
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
+
+  ScopedHandle job;
+  job.Set(CreateJobObject(NULL, NULL));
+  if (!job.IsValid()) {
+    LOG_GETLASTERROR(ERROR) << "Failed to create a job object";

[Wez, 2012/10/09 03:40:39]

nit: Elsewhere you're CHECK()ing that APIs succeed; is this one any more likely
to fail than the others?

[alexeypa (please no reviews), 2012/10/09 19:42:04]

I removed most of CHECKs. The one checking the result of CreateSessionToken was
definitely wrong. Creation of an unnamed even rarely fails, but now the code can
handle it, so I removed CHECK there as well.

+    return;
+  }
+
+  // Limit the number of active processes in the job to two (the process
+  // performing elevation and the host) and make sure that all processes will be

[Wez, 2012/10/09 03:40:39]

nit: Is there a risk that the APIs we use for elevation, or something hooking
them, require a process to be launched, and that this limit therefore breaks
them?

[alexeypa (please no reviews), 2012/10/09 19:42:04]

I prefer to be very strict in this case because we are launching the processes
under SYSTEM account. 

+  // killed once the job object is destroyed.
+  JOBOBJECT_EXTENDED_LIMIT_INFORMATION info;
+  memset(&info, 0, sizeof(info));
+  info.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_ACTIVE_PROCESS |
+      JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE;
+  info.BasicLimitInformation.ActiveProcessLimit = 2;
+  if (!SetInformationJobObject(job,
+                               JobObjectExtendedLimitInformation,
+                               &info,
+                               sizeof(info))) {
+    LOG_GETLASTERROR(ERROR) << "Failed to set limits on the job object";
+    return;
+  }
+
+  // Register the job object with the completion port in the I/O thread to

[Wez, 2012/10/09 03:40:39]

nit: "Register to receive job notifications via the I/O thread's completion
port"

[alexeypa (please no reviews), 2012/10/09 19:42:04]

Done.

+  // receive job notifications.
+  if (!MessageLoopForIO::current()->RegisterJobObject(job, this)) {
+    LOG_GETLASTERROR(ERROR)
+        << "Failed to associate the job object with a completion port";
+    return;
+  }
+
+  // ScopedHandle is not compatible with base::Passed, so we wrap it to a scoped
+  // pointer.
+  scoped_ptr<ScopedHandle> job_wrapper(new ScopedHandle());
+  *job_wrapper = job.Pass();
+
+  // Let the main thread know that initialization is complete.
+  main_task_runner_->PostTask(FROM_HERE, base::Bind(
+      &WtsSessionProcessDelegateImpl::InitializeJobCompleted, this,
+      base::Passed(&job_wrapper)));
+}
+
+void WtsSessionProcessDelegateImpl::InitializeJobCompleted(
+    scoped_ptr<ScopedHandle> job) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  DCHECK(!job_.IsValid());
+
+  job_ = job->Pass();

[Wez, 2012/10/09 03:40:39]

Isn't there a race condition here between the job being started on the I/O
thread & this task being posted, if the caller calls Stop() immediately after
LaunchProcess()?

[alexeypa (please no reviews), 2012/10/09 19:42:04]

This race is addressed by the check in DrainJobNotificationsCompleted().

+}
+
+void WtsSessionProcessDelegateImpl::OnJobNotification(DWORD message,
+                                                      DWORD pid) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  switch (message) {
+    case JOB_OBJECT_MSG_ACTIVE_PROCESS_ZERO:
+      CHECK(SetEvent(process_exit_event_));
+      break;
+
+    case JOB_OBJECT_MSG_NEW_PROCESS:
+      // We report the exit code of the worker process to be |CONTROL_C_EXIT|
+      // if we cannot get the actual exit code. So here we can safely ignore
+      // the error returned by OpenProcess().
+      worker_process_.Set(OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid));
+      break;
+  }
+}
+
+WtsSessionProcessDelegate::WtsSessionProcessDelegate(
+    scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
+    const FilePath& binary_path,
+    uint32 session_id,
+    bool launch_elevated) {
+  impl_ = new WtsSessionProcessDelegateImpl(main_task_runner, io_task_runner,
+                                            binary_path, session_id,
+                                            launch_elevated);
+}
+
+WtsSessionProcessDelegate::~WtsSessionProcessDelegate() {
+  impl_->Stop();
+}
+
+DWORD WtsSessionProcessDelegate::GetExitCode() {
+  return impl_->GetExitCode();
+}
+
+void WtsSessionProcessDelegate::KillProcess(DWORD exit_code) {
+  impl_->KillProcess(exit_code);
+}
+
+bool WtsSessionProcessDelegate::LaunchProcess(
+    const std::string& channel_name,
+    base::win::ScopedHandle* process_exit_event_out) {
+  return impl_->LaunchProcess(channel_name, process_exit_event_out);
+}
+
+} // namespace remoting