diff options
| author | sergeyu <sergeyu@chromium.org> | 2015-02-02 10:01:26 -0800 |
|---|---|---|
| committer | Commit bot <commit-bot@chromium.org> | 2015-02-02 18:02:19 +0000 |
| commit | c099813e194006d7ba33b71d7f62cc782a36d6a6 (patch) | |
| tree | e831a26c68b6824e27f64542ad5ff870106deba8 | |
| parent | fd0b94cd15337a91d167a3b931ac4252e1174042 (diff) | |
| download | chromium_src-c099813e194006d7ba33b71d7f62cc782a36d6a6.zip chromium_src-c099813e194006d7ba33b71d7f62cc782a36d6a6.tar.gz chromium_src-c099813e194006d7ba33b71d7f62cc782a36d6a6.tar.bz2 | |
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}
| -rw-r--r-- | remoting/host/chromoting_host_context.cc | 13 | ||||
| -rw-r--r-- | remoting/host/host_signaling_manager.cc | 11 | ||||
| -rw-r--r-- | remoting/host/host_signaling_manager.h | 12 | ||||
| -rw-r--r-- | 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<AutoThreadTaskRunner> ui_task_runner, scoped_refptr<AutoThreadTaskRunner> audio_task_runner, @@ -96,9 +106,12 @@ scoped_ptr<ChromotingHostContext> ChromotingHostContext::Create( scoped_refptr<AutoThreadTaskRunner> file_task_runner = AutoThread::CreateWithType("ChromotingFileThread", ui_task_runner, base::MessageLoop::TYPE_IO); + scoped_refptr<AutoThreadTaskRunner> 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<base::WeakPtrFactory<Listener>> weak_factory_for_listener, const scoped_refptr<AutoThreadTaskRunner>& network_task_runner, - scoped_ptr<net::NetworkChangeNotifier> network_change_notifier, scoped_ptr<SignalStrategy> signal_strategy, scoped_ptr<SignalingConnector> signaling_connector, scoped_ptr<HeartbeatSender> 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> HostSignalingManager::Create( scoped_ptr<base::WeakPtrFactory<Listener>> weak_factory( new base::WeakPtrFactory<Listener>(listener)); - scoped_ptr<net::NetworkChangeNotifier> network_change_notifier( - net::NetworkChangeNotifier::Create()); - scoped_ptr<XmppSignalStrategy> signal_strategy( new XmppSignalStrategy(net::ClientSocketFactory::GetDefaultFactory(), url_request_context_getter, xmpp_server_config)); @@ -77,9 +71,8 @@ scoped_ptr<HostSignalingManager> HostSignalingManager::Create( host_id, signal_strategy.get(), host_key_pair, directory_bot_jid)); return scoped_ptr<HostSignalingManager>(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<base::WeakPtrFactory<Listener>> weak_factory_for_listener, const scoped_refptr<AutoThreadTaskRunner>& network_task_runner, - scoped_ptr<net::NetworkChangeNotifier> network_change_notifier, scoped_ptr<SignalStrategy> signal_strategy, scoped_ptr<SignalingConnector> signaling_connector, scoped_ptr<HeartbeatSender> heartbeat_sender); @@ -109,14 +108,9 @@ class HostSignalingManager { // host-offline-reason. scoped_refptr<AutoThreadTaskRunner> 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<net::NetworkChangeNotifier> 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<SignalStrategy> signal_strategy_; scoped_ptr<SignalingConnector> signaling_connector_; scoped_ptr<HeartbeatSender> 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<HostSignalingManager> 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<PolicyWatcher> 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<ChromotingHostContext> 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<base::DictionaryValue> 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<base::DictionaryValue> 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<HostSignalingManager> 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<net::NetworkChangeNotifier> network_change_notifier( + net::NetworkChangeNotifier::Create()); + // Create the main message loop and start helper threads. base::MessageLoopForUI message_loop; scoped_ptr<ChromotingHostContext> 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<net::URLRequestContextGetter> 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. |