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 f2aa98331acea8f1f8076cf4540bed9a2c5220c0..9b7d9fda1b03b525aea8b204e3a51f53c6a89354 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,8 +45,6 @@
 #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"
@@ -58,10 +56,10 @@
 #include "remoting/host/ipc_host_event_logger.h"
 #include "remoting/host/logging.h"
 #include "remoting/host/me2me_desktop_environment.h"
+#include "remoting/host/minimum_heartbeat_supporter.h"
 #include "remoting/host/pairing_registry_delegate.h"
 #include "remoting/host/policy_hack/policy_watcher.h"
 #include "remoting/host/session_manager_factory.h"
-#include "remoting/host/signaling_connector.h"
 #include "remoting/host/single_window_desktop_environment.h"
 #include "remoting/host/token_validator_factory_impl.h"
 #include "remoting/host/usage_stats_consent.h"
@@ -240,6 +238,8 @@ class HostProcess
   bool OnPairingPolicyUpdate(base::DictionaryValue* policies);
   bool OnGnubbyAuthPolicyUpdate(base::DictionaryValue* policies);
 
+  scoped_refptr<MinimumHeartbeatSupporter> CreateMinimumHeartbeatSupporter();
+
   void StartHost();
 
   void OnHeartbeatSuccessful();
@@ -252,8 +252,6 @@ class HostProcess
   // Stops the host and shuts down the process with the specified |exit_code|.
   void ShutdownHost(HostExitCodes exit_code);
 
-  void ScheduleHostShutdown();
-
   void ShutdownOnNetworkThread();
 
   void OnPolicyWatcherShutdown();
@@ -267,9 +265,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_;
 
@@ -319,10 +314,7 @@ 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_;
+  scoped_refptr<MinimumHeartbeatSupporter> minimum_heartbeat_supporter_;
   scoped_ptr<HostChangeNotificationListener> host_change_notification_listener_;
   scoped_ptr<HostStatusLogger> host_status_logger_;
   scoped_ptr<HostEventLogger> host_event_logger_;
@@ -340,6 +332,7 @@ class HostProcess
   bool signal_parent_;
 
   scoped_ptr<PairingRegistry::Delegate> pairing_registry_delegate_;
+  base::WeakPtrFactory<HostProcess> weak_factory_for_network_task_runner_;

[Lambros, 2014/12/05 22:19:49]

It seems strange to move network_task_runner_ into MinimumHeartbeatSupporter,
but keep this WeakPtrFactory here. Maybe you could move this as well? Make this
a TODO if you do it in another CL.

[Łukasz Anforowicz, 2014/12/05 23:59:28]

This is here to stay for the long term.

1. network_task_runner is not embedded/hidden inside MinimumHeartbeatSupporter
(unlike signaling_connector, heartbeat_sender and network_change_notifier which
are now only used inside MinimumHeartbeatSupporter).  The network_task_runner is
primarily controlled by ChromotingHostContext.

2. This WeakPtrFactory is needed to invalidate callbacks that HostProcess
creates and passes to MinimumHeartbeatSupporter.

I think the name is also fine, but that a comment is needed to explain what the
name means - that this is a "weak factory for weak pointers to |this| that will
to be referenced from the network thread".  

I did think for a moment, that this could be named
"weak_factory_for_heartbeat_callbacks", because this is what it is used for at
the moment, but affinity with network thread is a more generic, truer property
of this field.

 };
 
 HostProcess::HostProcess(scoped_ptr<ChromotingHostContext> context,
@@ -364,7 +357,8 @@ HostProcess::HostProcess(scoped_ptr<ChromotingHostContext> context,
 #endif  // defined(REMOTING_MULTI_PROCESS)
       self_(this),
       exit_code_out_(exit_code_out),
-      signal_parent_(false) {
+      signal_parent_(false),
+      weak_factory_for_network_task_runner_(this) {
   StartOnUiThread();
 }
 
@@ -746,7 +740,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();
 
@@ -762,7 +755,6 @@ void HostProcess::ShutdownOnUiThread() {
 #endif
 }
 
-// Overridden from HeartbeatSender::Listener
 void HostProcess::OnUnknownHostIdError() {
   LOG(ERROR) << "Host ID not found.";
   ShutdownHost(kInvalidHostIdExitCode);
@@ -849,7 +841,8 @@ bool HostProcess::ApplyConfig(const base::DictionaryValue& config) {
   }
 
   if (!oauth_refresh_token_.empty()) {
-    // SignalingConnector is responsible for getting OAuth token.
+    // SignalingConnector (inside MinimumHeartbeatSupporter) is responsible for
+    // getting OAuth token.
     xmpp_server_config_.auth_token = "";
     xmpp_server_config_.auth_service = "oauth2";
   } else if (!config.GetString(kXmppAuthServiceConfigPath,
@@ -1229,44 +1222,30 @@ bool HostProcess::OnGnubbyAuthPolicyUpdate(base::DictionaryValue* policies) {
   return true;
 }
 
+scoped_refptr<MinimumHeartbeatSupporter>
+HostProcess::CreateMinimumHeartbeatSupporter() {
+  DCHECK(!host_id_.empty());  // Assumming |ApplyConfig| has already been run.
+  return MinimumHeartbeatSupporter::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_, xmpp_server_config_, talkgadget_prefix_, host_id_, key_pair_,
+      directory_bot_jid_, oauth_refresh_token_, use_service_account_);
+}
+
 void HostProcess::StartHost() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
   DCHECK(!host_);
-  DCHECK(!signal_strategy_.get());
+  DCHECK(!minimum_heartbeat_supporter_.get());
+
   DCHECK(state_ == HOST_INITIALIZING || state_ == HOST_STOPPING_TO_RESTART ||
          state_ == HOST_STOPPED) << 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());
-  }
+  minimum_heartbeat_supporter_ = CreateMinimumHeartbeatSupporter();
 
   uint32 network_flags = 0;
   if (allow_nat_traversal_) {
@@ -1290,15 +1269,14 @@ void HostProcess::StartHost() {
   }
 
   host_.reset(new ChromotingHost(
-      signal_strategy_.get(),
+      minimum_heartbeat_supporter_->signal_strategy(),
       desktop_environment_factory_.get(),
-      CreateHostSessionManager(signal_strategy_.get(), network_settings,
+      CreateHostSessionManager(minimum_heartbeat_supporter_->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_) {
@@ -1320,17 +1298,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_, minimum_heartbeat_supporter_->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,
+      minimum_heartbeat_supporter_->signal_strategy(), directory_bot_jid_));
 
   // Set up reporting the host status notifications.
 #if defined(REMOTING_MULTI_PROCESS)
@@ -1367,14 +1341,23 @@ void HostProcess::ShutdownHost(HostExitCodes exit_code) {
   switch (state_) {
     case HOST_INITIALIZING:
       state_ = HOST_STOPPING;
+      {
+        DCHECK(!minimum_heartbeat_supporter_.get());
+        scoped_refptr<MinimumHeartbeatSupporter> tmpHeartbeatSupporter =
+            CreateMinimumHeartbeatSupporter();
+        tmpHeartbeatSupporter->SendHostOfflineReason(
+            ExitCodeToString(exit_code),
+            base::TimeDelta::FromSeconds(30) /* timeout */);

[Lambros, 2014/12/05 22:19:49]

30s feels much too long to be delaying process exit. I think the Python script
sends SIGTERM, waits 10s, then sends SIGKILL. So anything more than 10s is
pointless here, unless you update the Python script as well.
See
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/linu...

[Łukasz Anforowicz, 2014/12/05 23:59:28]

I'll change this to 10 seconds.

Note that this timeout has no relation to the timeout used by the python script
- everything around SendHostOfflineReason happens within the host process,
without input or interference from the python script (i.e. I assume that you
pointed the python script's behavior as an example of a reasonable process
shutdown timeout, not to point out that there is an interaction of some kind
between the python script and the host sending a host-offline-reason).

[Lambros, 2014/12/06 01:25:05]

No, there is an actual interaction. The Python script sends SIGTERM to this host
process. That gets handled by SigTermHandler() defined in this file, which then
calls through to this function here. Python will then wait only 10s before
sending SIGKILL to this process, which cannot be trapped.

+      }
       ShutdownOnNetworkThread();
       break;
 
     case HOST_STARTED:
       state_ = HOST_STOPPING;
-      heartbeat_sender_->SetHostOfflineReason(
-          ExitCodeToString(exit_code), base::Bind(base::DoNothing));
-      ScheduleHostShutdown();
+      minimum_heartbeat_supporter_->SendHostOfflineReason(
+          ExitCodeToString(exit_code),
+          base::TimeDelta::FromSeconds(30) /* timeout */);

[Lambros, 2014/12/05 22:19:48]

Same here.

[Łukasz Anforowicz, 2014/12/05 23:59:28]

Done.

+      ShutdownOnNetworkThread();
       break;
 
     case HOST_STOPPING_TO_RESTART:
@@ -1388,28 +1371,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();
+  minimum_heartbeat_supporter_ = nullptr;
   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();