From 71eb23d453937c320f1c7a3f46c1aae335df6269 Mon Sep 17 00:00:00 2001 From: "glen@chromium.org" Date: Tue, 10 Aug 2010 02:46:54 +0000 Subject: Revert 55507 - 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/3084024 TBR=hclam@chromium.org Review URL: http://codereview.chromium.org/3110004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@55525 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/service/service_main.cc | 58 ++++++++++++- chrome/service/service_process.cc | 125 +++-------------------------- chrome/service/service_process.h | 50 ++---------- chrome/service/service_process_unittest.cc | 70 +--------------- remoting/host/heartbeat_sender.cc | 6 +- remoting/jingle_glue/jingle_client.cc | 15 +--- remoting/remoting.gyp | 4 +- 7 files changed, 85 insertions(+), 243 deletions(-) diff --git a/chrome/service/service_main.cc b/chrome/service/service_main.cc index 2670c5c..8994c12 100644 --- a/chrome/service/service_main.cc +++ b/chrome/service/service_main.cc @@ -11,6 +11,22 @@ #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; @@ -24,7 +40,7 @@ int ServiceProcessMain(const MainFunctionParams& parameters) { #endif // defined(OS_WIN) ServiceProcess service_process; - service_process.Initialize(&main_message_loop); + service_process.Initialize(); // Enable Cloud Print if needed. if (parameters.command_line_.HasSwitch(switches::kEnableCloudPrintProxy)) { @@ -34,7 +50,47 @@ 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 chromoting_config; + scoped_ptr chromoting_context; + scoped_refptr 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 fbd9c8d..60229d6 100644 --- a/chrome/service/service_process.cc +++ b/chrome/service/service_process.cc @@ -16,17 +16,16 @@ #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/json_host_config.h" +#include "remoting/host/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_fake.h" +#include "remoting/host/capturer_linux.h" #include "remoting/host/event_executor_linux.h" #elif defined(OS_MACOSX) #include "remoting/host/capturer_mac.h" @@ -36,15 +35,12 @@ ServiceProcess* g_service_process = NULL; -ServiceProcess::ServiceProcess() - : shutdown_event_(true, false), - main_message_loop_(NULL) { +ServiceProcess::ServiceProcess() : shutdown_event_(true, false) { DCHECK(!g_service_process); g_service_process = this; } -bool ServiceProcess::Initialize(MessageLoop* message_loop) { - main_message_loop_ = message_loop; +bool ServiceProcess::Initialize() { network_change_notifier_.reset(net::NetworkChangeNotifier::Create()); base::Thread::Options options; options.message_loop_type = MessageLoop::TYPE_IO; @@ -78,16 +74,9 @@ bool ServiceProcess::Initialize(MessageLoop* message_loop) { } bool ServiceProcess::Teardown() { - if (service_prefs_.get()) { - service_prefs_->WritePrefs(); - service_prefs_.reset(); - } + 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. @@ -97,7 +86,6 @@ 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; } @@ -110,29 +98,19 @@ CloudPrintProxy* ServiceProcess::GetCloudPrintProxy() { } #if defined(ENABLE_REMOTING) -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. +remoting::ChromotingHost* ServiceProcess::CreateChromotingHost( + remoting::ChromotingHostContext* context, + remoting::MutableHostConfig* config) { scoped_ptr capturer; scoped_ptr encoder; scoped_ptr 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::CapturerFake()); + capturer.reset(new remoting::CapturerLinux()); executor.reset(new remoting::EventExecutorLinux()); #elif defined(OS_MACOSX) capturer.reset(new remoting::CapturerMac()); @@ -140,81 +118,8 @@ bool ServiceProcess::StartChromotingHost() { #endif encoder.reset(new remoting::EncoderZlib()); - // 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. + return new remoting::ChromotingHost(context, config, capturer.release(), + encoder.release(), executor.release()); } #endif @@ -222,7 +127,3 @@ 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 599fa00..ce476c6 100644 --- a/chrome/service/service_process.h +++ b/chrome/service/service_process.h @@ -6,7 +6,6 @@ #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" @@ -22,7 +21,7 @@ class NetworkChangeNotifier; namespace remoting { class ChromotingHost; class ChromotingHostContext; -class JsonHostConfig; +class MutableHostConfig; } // The ServiceProcess does not inherit from ChildProcess because this @@ -32,8 +31,7 @@ class ServiceProcess { ServiceProcess(); ~ServiceProcess(); - // Initialize the ServiceProcess with the message loop that it should run on. - bool Initialize(MessageLoop* message_loop); + bool Initialize(); bool Teardown(); // TODO(sanjeevr): Change various parts of the code such as // net::ProxyService::CreateSystemProxyConfigService to take in @@ -66,62 +64,26 @@ class ServiceProcess { } CloudPrintProxy* GetCloudPrintProxy(); - #if defined(ENABLE_REMOTING) - // 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(); + remoting::ChromotingHost* CreateChromotingHost( + remoting::ChromotingHostContext* context, + remoting::MutableHostConfig* config); #endif private: -#if defined(ENABLE_REMOTING) - FRIEND_TEST(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 network_change_notifier_; scoped_ptr io_thread_; scoped_ptr file_thread_; scoped_ptr cloud_print_proxy_; scoped_ptr service_prefs_; scoped_ptr ipc_server_; - -#if defined(ENABLE_REMOTING) - scoped_refptr chromoting_config_; - scoped_ptr chromoting_context_; - scoped_refptr 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 aeea3cb..b3ba608 100644 --- a/chrome/service/service_process_unittest.cc +++ b/chrome/service/service_process_unittest.cc @@ -2,76 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include - -#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) { - 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 key(base::RSAPrivateKey::Create(2048)); - - std::vector 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; -} - -TEST(ServiceProcessTest, 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()); -} - -// This test is disabled and should be moved to an offline test suite because -// it actually trys to make connect. It'll take very long to get a failure. -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()); + // TODO(hclam): Implement this test. } -#endif // defined(ENABLE_REMOTING) diff --git a/remoting/host/heartbeat_sender.cc b/remoting/host/heartbeat_sender.cc index e6a78f7..b62b268a 100644 --- a/remoting/host/heartbeat_sender.cc +++ b/remoting/host/heartbeat_sender.cc @@ -65,7 +65,7 @@ void HeartbeatSender::Start() { return; } - DCHECK_EQ(INITIALIZED, state_); + DCHECK(state_ == INITIALIZED); state_ = STARTED; request_.reset(jingle_client_->CreateIqRequest()); @@ -82,9 +82,7 @@ void HeartbeatSender::Stop() { return; } - // We may call Stop() even if we have not started. - if (state_ != STARTED) - return; + DCHECK(state_ == STARTED); state_ = STOPPED; request_.reset(NULL); } diff --git a/remoting/jingle_glue/jingle_client.cc b/remoting/jingle_glue/jingle_client.cc index b3ea334..eb89e21 100644 --- a/remoting/jingle_glue/jingle_client.cc +++ b/remoting/jingle_glue/jingle_client.cc @@ -76,11 +76,6 @@ void JingleClient::DoConnect(scoped_refptr 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)); } @@ -90,14 +85,12 @@ void JingleClient::DoClose() { // If we have not yet initialized and the client is already closed then // don't close again. - if (state_ == CLOSED) + if (!callback_ || state_ == CLOSED) return; - if (client_) { - client_->Disconnect(); - // Client is deleted by TaskRunner. - client_ = NULL; - } + 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 b1c6e1a..51afbbf 100644 --- a/remoting/remoting.gyp +++ b/remoting/remoting.gyp @@ -172,8 +172,6 @@ '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', @@ -263,6 +261,8 @@ '../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', -- cgit v1.1