From c099813e194006d7ba33b71d7f62cc782a36d6a6 Mon Sep 17 00:00:00 2001 From: sergeyu Date: Mon, 2 Feb 2015 10:01:26 -0800 Subject: Add blocking IO restriction on the network thread. Now ChromotingHostContext disallows blocking operations on then network thread. This will help to avoid issues such as the one that was fixed in https://codereview.chromium.org/886913002/ (Network thread was used to load policies from disk). Also made the following changes to avoid blocking operation on the network thread: - Moved PolicyWatcher to the main thread in the Me2Me host. PolicyLoader uses blocking IO for initialization. Also this is more consistent with the It2Me host where PolicyWatcher also runs on the UI thread. - Moved NetworkChangeNotifier initialization out of HostSignalingManager. NetworkChangeNotifier creates a thread internally and therefore should be destroyed on the main thread. - Added a reference to URLRequestContextGetter in HostProcessMain to make sure it's destroyed on the main thread. This is necessary because BasicURLRequestContext owns threads and needs to be deleted on the main thread. Review URL: https://codereview.chromium.org/891663005 Cr-Commit-Position: refs/heads/master@{#314165} --- remoting/host/chromoting_host_context.cc | 13 +++++ remoting/host/host_signaling_manager.cc | 11 +--- remoting/host/host_signaling_manager.h | 12 ++--- remoting/host/remoting_me2me_host.cc | 89 +++++++++++++++++++++++--------- 4 files changed, 84 insertions(+), 41 deletions(-) diff --git a/remoting/host/chromoting_host_context.cc b/remoting/host/chromoting_host_context.cc index 60e3a73..319cc77 100644 --- a/remoting/host/chromoting_host_context.cc +++ b/remoting/host/chromoting_host_context.cc @@ -4,12 +4,22 @@ #include "remoting/host/chromoting_host_context.h" +#include "base/threading/thread_restrictions.h" #include "content/public/browser/browser_thread.h" #include "remoting/base/auto_thread.h" #include "remoting/base/url_request_context_getter.h" namespace remoting { +namespace { + +void DisallowBlockingOperations() { + base::ThreadRestrictions::SetIOAllowed(false); + base::ThreadRestrictions::DisallowWaiting(); +} + +} // namespace + ChromotingHostContext::ChromotingHostContext( scoped_refptr ui_task_runner, scoped_refptr audio_task_runner, @@ -96,9 +106,12 @@ scoped_ptr ChromotingHostContext::Create( scoped_refptr file_task_runner = AutoThread::CreateWithType("ChromotingFileThread", ui_task_runner, base::MessageLoop::TYPE_IO); + scoped_refptr network_task_runner = AutoThread::CreateWithType("ChromotingNetworkThread", ui_task_runner, base::MessageLoop::TYPE_IO); + network_task_runner->PostTask(FROM_HERE, + base::Bind(&DisallowBlockingOperations)); return make_scoped_ptr(new ChromotingHostContext( ui_task_runner, diff --git a/remoting/host/host_signaling_manager.cc b/remoting/host/host_signaling_manager.cc index 37da304..20abbd1 100644 --- a/remoting/host/host_signaling_manager.cc +++ b/remoting/host/host_signaling_manager.cc @@ -6,7 +6,6 @@ #include "base/bind.h" #include "base/time/time.h" -#include "net/base/network_change_notifier.h" #include "net/socket/client_socket_factory.h" #include "remoting/base/auto_thread_task_runner.h" #include "remoting/base/logging.h" @@ -22,13 +21,11 @@ namespace remoting { HostSignalingManager::HostSignalingManager( scoped_ptr> weak_factory_for_listener, const scoped_refptr& network_task_runner, - scoped_ptr network_change_notifier, scoped_ptr signal_strategy, scoped_ptr signaling_connector, scoped_ptr heartbeat_sender) : weak_factory_for_listener_(weak_factory_for_listener.Pass()), network_task_runner_(network_task_runner), - network_change_notifier_(network_change_notifier.Pass()), signal_strategy_(signal_strategy.Pass()), signaling_connector_(signaling_connector.Pass()), heartbeat_sender_(heartbeat_sender.Pass()) { @@ -50,9 +47,6 @@ scoped_ptr HostSignalingManager::Create( scoped_ptr> weak_factory( new base::WeakPtrFactory(listener)); - scoped_ptr network_change_notifier( - net::NetworkChangeNotifier::Create()); - scoped_ptr signal_strategy( new XmppSignalStrategy(net::ClientSocketFactory::GetDefaultFactory(), url_request_context_getter, xmpp_server_config)); @@ -77,9 +71,8 @@ scoped_ptr HostSignalingManager::Create( host_id, signal_strategy.get(), host_key_pair, directory_bot_jid)); return scoped_ptr(new HostSignalingManager( - weak_factory.Pass(), network_task_runner, network_change_notifier.Pass(), - signal_strategy.Pass(), signaling_connector.Pass(), - heartbeat_sender.Pass())); + weak_factory.Pass(), network_task_runner, signal_strategy.Pass(), + signaling_connector.Pass(), heartbeat_sender.Pass())); } HostSignalingManager::~HostSignalingManager() { diff --git a/remoting/host/host_signaling_manager.h b/remoting/host/host_signaling_manager.h index 1004dd8..22f0567 100644 --- a/remoting/host/host_signaling_manager.h +++ b/remoting/host/host_signaling_manager.h @@ -90,7 +90,6 @@ class HostSignalingManager { HostSignalingManager( scoped_ptr> weak_factory_for_listener, const scoped_refptr& network_task_runner, - scoped_ptr network_change_notifier, scoped_ptr signal_strategy, scoped_ptr signaling_connector, scoped_ptr heartbeat_sender); @@ -109,14 +108,9 @@ class HostSignalingManager { // host-offline-reason. scoped_refptr network_task_runner_; - // Order of fields below is important for destroying them in the right order. - // - |heartbeat_sender_| and |signaling_connector_| have to be destroyed - // before |signal_strategy_| because their destructors need to call - // signal_strategy_->RemoveListener(this) - // - |signaling_connector_| has to be destroyed before - // |network_change_notifier_| because its destructor needs to deregister - // network change notifications - scoped_ptr network_change_notifier_; + // |heartbeat_sender_| and |signaling_connector_| have to be destroyed before + // |signal_strategy_| because their destructors need to call + // signal_strategy_->RemoveListener(this) scoped_ptr signal_strategy_; scoped_ptr signaling_connector_; scoped_ptr heartbeat_sender_; diff --git a/remoting/host/remoting_me2me_host.cc b/remoting/host/remoting_me2me_host.cc index aa0bf6f..291aba6 100644 --- a/remoting/host/remoting_me2me_host.cc +++ b/remoting/host/remoting_me2me_host.cc @@ -26,6 +26,7 @@ #include "ipc/ipc_channel_proxy.h" #include "ipc/ipc_listener.h" #include "media/base/media.h" +#include "net/base/network_change_notifier.h" #include "net/socket/client_socket_factory.h" #include "net/socket/ssl_server_socket.h" #include "net/url_request/url_fetcher.h" @@ -259,6 +260,7 @@ class HostProcess : public ConfigWatcher::Delegate, scoped_ptr CreateHostSignalingManager(); + void StartHostIfReady(); void StartHost(); // Overrides for HostSignalingManager::Listener interface. @@ -323,6 +325,7 @@ class HostProcess : public ConfigWatcher::Delegate, int64_t frame_recorder_buffer_size_; scoped_ptr policy_watcher_; + bool policies_loaded_; std::string host_domain_; bool host_username_match_required_; bool allow_nat_traversal_; @@ -336,7 +339,7 @@ class HostProcess : public ConfigWatcher::Delegate, ThirdPartyAuthConfig third_party_auth_config_; bool enable_gnubby_auth_; - // Boolean to change flow, where ncessary, if we're + // Boolean to change flow, where necessary, if we're // capturing a window instead of the entire desktop. bool enable_window_capture_; @@ -378,6 +381,7 @@ HostProcess::HostProcess(scoped_ptr context, use_service_account_(false), enable_vp9_(false), frame_recorder_buffer_size_(0), + policies_loaded_(false), host_username_match_required_(false), allow_nat_traversal_(true), allow_relay_(true), @@ -548,26 +552,18 @@ void HostProcess::OnConfigUpdated( } if (state_ == HOST_INITIALIZING) { - // TODO(sergeyu): Currently OnPolicyUpdate() assumes that host config is - // already loaded so PolicyWatcher has to be started here. Separate policy - // loading from policy verifications and move |policy_watcher_| - // initialization to StartOnNetworkThread(). - policy_watcher_ = - PolicyWatcher::Create(nullptr, context_->file_task_runner()); - policy_watcher_->StartWatching( - base::Bind(&HostProcess::OnPolicyUpdate, base::Unretained(this)), - base::Bind(&HostProcess::OnPolicyError, base::Unretained(this))); - } else { + StartHostIfReady(); + } else if (state_ == HOST_STARTED) { + DCHECK(policies_loaded_); + // Reapply policies that could be affected by a new config. ApplyHostDomainPolicy(); ApplyUsernamePolicy(); - if (state_ == HOST_STARTED) { - // TODO(sergeyu): Here we assume that PIN is the only part of the config - // that may change while the service is running. Change ApplyConfig() to - // detect other changes in the config and restart host if necessary here. - CreateAuthenticatorFactory(); - } + // TODO(sergeyu): Here we assume that PIN is the only part of the config + // that may change while the service is running. Change ApplyConfig() to + // detect other changes in the config and restart host if necessary here. + CreateAuthenticatorFactory(); } } @@ -724,6 +720,12 @@ void HostProcess::StartOnUiThread() { return; } + policy_watcher_ = + PolicyWatcher::Create(nullptr, context_->file_task_runner()); + policy_watcher_->StartWatching( + base::Bind(&HostProcess::OnPolicyUpdate, base::Unretained(this)), + base::Bind(&HostProcess::OnPolicyError, base::Unretained(this))); + #if defined(OS_LINUX) // If an audio pipe is specific on the command-line then initialize // AudioCapturerLinux to capture from it. @@ -784,6 +786,8 @@ void HostProcess::ShutdownOnUiThread() { daemon_channel_.reset(); desktop_environment_factory_.reset(); + policy_watcher_.reset(); + // It is now safe for the HostProcess to be deleted. self_ = nullptr; @@ -956,7 +960,12 @@ bool HostProcess::ApplyConfig(const base::DictionaryValue& config) { } void HostProcess::OnPolicyUpdate(scoped_ptr policies) { - DCHECK(context_->network_task_runner()->BelongsToCurrentThread()); + if (!context_->network_task_runner()->BelongsToCurrentThread()) { + context_->network_task_runner()->PostTask( + FROM_HERE, base::Bind(&HostProcess::OnPolicyUpdate, this, + base::Passed(&policies))); + return; + } bool restart_required = false; restart_required |= OnHostDomainPolicyUpdate(policies.get()); @@ -971,20 +980,30 @@ void HostProcess::OnPolicyUpdate(scoped_ptr policies) { restart_required |= OnPairingPolicyUpdate(policies.get()); restart_required |= OnGnubbyAuthPolicyUpdate(policies.get()); + policies_loaded_ = true; + if (state_ == HOST_INITIALIZING) { - StartHost(); - } else if (state_ == HOST_STARTED && restart_required) { - RestartHost(); + StartHostIfReady(); + } else if (state_ == HOST_STARTED) { + if (restart_required) + RestartHost(); } } void HostProcess::OnPolicyError() { - DCHECK(context_->network_task_runner()->BelongsToCurrentThread()); + if (!context_->network_task_runner()->BelongsToCurrentThread()) { + context_->network_task_runner()->PostTask( + FROM_HERE, base::Bind(&HostProcess::OnPolicyError, this)); + return; + } ShutdownHost(kInvalidHostConfigurationExitCode); } void HostProcess::ApplyHostDomainPolicy() { + if (state_ != HOST_STARTED) + return; + HOST_LOG << "Policy sets host domain: " << host_domain_; if (!host_domain_.empty()) { @@ -1019,6 +1038,9 @@ bool HostProcess::OnHostDomainPolicyUpdate(base::DictionaryValue* policies) { } void HostProcess::ApplyUsernamePolicy() { + if (state_ != HOST_STARTED) + return; + if (host_username_match_required_) { HOST_LOG << "Policy requires host username match."; @@ -1284,6 +1306,15 @@ scoped_ptr HostProcess::CreateHostSignalingManager() { oauth_credentials.Pass()); } +void HostProcess::StartHostIfReady() { + DCHECK(context_->network_task_runner()->BelongsToCurrentThread()); + DCHECK_EQ(state_, HOST_INITIALIZING); + + // Start the host if both the config and the policies are loaded. + if (!serialized_config_.empty() && policies_loaded_) + StartHost(); +} + void HostProcess::StartHost() { DCHECK(context_->network_task_runner()->BelongsToCurrentThread()); DCHECK(!host_); @@ -1368,6 +1399,9 @@ void HostProcess::StartHost() { host_->Start(host_owner_email_); CreateAuthenticatorFactory(); + + ApplyHostDomainPolicy(); + ApplyUsernamePolicy(); } void HostProcess::OnAuthFailed() { @@ -1439,7 +1473,6 @@ void HostProcess::ShutdownOnNetworkThread() { shutdown_watchdog_->Arm(); config_watcher_.reset(); - policy_watcher_.reset(); // Complete the rest of shutdown on the main thread. context_->ui_task_runner()->PostTask( @@ -1481,6 +1514,9 @@ int HostProcessMain() { // Ensures runtime specific CPU features are initialized. media::InitializeCPUSpecificMediaFeatures(); + scoped_ptr network_change_notifier( + net::NetworkChangeNotifier::Create()); + // Create the main message loop and start helper threads. base::MessageLoopForUI message_loop; scoped_ptr context = @@ -1489,6 +1525,13 @@ int HostProcessMain() { if (!context) return kInitializationFailed; + // BasicURLRequestContext holds references to threads, so it needs to be + // dereferences on UI threads. Store the reference to the URLRequestGetter to + // make sure it's not destroyed on other threads. + // TODO(sergeyu): Consider fixing it in BasicURLRequestContext. + scoped_refptr url_request_context_getter = + context->url_request_context_getter(); + // Create & start the HostProcess using these threads. // TODO(wez): The HostProcess holds a reference to itself until Shutdown(). // Remove this hack as part of the multi-process refactoring. -- cgit v1.1