[Chromoting] Refactored worker process launching code and speeded up the desktop process launch.

remoting/host/win/worker_process_launcher.cc

Index: remoting/host/win/worker_process_launcher.cc
diff --git a/remoting/host/win/worker_process_launcher.cc b/remoting/host/win/worker_process_launcher.cc
index 704ebcde39b5acc6fd1b662681661259460babac..47e0cf2efc06a8cc0a7a66538db8cec721c217d6 100644
--- a/remoting/host/win/worker_process_launcher.cc
+++ b/remoting/host/win/worker_process_launcher.cc
@@ -8,13 +8,10 @@
 #include "base/logging.h"
 #include "base/single_thread_task_runner.h"
 #include "base/time.h"
-#include "base/timer.h"
-#include "base/win/object_watcher.h"
 #include "base/win/windows_version.h"
-#include "ipc/ipc_listener.h"
 #include "ipc/ipc_message.h"
-#include "net/base/backoff_entry.h"
 #include "remoting/host/chromoting_messages.h"
+#include "remoting/host/host_exit_codes.h"
 #include "remoting/host/worker_process_ipc_delegate.h"
 
 using base::TimeDelta;
@@ -26,7 +23,7 @@ const net::BackoffEntry::Policy kDefaultBackoffPolicy = {
   0,
 
   // Initial delay for exponential back-off in ms.
-  1000,
+  100,
 
   // Factor by which the waiting time will be multiplied.
   2,
@@ -47,162 +44,38 @@ const net::BackoffEntry::Policy kDefaultBackoffPolicy = {
 };
 
 const int kKillProcessTimeoutSeconds = 5;
-const int kLaunchSuccessTimeoutSeconds = 2;
+const int kStartProcessTimeoutSeconds = 5;
 
 namespace remoting {
 
-// Launches a worker process that is controlled via an IPC channel. All
-// interaction with the spawned process is through the IPC::Listener and Send()
-// method. In case of error the channel is closed and the worker process is
-// terminated.
-class WorkerProcessLauncher::Core
-    : public base::RefCountedThreadSafe<Core, Core>,
-      public base::win::ObjectWatcher::Delegate,
-      public IPC::Listener {
- public:
-  // Creates the launcher that will use |launcher_delegate| to manage the worker
-  // process and |worker_delegate| to handle IPCs. The caller must ensure that
-  // |worker_delegate| remains valid until Stoppable::Stop() method has been
-  // called.
-  //
-  // The caller should call all the methods on this class on
-  // the |caller_task_runner| thread. Methods of both delegate interfaces are
-  // called on the |caller_task_runner| thread as well.
-  Core(scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner,
-       scoped_ptr<WorkerProcessLauncher::Delegate> launcher_delegate,
-       WorkerProcessIpcDelegate* worker_delegate);
-
-  // Launches the worker process.
-  void Start();
-
-  // Stops the worker process asynchronously. The caller can drop the reference
-  // to |this| as soon as Stop() returns.
-  void Stop();
-
-  // See WorkerProcessLauncher::Crash().
-  void Crash(const tracked_objects::Location& location);
-
-  // See WorkerProcessLauncher::Send().
-  void Send(IPC::Message* message);
-
-  // Used to emulate |launch_success_timer_| expiration.
-  void ResetLaunchSuccessTimeoutForTest();
-
-  // Set the desired timeout for |kill_process_timer_|.
-  void SetKillProcessTimeoutForTest(const base::TimeDelta& timeout);
-
-  // base::win::ObjectWatcher::Delegate implementation.
-  virtual void OnObjectSignaled(HANDLE object) OVERRIDE;
-
-  // IPC::Listener implementation.
-  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
-  virtual void OnChannelConnected(int32 peer_pid) OVERRIDE;
-  virtual void OnChannelError() OVERRIDE;
-
-  // Destroys |Core| instances on the |caller_task_runner_| thread. This is
-  // needed because base::OneShotTimer instances must be destroyed on the same
-  // thread they were  created on.
-  static void Destruct(const Core* core);
-
- private:
-  friend class base::DeleteHelper<Core>;
-  virtual ~Core();
-
-  // Attempts to launch the worker process. Schedules next launch attempt if
-  // creation of the process fails.
-  void LaunchWorker();
-
-  // Records a successful launch attempt.
-  void RecordSuccessfulLaunch();
-
-  // Stops the worker process asynchronously and schedules next launch attempt
-  // unless Stop() has been called already.
-  void StopWorker();
-
-  // All public methods are called on the |caller_task_runner| thread.
-  scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner_;
-
-  // Implements specifics of launching a worker process.
-  scoped_ptr<WorkerProcessLauncher::Delegate> launcher_delegate_;
-
-  // Handles IPC messages sent by the worker process.
-  WorkerProcessIpcDelegate* worker_delegate_;
-
-  // Pointer to GetNamedPipeClientProcessId() API if it is available.
-  typedef BOOL (WINAPI * GetNamedPipeClientProcessIdFn)(HANDLE, DWORD*);
-  GetNamedPipeClientProcessIdFn get_named_pipe_client_pid_;
-
-  // True if IPC messages should be passed to |worker_delegate_|.
-  bool ipc_enabled_;
-
-  // The timer used to delay termination of the worker process when an IPC error
-  // occured or when Crash() request is pending
-  base::OneShotTimer<Core> kill_process_timer_;
-
-  // The default timeout for |kill_process_timer_|.
-  base::TimeDelta kill_process_timeout_;
-
-  // Launch backoff state.
-  net::BackoffEntry launch_backoff_;
-
-  // Timer used to delay recording a successfull launch.
-  base::OneShotTimer<Core> launch_success_timer_;
-
-  // Timer used to schedule the next attempt to launch the process.
-  base::OneShotTimer<Core> launch_timer_;
-
-  // Used to determine when the launched process terminates.
-  base::win::ObjectWatcher process_watcher_;
-
-  // A waiting handle that becomes signalled once the launched process has
-  // been terminated.
-  ScopedHandle process_exit_event_;
-
-  // True when Stop() has been called.
-  bool stopping_;
-
-  DISALLOW_COPY_AND_ASSIGN(Core);
-};
-
 WorkerProcessLauncher::Delegate::~Delegate() {
 }
 
-WorkerProcessLauncher::Core::Core(
-    scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner,
+WorkerProcessLauncher::WorkerProcessLauncher(
     scoped_ptr<WorkerProcessLauncher::Delegate> launcher_delegate,
     WorkerProcessIpcDelegate* worker_delegate)
-    : caller_task_runner_(caller_task_runner),
-      launcher_delegate_(launcher_delegate.Pass()),
+    : launcher_delegate_(launcher_delegate.Pass()),
       worker_delegate_(worker_delegate),
-      get_named_pipe_client_pid_(NULL),
+      exit_code_(CONTROL_C_EXIT),
       ipc_enabled_(false),
       kill_process_timeout_(
           base::TimeDelta::FromSeconds(kKillProcessTimeoutSeconds)),
       launch_backoff_(&kDefaultBackoffPolicy),
       stopping_(false) {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
-}
-
-void WorkerProcessLauncher::Core::Start() {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
-  DCHECK(!stopping_);
-
   LaunchWorker();
 }
 
-void WorkerProcessLauncher::Core::Stop() {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
+WorkerProcessLauncher::~WorkerProcessLauncher() {
+  DCHECK(CalledOnValidThread());
 
-  if (!stopping_) {
-    stopping_ = true;
-    worker_delegate_ = NULL;
-    StopWorker();
-  }
+  stopping_ = true;
+  worker_delegate_ = NULL;

[Wez, 2013/05/14 03:45:22]

nit: You could turn stopping_ into a private stopping() helper that just tests
whether worker_delegate_ is NULL, to avoid duplicating state.

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+  StopWorker();
 }
 
-void WorkerProcessLauncher::Core::Crash(
+void WorkerProcessLauncher::Crash(
     const tracked_objects::Location& location) {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
+  DCHECK(CalledOnValidThread());
 
   // Ask the worker process to crash voluntarily if it is still connected.
   if (ipc_enabled_) {
@@ -218,12 +91,12 @@ void WorkerProcessLauncher::Core::Crash(
   // Give the worker process some time to crash.
   if (!kill_process_timer_.IsRunning()) {
     kill_process_timer_.Start(FROM_HERE, kill_process_timeout_, this,
-                              &Core::StopWorker);
+                              &WorkerProcessLauncher::StopWorker);
   }
 }
 
-void WorkerProcessLauncher::Core::Send(IPC::Message* message) {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
+void WorkerProcessLauncher::Send(IPC::Message* message) {
+  DCHECK(CalledOnValidThread());
 
   if (ipc_enabled_) {
     launcher_delegate_->Send(message);
@@ -232,32 +105,32 @@ void WorkerProcessLauncher::Core::Send(IPC::Message* message) {
   }
 }
 
-void WorkerProcessLauncher::Core::ResetLaunchSuccessTimeoutForTest() {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
+void WorkerProcessLauncher::OnProcessLaunched(
+    base::win::ScopedHandle worker_process) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!ipc_enabled_);
+  DCHECK(!launch_timer_.IsRunning());
+  DCHECK(!process_watcher_.GetWatchedObject());
+  DCHECK(!worker_process_.IsValid());
 
-  if (launch_success_timer_.IsRunning()) {
-    launch_success_timer_.Stop();
-    RecordSuccessfulLaunch();
+  if (!process_watcher_.StartWatching(worker_process, this)) {
+    StopWorker();
+    return;
   }
-}
-
-void WorkerProcessLauncher::Core::SetKillProcessTimeoutForTest(
-    const base::TimeDelta& timeout) {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
-  kill_process_timeout_ = timeout;
+  ipc_enabled_ = true;
+  worker_process_ = worker_process.Pass();
 }
 
-void WorkerProcessLauncher::Core::OnObjectSignaled(HANDLE object) {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
-  DCHECK(process_watcher_.GetWatchedObject() == NULL);
+void WorkerProcessLauncher::OnFatalError() {
+  DCHECK(CalledOnValidThread());
 
   StopWorker();
 }
 
-bool WorkerProcessLauncher::Core::OnMessageReceived(
-    const IPC::Message& message) {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
+bool WorkerProcessLauncher::OnMessageReceived(
+  const IPC::Message& message) {
+  DCHECK(CalledOnValidThread());
 
   if (!ipc_enabled_)
     return false;
@@ -265,30 +138,19 @@ bool WorkerProcessLauncher::Core::OnMessageReceived(
   return worker_delegate_->OnMessageReceived(message);
 }
 
-void WorkerProcessLauncher::Core::OnChannelConnected(int32 peer_pid) {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
+void WorkerProcessLauncher::OnChannelConnected(int32 peer_pid) {
+  DCHECK(CalledOnValidThread());
 
   if (!ipc_enabled_)
     return;
 
-  // Verify |peer_pid| because it is controlled by the client and cannot be
-  // trusted.
-  DWORD actual_pid = launcher_delegate_->GetProcessId();
-  if (peer_pid != static_cast<int32>(actual_pid)) {
-    LOG(ERROR) << "The actual client PID " << actual_pid
-               << " does not match the one reported by the client: "
-               << peer_pid;
-    StopWorker();
-    return;
-  }
-
   // This can result in |this| being deleted, so this call must be the last in
   // this method.
   worker_delegate_->OnChannelConnected(peer_pid);
 }
 
-void WorkerProcessLauncher::Core::OnChannelError() {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
+void WorkerProcessLauncher::OnChannelError() {
+  DCHECK(CalledOnValidThread());
 
   // Schedule a delayed termination of the worker process. Usually, the pipe is
   // disconnected when the worker process is about to exit. Waiting a little bit
@@ -297,129 +159,111 @@ void WorkerProcessLauncher::Core::OnChannelError() {
   // the original exit code reported by the worker process will be retrieved.
   if (!kill_process_timer_.IsRunning()) {
     kill_process_timer_.Start(FROM_HERE, kill_process_timeout_, this,
-                              &Core::StopWorker);
+                              &WorkerProcessLauncher::StopWorker);
   }
 }
 
-// static
-void WorkerProcessLauncher::Core::Destruct(const Core* core) {
-  core->caller_task_runner_->DeleteSoon(FROM_HERE, core);
-}
+void WorkerProcessLauncher::OnObjectSignaled(HANDLE object) {

[Wez, 2013/05/14 03:45:22]

nit: Check that |object| is the |worker_process_|?

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+  DCHECK(CalledOnValidThread());
+  DCHECK(!process_watcher_.GetWatchedObject());
+  DCHECK_EQ(exit_code_, CONTROL_C_EXIT);
 
-WorkerProcessLauncher::Core::~Core() {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
-  DCHECK(stopping_);
+  // Get exit code of the worker process if it is available.
+  if (!::GetExitCodeProcess(worker_process_, &exit_code_)) {
+    LOG_GETLASTERROR(INFO)
+        << "Failed to query the exit code of the worker process";
+    exit_code_ = CONTROL_C_EXIT;
+  }
+
+  worker_process_.Close();
+  StopWorker();
 }
 
-void WorkerProcessLauncher::Core::LaunchWorker() {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
+void WorkerProcessLauncher::LaunchWorker() {
+  DCHECK(CalledOnValidThread());
   DCHECK(!ipc_enabled_);
-  DCHECK(!launch_success_timer_.IsRunning());
+  DCHECK(!kill_process_timer_.IsRunning());
   DCHECK(!launch_timer_.IsRunning());
-  DCHECK(!process_exit_event_.IsValid());
-  DCHECK(process_watcher_.GetWatchedObject() == NULL);
-
-  // Launch the process and attach an object watcher to the returned process
-  // handle so that we get notified if the process terminates.
-  if (launcher_delegate_->LaunchProcess(this, &process_exit_event_)) {
-    if (process_watcher_.StartWatching(process_exit_event_, this)) {
-      ipc_enabled_ = true;
-      // Record a successful launch once the process has been running for at
-      // least two seconds.
-      launch_success_timer_.Start(
-          FROM_HERE, base::TimeDelta::FromSeconds(kLaunchSuccessTimeoutSeconds),
-          this, &Core::RecordSuccessfulLaunch);
-      return;
-    }
-
-    launcher_delegate_->KillProcess(CONTROL_C_EXIT);
-  }
+  DCHECK(!process_watcher_.GetWatchedObject());
+  DCHECK(!start_process_timer_.IsRunning());
 
-  launch_backoff_.InformOfRequest(false);
-  StopWorker();
+  exit_code_ = CONTROL_C_EXIT;
+
+  // Make sure launching a process will not take forever.
+  start_process_timer_.Start(
+      FROM_HERE, base::TimeDelta::FromSeconds(kStartProcessTimeoutSeconds),
+      this, &WorkerProcessLauncher::RecordSuccessfulLaunch);
+
+  launcher_delegate_->LaunchProcess(this);
 }
 
-void WorkerProcessLauncher::Core::RecordSuccessfulLaunch() {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
+void WorkerProcessLauncher::RecordSuccessfulLaunch() {

[Wez, 2013/05/14 03:45:22]

This method records both successful and unsuccessful launches, so call it
RecordLaunchResult?

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+  DCHECK(CalledOnValidThread());
+
+  if (!worker_process_.IsValid()) {
+    LOG(WARNING) << "A worker process didn't start in "

[Wez, 2013/05/14 03:45:22]

nit: "... failed to start within ..."

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+                 << kStartProcessTimeoutSeconds << " seconds.";
+
+    launch_backoff_.InformOfRequest(false);
+    StopWorker();
+    return;
+  }
 
+  // Conside the launch attemp successful if the worker process has been running
+  // for a few seconds.

[Wez, 2013/05/14 03:45:22]

nit: typos

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

   launch_backoff_.InformOfRequest(true);
 }
 
-void WorkerProcessLauncher::Core::StopWorker() {
-  DCHECK(caller_task_runner_->BelongsToCurrentThread());
+void WorkerProcessLauncher::RecordSuccessfulLaunchForTest() {
+  DCHECK(CalledOnValidThread());
+
+  if (start_process_timer_.IsRunning()) {
+    start_process_timer_.Stop();
+    RecordSuccessfulLaunch();
+  }
+}
+
+void WorkerProcessLauncher::SetKillProcessTimeoutForTest(
+    const base::TimeDelta& timeout) {
+  DCHECK(CalledOnValidThread());
+
+  kill_process_timeout_ = timeout;
+}
 
-  // Keep the object alive in case one of delegates decides to delete |this|.
-  scoped_refptr<Core> self = this;
+void WorkerProcessLauncher::StopWorker() {
+  DCHECK(CalledOnValidThread());
 
   // Record a launch failure if the process exited too soon.
-  if (launch_success_timer_.IsRunning()) {
-    launch_success_timer_.Stop();
+  if (start_process_timer_.IsRunning()) {
     launch_backoff_.InformOfRequest(false);
+    start_process_timer_.Stop();
   }
 
   // Ignore any remaining IPC messages.
   ipc_enabled_ = false;
 
-  // Kill the process if it has been started already.
-  if (process_watcher_.GetWatchedObject() != NULL) {
-    launcher_delegate_->KillProcess(CONTROL_C_EXIT);
-
-    // Wait until the process is actually stopped if the caller keeps
-    // a reference to |this|. Otherwise terminate everything right now - there
-    // won't be a second chance.
-    if (!stopping_)
-      return;
-
-    process_watcher_.StopWatching();
-  }
+  // Stop monitoring the worker process termination.

[Wez, 2013/05/14 03:45:22]

nit: lose "termination".

[alexeypa (please no reviews), 2013/05/14 18:31:10]

Done.

+  process_watcher_.StopWatching();
+  worker_process_.Close();
 
   kill_process_timer_.Stop();
-  process_exit_event_.Close();
+  launcher_delegate_->KillProcess();
 
   // Do not relaunch the worker process if the caller has asked us to stop.
   if (stopping_)
     return;
 
-  if (launcher_delegate_->IsPermanentError(launch_backoff_.failure_count())) {
-    if (!stopping_)
-      worker_delegate_->OnPermanentError();
-  } else {
-    // Schedule the next attempt to launch the worker process.
-    launch_timer_.Start(FROM_HERE, launch_backoff_.GetTimeUntilRelease(), this,
-                        &Core::LaunchWorker);
+  // Stop trying to restart the worker process if it exited due to
+  // misconfiguration.
+  if (kMinPermanentErrorExitCode <= exit_code_ &&
+      exit_code_ <= kMaxPermanentErrorExitCode) {
+    worker_delegate_->OnPermanentError();
+    return;
   }
-}
-
-WorkerProcessLauncher::WorkerProcessLauncher(
-    scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner,
-    scoped_ptr<Delegate> launcher_delegate,
-    WorkerProcessIpcDelegate* worker_delegate) {
-  core_ = new Core(caller_task_runner, launcher_delegate.Pass(),
-                   worker_delegate);
-  core_->Start();
-}
-
-WorkerProcessLauncher::~WorkerProcessLauncher() {
-  core_->Stop();
-  core_ = NULL;
-}
-
-void WorkerProcessLauncher::Crash(const tracked_objects::Location& location) {
-  core_->Crash(location);
-}
-
-void WorkerProcessLauncher::Send(IPC::Message* message) {
-  core_->Send(message);
-}
-
-void WorkerProcessLauncher::ResetLaunchSuccessTimeoutForTest() {
-  core_->ResetLaunchSuccessTimeoutForTest();
-}
 
-void WorkerProcessLauncher::SetKillProcessTimeoutForTest(
-    const base::TimeDelta& timeout) {
-  core_->SetKillProcessTimeoutForTest(timeout);
+  // Schedule the next attempt to launch the worker process.
+  launch_timer_.Start(FROM_HERE, launch_backoff_.GetTimeUntilRelease(), this,
+                      &WorkerProcessLauncher::LaunchWorker);
 }
 
-
 } // namespace remoting