Suspend (rather than shut down) the host upon policy errors.

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 b66f8376f791bea52a9ff8cc57ce6339324331d2..88e647dce403e9e226417e4a15f1e5aaf3ebcdcd 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -147,6 +147,11 @@ const int kShutdownTimeoutSeconds = 15;
 // before continuing normal process shutdown.
 const int kHostOfflineReasonTimeoutSeconds = 10;
 
+// Host offline reasons not associated with shutting down the host process
+// and therefore not expressible through HostExitCodes enum.
+const char kHostOfflineBecauseOfRestarting[] = "RESTARTING";

[Sergey Ulanov, 2015/02/13 02:59:29]

s/Because/Reason/ to match field name

+const char kHostOfflineBecauseOfPolicyReadError[] = "POLICY_READ_ERROR";
+
 }  // namespace
 
 namespace remoting {
@@ -213,6 +218,21 @@ class HostProcess : public ConfigWatcher::Delegate,
     // nullptr in all other states.
   };
 
+  enum PolicyState {
+    // Cannot start the host, because a valid policy has not been read yet.
+    POLICY_INITIALIZING,
+
+    // Policy was loaded successfully.
+    POLICY_LOADED,
+
+    // Policy error was detected, and we haven't yet sent out a
+    // host-offline-reason (i.e. because we haven't yet read the config).
+    POLICY_ERROR_REPORT_PENDING,
+
+    // Policy error was detected, and we have sent out a host-offline-reason.
+    POLICY_ERROR_REPORTED,
+  };
+
   friend class base::RefCountedThreadSafe<HostProcess>;
   ~HostProcess() override;
 
@@ -245,6 +265,7 @@ class HostProcess : public ConfigWatcher::Delegate,
   // Handles policy updates, by calling On*PolicyUpdate methods.
   void OnPolicyUpdate(scoped_ptr<base::DictionaryValue> policies);
   void OnPolicyError();
+  void ReportPolicyErrorAndRestartHost();
   void ApplyHostDomainPolicy();
   void ApplyUsernamePolicy();
   bool OnHostDomainPolicyUpdate(base::DictionaryValue* policies);
@@ -273,11 +294,9 @@ class HostProcess : public ConfigWatcher::Delegate,
   // Stops the host and shuts down the process with the specified |exit_code|.
   void ShutdownHost(HostExitCodes exit_code);
 
-  // Private helper used by ShutdownHost method to initiate sending of
-  // host-offline-reason before continuing shutdown.
-  void SendOfflineReasonAndShutdownOnNetworkThread(HostExitCodes exit_code);
-
-  void ShutdownOnNetworkThread();
+  // Starts host shut down with an optional |host_offline_reason|.
+  void ShutdownOnNetworkThread(const std::string& host_offline_reason);
+  void OnHostOfflineReasonAck(bool success);
 
 #if defined(OS_WIN)
   // Initializes the pairing registry on Windows. This should be invoked on the
@@ -325,7 +344,7 @@ class HostProcess : public ConfigWatcher::Delegate,
   int64_t frame_recorder_buffer_size_;
 
   scoped_ptr<PolicyWatcher> policy_watcher_;
-  bool policies_loaded_;
+  PolicyState policy_state_;
   std::string host_domain_;
   bool host_username_match_required_;
   bool allow_nat_traversal_;
@@ -381,7 +400,7 @@ HostProcess::HostProcess(scoped_ptr<ChromotingHostContext> context,
       use_service_account_(false),
       enable_vp9_(false),
       frame_recorder_buffer_size_(0),
-      policies_loaded_(false),
+      policy_state_(POLICY_INITIALIZING),
       host_username_match_required_(false),
       allow_nat_traversal_(true),
       allow_relay_(true),
@@ -551,12 +570,11 @@ void HostProcess::OnConfigUpdated(
     return;
   }
 
-  if (state_ == HOST_INITIALIZING) {
+  if (state_ == HOST_INITIALIZING || state_ == HOST_STOPPING_TO_RESTART) {
     StartHostIfReady();
   } else if (state_ == HOST_STARTED) {
-    DCHECK(policies_loaded_);
-
     // Reapply policies that could be affected by a new config.
+    DCHECK_EQ(policy_state_, POLICY_LOADED);
     ApplyHostDomainPolicy();
     ApplyUsernamePolicy();
 
@@ -981,9 +999,9 @@ void HostProcess::OnPolicyUpdate(scoped_ptr<base::DictionaryValue> policies) {
   restart_required |= OnPairingPolicyUpdate(policies.get());
   restart_required |= OnGnubbyAuthPolicyUpdate(policies.get());
 
-  policies_loaded_ = true;
+  policy_state_ = POLICY_LOADED;
 
-  if (state_ == HOST_INITIALIZING) {
+  if (state_ == HOST_INITIALIZING || state_ == HOST_STOPPING_TO_RESTART) {

[Sergey Ulanov, 2015/02/13 02:59:29]

What if policies are updated while we are still waiting response to the
host-offline-reason when restarting the host due to the previous policy change?

[Łukasz Anforowicz, 2015/02/13 20:41:00]

This should be fine, because policy_state_ will be taken into account when
restarting (i.e. once we get to OnHostOfflineReasonAck, it will call to
StartHostIfReady which will start the host if policy are fine at this point).

     StartHostIfReady();
   } else if (state_ == HOST_STARTED) {
     if (restart_required)
@@ -998,7 +1016,26 @@ void HostProcess::OnPolicyError() {
     return;
   }
 
-  ShutdownHost(kInvalidHostConfigurationExitCode);
+  if (policy_state_ != POLICY_ERROR_REPORTED) {
+    policy_state_ = POLICY_ERROR_REPORT_PENDING;
+    if (state_ == HOST_INITIALIZING) {
+      StartHostIfReady();

[Sergey Ulanov, 2015/02/13 02:59:28]

This also calls ReportPolicyErrorAndRestartHost(), but only if config is ready.
Its also confusing that StartHostIfReady() is called when policies are broken.
So maybe always call ReportPolicyErrorAndRestartHost() here, just check that the
config has been loaded?

[Łukasz Anforowicz, 2015/02/13 20:41:00]

Good point.  Thanks.

+    } else if (state_ == HOST_STARTED) {
+      ReportPolicyErrorAndRestartHost();
+    }
+  }
+}
+
+void HostProcess::ReportPolicyErrorAndRestartHost() {
+  DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+  DCHECK_EQ(policy_state_, POLICY_ERROR_REPORT_PENDING);
+
+  LOG(INFO) << "Restarting the host due to policy errors.";
+
+  policy_state_ = POLICY_ERROR_REPORTED;
+  state_ = HOST_STOPPING_TO_RESTART;

[Sergey Ulanov, 2015/02/13 02:59:29]

Call RestartHost()?

[Łukasz Anforowicz, 2015/02/13 20:41:00]

Good point.  Note that to do that I removed source-state DCHECK in RestartHost
(but this DCHECK is at any rate made obsolete by the DCHECKs I introduced in
SetState).

+
+  ShutdownOnNetworkThread(kHostOfflineBecauseOfPolicyReadError);
 }
 
 void HostProcess::ApplyHostDomainPolicy() {
@@ -1300,20 +1337,24 @@ scoped_ptr<HostSignalingManager> HostProcess::CreateHostSignalingManager() {
                                              oauth_refresh_token_,
                                              use_service_account_));
 
-  return HostSignalingManager::Create(this, context_->network_task_runner(),
-                                      context_->url_request_context_getter(),
-                                      xmpp_server_config_, talkgadget_prefix_,
-                                      host_id_, key_pair_, directory_bot_jid_,
-                                      oauth_credentials.Pass());
+  return HostSignalingManager::Create(
+      this, context_->url_request_context_getter(), xmpp_server_config_,
+      talkgadget_prefix_, host_id_, key_pair_, directory_bot_jid_,
+      oauth_credentials.Pass());
 }
 
 void HostProcess::StartHostIfReady() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
-  DCHECK_EQ(state_, HOST_INITIALIZING);
+  DCHECK((state_ == HOST_INITIALIZING) || (state_ == HOST_STOPPING_TO_RESTART));
 
   // Start the host if both the config and the policies are loaded.
-  if (!serialized_config_.empty() && policies_loaded_)
-    StartHost();
+  if (!serialized_config_.empty()) {
+    if (policy_state_ == POLICY_LOADED) {
+      StartHost();
+    } else if (policy_state_ == POLICY_ERROR_REPORT_PENDING) {
+      ReportPolicyErrorAndRestartHost();
+    }
+  }
 }
 
 void HostProcess::StartHost() {
@@ -1414,35 +1455,20 @@ void HostProcess::RestartHost() {
   DCHECK_EQ(state_, HOST_STARTED);
 
   state_ = HOST_STOPPING_TO_RESTART;
-  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();
+  ShutdownOnNetworkThread(kHostOfflineBecauseOfRestarting);
 }
 
 void HostProcess::ShutdownHost(HostExitCodes exit_code) {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
 
   *exit_code_out_ = exit_code;
+  std::string host_offline_reason = ExitCodeToString(exit_code);
 
   switch (state_) {
     case HOST_INITIALIZING:
-      state_ = HOST_STOPPING;
-      DCHECK(!host_signaling_manager_);
-      host_signaling_manager_ = CreateHostSignalingManager();
-      SendOfflineReasonAndShutdownOnNetworkThread(exit_code);
-      break;
-
     case HOST_STARTED:
       state_ = HOST_STOPPING;
-      SendOfflineReasonAndShutdownOnNetworkThread(exit_code);
+      ShutdownOnNetworkThread(host_offline_reason);
       break;
 
     case HOST_STOPPING_TO_RESTART:
@@ -1456,17 +1482,39 @@ void HostProcess::ShutdownHost(HostExitCodes exit_code) {
   }
 }
 
-void HostProcess::ShutdownOnNetworkThread() {
+void HostProcess::ShutdownOnNetworkThread(

[Sergey Ulanov, 2015/02/13 02:59:28]

This function handles two separate cases: host restart and host shutdown. It did
make sense in the past when restart was multi-step process that required jumping
to other threads. But it's not the case anymore. So I suggest splitting these
two cases. Also HOST_STOPPING_TO_RESTART state is not useful anymore - host can
be restarted synchronously. Instead you need a new state for waiting for offline
reason ACK and another one for the vegetative state when policies are broken (or
maybe reuse INITIALIZING for that).

[Łukasz Anforowicz, 2015/02/13 20:41:00]

Host doesn't restart synchronously - there is the waiting for
OnHostOfflineReasonAck (and during this time HOST_STOPPING_TO_RESTART can
possibly change into HOST_STOPPING, so I think it is still useful to keep these
2 states).  OTOH, I probably should rename them to HOST_GOING_OFFLINE_TO_RESTART
and HOST_GOING_OFFLINE_TO_STOP.  And I should go from
HOST_GOING_OFFLINE_TO_RESTART to HOST_INITIALIZING (if policy is not ready at
this point).

+    const std::string& host_offline_reason) {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
 
+  // Shut down everything except the HostSignalingManager.
   host_.reset();
   host_event_logger_.reset();
   host_status_logger_.reset();
-  host_signaling_manager_.reset();
   host_change_notification_listener_.reset();
 
+  // Before shutting down HostSignalingManager, send the |host_offline_reason|,
+  // if any.
+  if (!host_offline_reason.empty()) {
+    if (!serialized_config_.empty()) {
+      if (!host_signaling_manager_) {
+        host_signaling_manager_ = CreateHostSignalingManager();
+      }
+
+      host_signaling_manager_->SendHostOfflineReason(
+          host_offline_reason,
+          base::TimeDelta::FromSeconds(kHostOfflineReasonTimeoutSeconds),
+          base::Bind(&HostProcess::OnHostOfflineReasonAck,
+                     base::Unretained(this)));
+      return;  // Shutdown will resume after OnHostOfflineReasonAck.
+    } else {
+      HOST_LOG << "SendHostOfflineReason( " << host_offline_reason << ") "
+               << "not possible without a config...";
+    }
+  }
+  host_signaling_manager_.reset();
+
   if (state_ == HOST_STOPPING_TO_RESTART) {
-    StartHost();
+    StartHostIfReady();
   } else if (state_ == HOST_STOPPING) {
     state_ = HOST_STOPPED;
 
@@ -1484,6 +1532,18 @@ void HostProcess::ShutdownOnNetworkThread() {
   }
 }
 
+void HostProcess::OnHostOfflineReasonAck(bool success) {
+  DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+
+  if (success) {

[Sergey Ulanov, 2015/02/13 02:59:28]

HOST_LOG << "SendHostOfflineReason "
         << (success ? "succeeded" : "timed out");

[Łukasz Anforowicz, 2015/02/13 20:41:00]

Done.

+    HOST_LOG << "SendHostOfflineReason: succeeded";
+  } else {
+    HOST_LOG << "SendHostOfflineReason: timed out";
+  }
+
+  ShutdownOnNetworkThread(std::string());
+}
+
 void HostProcess::OnCrash(const std::string& function_name,
                           const std::string& file_name,
                           const int& line_number) {