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

remoting/host/win/worker_process_launcher_unittest.cc

Index: remoting/host/win/worker_process_launcher_unittest.cc
diff --git a/remoting/host/win/worker_process_launcher_unittest.cc b/remoting/host/win/worker_process_launcher_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9f430c85f8a97eaae748e381be69b1c67bab4cca
--- /dev/null
+++ b/remoting/host/win/worker_process_launcher_unittest.cc
@@ -0,0 +1,339 @@
+// 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 "base/bind.h"
+#include "base/memory/ref_counted.h"
+#include "base/message_loop.h"
+#include "base/win/scoped_handle.h"
+#include "ipc/ipc_channel.h"
+#include "ipc/ipc_channel_proxy.h"
+#include "ipc/ipc_listener.h"
+#include "ipc/ipc_message.h"
+#include "remoting/base/auto_thread_task_runner.h"
+#include "remoting/host/host_exit_codes.h"
+#include "remoting/host/win/worker_process_launcher.h"
+#include "remoting/host/worker_process_ipc_delegate.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using base::win::ScopedHandle;
+using testing::_;
+using testing::AnyNumber;
+using testing::DoAll;
+using testing::Expectation;
+using testing::Invoke;
+using testing::Return;
+using testing::ReturnPointee;
+
+namespace remoting {
+
+namespace {
+
+const char kIpcSecurityDescriptor[] = "D:(A;;GA;;;AU)";
+
+class MockProcessLauncherDelegate
+    : public WorkerProcessLauncher::Delegate {
+ public:
+  MockProcessLauncherDelegate() {}
+  virtual ~MockProcessLauncherDelegate() {}
+
+  // WorkerProcessLauncher::Delegate implementation
+  MOCK_METHOD0(GetExitCode, DWORD());
+  MOCK_METHOD1(KillProcess, void(DWORD));
+  MOCK_METHOD2(LaunchProcess, bool(const std::string&, ScopedHandle*));
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockProcessLauncherDelegate);
+};
+
+class MockIpcDelegate
+    : public WorkerProcessIpcDelegate {
+ public:
+  MockIpcDelegate() {}
+  virtual ~MockIpcDelegate() {}
+
+  // WorkerProcessIpcDelegate implementation
+  MOCK_METHOD0(OnChannelConnected, void());
+  MOCK_METHOD1(OnMessageReceived, bool(const IPC::Message&));
+  MOCK_METHOD0(OnPermanentError, void());
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockIpcDelegate);
+};
+
+}  // namespace
+
+class WorkerProcessLauncherTest
+    : public testing::Test,
+      public IPC::Listener {
+ public:
+  WorkerProcessLauncherTest();
+  virtual ~WorkerProcessLauncherTest();
+
+  virtual void SetUp() OVERRIDE;
+  virtual void TearDown() OVERRIDE;
+
+  // IPC::Listener implementation
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+
+  // WorkerProcessLauncher::Delegate mocks
+  void KillProcess(DWORD exit_code);
+  bool LaunchProcess(const std::string& channel_name,
+                     ScopedHandle* process_exit_event_out);
+  bool LaunchProcessAndConnect(const std::string& channel_name,
+                               ScopedHandle* process_exit_event_out);
+
+  void ConnectTo(const std::string& channel_name);
+  void Disconnect();
+
+  // Starts the worker.
+  void StartWorker();
+
+  // Stops the worker.
+  void StopWorker();
+
+  // Quits |message_loop_|.
+  void QuitMainMessageLoop();
+
+ protected:
+  MessageLoop message_loop_;
+  scoped_refptr<AutoThreadTaskRunner> task_runner_;
+
+  // Exit code of the worker process.
+  DWORD exit_code_;
+
+  MockIpcDelegate ipc_delegate_;
+  scoped_ptr<MockProcessLauncherDelegate> launcher_delegate_;
+
+  // Client end of the IPC channel.
+  scoped_ptr<IPC::ChannelProxy> ipc_channel_;
+
+  // The worker process launcher.
+  scoped_refptr<WorkerProcessLauncher> launcher_;
+
+  // The event signalling termination of the worker process.
+  ScopedHandle process_exit_event_;
+};
+
+
+WorkerProcessLauncherTest::WorkerProcessLauncherTest()
+    : message_loop_(MessageLoop::TYPE_IO) {
+}
+
+WorkerProcessLauncherTest::~WorkerProcessLauncherTest() {
+}
+
+void WorkerProcessLauncherTest::SetUp() {
+  task_runner_ = new AutoThreadTaskRunner(
+      message_loop_.message_loop_proxy(),
+      base::Bind(&WorkerProcessLauncherTest::QuitMainMessageLoop,
+                 base::Unretained(this)));
+
+  exit_code_ = STILL_ACTIVE;
+
+  // Set up process launcher delegate
+  launcher_delegate_.reset(new MockProcessLauncherDelegate());
+  EXPECT_CALL(*launcher_delegate_, GetExitCode())
+      .Times(AnyNumber())
+      .WillRepeatedly(ReturnPointee(&exit_code_));
+  EXPECT_CALL(*launcher_delegate_, KillProcess(_))
+      .Times(AnyNumber())
+      .WillRepeatedly(Invoke(this, &WorkerProcessLauncherTest::KillProcess));
+
+  // Set up IPC delegate.
+  EXPECT_CALL(ipc_delegate_, OnMessageReceived(_))
+      .Times(AnyNumber())
+      .WillRepeatedly(Return(false));
+}
+
+void WorkerProcessLauncherTest::TearDown() {
+}
+
+bool WorkerProcessLauncherTest::OnMessageReceived(const IPC::Message& message) {
+  return false;
+}
+
+void WorkerProcessLauncherTest::KillProcess(DWORD exit_code) {
+  exit_code_ = exit_code;
+  BOOL result = SetEvent(process_exit_event_);  EXPECT_TRUE(result);

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

nit: EXPECT_TRUE on the wrong line.

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

Done.

+}
+
+bool WorkerProcessLauncherTest::LaunchProcess(
+    const std::string& channel_name,
+    ScopedHandle* process_exit_event_out) {
+  return LaunchProcessAndConnect("", process_exit_event_out);
+}
+
+bool WorkerProcessLauncherTest::LaunchProcessAndConnect(
+    const std::string& channel_name,
+    ScopedHandle* process_exit_event_out) {
+  process_exit_event_.Set(CreateEvent(NULL, TRUE, FALSE, NULL));
+  if (!process_exit_event_.IsValid())
+    return false;
+
+  if (!channel_name.empty()) {
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&WorkerProcessLauncherTest::ConnectTo,
+                   base::Unretained(this),
+                   channel_name));
+  }
+
+  exit_code_ = STILL_ACTIVE;
+  return DuplicateHandle(GetCurrentProcess(),
+                         process_exit_event_,
+                         GetCurrentProcess(),
+                         process_exit_event_out->Receive(),
+                         0,
+                         FALSE,
+                         DUPLICATE_SAME_ACCESS) != FALSE;
+}
+
+void WorkerProcessLauncherTest::ConnectTo(const std::string& channel_name) {
+  ipc_channel_.reset(new IPC::ChannelProxy(
+      IPC::ChannelHandle(channel_name),
+      IPC::Channel::MODE_CLIENT,
+      this,
+      task_runner_));
+}
+
+void WorkerProcessLauncherTest::Disconnect() {
+  ipc_channel_.reset();
+}
+
+void WorkerProcessLauncherTest::StartWorker() {
+  launcher_ = new WorkerProcessLauncher(task_runner_,
+                                        task_runner_,
+                                        launcher_delegate_.Pass(),
+                                        &ipc_delegate_,
+                                        kIpcSecurityDescriptor);
+  launcher_->Start();
+}
+
+void WorkerProcessLauncherTest::StopWorker() {
+  launcher_->Stop();
+  launcher_ = NULL;
+  Disconnect();
+  task_runner_ = NULL;
+}
+
+void WorkerProcessLauncherTest::QuitMainMessageLoop() {
+  message_loop_.PostTask(FROM_HERE, MessageLoop::QuitClosure());
+}
+
+TEST_F(WorkerProcessLauncherTest, Start) {
+  EXPECT_CALL(*launcher_delegate_, LaunchProcess(_, _))
+      .Times(AnyNumber())

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

Why is this AnyNumber(); shouldn't it be exactly 1 in this test?

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

Done.

+      .WillRepeatedly(Invoke(this, &WorkerProcessLauncherTest::LaunchProcess));
+
+  EXPECT_CALL(ipc_delegate_, OnChannelConnected())
+      .Times(0);
+  EXPECT_CALL(ipc_delegate_, OnPermanentError())
+      .Times(0);
+
+  StartWorker();
+  StopWorker();
+  message_loop_.Run();
+
+  EXPECT_EQ(exit_code_, CONTROL_C_EXIT);
+}
+
+// Starts and connects to the worker process. Expect OnChannelConnected to be
+// called.
+TEST_F(WorkerProcessLauncherTest, StartAndConnect) {
+  EXPECT_CALL(*launcher_delegate_, LaunchProcess(_, _))
+      .Times(AnyNumber())

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

Similarly here.

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

Done.

+      .WillRepeatedly(Invoke(
+          this, &WorkerProcessLauncherTest::LaunchProcessAndConnect));
+
+  EXPECT_CALL(ipc_delegate_, OnChannelConnected())
+      .Times(1)
+      .WillOnce(Invoke(this, &WorkerProcessLauncherTest::StopWorker));
+  EXPECT_CALL(ipc_delegate_, OnPermanentError())
+      .Times(0);
+
+  StartWorker();
+  message_loop_.Run();
+
+  EXPECT_EQ(exit_code_, CONTROL_C_EXIT);
+}
+
+// Kills the worker process after the 1st connect and expects it to be
+// restarted.
+TEST_F(WorkerProcessLauncherTest, Restart) {
+  EXPECT_CALL(*launcher_delegate_, LaunchProcess(_, _))
+      .Times(AnyNumber())
+      .WillRepeatedly(Invoke(
+          this, &WorkerProcessLauncherTest::LaunchProcessAndConnect));
+
+  base::Closure terminate_worker =
+      base::Bind(&WorkerProcessLauncherTest::KillProcess,
+                 base::Unretained(this),
+                 CONTROL_C_EXIT);
+  Expectation first_connect =
+      EXPECT_CALL(ipc_delegate_, OnChannelConnected())
+          .Times(2)
+          .WillOnce(Invoke(&terminate_worker, &base::Closure::Run))

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

nit: Is there no way to do parameterized invoke, i.e. Invoke(this,
&WorkerProcessLauncherTest::KillProcess, CONTROL_C_EXIT)? Or could you just
provide a KillProcessByCtrlC() helper method to avoid the need for this Closure?

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

There is testing::CreateFunctor.
I tried that. It looks strange that there are multiple methods doing nearly the
same thing and the only difference between them is the exit code.

+          .WillOnce(Invoke(this, &WorkerProcessLauncherTest::StopWorker));
+
+  EXPECT_CALL(ipc_delegate_, OnPermanentError())
+      .Times(0);
+
+  StartWorker();
+  message_loop_.Run();
+
+  EXPECT_EQ(exit_code_, CONTROL_C_EXIT);
+}
+
+// Drops connection to the worker process after the 1st connect and expects
+// the worker process to be restarted.
+// restarted.
+TEST_F(WorkerProcessLauncherTest, DropConnection) {

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

nit: DropIpcChannel, to avoid ambiguity w/ use of "connection" for client/host
connections

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

Done.

+  EXPECT_CALL(*launcher_delegate_, LaunchProcess(_, _))
+      .Times(AnyNumber())
+      .WillRepeatedly(Invoke(
+          this, &WorkerProcessLauncherTest::LaunchProcessAndConnect));
+
+  Expectation first_connect =
+      EXPECT_CALL(ipc_delegate_, OnChannelConnected())
+          .Times(2)
+          .WillOnce(Invoke(this, &WorkerProcessLauncherTest::Disconnect))
+          .WillOnce(Invoke(this, &WorkerProcessLauncherTest::StopWorker));
+
+  EXPECT_CALL(ipc_delegate_, OnPermanentError())
+      .Times(0);
+
+  StartWorker();
+  message_loop_.Run();
+
+  EXPECT_EQ(exit_code_, CONTROL_C_EXIT);
+}
+
+// Returns a permanent error exit code and expects OnPermanentError() to be
+// invoked.
+TEST_F(WorkerProcessLauncherTest, PermanentError) {
+  EXPECT_CALL(*launcher_delegate_, LaunchProcess(_, _))
+      .Times(AnyNumber())
+      .WillRepeatedly(Invoke(
+          this, &WorkerProcessLauncherTest::LaunchProcessAndConnect));
+
+  base::Closure terminate_permanently =
+      base::Bind(&WorkerProcessLauncherTest::KillProcess,
+                 base::Unretained(this),
+                 kInvalidHostConfigurationExitCode);
+  EXPECT_CALL(ipc_delegate_, OnChannelConnected())
+      .Times(AnyNumber())
+      .WillRepeatedly(Invoke(&terminate_permanently, &base::Closure::Run));
+
+  EXPECT_CALL(ipc_delegate_, OnPermanentError())
+      .Times(1)
+      .WillOnce(Invoke(this, &WorkerProcessLauncherTest::StopWorker));
+
+  StartWorker();
+  message_loop_.Run();
+
+  EXPECT_EQ(exit_code_, kInvalidHostConfigurationExitCode);
+}
+
+}  // namespace remoting