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..77f9f172a1afb8fadb6ea987cefe95f17bf8a6e9 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";
+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(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) {
     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();
+    } 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;
+
+  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,37 @@ void HostProcess::ShutdownHost(HostExitCodes exit_code) {
   }
 }
 
-void HostProcess::ShutdownOnNetworkThread() {
+void HostProcess::ShutdownOnNetworkThread(
+    const std::string& host_offline_reason) {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
 
+  // Shut down most host subsystems ...
   host_.reset();
   host_event_logger_.reset();
   host_status_logger_.reset();
-  host_signaling_manager_.reset();
   host_change_notification_listener_.reset();
 
+  // ... but before shutting down |host_signaling_manager_|, check if we still
+  // need it for sending a |host_offline_reason| first.
+  if (!host_offline_reason.empty() && serialized_config_.empty()) {

[Wez, 2015/02/12 21:07:07]

This would read more clearly as:

if (!host_offline_reason.empty()) {
  if (!serialized_config_.empty()) {
    ... send offline reason and return ...
  } else {
    .. error message ...
  }
}

... shut down ALL da things ...

[Łukasz Anforowicz, 2015/02/12 22:51:13]

Done.

+    HOST_LOG << "SendHostOfflineReason( " << host_offline_reason << ") "
+             << "not possible without a config...";
+  } else if (!host_offline_reason.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.
+  }
+  host_signaling_manager_.reset();
+
   if (state_ == HOST_STOPPING_TO_RESTART) {
-    StartHost();
+    StartHostIfReady();
   } else if (state_ == HOST_STOPPING) {
     state_ = HOST_STOPPED;
 
@@ -1484,6 +1530,18 @@ void HostProcess::ShutdownOnNetworkThread() {
   }
 }
 
+void HostProcess::OnHostOfflineReasonAck(bool success) {
+  DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+
+  if (success) {
+    HOST_LOG << "SendHostOfflineReason: succeeded";
+  } else {
+    HOST_LOG << "SendHostOfflineReason: timed out";
+  }
+
+  ShutdownOnNetworkThread(std::string());

[Wez, 2015/02/12 21:07:07]

Now that I see this written out, it seems less clear than splitting
ShutdownOnNetworkThread() and ShutdownOnNetworkThreadWithReason(). WDYT?

[Łukasz Anforowicz, 2015/02/12 22:51:13]

I think I like it better this way.  

If we have a pair of functions, then one of these functions (ContinueShutdown)
is not really callable from anywhere other than Shutdown + OnAck.  If we want to
make ContinueShutdown callable separately (and name it
ShutdownOnNetworkThread[WithoutReason]) then it has to duplicate the code that
resets the 4 members (host, host_event_logger, ...) or we have to move this code
to yet another method.

OTOH, there are 2 things that bother me about the current approach:
1) a bit complicated logic (3 levels of nesting! :-) for figuring out when to
call SendHostOfflineReason (i.e. have to check host_offline_reason.empty(),
host_signaling_manager_ not null, serializer_config_.empty).
2) the whole ShutdownOnNetworkThread got a bit long

But I think this is still ok.  ShutdownOnNetworkThread is not that difficult to
follow (and I think surely not more difficult than having to understand how 2 or
3 methods work together / which one of them can be invoked standalone vs only
from the other one).

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

Actually, when addressing Sergey's feedback (and trying to clean-up state
transitions in general) I went ahead with 2 functions in patchset #4.  I think
things are in a better shape right now (better than baseline + better than the
1st two patchsets).

+}
+
 void HostProcess::OnCrash(const std::string& function_name,
                           const std::string& file_name,
                           const int& line_number) {