Add blocking IO restriction on the network thread.

remoting/host/remoting_me2me_host.cc

 // 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.
 //
 // This file implements a standalone host process for Me2Me.
 
 #include <string>
 
 #include "base/at_exit.h"
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/debug/alias.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread.h"
 #include "build/build_config.h"
 #include "crypto/nss_util.h"
 #include "ipc/ipc_channel.h"
 #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"
 #include "policy/policy_constants.h"
 #include "remoting/base/auto_thread_task_runner.h"
 #include "remoting/base/breakpad.h"
 #include "remoting/base/constants.h"
 #include "remoting/base/logging.h"
 #include "remoting/base/rsa_key_pair.h"
 #include "remoting/base/service_urls.h"
@@ skipping 277 matching lines @@
   scoped_refptr<RsaKeyPair> key_pair_;
   std::string oauth_refresh_token_;
   std::string serialized_config_;
   std::string host_owner_;
   std::string host_owner_email_;
   bool use_service_account_;
   bool enable_vp9_;
   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_;
   bool allow_relay_;
   uint16 min_udp_port_;
   uint16 max_udp_port_;
   std::string talkgadget_prefix_;
   bool allow_pairing_;
 
   bool curtain_required_;
   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_;
 
   // Used to specify which window to stream, if enabled.
   webrtc::WindowId window_id_;
 
   // Used to send heartbeats while running, and the reason for going offline
   // when shutting down.
   scoped_ptr<HostSignalingManager> host_signaling_manager_;
 
@@ skipping 21 matching lines @@
 };
 
 HostProcess::HostProcess(scoped_ptr<ChromotingHostContext> context,
                          int* exit_code_out,
                          ShutdownWatchdog* shutdown_watchdog)
     : context_(context.Pass()),
       state_(HOST_INITIALIZING),
       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),
       min_udp_port_(0),
       max_udp_port_(0),
       allow_pairing_(true),
       curtain_required_(false),
       enable_gnubby_auth_(false),
       enable_window_capture_(false),
       window_id_(0),
@@ skipping 149 matching lines @@
     ShutdownHost(kInvalidHostConfigurationExitCode);
     return;
   }
 
   if (!ApplyConfig(*config)) {
     LOG(ERROR) << "Failed to apply the configuration.";
     ShutdownHost(kInvalidHostConfigurationExitCode);
     return;
   }
 
+  if (!policies_loaded_)

[Łukasz Anforowicz, 2015/01/31 00:02:34]

The early return is not needed - this can be made more similar to
OnPolicyUpdated which does a similar "if" before calling StartHost:

if (state_ == HOST_INITIALIZING) {
  if (policies_loaded_) {
    StartHost();
  }
} else if (state == HOST_STARTED) {
  DCHECK(policies_loaded_);
  ...
}

[Sergey Ulanov, 2015/01/31 00:34:26]

Done.

+    return;
+
   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 {
+     StartHost();

[Łukasz Anforowicz, 2015/01/31 00:02:34]

It seems that StartHost can only proceed if
1) policies_loaded_
and
2) !serialized_config_.empty()
Would it make sense to check these 2 conditions *inside* StartHost method,
rather than in OnConfigUpdated and OnPolicyUpdated methods?

[Sergey Ulanov, 2015/01/31 00:34:26]

Added StartHostIfReady() that checks these two conditions and call StartHost();

StartHost() is called in other places, particularly when restarting.

+  } else if (state_ == HOST_STARTED) {
     // Reapply policies that could be affected by a new config.
     ApplyHostDomainPolicy();

[Łukasz Anforowicz, 2015/01/31 00:02:34]

Would it make sense to add DCHECK(policy_loaded_) inside ApplyHostDomainPolicy
method (and other ApplyXxx methods)?

If not, then maybe a DCHECK here would help understand the code (because when
state_ == HOST_STARTED, then we have to have policies_loaded_, right?).

[Sergey Ulanov, 2015/01/31 00:34:26]

Added DCHECK here. I don't think we need a DCHECK in Apply*(). 

     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();
   }
 }
 
 void HostProcess::OnConfigWatcherError() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
   ShutdownHost(kInvalidHostConfigurationExitCode);
 }
 
 void HostProcess::StartOnNetworkThread() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
@@ skipping 136 matching lines @@
   DCHECK(context_->ui_task_runner()->BelongsToCurrentThread());
 
   if (!InitWithCommandLine(base::CommandLine::ForCurrentProcess())) {
     // Shutdown the host if the command line is invalid.
     context_->network_task_runner()->PostTask(
         FROM_HERE, base::Bind(&HostProcess::ShutdownHost, this,
                               kUsageExitCode));
     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.
   base::FilePath audio_pipe_name = base::CommandLine::ForCurrentProcess()->
       GetSwitchValuePath(kAudioPipeSwitchName);
   if (!audio_pipe_name.empty()) {
     remoting::AudioCapturerLinux::InitializePipeReader(
         context_->audio_task_runner(), audio_pipe_name);
   }
 
@@ skipping 40 matching lines @@
       base::Bind(&HostProcess::StartOnNetworkThread, this));
 }
 
 void HostProcess::ShutdownOnUiThread() {
   DCHECK(context_->ui_task_runner()->BelongsToCurrentThread());
 
   // Tear down resources that need to be torn down on the UI thread.
   daemon_channel_.reset();
   desktop_environment_factory_.reset();
 
+  policy_watcher_.reset();
+
   // It is now safe for the HostProcess to be deleted.
   self_ = nullptr;
 
 #if defined(OS_LINUX)
   // Cause the global AudioPipeReader to be freed, otherwise the audio
   // thread will remain in-use and prevent the process from exiting.
   // TODO(wez): DesktopEnvironmentFactory should own the pipe reader.
   // See crbug.com/161373 and crbug.com/104544.
   AudioCapturerLinux::InitializePipeReader(nullptr, base::FilePath());
 #endif
@@ skipping 152 matching lines @@
                       &frame_recorder_buffer_kb);
   }
   if (frame_recorder_buffer_kb > 0) {
     frame_recorder_buffer_size_ = 1024LL * frame_recorder_buffer_kb;
   }
 
   return true;
 }
 
 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());
   restart_required |= OnCurtainPolicyUpdate(policies.get());
   // Note: UsernamePolicyUpdate must run after OnCurtainPolicyUpdate.
   restart_required |= OnUsernamePolicyUpdate(policies.get());
   restart_required |= OnNatPolicyUpdate(policies.get());
   restart_required |= OnRelayPolicyUpdate(policies.get());
   restart_required |= OnUdpPortPolicyUpdate(policies.get());
   restart_required |= OnHostTalkGadgetPrefixPolicyUpdate(policies.get());
   restart_required |= OnHostTokenUrlPolicyUpdate(policies.get());
   restart_required |= OnPairingPolicyUpdate(policies.get());
   restart_required |= OnGnubbyAuthPolicyUpdate(policies.get());
 
+  policies_loaded_ = true;

[Łukasz Anforowicz, 2015/01/31 00:02:34]

If adding DCHECK (as I suggested elsewhere) to ApplyXxx methods, then
policies_loaded_ = true would have to be set slightly earlier, before calling
any of OnXxx from this method.

[Sergey Ulanov, 2015/01/31 00:34:26]

I'd like to avoid too many changes in how policies are handled in this CL.

+
   if (state_ == HOST_INITIALIZING) {
-    StartHost();
-  } else if (state_ == HOST_STARTED && restart_required) {
-    RestartHost();
+    if (!serialized_config_.empty())
+      StartHost();
+  } 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()) {
     // If the user does not have a Google email, their client JID will not be
     // based on their email. In that case, the username/host domain policies
     // would be meaningless, since there is no way to check that the JID
     // trying to connect actually corresponds to the owner email in question.
     if (host_owner_ != host_owner_email_) {
       LOG(ERROR) << "The username and host domain policies cannot be enabled "
                  << "for accounts with a non-Google email.";
@@ skipping 14 matching lines @@
   if (!policies->GetString(policy::key::kRemoteAccessHostDomain,
                            &host_domain_)) {
     return false;
   }
 
   ApplyHostDomainPolicy();
   return false;
 }
 
 void HostProcess::ApplyUsernamePolicy() {
+  if (state_ != HOST_STARTED)
+    return;
+
   if (host_username_match_required_) {
     HOST_LOG << "Policy requires host username match.";
 
     // See comment in ApplyHostDomainPolicy.
     if (host_owner_ != host_owner_email_) {
       LOG(ERROR) << "The username and host domain policies cannot be enabled "
                  << "for accounts with a non-Google email.";
       ShutdownHost(kUsernameMismatchExitCode);
     }
 
@@ skipping 329 matching lines @@
       new IpcHostEventLogger(host_->AsWeakPtr(), daemon_channel_.get()));
 #else  // !defined(REMOTING_MULTI_PROCESS)
   host_event_logger_ =
       HostEventLogger::Create(host_->AsWeakPtr(), kApplicationName);
 #endif  // !defined(REMOTING_MULTI_PROCESS)
 
   host_->SetEnableCurtaining(curtain_required_);
   host_->Start(host_owner_email_);
 
   CreateAuthenticatorFactory();
+
+  ApplyHostDomainPolicy();
+  ApplyUsernamePolicy();
 }
 
 void HostProcess::OnAuthFailed() {
   ShutdownHost(kInvalidOauthCredentialsExitCode);
 }
 
 void HostProcess::RestartHost() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
   DCHECK_EQ(state_, HOST_STARTED);
 
@@ skipping 51 matching lines @@
 
   if (state_ == HOST_STOPPING_TO_RESTART) {
     StartHost();
   } else if (state_ == HOST_STOPPING) {
     state_ = HOST_STOPPED;
 
     shutdown_watchdog_->SetExitCode(*exit_code_out_);
     shutdown_watchdog_->Arm();
 
     config_watcher_.reset();
-    policy_watcher_.reset();
 
     // Complete the rest of shutdown on the main thread.
     context_->ui_task_runner()->PostTask(
         FROM_HERE, base::Bind(&HostProcess::ShutdownOnUiThread, this));
   } else {
     // This method is only called in STOPPING_TO_RESTART and STOPPING states.
     NOTREACHED();
   }
 }
 
@@ skipping 21 matching lines @@
   gtk_init(nullptr, nullptr);
 #endif
 
   // Enable support for SSL server sockets, which must be done while still
   // single-threaded.
   net::EnableSSLServerSockets();
 
   // Ensures runtime specific CPU features are initialized.
   media::InitializeCPUSpecificMediaFeatures();
 
+  scoped_ptr<net::NetworkChangeNotifier> network_change_notifier;

[Łukasz Anforowicz, 2015/01/31 00:02:34]

I don't understand what this does - this will always be wrapping a nullptr /
this local variable is never assigned to.  Did you want to drop scoped_ptr and
just have a net::NetworkChangeNotifier value here?

[Sergey Ulanov, 2015/01/31 00:34:26]

Yes. Thank you.

+
   // Create the main message loop and start helper threads.
   base::MessageLoopForUI message_loop;
   scoped_ptr<ChromotingHostContext> context =
       ChromotingHostContext::Create(new AutoThreadTaskRunner(
           message_loop.message_loop_proxy(), base::MessageLoop::QuitClosure()));
   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

[Łukasz Anforowicz, 2015/01/31 00:02:34]

I don't understand :-(  I would understand if you said that threads managed by
BasicURLRequestContext need to be destroyed as late as possible / from UI
thread.  I don't understand why holding references to threads would mean having
to be dereferenced from UI threads.

[Sergey Ulanov, 2015/01/31 00:34:26]

Thread::Stop() is a blocking operation (because it joins the thread). So it now
DCHECKs if it runs on the network thread.

[Łukasz Anforowicz, 2015/01/31 00:41:35]

Acknowledged.

+  // 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.
   int exit_code = kSuccessExitCode;
   ShutdownWatchdog shutdown_watchdog(
       base::TimeDelta::FromSeconds(kShutdownTimeoutSeconds));
   new HostProcess(context.Pass(), &exit_code, &shutdown_watchdog);
 
   // Run the main (also UI) message loop until the host no longer needs it.
   message_loop.Run();
 
   return exit_code;
 }
 
 }  // namespace remoting