Reporting of policy errors via host-offline-reason: part 3

remoting/host/remoting_me2me_host.cc

Index: remoting/host/remoting_me2me_host.cc
diff --git a/remoting/host/remoting_me2me_host.cc b/remoting/host/remoting_me2me_host.cc
index 2b80763a2386f4ca7914706c12c136cfa9f420a9..55d1aaea2fea89c12c35386f531cace5b4aa298a 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -14,6 +14,7 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
@@ -26,7 +27,6 @@
 #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"
@@ -45,13 +45,12 @@
 #include "remoting/host/config_watcher.h"
 #include "remoting/host/desktop_environment.h"
 #include "remoting/host/desktop_session_connector.h"
-#include "remoting/host/dns_blackhole_checker.h"
-#include "remoting/host/heartbeat_sender.h"
 #include "remoting/host/host_change_notification_listener.h"
 #include "remoting/host/host_config.h"
 #include "remoting/host/host_event_logger.h"
 #include "remoting/host/host_exit_codes.h"
 #include "remoting/host/host_main.h"
+#include "remoting/host/host_signaling_manager.h"
 #include "remoting/host/host_status_logger.h"
 #include "remoting/host/ipc_constants.h"
 #include "remoting/host/ipc_desktop_environment.h"
@@ -143,6 +142,10 @@ const char kWindowIdSwitchName[] = "window-id";
 // of the process.
 const int kShutdownTimeoutSeconds = 15;
 
+// Maximum time to wait for reporting host-offline-reason to the service,
+// before continuing normal process shutdown.
+const int kHostOfflineReasonTimeoutSeconds = 10;
+
 }  // namespace
 
 namespace remoting {
@@ -254,6 +257,8 @@ class HostProcess
   bool OnPairingPolicyUpdate(base::DictionaryValue* policies);
   bool OnGnubbyAuthPolicyUpdate(base::DictionaryValue* policies);
 
+  scoped_ptr<HostSignalingManager> CreateHostSignalingManager();
+
   void StartHost();
 
   void OnHeartbeatSuccessful();
@@ -266,7 +271,9 @@ class HostProcess
   // Stops the host and shuts down the process with the specified |exit_code|.
   void ShutdownHost(HostExitCodes exit_code);
 
-  void ScheduleHostShutdown();
+  // Private helper used by ShutdownHost method to initiate sending of
+  // host-offline-reason before continuing shutdown.
+  void SendOfflineReasonAndShutdownOnNetworkThread(HostExitCodes exit_code);
 
   void ShutdownOnNetworkThread();
 
@@ -289,9 +296,6 @@ class HostProcess
 
   scoped_ptr<ChromotingHostContext> context_;
 
-  // Created on the UI thread but used from the network thread.
-  scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
-
   // Accessed on the UI thread.
   scoped_ptr<IPC::ChannelProxy> daemon_channel_;
 
@@ -341,10 +345,10 @@ class HostProcess
   // Used to specify which window to stream, if enabled.
   webrtc::WindowId window_id_;
 
-  scoped_ptr<OAuthTokenGetter> oauth_token_getter_;
-  scoped_ptr<XmppSignalStrategy> signal_strategy_;
-  scoped_ptr<SignalingConnector> signaling_connector_;
-  scoped_ptr<HeartbeatSender> heartbeat_sender_;
+  // Used to send heartbeats while running, and the reason for going offline
+  // when shutting down.
+  scoped_ptr<HostSignalingManager> host_signaling_manager_;
+
   scoped_ptr<HostChangeNotificationListener> host_change_notification_listener_;
   scoped_ptr<HostStatusLogger> host_status_logger_;
   scoped_ptr<HostEventLogger> host_event_logger_;
@@ -364,6 +368,12 @@ class HostProcess
   scoped_refptr<PairingRegistry> pairing_registry_;
 
   ShutdownWatchdog* shutdown_watchdog_;
+
+  // Factory of weak pointers to |this| that will to be referenced from the

[Wez, 2015/01/09 02:55:45]

typo: that will to be

[Łukasz Anforowicz, 2015/01/09 19:00:14]

Done.

+  // network thread (weak pointers are constrained to a single thread).

[Wez, 2015/01/09 02:55:46]

Weak pointers are constrained to a single thread is not necessary; it's implicit
in the nature of their function.

[Łukasz Anforowicz, 2015/01/09 19:00:14]

Done.

+  base::WeakPtrFactory<HostProcess> weak_factory_for_network_task_runner_;

[Wez, 2015/01/09 02:55:46]

Just weak_factory_, please - it's implicit that this is dispensing WeakPtrs for
dereference on the network task runner, since that's the one this object is
dereferenced on.

[Łukasz Anforowicz, 2015/01/09 19:00:14]

I thought that the qualification is useful because:
1. some HostProcess's instance methods can be invoked on other threads, for
example: OnInitializePairingRegistry, StartOnUiThread
2. There is both ShutdownOnUiThread and ShutdownOnNetworkThread.  I thought that
without a qualification reader of ShutdownOnNetworkThread will wonder why a
generic weak_factory is being invalidated here.

FWIW, I dropped the suffix from the name.

[Wez, 2015/01/10 01:43:32]

The point is that _any_ WeakPtrFactory _must_ be invalidated on the same thread
on which the WeakPtrs it dispenses are checked & dereferenced. e.g. it wouldn't
make sense to have a weak_factory_for_ui_thread_ and a
weak_factory_for_network_thread_ on the same object, unless you were doing
something really crazy!

[Łukasz Anforowicz, 2015/01/12 18:05:14]

Oh, ok.  I think I get your point: 1) there should be only one "main" thread
associated with an object and 2) there is no point in explicitly referring to
this thread everywhere.

+
+  DISALLOW_COPY_AND_ASSIGN(HostProcess);
 };
 
 HostProcess::HostProcess(scoped_ptr<ChromotingHostContext> context,
@@ -390,7 +400,8 @@ HostProcess::HostProcess(scoped_ptr<ChromotingHostContext> context,
       self_(this),
       exit_code_out_(exit_code_out),
       signal_parent_(false),
-      shutdown_watchdog_(shutdown_watchdog) {
+      shutdown_watchdog_(shutdown_watchdog) ,
+      weak_factory_for_network_task_runner_(this) {
   StartOnUiThread();
 }
 
@@ -777,7 +788,6 @@ void HostProcess::ShutdownOnUiThread() {
   DCHECK(context_->ui_task_runner()->BelongsToCurrentThread());
 
   // Tear down resources that need to be torn down on the UI thread.
-  network_change_notifier_.reset();
   daemon_channel_.reset();
   desktop_environment_factory_.reset();
 
@@ -793,7 +803,6 @@ void HostProcess::ShutdownOnUiThread() {
 #endif
 }
 
-// Overridden from HeartbeatSender::Listener
 void HostProcess::OnUnknownHostIdError() {
   LOG(ERROR) << "Host ID not found.";
   ShutdownHost(kInvalidHostIdExitCode);
@@ -899,7 +908,8 @@ bool HostProcess::ApplyConfig(const base::DictionaryValue& config) {
   }
 
   if (!oauth_refresh_token_.empty()) {
-    // SignalingConnector is responsible for getting OAuth token.
+    // SignalingConnector (inside HostSignalingManager) is responsible for
+    // getting OAuth token.
     xmpp_server_config_.auth_token = "";
     xmpp_server_config_.auth_service = "oauth2";
   } else if (!config.GetString(kXmppAuthServiceConfigPath,
@@ -1279,44 +1289,37 @@ bool HostProcess::OnGnubbyAuthPolicyUpdate(base::DictionaryValue* policies) {
   return true;
 }
 
+scoped_ptr<HostSignalingManager> HostProcess::CreateHostSignalingManager() {
+  DCHECK(!host_id_.empty());  // |ApplyConfig| should already have been run.
+
+  scoped_ptr<OAuthTokenGetter::OAuthCredentials> oauth_credentials(
+      new OAuthTokenGetter::OAuthCredentials(xmpp_server_config_.username,
+                                             oauth_refresh_token_,
+                                             use_service_account_));
+
+  return HostSignalingManager::Create(
+      base::Bind(&HostProcess::OnHeartbeatSuccessful,
+                 weak_factory_for_network_task_runner_.GetWeakPtr()),
+      base::Bind(&HostProcess::OnUnknownHostIdError,
+                 weak_factory_for_network_task_runner_.GetWeakPtr()),
+      base::Bind(&HostProcess::OnAuthFailed,
+                 weak_factory_for_network_task_runner_.GetWeakPtr()),
+      context_->network_task_runner(), context_->url_request_context_getter(),
+      xmpp_server_config_, talkgadget_prefix_, host_id_, key_pair_,
+      directory_bot_jid_, oauth_credentials.Pass());
+}
+
 void HostProcess::StartHost() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
   DCHECK(!host_);
-  DCHECK(!signal_strategy_.get());
+  DCHECK(!host_signaling_manager_);
+
   DCHECK(state_ == HOST_INITIALIZING || state_ == HOST_STOPPING_TO_RESTART ||
-         state_ == HOST_STOPPED) << state_;
+         state_ == HOST_STOPPED)
+      << "state_ = " << state_;
   state_ = HOST_STARTED;
 
-  signal_strategy_.reset(
-      new XmppSignalStrategy(net::ClientSocketFactory::GetDefaultFactory(),
-                             context_->url_request_context_getter(),
-                             xmpp_server_config_));
-
-  scoped_ptr<DnsBlackholeChecker> dns_blackhole_checker(
-      new DnsBlackholeChecker(context_->url_request_context_getter(),
-                              talkgadget_prefix_));
-
-  // Create a NetworkChangeNotifier for use by the signaling connector.
-  network_change_notifier_.reset(net::NetworkChangeNotifier::Create());
-
-  signaling_connector_.reset(new SignalingConnector(
-      signal_strategy_.get(),
-      dns_blackhole_checker.Pass(),
-      base::Bind(&HostProcess::OnAuthFailed, this)));
-
-  if (!oauth_refresh_token_.empty()) {
-    scoped_ptr<OAuthTokenGetter::OAuthCredentials> oauth_credentials;
-    oauth_credentials.reset(
-        new OAuthTokenGetter::OAuthCredentials(
-            xmpp_server_config_.username, oauth_refresh_token_,
-            use_service_account_));
-
-    oauth_token_getter_.reset(new OAuthTokenGetter(
-        oauth_credentials.Pass(), context_->url_request_context_getter(),
-        false));
-
-    signaling_connector_->EnableOAuth(oauth_token_getter_.get());
-  }
+  host_signaling_manager_ = CreateHostSignalingManager();
 
   uint32 network_flags = 0;
   if (allow_nat_traversal_) {
@@ -1340,15 +1343,14 @@ void HostProcess::StartHost() {
   }
 
   host_.reset(new ChromotingHost(
-      signal_strategy_.get(),
+      host_signaling_manager_->signal_strategy(),
       desktop_environment_factory_.get(),
-      CreateHostSessionManager(signal_strategy_.get(), network_settings,
+      CreateHostSessionManager(host_signaling_manager_->signal_strategy(),
+                               network_settings,
                                context_->url_request_context_getter()),
-      context_->audio_task_runner(),
-      context_->input_task_runner(),
+      context_->audio_task_runner(), context_->input_task_runner(),
       context_->video_capture_task_runner(),
-      context_->video_encode_task_runner(),
-      context_->network_task_runner(),
+      context_->video_encode_task_runner(), context_->network_task_runner(),
       context_->ui_task_runner()));
 
   if (enable_vp9_) {
@@ -1370,17 +1372,13 @@ void HostProcess::StartHost() {
   host_->SetMaximumSessionDuration(base::TimeDelta::FromHours(20));
 #endif
 
-  heartbeat_sender_.reset(new HeartbeatSender(
-      base::Bind(&HostProcess::OnHeartbeatSuccessful, base::Unretained(this)),
-      base::Bind(&HostProcess::OnUnknownHostIdError, base::Unretained(this)),
-      host_id_, signal_strategy_.get(), key_pair_, directory_bot_jid_));
-
   host_change_notification_listener_.reset(new HostChangeNotificationListener(
-      this, host_id_, signal_strategy_.get(), directory_bot_jid_));
+      this, host_id_, host_signaling_manager_->signal_strategy(),
+      directory_bot_jid_));
 
-  host_status_logger_.reset(
-      new HostStatusLogger(host_->AsWeakPtr(), ServerLogEntry::ME2ME,
-                           signal_strategy_.get(), directory_bot_jid_));
+  host_status_logger_.reset(new HostStatusLogger(
+      host_->AsWeakPtr(), ServerLogEntry::ME2ME,
+      host_signaling_manager_->signal_strategy(), directory_bot_jid_));
 
   // Set up reporting the host status notifications.
 #if defined(REMOTING_MULTI_PROCESS)
@@ -1409,6 +1407,16 @@ void HostProcess::RestartHost() {
   ShutdownOnNetworkThread();
 }
 
+void HostProcess::SendOfflineReasonAndShutdownOnNetworkThread(
+    HostExitCodes exit_code) {
+  DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+  DCHECK(host_signaling_manager_);
+  host_signaling_manager_.release()->SendHostOfflineReasonAndDelete(
+      ExitCodeToString(exit_code),
+      base::TimeDelta::FromSeconds(kHostOfflineReasonTimeoutSeconds));
+  ShutdownOnNetworkThread();
+}
+
 void HostProcess::ShutdownHost(HostExitCodes exit_code) {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
 
@@ -1417,14 +1425,14 @@ void HostProcess::ShutdownHost(HostExitCodes exit_code) {
   switch (state_) {
     case HOST_INITIALIZING:
       state_ = HOST_STOPPING;
-      ShutdownOnNetworkThread();
+      DCHECK(!host_signaling_manager_);
+      host_signaling_manager_ = CreateHostSignalingManager();
+      SendOfflineReasonAndShutdownOnNetworkThread(exit_code);
       break;
 
     case HOST_STARTED:
       state_ = HOST_STOPPING;
-      heartbeat_sender_->SetHostOfflineReason(
-          ExitCodeToString(exit_code), base::Bind(base::DoNothing));
-      ScheduleHostShutdown();
+      SendOfflineReasonAndShutdownOnNetworkThread(exit_code);
       break;
 
     case HOST_STOPPING_TO_RESTART:
@@ -1438,28 +1446,17 @@ void HostProcess::ShutdownHost(HostExitCodes exit_code) {
   }
 }
 
-// TODO(weitaosu): shut down the host once we get an ACK for the offline status
-//                  XMPP message.
-void HostProcess::ScheduleHostShutdown() {
-  // Delay the shutdown by 2 second to allow SendOfflineStatus to complete.
-  context_->network_task_runner()->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&HostProcess::ShutdownOnNetworkThread, base::Unretained(this)),
-      base::TimeDelta::FromSeconds(2));
-}
-
 void HostProcess::ShutdownOnNetworkThread() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
 
+  // Need to invalidate WeakPtrs on the same thread where they are dereferenced.
+  weak_factory_for_network_task_runner_.InvalidateWeakPtrs();
+
   host_.reset();
   host_event_logger_.reset();
   host_status_logger_.reset();
-  heartbeat_sender_.reset();
+  host_signaling_manager_.reset();
   host_change_notification_listener_.reset();
-  signaling_connector_.reset();
-  oauth_token_getter_.reset();
-  signal_strategy_.reset();
-  network_change_notifier_.reset();
 
   if (state_ == HOST_STOPPING_TO_RESTART) {
     StartHost();