Resubmit "Start chromoting host in the service process though a method call"

This change exposes method calls to configure the chromoting host and allow
it to be started from a method.

This will allow us to use IPC message to start the chromoting host.

TEST=unit_tests --gtest_filter=ServiceProcess*
BUG=50243, 50244

Review URL: http://codereview.chromium.org/3105002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@55593 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 242 insertions, 85 deletions

diff --git a/chrome/service/service_main.cc b/chrome/service/service_main.cc
index 8994c12..2670c5c 100644
--- a/chrome/service/service_main.cc
+++ b/chrome/service/service_main.cc
@@ -11,22 +11,6 @@
 #include "chrome/service/cloud_print/cloud_print_proxy.h"
 #include "chrome/service/service_process.h"
 
-#if defined(ENABLE_REMOTING)
-#include "remoting/host/json_host_config.h"
-#include "remoting/host/chromoting_host.h"
-#include "remoting/host/chromoting_host_context.h"
-
-// This method is called as a signal that the Chromoting Host Process is
-// shutting down because of failure or a request made by the user.
-// We'll then post a task to |message_loop| to stop the chromoting host
-// context to finish the final cleanup.
-static void OnChromotingHostShutdown(
-    MessageLoop* message_loop, remoting::ChromotingHostContext* context) {
-  message_loop->PostTask(FROM_HERE,
-      NewRunnableMethod(context, &remoting::ChromotingHostContext::Stop));
-}
-#endif
-
 // Mainline routine for running as the service process.
 int ServiceProcessMain(const MainFunctionParams& parameters) {
   MessageLoopForUI main_message_loop;
@@ -40,7 +24,7 @@ int ServiceProcessMain(const MainFunctionParams& parameters) {
 #endif   // defined(OS_WIN)
 
   ServiceProcess service_process;
-  service_process.Initialize();
+  service_process.Initialize(&main_message_loop);
 
   // Enable Cloud Print if needed.
   if (parameters.command_line_.HasSwitch(switches::kEnableCloudPrintProxy)) {
@@ -50,47 +34,7 @@ int ServiceProcessMain(const MainFunctionParams& parameters) {
     service_process.GetCloudPrintProxy()->EnableForUser(lsid);
   }
 
-#if defined(ENABLE_REMOTING)
-  // Enable Chromoting Host if needed.
-  // TODO(hclam): Merge this config file with Cloud Printing.
-  // TODO(hclam): There is only start but not stop of the chromoting host
-  // process.
-  FilePath user_data_dir;
-  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
-  FilePath chromoting_config_path =
-      user_data_dir.Append(FILE_PATH_LITERAL(".ChromotingConfig.json"));
-  scoped_refptr<remoting::JsonHostConfig> chromoting_config;
-  scoped_ptr<remoting::ChromotingHostContext> chromoting_context;
-  scoped_refptr<remoting::ChromotingHost> chromoting_host;
-  if (parameters.command_line_.HasSwitch(switches::kEnableRemoting)) {
-    chromoting_config = new remoting::JsonHostConfig(
-        chromoting_config_path,
-        service_process.file_thread()->message_loop_proxy());
-    if (!chromoting_config->Read()) {
-      LOG(ERROR) << "Failed to read chromoting config file.";
-    } else {
-      chromoting_context.reset(new remoting::ChromotingHostContext());
-
-      // Create the Chromoting Host Process with the context and config.
-      chromoting_host = service_process.CreateChromotingHost(
-          chromoting_context.get(), chromoting_config);
-
-      // And start the context and the host process.
-      chromoting_context->Start();
-
-      // When ChromotingHost is shutdown because of failure or a request that
-      // we made. ShutdownChromotingTask() is calls.
-      // ShutdownChromotingTask() will then post a task to
-      // |main_message_loop| to shutdown the chromoting context.
-      chromoting_host->Start(
-          NewRunnableFunction(&OnChromotingHostShutdown,
-                              &main_message_loop, chromoting_context.get()));
-    }
-  }
-#endif
-
   MessageLoop::current()->Run();
   service_process.Teardown();
-
   return 0;
 }
diff --git a/chrome/service/service_process.cc b/chrome/service/service_process.cc
index 60229d6..fbd9c8d 100644
--- a/chrome/service/service_process.cc
+++ b/chrome/service/service_process.cc
@@ -16,16 +16,17 @@
 #include "net/base/network_change_notifier.h"
 
 #if defined(ENABLE_REMOTING)
+#include "remoting/base/constants.h"
 #include "remoting/base/encoder_zlib.h"
 #include "remoting/host/chromoting_host.h"
 #include "remoting/host/chromoting_host_context.h"
-#include "remoting/host/host_config.h"
+#include "remoting/host/json_host_config.h"
 
 #if defined(OS_WIN)
 #include "remoting/host/capturer_gdi.h"
 #include "remoting/host/event_executor_win.h"
 #elif defined(OS_LINUX)
-#include "remoting/host/capturer_linux.h"
+#include "remoting/host/capturer_fake.h"
 #include "remoting/host/event_executor_linux.h"
 #elif defined(OS_MACOSX)
 #include "remoting/host/capturer_mac.h"
@@ -35,12 +36,15 @@
 
 ServiceProcess* g_service_process = NULL;
 
-ServiceProcess::ServiceProcess() : shutdown_event_(true, false) {
+ServiceProcess::ServiceProcess()
+  : shutdown_event_(true, false),
+    main_message_loop_(NULL) {
   DCHECK(!g_service_process);
   g_service_process = this;
 }
 
-bool ServiceProcess::Initialize() {
+bool ServiceProcess::Initialize(MessageLoop* message_loop) {
+  main_message_loop_ = message_loop;
   network_change_notifier_.reset(net::NetworkChangeNotifier::Create());
   base::Thread::Options options;
   options.message_loop_type = MessageLoop::TYPE_IO;
@@ -74,9 +78,16 @@ bool ServiceProcess::Initialize() {
 }
 
 bool ServiceProcess::Teardown() {
-  service_prefs_->WritePrefs();
-  service_prefs_.reset();
+  if (service_prefs_.get()) {
+    service_prefs_->WritePrefs();
+    service_prefs_.reset();
+  }
   cloud_print_proxy_.reset();
+
+#if defined(ENABLE_REMOTING)
+  ShutdownChromotingHost();
+#endif
+
   ipc_server_.reset();
   // Signal this event before shutting down the service process. That way all
   // background threads can cleanup.
@@ -86,6 +97,7 @@ bool ServiceProcess::Teardown() {
   // The NetworkChangeNotifier must be destroyed after all other threads that
   // might use it have been shut down.
   network_change_notifier_.reset();
+
   return true;
 }
 
@@ -98,19 +110,29 @@ CloudPrintProxy* ServiceProcess::GetCloudPrintProxy() {
 }
 
 #if defined(ENABLE_REMOTING)
-remoting::ChromotingHost* ServiceProcess::CreateChromotingHost(
-    remoting::ChromotingHostContext* context,
-    remoting::MutableHostConfig* config) {
+bool ServiceProcess::StartChromotingHost() {
+  // We have already started.
+  if (chromoting_context_.get())
+    return true;
+
+  // Load chromoting config from the disk.
+  LoadChromotingConfig();
+
+  // Start the chromoting context first.
+  chromoting_context_.reset(new remoting::ChromotingHostContext());
+  chromoting_context_->Start();
+
+  // Create capturer, encoder and executor. The ownership will be transfered
+  // to the chromoting host.
   scoped_ptr<remoting::Capturer> capturer;
   scoped_ptr<remoting::Encoder> encoder;
   scoped_ptr<remoting::EventExecutor> executor;
 
-  // Select the capturer and encoder from |config|.
 #if defined(OS_WIN)
   capturer.reset(new remoting::CapturerGdi());
   executor.reset(new remoting::EventExecutorWin());
 #elif defined(OS_LINUX)
-  capturer.reset(new remoting::CapturerLinux());
+  capturer.reset(new remoting::CapturerFake());
   executor.reset(new remoting::EventExecutorLinux());
 #elif defined(OS_MACOSX)
   capturer.reset(new remoting::CapturerMac());
@@ -118,8 +140,81 @@ remoting::ChromotingHost* ServiceProcess::CreateChromotingHost(
 #endif
   encoder.reset(new remoting::EncoderZlib());
 
-  return new remoting::ChromotingHost(context, config, capturer.release(),
-                                      encoder.release(), executor.release());
+  // Create a chromoting host object.
+  chromoting_host_ = new remoting::ChromotingHost(chromoting_context_.get(),
+                                                  chromoting_config_,
+                                                  capturer.release(),
+                                                  encoder.release(),
+                                                  executor.release());
+
+  // Then start the chromoting host.
+  // When ChromotingHost is shutdown because of failure or a request that
+  // we made OnChromotingShutdown() is calls.
+  chromoting_host_->Start(
+      NewRunnableMethod(this, &ServiceProcess::OnChromotingHostShutdown));
+  return true;
+}
+
+bool ServiceProcess::ShutdownChromotingHost() {
+  // Chromoting host doesn't exist so return true.
+  if (!chromoting_host_)
+    return true;
+
+  // Shutdown the chromoting host asynchronously. This will signal the host to
+  // shutdown, we'll actually wait for all threads to stop when we destroy
+  // the chromoting context.
+  chromoting_host_->Shutdown();
+  chromoting_host_ = NULL;
+  return true;
+}
+
+// A util function to update the login information to host config.
+static void SaveChromotingConfigFunc(remoting::JsonHostConfig* config,
+                                     const std::string& login,
+                                     const std::string& token,
+                                     const std::string& host_id,
+                                     const std::string& host_name,
+                                     const std::string& private_key) {
+  config->SetString(remoting::kXmppLoginConfigPath, login);
+  config->SetString(remoting::kXmppAuthTokenConfigPath, token);
+  config->SetString(remoting::kHostIdConfigPath, host_id);
+  config->SetString(remoting::kHostNameConfigPath, host_name);
+  config->SetString(remoting::kPrivateKeyConfigPath, private_key);
+}
+
+void ServiceProcess::SaveChromotingConfig(const std::string& login,
+                                          const std::string& token,
+                                          const std::string& host_id,
+                                          const std::string& host_name,
+                                          const std::string& private_key) {
+  // First we need to load the config first.
+  LoadChromotingConfig();
+
+  // And then do the update.
+  chromoting_config_->Update(
+      NewRunnableFunction(&SaveChromotingConfigFunc, chromoting_config_.get(),
+                          login, token, host_id, host_name, private_key));
+}
+
+void ServiceProcess::LoadChromotingConfig() {
+  // TODO(hclam): We really should be doing this on IO thread so we are not
+  // blocked on file IOs.
+  if (chromoting_config_)
+    return;
+
+  FilePath user_data_dir;
+  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
+  FilePath chromoting_config_path =
+      user_data_dir.Append(FILE_PATH_LITERAL(".ChromotingConfig.json"));
+  chromoting_config_ = new remoting::JsonHostConfig(
+      chromoting_config_path, file_thread_->message_loop_proxy());
+  if (!chromoting_config_->Read()) {
+    LOG(INFO) << "Failed to read chromoting config file.";
+  }
+}
+
+void ServiceProcess::OnChromotingHostShutdown() {
+  // TODO(hclam): Implement.
 }
 #endif
 
@@ -127,3 +222,7 @@ ServiceProcess::~ServiceProcess() {
   Teardown();
   g_service_process = NULL;
 }
+
+// Disable refcounting for runnable method because it is really not needed
+// when we post tasks on the main message loop.
+DISABLE_RUNNABLE_METHOD_REFCOUNT(ServiceProcess);
diff --git a/chrome/service/service_process.h b/chrome/service/service_process.h
index ce476c6..d4babce 100644
--- a/chrome/service/service_process.h
+++ b/chrome/service/service_process.h
@@ -6,6 +6,7 @@
 #define CHROME_SERVICE_SERVICE_PROCESS_H_
 #pragma once
 
+#include "base/gtest_prod_util.h"
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "base/thread.h"
@@ -21,7 +22,7 @@ class NetworkChangeNotifier;
 namespace remoting {
 class ChromotingHost;
 class ChromotingHostContext;
-class MutableHostConfig;
+class JsonHostConfig;
 }
 
 // The ServiceProcess does not inherit from ChildProcess because this
@@ -31,7 +32,8 @@ class ServiceProcess {
   ServiceProcess();
   ~ServiceProcess();
 
-  bool Initialize();
+  // Initialize the ServiceProcess with the message loop that it should run on.
+  bool Initialize(MessageLoop* message_loop);
   bool Teardown();
   // TODO(sanjeevr): Change various parts of the code such as
   // net::ProxyService::CreateSystemProxyConfigService to take in
@@ -64,26 +66,62 @@ class ServiceProcess {
   }
 
   CloudPrintProxy* GetCloudPrintProxy();
+
 #if defined(ENABLE_REMOTING)
-  remoting::ChromotingHost* CreateChromotingHost(
-      remoting::ChromotingHostContext* context,
-      remoting::MutableHostConfig* config);
+  // Return the reference to the chromoting host only if it has started.
+  remoting::ChromotingHost* GetChromotingHost() { return chromoting_host_; }
+
+  // Start running the chromoting host asynchronously.
+  // Return true if chromoting host has started.
+  bool StartChromotingHost();
+
+  // Shutdown chromoting host. Return true if chromoting host was shutdown.
+  // The shutdown process will happen asynchronously.
+  bool ShutdownChromotingHost();
 #endif
 
  private:
+#if defined(ENABLE_REMOTING)
+  FRIEND_TEST_ALL_PREFIXES(ServiceProcessTest, RunChromoting);
+  FRIEND_TEST_ALL_PREFIXES(ServiceProcessTest, RunChromotingUntilShutdown);
+
+  // Save authenication token to the json config file.
+  void SaveChromotingConfig(
+      const std::string& login,
+      const std::string& token,
+      const std::string& host_id,
+      const std::string& host_name,
+      const std::string& private_key);
+
+  // Load settings for chromoting from json file.
+  void LoadChromotingConfig();
+
+  // This method is called when chromoting is shutting down. This is virtual
+  // for used in the test.
+  virtual void OnChromotingHostShutdown();
+#endif
+
   scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
   scoped_ptr<base::Thread> io_thread_;
   scoped_ptr<base::Thread> file_thread_;
   scoped_ptr<CloudPrintProxy> cloud_print_proxy_;
   scoped_ptr<JsonPrefStore> service_prefs_;
   scoped_ptr<ServiceIPCServer> ipc_server_;
+
+#if defined(ENABLE_REMOTING)
+  scoped_refptr<remoting::JsonHostConfig> chromoting_config_;
+  scoped_ptr<remoting::ChromotingHostContext> chromoting_context_;
+  scoped_refptr<remoting::ChromotingHost> chromoting_host_;
+#endif
   // An event that will be signalled when we shutdown.
   base::WaitableEvent shutdown_event_;
 
+  // The main message loop for the service process.
+  MessageLoop* main_message_loop_;
+
   DISALLOW_COPY_AND_ASSIGN(ServiceProcess);
 };
 
 extern ServiceProcess* g_service_process;
 
 #endif  // CHROME_SERVICE_SERVICE_PROCESS_H_
-
diff --git a/chrome/service/service_process_unittest.cc b/chrome/service/service_process_unittest.cc
index b3ba608..e72a1e4 100644
--- a/chrome/service/service_process_unittest.cc
+++ b/chrome/service/service_process_unittest.cc
@@ -2,8 +2,75 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <vector>
+
+#include "base/base64.h"
+#include "base/crypto/rsa_private_key.h"
+#include "base/message_loop.h"
+#include "base/waitable_event.h"
+#include "chrome/service/service_process.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 TEST(ServiceProcessTest, Run) {
-  // TODO(hclam): Implement this test.
+  MessageLoopForUI main_message_loop;
+  ServiceProcess process;
+  EXPECT_TRUE(process.Initialize(&main_message_loop));
+  EXPECT_TRUE(process.Teardown());
+}
+
+#if defined(ENABLE_REMOTING)
+// TODO(hclam): This method should be made into a util.
+static std::string GeneratePrivateKey() {
+  scoped_ptr<base::RSAPrivateKey> key(base::RSAPrivateKey::Create(2048));
+
+  std::vector<uint8> private_key_buf;
+  key->ExportPrivateKey(&private_key_buf);
+  std::string private_key_str(private_key_buf.begin(), private_key_buf.end());
+  std::string private_key_base64;
+  base::Base64Encode(private_key_str, &private_key_base64);
+  return private_key_base64;
+}
+
+// This test seems to break randomly so disabling it.
+TEST(ServiceProcessTest, DISABLED_RunChromoting) {
+  MessageLoopForUI main_message_loop;
+  ServiceProcess process;
+  EXPECT_TRUE(process.Initialize(&main_message_loop));
+
+  // Then config the chromoting host and start it.
+  process.SaveChromotingConfig("hello", "world", "it's a", "good day",
+                               GeneratePrivateKey());
+  EXPECT_TRUE(process.StartChromotingHost());
+  EXPECT_TRUE(process.ShutdownChromotingHost());
+  EXPECT_TRUE(process.Teardown());
+}
+
+class MockServiceProcess : public ServiceProcess {
+ private:
+  FRIEND_TEST_ALL_PREFIXES(ServiceProcessTest, RunChromotingUntilShutdown);
+  MOCK_METHOD0(OnChromotingHostShutdown, void());
+};
+
+ACTION_P(QuitMessageLoop, message_loop) {
+  message_loop->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+}
+
+TEST(ServiceProcessTest, DISABLED_RunChromotingUntilShutdown) {
+  MessageLoopForUI main_message_loop;
+  MockServiceProcess process;
+  EXPECT_TRUE(process.Initialize(&main_message_loop));
+
+  // Expect chromoting shutdown be called because the login token is invalid.
+  EXPECT_CALL(process, OnChromotingHostShutdown())
+      .WillOnce(QuitMessageLoop(&main_message_loop));
+
+  // Then config the chromoting host and start it.
+  process.SaveChromotingConfig("hello", "world", "it's a", "good day",
+                               GeneratePrivateKey());
+  EXPECT_TRUE(process.StartChromotingHost());
+  MessageLoop::current()->Run();
+
+  EXPECT_TRUE(process.Teardown());
 }
+#endif  // defined(ENABLE_REMOTING)
diff --git a/remoting/host/heartbeat_sender.cc b/remoting/host/heartbeat_sender.cc
index b62b268a..e6a78f7 100644
--- a/remoting/host/heartbeat_sender.cc
+++ b/remoting/host/heartbeat_sender.cc
@@ -65,7 +65,7 @@ void HeartbeatSender::Start() {
     return;
   }
 
-  DCHECK(state_ == INITIALIZED);
+  DCHECK_EQ(INITIALIZED, state_);
   state_ = STARTED;
 
   request_.reset(jingle_client_->CreateIqRequest());
@@ -82,7 +82,9 @@ void HeartbeatSender::Stop() {
     return;
   }
 
-  DCHECK(state_ == STARTED);
+  // We may call Stop() even if we have not started.
+  if (state_ != STARTED)
+    return;
   state_ = STOPPED;
   request_.reset(NULL);
 }
diff --git a/remoting/jingle_glue/jingle_client.cc b/remoting/jingle_glue/jingle_client.cc
index eb89e21..b3ea334 100644
--- a/remoting/jingle_glue/jingle_client.cc
+++ b/remoting/jingle_glue/jingle_client.cc
@@ -76,6 +76,11 @@ void JingleClient::DoConnect(scoped_refptr<JingleChannel> channel,
 }
 
 void JingleClient::Close() {
+  // Once we are closed we really shouldn't talk to the callback again. In the
+  // case when JingleClient outlives the owner access the callback is not safe.
+  // TODO(hclam): We need to lock to reset callback.
+  callback_ = NULL;
+
   message_loop()->PostTask(
       FROM_HERE, NewRunnableMethod(this, &JingleClient::DoClose));
 }
@@ -85,12 +90,14 @@ void JingleClient::DoClose() {
 
   // If we have not yet initialized and the client is already closed then
   // don't close again.
-  if (!callback_ || state_ == CLOSED)
+  if (state_ == CLOSED)
     return;
 
-  client_->Disconnect();
-  // Client is deleted by TaskRunner.
-  client_ = NULL;
+  if (client_) {
+    client_->Disconnect();
+    // Client is deleted by TaskRunner.
+    client_ = NULL;
+  }
   tunnel_session_client_.reset();
   port_allocator_.reset();
   session_manager_.reset();
diff --git a/remoting/remoting.gyp b/remoting/remoting.gyp
index 51afbbf..b1c6e1a 100644
--- a/remoting/remoting.gyp
+++ b/remoting/remoting.gyp
@@ -172,6 +172,8 @@
       'sources': [
         'host/capturer.cc',
         'host/capturer.h',
+        'host/capturer_fake.cc',
+        'host/capturer_fake.h',
         'host/chromoting_host.cc',
         'host/chromoting_host.h',
         'host/chromoting_host_context.cc',
@@ -261,8 +263,6 @@
         '../base/base.gyp:base_i18n',
       ],
       'sources': [
-        'host/capturer_fake.cc',
-        'host/capturer_fake.h',
         'host/capturer_fake_ascii.cc',
         'host/capturer_fake_ascii.h',
         'host/simple_host_process.cc',