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..b24786d6689d20b9412e2178e4bfe25008c49b56 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -147,6 +147,12 @@ 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 kHostOfflineReasonPolicyReadError[] = "POLICY_READ_ERROR";
+const char kHostOfflineReasonPolicyChangeRequiresRestart[] =
+    "POLICY_CHANGE_REQUIRES_RESTART";
+
 }  // namespace
 
 namespace remoting {
@@ -183,34 +189,83 @@ class HostProcess : public ConfigWatcher::Delegate,
 
  private:
   enum HostState {
-    // Host process has just been started. Waiting for config and policies to be
-    // read from the disk.
-    HOST_INITIALIZING,
+    // Waiting for valid config and policies to be read from the disk.
+    // Either the host process has just been started, or it is trying to start
+    // again after temporarily going offline due to policy change or error.
+    HOST_STARTING,
 
     // Host is started and running.
     HOST_STARTED,
 
-    // Host is being stopped and will need to be started again.
-    HOST_STOPPING_TO_RESTART,
+    // Host is sending offline reason, before trying to restart.
+    // Used when:
+    // 1) policy change requires a restart.
+    // 2) policy error puts the host offline until we have a valid policy again.

[Sergey Ulanov, 2015/02/18 00:28:31]

Isn't it in HOST_STARTING state in this case?

[Łukasz Anforowicz, 2015/02/18 17:03:11]

"Host is sending offline reason" implies state_ ==
HOST_GOING_OFFLINE_TO_RESTART.  After the offline reason is sent, the state_
will change to state_ == HOST_STARTING.

I will just remove the "Used when" part of the comment to avoid the confusion.

+    HOST_GOING_OFFLINE_TO_RESTART,
 
-    // Host is being stopped.
-    HOST_STOPPING,
+    // Host is sending offline reason, before shutting down.
+    HOST_GOING_OFFLINE_TO_STOP,
 
-    // Host has been stopped.
+    // Host has been stopped (host process will end soon).
     HOST_STOPPED,
 
-    // Allowed state transitions:
-    //   INITIALIZING->STARTED
-    //   INITIALIZING->STOPPED
-    //   STARTED->STOPPING_TO_RESTART
-    //   STARTED->STOPPING
-    //   STOPPING_TO_RESTART->STARTED
-    //   STOPPING_TO_RESTART->STOPPING
-    //   STOPPING->STOPPED
-    //   STOPPED->STARTED
+    // Allowed state transitions (enforced via DCHECKs in SetState method):

[Sergey Ulanov, 2015/02/18 00:28:31]

nit: Put this comment above HostState enum?

[Łukasz Anforowicz, 2015/02/18 17:03:11]

I moved this comment next to SetState method (so that the list of transitions is
kept next to the code that enforces the allowed transitions).

+    //   STARTING->STARTED (once we have valid config + policy)
+    //   STARTING->GOING_OFFLINE_TO_STOP
+    //   STARTING->GOING_OFFLINE_TO_RESTART
+    //   STARTED->GOING_OFFLINE_TO_STOP
+    //   STARTED->GOING_OFFLINE_TO_RESTART
+    //   GOING_OFFLINE_TO_RESTART->GOING_OFFLINE_TO_STOP
+    //   GOING_OFFLINE_TO_RESTART->STARTING (after OnHostOfflineReasonAck)
+    //   GOING_OFFLINE_TO_STOP->STOPPED (after OnHostOfflineReasonAck)
     //
-    // |host_| must be nullptr in INITIALIZING and STOPPED states and not
-    // nullptr in all other states.
+    // |host_| must be not-null in STARTED state and nullptr in all other states
+    // (although this invariant can be temporarily violated when doing
+    // synchronous processing on the networking thread).
+  };
+
+  void SetState(HostState target_state) {

[Sergey Ulanov, 2015/02/18 00:28:31]

Move this below in the class
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...).
Also move definition out of the class declaration.

[Łukasz Anforowicz, 2015/02/18 17:03:11]

I see.  Thanks for pointing this out.

Since I dislike having the "Allowed state transitions" comment far away from
SetState method that enforces the allowed transitions, I'll also move the
comment to be next to the SetState method's definition.

+    DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+
+    // DCHECKs below enforce state allowed transitions listed in HostState.
+    switch (state_) {
+      case HOST_STARTING:
+        DCHECK((target_state == HOST_STARTED) ||
+               (target_state == HOST_GOING_OFFLINE_TO_STOP) ||
+               (target_state == HOST_GOING_OFFLINE_TO_RESTART));

[Sergey Ulanov, 2015/02/18 00:28:31]

add something like '<< state_ << "->" << target_state' to make it easier to
debug failures in this DCHECK. Same for the other DCHECKs below.

[Łukasz Anforowicz, 2015/02/18 17:03:11]

Done.

+        break;
+      case HOST_STARTED:
+        DCHECK((target_state == HOST_GOING_OFFLINE_TO_STOP) ||
+               (target_state == HOST_GOING_OFFLINE_TO_RESTART));
+        break;
+      case HOST_GOING_OFFLINE_TO_RESTART:
+        DCHECK((target_state == HOST_GOING_OFFLINE_TO_STOP) ||
+               (target_state == HOST_STARTING));
+        break;
+      case HOST_GOING_OFFLINE_TO_STOP:
+        DCHECK_EQ(target_state, HOST_STOPPED);
+        break;
+      case HOST_STOPPED:
+      default:
+        NOTREACHED();  // HOST_STOPPED is a terminal state.
+        break;
+    }
+    state_ = target_state;
+  }
+
+  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>;
@@ -245,6 +300,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);
@@ -268,16 +324,12 @@ class HostProcess : public ConfigWatcher::Delegate,
   void OnUnknownHostIdError() override;
   void OnAuthFailed() override;
 
-  void RestartHost();
-
-  // Stops the host and shuts down the process with the specified |exit_code|.
+  void RestartHost(const std::string& host_offline_reason);
   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();
+  // Helper methods doing the work needed by RestartHost and ShutdownHost.
+  void GoOffline(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 +377,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_;
@@ -377,11 +429,11 @@ HostProcess::HostProcess(scoped_ptr<ChromotingHostContext> context,
                          int* exit_code_out,
                          ShutdownWatchdog* shutdown_watchdog)
     : context_(context.Pass()),
-      state_(HOST_INITIALIZING),
+      state_(HOST_STARTING),
       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 +603,11 @@ void HostProcess::OnConfigUpdated(
     return;
   }
 
-  if (state_ == HOST_INITIALIZING) {
+  if (state_ == HOST_STARTING) {
     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();
 
@@ -786,7 +837,6 @@ void HostProcess::ShutdownOnUiThread() {
   // Tear down resources that need to be torn down on the UI thread.
   daemon_channel_.reset();
   desktop_environment_factory_.reset();
-
   policy_watcher_.reset();
 
   // It is now safe for the HostProcess to be deleted.
@@ -981,13 +1031,13 @@ 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_STARTING) {
     StartHostIfReady();
   } else if (state_ == HOST_STARTED) {
     if (restart_required)
-      RestartHost();
+      RestartHost(kHostOfflineReasonPolicyChangeRequiresRestart);
   }
 }
 
@@ -998,7 +1048,23 @@ void HostProcess::OnPolicyError() {
     return;
   }
 
-  ShutdownHost(kInvalidHostConfigurationExitCode);
+  if (policy_state_ != POLICY_ERROR_REPORTED) {
+    policy_state_ = POLICY_ERROR_REPORT_PENDING;
+    if (((state_ == HOST_STARTING) || (state_ == HOST_STARTED)) &&

[Sergey Ulanov, 2015/02/18 00:28:31]

nit: IMO these extra parentheses around == operations hurt readability and I
think chromium code normally doesn't use them for conditions like this. Same for
for ! operation below. (you still want the parentheses around  || and &&). 
Style guide is ambiguous about it (Feel free to insert extra parentheses
judiciously because they can be very helpful in increasing readability when used
appropriately.), but the example it gives doesn't have parentheses

[Sergey Ulanov, 2015/02/18 00:28:31]

Maybe move these checks into ReportPolicyErrorAndRestartHost()? If you keep them
here then add DCHECKs for them in ReportPolicyErrorAndRestartHost().

[Łukasz Anforowicz, 2015/02/18 17:03:11]

Done.

[Łukasz Anforowicz, 2015/02/18 17:03:11]

I've added a DCHECK(!serialized_config.empty()) to
ReportPolicyErrorAndRestartHost.

+        (!serialized_config_.empty())) {
+      ReportPolicyErrorAndRestartHost();
+    }
+  }
+}
+
+void HostProcess::ReportPolicyErrorAndRestartHost() {
+  DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+
+  DCHECK_EQ(policy_state_, POLICY_ERROR_REPORT_PENDING);
+  policy_state_ = POLICY_ERROR_REPORTED;
+
+  LOG(INFO) << "Restarting the host due to policy errors.";
+  RestartHost(kHostOfflineReasonPolicyReadError);
 }
 
 void HostProcess::ApplyHostDomainPolicy() {
@@ -1300,20 +1366,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_EQ(state_, HOST_STARTING);
 
   // 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() {
@@ -1321,10 +1391,7 @@ void HostProcess::StartHost() {
   DCHECK(!host_);
   DCHECK(!host_signaling_manager_);
 
-  DCHECK(state_ == HOST_INITIALIZING || state_ == HOST_STOPPING_TO_RESTART ||
-         state_ == HOST_STOPPED)
-      << "state_ = " << state_;
-  state_ = HOST_STARTED;
+  SetState(HOST_STARTED);
 
   host_signaling_manager_ = CreateHostSignalingManager();
 
@@ -1409,22 +1476,12 @@ void HostProcess::OnAuthFailed() {
   ShutdownHost(kInvalidOauthCredentialsExitCode);
 }
 
-void HostProcess::RestartHost() {
+void HostProcess::RestartHost(const std::string& host_offline_reason) {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
-  DCHECK_EQ(state_, HOST_STARTED);
-
-  state_ = HOST_STOPPING_TO_RESTART;
-  ShutdownOnNetworkThread();
-}
+  DCHECK(!host_offline_reason.empty());
 
-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();
+  SetState(HOST_GOING_OFFLINE_TO_RESTART);
+  GoOffline(host_offline_reason);
 }
 
 void HostProcess::ShutdownHost(HostExitCodes exit_code) {
@@ -1433,42 +1490,67 @@ void HostProcess::ShutdownHost(HostExitCodes exit_code) {
   *exit_code_out_ = exit_code;
 
   switch (state_) {
-    case HOST_INITIALIZING:
-      state_ = HOST_STOPPING;
-      DCHECK(!host_signaling_manager_);
-      host_signaling_manager_ = CreateHostSignalingManager();
-      SendOfflineReasonAndShutdownOnNetworkThread(exit_code);
-      break;
-
+    case HOST_STARTING:
     case HOST_STARTED:
-      state_ = HOST_STOPPING;
-      SendOfflineReasonAndShutdownOnNetworkThread(exit_code);
+      SetState(HOST_GOING_OFFLINE_TO_STOP);
+      GoOffline(ExitCodeToString(exit_code));
       break;
 
-    case HOST_STOPPING_TO_RESTART:
-      state_ = HOST_STOPPING;
+    case HOST_GOING_OFFLINE_TO_RESTART:
+      SetState(HOST_GOING_OFFLINE_TO_STOP);
       break;
 
-    case HOST_STOPPING:
+    case HOST_GOING_OFFLINE_TO_STOP:
     case HOST_STOPPED:
       // Host is already stopped or being stopped. No action is required.
       break;
   }
 }
 
-void HostProcess::ShutdownOnNetworkThread() {
+void HostProcess::GoOffline(const std::string& host_offline_reason) {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+  DCHECK(!host_offline_reason.empty());
+  DCHECK((state_ == HOST_GOING_OFFLINE_TO_STOP) ||
+         (state_ == HOST_GOING_OFFLINE_TO_RESTART));
 
+  // 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();
 
-  if (state_ == HOST_STOPPING_TO_RESTART) {
-    StartHost();
-  } else if (state_ == HOST_STOPPING) {
-    state_ = HOST_STOPPED;
+  // Before shutting down HostSignalingManager, send the |host_offline_reason|
+  // if possible (i.e. if we have the config).
+  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, this));
+    return;  // Shutdown will resume after OnHostOfflineReasonAck.
+  }
+
+  // Continue the shutdown without sending the host offline reason.
+  HOST_LOG << "SendHostOfflineReason( " << host_offline_reason << ") "

[Sergey Ulanov, 2015/02/18 00:28:31]

Maybe something like "Can't send offline reason (<the reason>) without a valid
host config"?

[Łukasz Anforowicz, 2015/02/18 17:03:11]

Done.

+           << "not possible without a config...";
+  OnHostOfflineReasonAck(false);
+}
+
+void HostProcess::OnHostOfflineReasonAck(bool success) {
+  DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+  DCHECK(!host_);  // Assert that the host is really offline at this point.
+
+  HOST_LOG << "SendHostOfflineReason " << (success ? "succeeded." : "failed.");
+  host_signaling_manager_.reset();
+
+  if (state_ == HOST_GOING_OFFLINE_TO_RESTART) {
+    SetState(HOST_STARTING);
+    StartHostIfReady();
+  } else if (state_ == HOST_GOING_OFFLINE_TO_STOP) {
+    SetState(HOST_STOPPED);
 
     shutdown_watchdog_->SetExitCode(*exit_code_out_);
     shutdown_watchdog_->Arm();
@@ -1479,7 +1561,6 @@ void HostProcess::ShutdownOnNetworkThread() {
     context_->ui_task_runner()->PostTask(
         FROM_HERE, base::Bind(&HostProcess::ShutdownOnUiThread, this));
   } else {
-    // This method is only called in STOPPING_TO_RESTART and STOPPING states.
     NOTREACHED();
   }
 }