[Chromoting] Moving common logic responsible for launching child processes to WorkerProcessLauncher…

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
new file mode 100644
index 0000000000000000000000000000000000000000..4147857efb82784affaf0aa0756dee90cd691607
--- /dev/null
+++ b/remoting/host/win/worker_process_launcher.cc
@@ -0,0 +1,240 @@
+// 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.
+
+#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"
+#include "base/single_thread_task_runner.h"
+#include "base/process_util.h"
+#include "base/rand_util.h"
+#include "base/stringprintf.h"
+#include "base/utf_string_conversions.h"
+#include "base/win/scoped_handle.h"
+#include "ipc/ipc_channel_proxy.h"
+#include "ipc/ipc_message.h"
+
+using base::win::ScopedHandle;
+
+namespace {
+
+// Match the pipe name prefix used by Chrome IPC channels.

[Wez, 2012/08/08 20:13:41]

nit: Clarify why we need to match Chrome's prefix.

[alexeypa (please no reviews), 2012/08/08 21:49:58]

Done.

+const char kChromePipeNamePrefix[] = "\\\\.\\pipe\\chrome.";
+
+} // namespace
+
+namespace remoting {
+
+WorkerProcessLauncher::Delegate::~Delegate() {
+}
+
+WorkerProcessLauncher::WorkerProcessLauncher(
+    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),
+      delegate_(NULL),
+      main_task_runner_(main_task_runner),
+      ipc_task_runner_(ipc_task_runner),
+      process_watcher_armed_(false) {
+}
+
+WorkerProcessLauncher::~WorkerProcessLauncher() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  DCHECK(delegate_ == NULL);
+}
+
+void WorkerProcessLauncher::Start(Delegate* delegate,
+                                  const std::string& pipe_sddl) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  DCHECK(delegate_ == NULL);
+  DCHECK(ipc_channel_.get() == NULL);
+  DCHECK(!pipe_.IsValid());
+  DCHECK(process_watcher_.GetWatchedObject() == NULL);
+  DCHECK(!wait_.IsValid());
+
+  delegate_ = delegate;
+  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_));
+
+    // Launch the process and attach an object watcher to the returned process
+    // handle so that we get notified if the process terminates.
+    if (delegate_->DoLaunchProcess(channel_name, &wait_)) {
+      if (process_watcher_.StartWatching(wait_, this)) {
+        process_watcher_armed_ = true;

[Wez, 2012/08/08 20:13:41]

nit: Do we need a separate "armed" flag, rather than checking
GetWatchedObject()?

[alexeypa (please no reviews), 2012/08/08 21:49:58]

Agree. It looks like the watcher object takes care of synchronization, so we can
safely poll GetWatchedObject().

+        return;
+      }
+
+      delegate_->DoKillProcess(CONTROL_C_EXIT);
+    }
+  }
+
+  Stop();
+}
+
+void WorkerProcessLauncher::Send(IPC::Message* message) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  ipc_channel_->Send(message);
+}
+
+void WorkerProcessLauncher::OnObjectSignaled(HANDLE object) {
+  if (!main_task_runner_->BelongsToCurrentThread()) {
+    main_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&WorkerProcessLauncher::OnObjectSignaled,
+                              base::Unretained(this), object));

[Wez, 2012/08/08 20:13:41]

What guarantee do you have that WorkerProcessLauncher won't be deleted by the
caller between OnObjectSignalled and the task being processed? Or should this
use a WeakPtr?

[alexeypa (please no reviews), 2012/08/08 21:49:58]

This code has been removed. OnObjectSignalled is guaranteed to be called on the
same thread that called StartWatching().

+    return;
+  }
+
+  process_watcher_armed_ = false;
+  Stop();
+}
+
+bool WorkerProcessLauncher::OnMessageReceived(const IPC::Message& message) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  DCHECK(ipc_channel_.get() != NULL);
+  DCHECK(pipe_.IsValid());
+  DCHECK(wait_.IsValid());
+
+  return delegate_->OnMessageReceived(message);
+}
+
+void WorkerProcessLauncher::OnChannelConnected(int32 peer_pid) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  DCHECK(ipc_channel_.get() != NULL);
+  DCHECK(pipe_.IsValid());
+  DCHECK(wait_.IsValid());
+
+  // Verify that the launched process and the one connected to the pipe is
+  // the same process.

[Wez, 2012/08/08 20:13:41]

nit: is -> are

[alexeypa (please no reviews), 2012/08/08 21:49:58]

Done.

+  ULONG process_id;
+  if (!GetNamedPipeClientProcessId(pipe_, &process_id)) {

[Wez, 2012/08/08 20:13:41]

Do you mean to actually verify the ID here?

[alexeypa (please no reviews), 2012/08/08 21:49:58]

The code has changed since that time.

+    LOG_GETLASTERROR(ERROR) << "Failed to verify PID of the client process";

[Wez, 2012/08/08 20:13:41]

nit: verify -> query

[alexeypa (please no reviews), 2012/08/08 21:49:58]

Done.

+    Stop();
+    return;
+  }
+
+  ScopedHandle peer;
+  peer.Set(OpenProcess(MAXIMUM_ALLOWED, FALSE, process_id));

[Wez, 2012/08/08 20:13:41]

What guarantee is there that the peer process doesn't close between these two
calls and expose the risk that we grab the wrong process' handle?

[alexeypa (please no reviews), 2012/08/08 21:49:58]

There is no guarantee. The caller has to keep the worker's handle open to make
sure that PID will not be reused.

+  if (!peer.IsValid()) {
+    LOG_GETLASTERROR(ERROR) << "Failed to open the process, PID=" << process_id;
+    Stop();
+    return;
+  }
+
+  delegate_->OnChannelConnected(peer.Pass());
+}
+
+void WorkerProcessLauncher::OnChannelError() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  DCHECK(ipc_channel_.get() != NULL);
+  DCHECK(pipe_.IsValid());
+  DCHECK(wait_.IsValid());
+
+  Stop();
+}
+
+void WorkerProcessLauncher::DoStop() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  switch (stoppable_state()) {
+    case Stoppable::kRunning:
+      ipc_channel_.reset();
+      pipe_.Close();
+
+      // Kill the process if it has been started already.
+      if (process_watcher_armed_) {
+        delegate_->DoKillProcess(CONTROL_C_EXIT);
+        return;
+      }
+
+      // fall through
+
+    case Stoppable::kStopping:
+      DCHECK(ipc_channel_.get() == NULL);
+      DCHECK(!pipe_.IsValid());
+      DCHECK(process_watcher_.GetWatchedObject() == NULL);
+
+      wait_.Close();
+      delegate_ = NULL;
+      CompleteStopping();
+      break;
+
+    case Stoppable::kStopped:
+    default:
+      DCHECK(!"Unexpected state");
+  }
+}
+
+// Creates the server end of the Chromoting IPC channel.
+bool WorkerProcessLauncher::CreatePipeForIpcChannel(
+    const std::string& channel_name,
+    const std::string& pipe_sddl,
+    ScopedHandle* pipe_out) {
+  // Create security descriptor for the channel.
+  SECURITY_ATTRIBUTES security_attributes;
+  security_attributes.nLength = sizeof(security_attributes);
+  security_attributes.bInheritHandle = FALSE;
+
+  ULONG security_descriptor_length = 0;
+  if (!ConvertStringSecurityDescriptorToSecurityDescriptorA(

[Wez, 2012/08/08 20:13:41]

nit: Strictly |pipe_sddl| is UTF-8 and this function takes Windows' ANSI, which
might malfunction in fun ways in some locales.

[alexeypa (please no reviews), 2012/08/08 21:49:58]

Done.

+           pipe_sddl.c_str(),
+           SDDL_REVISION_1,
+           reinterpret_cast<PSECURITY_DESCRIPTOR*>(
+               &security_attributes.lpSecurityDescriptor),
+           &security_descriptor_length)) {
+    LOG_GETLASTERROR(ERROR) <<
+        "Failed to create a security descriptor for the Chromoting IPC channel";
+    return false;
+  }
+
+  // Convert the channel name to the pipe name.
+  std::string pipe_name(kChromePipeNamePrefix);
+  pipe_name.append(channel_name);
+
+  // Create the server end of the pipe. This code should match the code in
+  // IPC::Channel with exception of passing a non-default security descriptor.
+  base::win::ScopedHandle pipe;
+  pipe.Set(CreateNamedPipeW(UTF8ToUTF16(pipe_name).c_str(),
+                            PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED |
+                                FILE_FLAG_FIRST_PIPE_INSTANCE,

[Wez, 2012/08/08 20:13:41]

nit: This would be more readable if the parameters were _not_ indented to the
opening bracket, so that the access flags didn't need to be wrapped...

[alexeypa (please no reviews), 2012/08/08 21:49:58]

Done.

+                            PIPE_TYPE_BYTE | PIPE_READMODE_BYTE,
+                            1,
+                            IPC::Channel::kReadBufferSize,
+                            IPC::Channel::kReadBufferSize,
+                            5000,
+                            &security_attributes));
+  if (!pipe.IsValid()) {
+    LOG_GETLASTERROR(ERROR) <<
+        "Failed to create the server end of the Chromoting IPC channel";
+    LocalFree(security_attributes.lpSecurityDescriptor);
+    return false;
+  }
+
+  LocalFree(security_attributes.lpSecurityDescriptor);
+
+  *pipe_out = pipe.Pass();
+  return true;
+}
+
+// N.B. Stolen from src/content/common/child_process_host_impl.cc

[Wez, 2012/08/08 20:13:41]

nit: Stolen -> Copied ;)

[alexeypa (please no reviews), 2012/08/08 21:49:58]

Your honor, I object! :-)

[Wez, 2012/08/10 00:09:23]

ChildProcessHostImpl still has this code, so you clearly didn't _steal_ it... :P

[alexeypa (please no reviews), 2012/08/10 16:18:44]

Done.

+std::string WorkerProcessLauncher::GenerateRandomChannelId() {
+  return base::StringPrintf("%d.%p.%d",
+                            base::GetCurrentProcId(), this,
+                            base::RandInt(0, std::numeric_limits<int>::max()));
+}
+
+} // namespace remoting