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..ec742e5d2d5fcb90cb5e3c440b1e151d1fa7862a 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -213,6 +213,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 +260,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 +289,10 @@ 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);

[Wez, 2015/02/12 02:51:25]

Do we need to express "no reason" via nullptr - could we use an empty string
instead?

[Łukasz Anforowicz, 2015/02/12 18:08:01]

We can use an empty string.  Thanks for bringing this up.  I did think about it,
not sure why I picked the pointer option now...  It seemed like a more distinct
/ more obvious option, but HeartbeatSender already treats an empty string as
special, so it is probably better to be consistent.

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

Acknowledged.

+  void ContinueShutdownOnNetworkThread();
+  void OnHostOfflineReasonAck(bool success);
 
 #if defined(OS_WIN)
   // Initializes the pairing registry on Windows. This should be invoked on the
@@ -325,7 +340,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 +396,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 +566,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 +995,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 +1012,24 @@ 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() {

[Wez, 2015/02/12 02:51:25]

Please put some blank lines in this function to break it up into logical blocks,
e.g. the initial DCHECK()s express preconditions, then there's some setting of
internal state, and finally the shutdown call.

[Łukasz Anforowicz, 2015/02/12 18:08:01]

Done.

+  DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+  DCHECK_EQ(policy_state_, POLICY_ERROR_REPORT_PENDING);
+  LOG(INFO) << "Restarting the host due to policy errors.";
+  state_ = HOST_STOPPING_TO_RESTART;
+  policy_state_ = POLICY_ERROR_REPORTED;
+  std::string host_offline_reason = "POLICY_READ_ERROR";
+  ShutdownOnNetworkThread(&host_offline_reason);
 }
 
 void HostProcess::ApplyHostDomainPolicy() {
@@ -1300,20 +1331,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 +1449,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(nullptr);

[Wez, 2015/02/12 02:51:25]

Is "no reason" the right thing to report here? Why not report "restarting"?

[Łukasz Anforowicz, 2015/02/12 18:08:01]

Good point.  Done.

I was also struggling with a decision whether to just have string literals with
host-offline-reason at the point of use VS introduce string constants somewhere
(maybe in host_exit_codes.h, maybe in a new header...).  Having a second string
in here, pushed me to at least add string constants in an anonymous namespace in
the same file.

 }
 
 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 +1476,41 @@ void HostProcess::ShutdownHost(HostExitCodes exit_code) {
   }
 }
 
-void HostProcess::ShutdownOnNetworkThread() {
+void HostProcess::ShutdownOnNetworkThread(
+    const std::string* host_offline_reason) {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
 
   host_.reset();
   host_event_logger_.reset();
   host_status_logger_.reset();
-  host_signaling_manager_.reset();
   host_change_notification_listener_.reset();
 
+  if (host_offline_reason == nullptr) {

[Wez, 2015/02/12 02:51:25]

nit: Please add some comments to these two blocks to clarify the intended
behaviour.

[Łukasz Anforowicz, 2015/02/12 18:08:01]

Done (at least I tried to add sensible comments).

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

These look good in terms of content, but instead of the ellipses, suggest

"// Shut down everything except the HostSignalingManager."

"// Before tearing down HostSignalingManager, send the |host_offline_reason|, if
any."

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

Done.

+    ContinueShutdownOnNetworkThread();
+    return;
+  }
+  if (serialized_config_.empty()) {
+    HOST_LOG << "SendHostOfflineReason( " << host_offline_reason << ") "
+             << "not possible without a config...";
+    ContinueShutdownOnNetworkThread();
+    return;
+  }
+
+  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)));

[Wez, 2015/02/12 02:51:25]

With things structured as they are, with a nullptr when there is no
offline-reason to send, you don't really need the
ShutdownOnNetworkThread/ContinueShutdownOnNetworkThread split - you can just
have ShutdownOnNetworkThread send the shutdown reason if one is specified, and
bind ShutdownOnNetworkThread(nullptr) to the completion callback. Even if you
want to log whether the send succeeded or failed, OnHostOfflineReasonAck() can
just call ShutdownOnNetworkThread(nullptr) to complete the shutdown.

[Łukasz Anforowicz, 2015/02/12 18:08:01]

Done.  I think things look better now.

- I didn't go with WithReason/WithNoReason function pair, because the
WithNoReason part (if treated as "public" to the rest of this class) has to
duplicate resetting of host, host_event_logger, host_status_logger,
host_change_notification_listener.

- Part of me wants to treat "not possible without a config" as an assert/DCHECK,
but this requires unnecessary knowledge at callsites of ShutdownOnNetworkThread
(and ShutdownHost!) whether or not config should be read already.  Things seem
better with a log instead of an assert (I think).

+}
+
+void HostProcess::ContinueShutdownOnNetworkThread() {

[Wez, 2015/02/12 02:51:25]

If you prefer to keep this as-is then I'd suggest renaming this to
ShutdownOnNetworkThread() and the other to ...WithReason().

[Łukasz Anforowicz, 2015/02/12 18:08:01]

N/A (I went with one function as you suggested above).

+  DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+  host_signaling_manager_.reset();
+
   if (state_ == HOST_STOPPING_TO_RESTART) {
-    StartHost();
+    StartHostIfReady();
   } else if (state_ == HOST_STOPPING) {
     state_ = HOST_STOPPED;
 
@@ -1484,6 +1528,17 @@ void HostProcess::ShutdownOnNetworkThread() {
   }
 }
 
+void HostProcess::OnHostOfflineReasonAck(bool success) {
+  DCHECK(context_->network_task_runner()->BelongsToCurrentThread());

[Wez, 2015/02/12 02:51:25]

As above, suggest blank line between the pre-condition DCHECK and the actual
logic.

[Łukasz Anforowicz, 2015/02/12 18:08:01]

Done.

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