[Chromoting] The daemon process now starts the networking process and passes the host configuration…

remoting/host/daemon_process_win.cc

Index: remoting/host/daemon_process_win.cc
diff --git a/remoting/host/daemon_process_win.cc b/remoting/host/daemon_process_win.cc
index 2f7706db71d8ee38b63cefd07a9748a91050ae33..439c45e7020fa5849c3e738710e80abf322f9741 100644
--- a/remoting/host/daemon_process_win.cc
+++ b/remoting/host/daemon_process_win.cc
@@ -4,13 +4,51 @@
 
 #include "remoting/host/daemon_process.h"
 
+#include "base/base_switches.h"
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/logging.h"
+#include "base/path_service.h"
 #include "base/single_thread_task_runner.h"
+#include "base/time.h"
+#include "base/timer.h"
+#include "base/utf_string_conversions.h"
 #include "base/win/scoped_handle.h"
+#include "remoting/host/constants.h"
+#include "remoting/host/win/launch_process_with_token.h"
+#include "remoting/host/win/worker_process_launcher.h"
+
+using base::win::ScopedHandle;
+using base::TimeDelta;
+
+namespace {
+
+// The minimum and maximum delays between attempts to launch the networking
+// process.
+const int kMaxLaunchDelaySeconds = 60;
+const int kMinLaunchDelaySeconds = 1;
+
+const FilePath::CharType kMe2meHostBinaryName[] =
+    FILE_PATH_LITERAL("remoting_me2me_host.exe");
+
+// The IPC channel name is passed to the networking process in the command line.
+const char kDaemonPipeSwitchName[] = "daemon-pipe";
+
+// The command line parameters that should be copied from the service's command
+// line to the host process.

[Wez, 2012/08/21 00:18:57]

nit: host process -> network process?

[alexeypa (please no reviews), 2012/08/23 23:53:01]

Done.

+const char* kCopiedSwitchNames[] = {
+    "auth-config", "host-config", switches::kV, switches::kVModule };
+
+// The security descriptor of the Chromoting IPC channel. It gives full access

[Wez, 2012/08/21 00:18:57]

nit: Chromoting -> daemon, or just remove qualifier?

[alexeypa (please no reviews), 2012/08/23 23:53:01]

Done.

+// to LocalSystem and denies access by anyone else.
+const char kDaemonPipeSecurityDescriptor[] = "O:SYG:SYD:(A;;GA;;;SY)";
+
+} // namespace
 
 namespace remoting {
 
-class DaemonProcessWin : public DaemonProcess {
+class DaemonProcessWin : public DaemonProcess,
+                         public WorkerProcessLauncher::Delegate {
  public:
   DaemonProcessWin(scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
                    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
@@ -18,9 +56,45 @@ class DaemonProcessWin : public DaemonProcess {
   virtual ~DaemonProcessWin();
 
   // DaemonProcess implementation.
-  virtual bool LaunchNetworkProcess() OVERRIDE;
+  virtual void LaunchNetworkProcess() OVERRIDE;
+
+  // WorkerProcessDelegate implementation.

[Wez, 2012/08/21 00:18:57]

nit: WorkerProcessDelegate is inherited via DaemonProcess, so it's a little
confusing to see you refer to it directly here.

[alexeypa (please no reviews), 2012/08/23 23:53:01]

Done.

+  virtual void OnChannelConnected() OVERRIDE;
+
+  // Sends an IPC message to the worker process. This method can be called only
+  // after successful Start() and until Stop() is called or an error occurred.
+  virtual void Send(IPC::Message* message) OVERRIDE;
+
+  // WorkerProcessLauncher::Delegate implementation.
+  virtual bool DoLaunchProcess(
+      const std::string& channel_name,
+      ScopedHandle* process_exit_event_out) OVERRIDE;
+  virtual void DoKillProcess(DWORD exit_code) OVERRIDE;
+
+ protected:
+  // Stoppable implementation.
+  virtual void DoStop() OVERRIDE;
 
  private:
+  // Called when the launcher reports the process to be stopped.

[Wez, 2012/08/21 00:18:57]

nit: ... reports the worker process has stopped.

[alexeypa (please no reviews), 2012/08/23 23:53:01]

Done.

+  void OnLauncherStopped();
+
+  // True if the network process is connected.

[Wez, 2012/08/21 00:18:57]

nit: Connected to the Daemon, connected to Talk, or has at least one client
connected, or something else?

[alexeypa (please no reviews), 2012/08/23 23:53:01]

Done.

+  bool connected_;
+
+  // Time of the last launch attempt.
+  base::Time launch_time_;
+
+  // Current backoff delay.
+  base::TimeDelta launch_backoff_;
+
+  // Timer used to schedule the next attempt to launch the process.
+  base::OneShotTimer<DaemonProcessWin> timer_;
+
+  scoped_ptr<WorkerProcessLauncher> launcher_;
+
+  ScopedHandle network_process_;
+
   DISALLOW_COPY_AND_ASSIGN(DaemonProcessWin);
 };
 
@@ -28,15 +102,180 @@ DaemonProcessWin::DaemonProcessWin(
     scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     const base::Closure& stopped_callback)
-    : DaemonProcess(main_task_runner, io_task_runner, stopped_callback) {
+    : DaemonProcess(main_task_runner, io_task_runner, stopped_callback),
+      connected_(false) {
 }
 
 DaemonProcessWin::~DaemonProcessWin() {
+  // Make sure that the object is completely stopped. The same check exists
+  // in Stoppable::~Stoppable() but this one allows us to examine the state of
+  // the object before destruction.

[Wez, 2012/08/21 00:18:57]

But you're not examining any state; aren't you really sanity-checking before
teardown of the object's members?

[alexeypa (please no reviews), 2012/08/21 17:16:36]

In this case "us" are human beings equipped with a debugger. :-)

[Wez, 2012/08/24 22:20:37]

OK, so the comment is just misleading. ;) It's implicit in there being a CHECK
that the aim is to trap a bad condition at a point where a human can step in and
see what's wrong in a debugger; the point of putting the CHECK here is to trap
*before* we potentially crashing while tearing stuff down.

[alexeypa (please no reviews), 2012/08/27 23:16:41]

OK, let try another version.

In the cases like this it would help greatly help if you suggest alternative
wording at once. This is forth or so iteration of this comment (and it went
thought one another review already), which surely means that I've run out of
ideas already. :P

[Wez, 2012/08/30 20:36:04]

You're right - sorry, I'll try to do that in future.

+  CHECK_EQ(stoppable_state(), Stoppable::kStopped);
+}
+
+void DaemonProcessWin::LaunchNetworkProcess() {
+  DCHECK(main_task_runner()->BelongsToCurrentThread());
+  DCHECK(launcher_.get() == NULL);
+  DCHECK(!network_process_.IsValid());
+  DCHECK(!timer_.IsRunning());
+
+  launch_time_ = base::Time::Now();
+  launcher_.reset(new WorkerProcessLauncher(
+      this, this,
+      base::Bind(&DaemonProcessWin::OnLauncherStopped, base::Unretained(this)),
+      main_task_runner(),
+      io_task_runner()));
+  launcher_->Start(kDaemonPipeSecurityDescriptor);
+}
+
+void DaemonProcessWin::OnChannelConnected() {
+  connected_ = true;
+  DaemonProcess::OnChannelConnected();
+}
+
+void DaemonProcessWin::Send(IPC::Message* message) {
+  if (connected_) {
+    launcher_->Send(message);
+  } else {
+    delete message;
+  }
 }
 
-bool DaemonProcessWin::LaunchNetworkProcess() {
-  NOTIMPLEMENTED();
-  return false;
+bool DaemonProcessWin::DoLaunchProcess(
+    const std::string& channel_name,
+    ScopedHandle* process_exit_event_out) {
+  DCHECK(main_task_runner()->BelongsToCurrentThread());
+  DCHECK(!network_process_.IsValid());
+
+  // Construct the host 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 host_binary = dir_path.Append(kMe2meHostBinaryName);
+
+  // Create the host process command line passing the name of the IPC channel
+  // to use and copying known switches from the service's command line.
+  CommandLine command_line(host_binary);
+  command_line.AppendSwitchNative(kDaemonPipeSwitchName,
+                                  UTF8ToWide(channel_name));
+  command_line.CopySwitchesFrom(*CommandLine::ForCurrentProcess(),
+                                kCopiedSwitchNames,
+                                _countof(kCopiedSwitchNames));
+
+  // TODO(alexeypa): replace with a restricted token.
+  // See http://crbug.com/134694.

[Wez, 2012/08/21 00:18:57]

nit: It's not clear what this comment means, even in the context of the bug; I
think you really want a comment on the LaunchProcessWithToken() call saying that
we should create a restricted token to launch under, rather than here, where we
have no control over the token we'll get back from OpenProcessToken().

[alexeypa (please no reviews), 2012/08/23 23:53:01]

Done.

+  ScopedHandle token;
+  if (!OpenProcessToken(GetCurrentProcess(),
+                        MAXIMUM_ALLOWED,
+                        token.Receive())) {
+    LOG_GETLASTERROR(ERROR) << "Failed to open process token";

[Wez, 2012/08/21 00:18:57]

Is there ever a situation in which this can happen which isn't essentially
fatal? i.e. should we just CHECK here?

[alexeypa (please no reviews), 2012/08/23 23:53:01]

Done.

+    return false;
+  }
+
+  // Try to launch the process and attach an object watcher to the returned
+  // handle so that we get notified when the process terminates.
+  ScopedHandle worker_thread;
+  if (!LaunchProcessWithToken(host_binary,
+                              command_line.GetCommandLineString(),
+                              token,
+                              0,
+                              &network_process_,
+                              &worker_thread)) {
+    return false;
+  }
+
+  ScopedHandle process_exit_event;
+  if (!DuplicateHandle(GetCurrentProcess(),
+                       network_process_,
+                       GetCurrentProcess(),
+                       process_exit_event.Receive(),
+                       SYNCHRONIZE,
+                       FALSE,
+                       0)) {
+    LOG_GETLASTERROR(ERROR) << "Failed to duplicate a handle";
+    DoKillProcess(CONTROL_C_EXIT);
+    return false;
+  }
+
+  *process_exit_event_out = process_exit_event.Pass();
+  return true;
+}
+
+void DaemonProcessWin::DoKillProcess(DWORD exit_code) {
+  DCHECK(main_task_runner()->BelongsToCurrentThread());
+
+  if (network_process_.IsValid()) {
+    TerminateProcess(network_process_, exit_code);
+  }
+}
+
+void DaemonProcessWin::DoStop() {
+  DCHECK(main_task_runner()->BelongsToCurrentThread());
+
+  launch_backoff_ = base::TimeDelta();

[Wez, 2012/08/21 00:18:57]

Why zero the backoff here? Isn't it useful to let the backoff also apply even if
calling code is misbehaving & starting/stopping us rapidly?

[alexeypa (please no reviews), 2012/08/21 17:16:36]

It will not start starting/stopping rapidly because at this time the object is
being torn down. I.e. not more processes will be started. Zeroing
|launch_backoff_| is not really necessary here as well. It seems to be a remnant
of old code I took from WtsSessionProcessLauncher. I'll remove it.

+  timer_.Stop();
+
+  if (launcher_.get() != NULL) {
+    launcher_->Stop();
+  }
+
+  // Wait for |launcher_| to be completely stopped.

[Wez, 2012/08/21 00:18:57]

nit: Suggest: "Early exit if we're still waiting for |launcher_| to stop."

[alexeypa (please no reviews), 2012/08/23 23:53:01]

Done.

+  if (launcher_.get() != NULL) {
+    return;
+  }
+
+  DaemonProcess::DoStop();
+}
+
+void DaemonProcessWin::OnLauncherStopped() {
+  DCHECK(main_task_runner()->BelongsToCurrentThread());
+
+  DWORD exit_code = CONTROL_C_EXIT;
+  if (network_process_.IsValid()) {

[Wez, 2012/08/21 00:18:57]

Should it be possible for |network_process_| not to be valid here? If not then
CHECK it?

[alexeypa (please no reviews), 2012/08/23 23:53:01]

Done.

+    if (!::GetExitCodeProcess(network_process_, &exit_code)) {
+      LOG_GETLASTERROR(INFO)
+          << "Failed to query the exit code of the worker process";
+      exit_code = CONTROL_C_EXIT;
+    }
+
+    network_process_.Close();
+  }
+
+  connected_ = false;
+  launcher_.reset(NULL);
+
+  // Do not relaunch the network process if the caller has asked us to stop.
+  if (stoppable_state() != Stoppable::kRunning) {
+    Stop();
+    return;
+  }
+
+  // Stop trying to restart the worker process if its process exited due to
+  // misconfiguration.
+  if (kMinPermanentErrorExitCode <= exit_code &&
+      exit_code <= kMaxPermanentErrorExitCode) {
+    Stop();
+    return;
+  }
+
+  // Expand the backoff interval if the process has died quickly or reset it
+  // if it was up longer than the maximum backoff delay.
+  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.
+  timer_.Start(FROM_HERE, launch_backoff_,
+               this, &DaemonProcessWin::LaunchNetworkProcess);
 }
 
 scoped_ptr<DaemonProcess> DaemonProcess::Create(