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

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 834b8bb6338a885c573c5b6b158db8a66115a027..946a9e80487b78cd2b41585fdf7069057d967ea0 100644
--- a/remoting/host/win/worker_process_launcher.cc
+++ b/remoting/host/win/worker_process_launcher.cc
@@ -4,11 +4,9 @@
 
 #include "remoting/host/win/worker_process_launcher.h"
 
-#include <windows.h>
 #include <sddl.h>
 #include <limits>
 
-#include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/logging.h"
@@ -16,14 +14,15 @@
 #include "base/process_util.h"
 #include "base/rand_util.h"
 #include "base/stringprintf.h"
-#include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "base/win/scoped_handle.h"
 #include "ipc/ipc_channel_proxy.h"
 #include "ipc/ipc_message.h"
+#include "remoting/host/host_exit_codes.h"
 #include "remoting/host/worker_process_ipc_delegate.h"
 
 using base::win::ScopedHandle;
+using base::TimeDelta;
 
 namespace {
 
@@ -31,6 +30,11 @@ namespace {
 // could use Chrome IPC APIs instead of connecting manually.
 const char kChromePipeNamePrefix[] = "\\\\.\\pipe\\chrome.";
 
+// The minimum and maximum delays between attempts to inject host process into
+// a session.
+const int kMaxLaunchDelaySeconds = 60;
+const int kMinLaunchDelaySeconds = 1;
+
 } // namespace
 
 namespace remoting {
@@ -39,51 +43,39 @@ WorkerProcessLauncher::Delegate::~Delegate() {
 }
 
 WorkerProcessLauncher::WorkerProcessLauncher(
-    Delegate* launcher_delegate,
-    WorkerProcessIpcDelegate* worker_delegate,
-    const base::Closure& stopped_callback,
     scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
-    scoped_refptr<base::SingleThreadTaskRunner> ipc_task_runner)
-    : Stoppable(main_task_runner, stopped_callback),
-      launcher_delegate_(launcher_delegate),
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
+    scoped_ptr<Delegate> launcher_delegate,
+    WorkerProcessIpcDelegate* worker_delegate,
+    const std::string& pipe_security_descriptor)
+    : main_task_runner_(main_task_runner),
+      io_task_runner_(io_task_runner),
+      launcher_delegate_(launcher_delegate.Pass()),
       worker_delegate_(worker_delegate),
-      main_task_runner_(main_task_runner),
-      ipc_task_runner_(ipc_task_runner) {
-}
-
-WorkerProcessLauncher::~WorkerProcessLauncher() {
+      pipe_security_descriptor_(pipe_security_descriptor),
+      stopping_(false) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  // base::OneShotTimer must be destroyed on the same thread it was created on.
+  ipc_error_timer_.reset(new base::OneShotTimer<WorkerProcessLauncher>());
+  launch_timer_.reset(new base::OneShotTimer<WorkerProcessLauncher>());
 }
 
-void WorkerProcessLauncher::Start(const std::string& pipe_sddl) {
+void WorkerProcessLauncher::Start() {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
-  DCHECK(ipc_channel_.get() == NULL);
-  DCHECK(!pipe_.IsValid());
-  DCHECK(!process_exit_event_.IsValid());
-  DCHECK(process_watcher_.GetWatchedObject() == NULL);
+  DCHECK(!stopping_);
 
-  std::string channel_name = GenerateRandomChannelId();
-  if (CreatePipeForIpcChannel(channel_name, pipe_sddl, &pipe_)) {
-    // Wrap the pipe into an IPC channel.
-    ipc_channel_.reset(new IPC::ChannelProxy(
-        IPC::ChannelHandle(pipe_),
-        IPC::Channel::MODE_SERVER,
-        this,
-        ipc_task_runner_));
+  LaunchWorker();
+}
 
-    // 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_->DoLaunchProcess(channel_name,
-                                            &process_exit_event_)) {
-      if (process_watcher_.StartWatching(process_exit_event_, this)) {
-        return;
-      }
+void WorkerProcessLauncher::Stop() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
 
-      launcher_delegate_->DoKillProcess(CONTROL_C_EXIT);
-    }
+  if (!stopping_) {
+    stopping_ = true;
+    worker_delegate_ = NULL;
+    StopWorker();
   }
-
-  Stop();
 }
 
 void WorkerProcessLauncher::Send(IPC::Message* message) {
@@ -100,14 +92,12 @@ void WorkerProcessLauncher::OnObjectSignaled(HANDLE object) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   DCHECK(process_watcher_.GetWatchedObject() == NULL);
 
-  Stop();
+  StopWorker();
 }
 
 bool WorkerProcessLauncher::OnMessageReceived(const IPC::Message& message) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   DCHECK(ipc_channel_.get() != NULL);
-  DCHECK(pipe_.IsValid());
-  DCHECK(process_exit_event_.IsValid());
 
   return worker_delegate_->OnMessageReceived(message);
 }
@@ -115,8 +105,6 @@ bool WorkerProcessLauncher::OnMessageReceived(const IPC::Message& message) {
 void WorkerProcessLauncher::OnChannelConnected(int32 peer_pid) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   DCHECK(ipc_channel_.get() != NULL);
-  DCHECK(pipe_.IsValid());
-  DCHECK(process_exit_event_.IsValid());
 
   // |peer_pid| is send by the client and cannot be trusted.
   // GetNamedPipeClientProcessId() is not available on XP. The pipe's security
@@ -132,44 +120,24 @@ void WorkerProcessLauncher::OnChannelConnected(int32 peer_pid) {
 void WorkerProcessLauncher::OnChannelError() {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   DCHECK(ipc_channel_.get() != NULL);
-  DCHECK(pipe_.IsValid());
-  DCHECK(process_exit_event_.IsValid());
 
   // 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
   // here allows the worker to exit completely and so, notify
-  // |process_watcher_|. As the result DoKillProcess() will not be called and
+  // |process_watcher_|. As the result KillProcess() will not be called and
   // the original exit code reported by the worker process will be retrieved.
-  ipc_error_timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(5),
-                         this, &WorkerProcessLauncher::Stop);
+  ipc_error_timer_->Start(FROM_HERE, base::TimeDelta::FromSeconds(5),
+                          this, &WorkerProcessLauncher::StopWorker);
 }
 
-void WorkerProcessLauncher::DoStop() {
-  DCHECK(main_task_runner_->BelongsToCurrentThread());
-
-  ipc_channel_.reset();
-  pipe_.Close();
-
-  // Kill the process if it has been started already.
-  if (process_watcher_.GetWatchedObject() != NULL) {
-    launcher_delegate_->DoKillProcess(CONTROL_C_EXIT);
-    return;
-  }
-
-  ipc_error_timer_.Stop();
-
-  DCHECK(ipc_channel_.get() == NULL);
-  DCHECK(!pipe_.IsValid());
-  DCHECK(process_watcher_.GetWatchedObject() == NULL);
-
-  process_exit_event_.Close();
-  CompleteStopping();
+WorkerProcessLauncher::~WorkerProcessLauncher() {
+  DCHECK(stopping_);
 }
 
 // Creates the server end of the Chromoting IPC channel.
 bool WorkerProcessLauncher::CreatePipeForIpcChannel(
     const std::string& channel_name,
-    const std::string& pipe_sddl,
+    const std::string& pipe_security_descriptor,
     ScopedHandle* pipe_out) {
   // Create security descriptor for the channel.
   SECURITY_ATTRIBUTES security_attributes;
@@ -178,7 +146,7 @@ bool WorkerProcessLauncher::CreatePipeForIpcChannel(
 
   ULONG security_descriptor_length = 0;
   if (!ConvertStringSecurityDescriptorToSecurityDescriptor(
-          UTF8ToUTF16(pipe_sddl).c_str(),
+          UTF8ToUTF16(pipe_security_descriptor).c_str(),
           SDDL_REVISION_1,
           reinterpret_cast<PSECURITY_DESCRIPTOR*>(
               &security_attributes.lpSecurityDescriptor),
@@ -224,4 +192,101 @@ std::string WorkerProcessLauncher::GenerateRandomChannelId() {
                             base::RandInt(0, std::numeric_limits<int>::max()));
 }
 
+void WorkerProcessLauncher::LaunchWorker() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  DCHECK(ipc_channel_.get() == NULL);
+  DCHECK(!launch_timer_->IsRunning());
+  DCHECK(!pipe_.IsValid());
+  DCHECK(!process_exit_event_.IsValid());
+  DCHECK(process_watcher_.GetWatchedObject() == NULL);
+
+  launch_time_ = base::Time::Now();
+  std::string channel_name = GenerateRandomChannelId();
+  if (CreatePipeForIpcChannel(channel_name, pipe_security_descriptor_,
+                              &pipe_)) {
+    // Wrap the pipe into an IPC channel.
+    ipc_channel_.reset(new IPC::ChannelProxy(
+        IPC::ChannelHandle(pipe_),
+        IPC::Channel::MODE_SERVER,
+        this,
+        io_task_runner_));
+
+    // 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(channel_name, &process_exit_event_)) {
+      if (process_watcher_.StartWatching(process_exit_event_, this)) {
+        return;
+      }
+
+      launcher_delegate_->KillProcess(CONTROL_C_EXIT);
+    }
+  }
+
+  StopWorker();
+}
+
+void WorkerProcessLauncher::StopWorker() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  // Keep |this| alive until the worker process is terminated.
+  self_ = this;
+
+  ipc_channel_.reset();
+  pipe_.Close();
+
+  // Kill the process if it has been started already.
+  if (process_watcher_.GetWatchedObject() != NULL) {
+    launcher_delegate_->KillProcess(CONTROL_C_EXIT);
+    return;
+  }
+
+  ipc_error_timer_->Stop();
+
+  DCHECK(ipc_channel_.get() == NULL);
+  DCHECK(!pipe_.IsValid());
+  DCHECK(process_watcher_.GetWatchedObject() == NULL);
+
+  process_exit_event_.Close();
+
+  // Do not relaunch the worker process if the caller has asked us to stop.
+  if (stopping_) {
+    ipc_error_timer_.reset();
+    launch_timer_.reset();
+    self_ = NULL;
+    return;
+  }
+
+  self_ = NULL;
+
+  // Stop trying to restart the worker process if it exited due to
+  // misconfiguration.
+  DWORD exit_code = launcher_delegate_->GetExitCode();
+  if (kMinPermanentErrorExitCode <= exit_code &&
+      exit_code <= kMaxPermanentErrorExitCode) {
+    // |delegate_| must be valid because Stop() hasn't been called yet and
+    // |running_| is true. |worker_delegate_| is valid here because Stop()
+    // hasn't been called yet (|stopping_| is false).
+    worker_delegate_->OnPermanentError();
+    return;
+  }
+
+  // Try to restart the worker process. Expand the backoff interval if
+  // the process has died quickly or reset it if it was up longer than
+  // the maximum backoff delay.

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

We use net::BackoffEntry to handle authentication back-off; would it make sense
to re-use it here?

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

Done.

+  base::TimeDelta delta = base::Time::Now() - launch_time_;
+  if (delta < base::TimeDelta() ||
+      delta >= base::TimeDelta::FromSeconds(kMaxLaunchDelaySeconds)) {
+    launch_backoff_ = base::TimeDelta();
+  } else {
+    launch_backoff_ = std::max(
+        launch_backoff_ * 2, TimeDelta::FromSeconds(kMinLaunchDelaySeconds));
+    launch_backoff_ = std::min(
+        launch_backoff_, TimeDelta::FromSeconds(kMaxLaunchDelaySeconds));
+  }
+
+  // Try to launch the worker process.
+  launch_timer_->Start(FROM_HERE, launch_backoff_, this,
+                       &WorkerProcessLauncher::LaunchWorker);
+}
+
 } // namespace remoting