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

remoting/host/remoting_me2me_host.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // This file implements a standalone host process for Me2Me.
 
 #include <string>
 
 #include "base/at_exit.h"
 #include "base/bind.h"
@@ skipping 129 matching lines @@
 const char kWindowIdSwitchName[] = "window-id";
 
 // Maximum time to wait for clean shutdown to occur, before forcing termination
 // of the process.
 const int kShutdownTimeoutSeconds = 15;
 
 // Maximum time to wait for reporting host-offline-reason to the service,
 // 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 {
 
 class HostProcess : public ConfigWatcher::Delegate,
                     public HostSignalingManager::Listener,
                     public HostChangeNotificationListener::Listener,
                     public IPC::Listener,
                     public base::RefCountedThreadSafe<HostProcess> {
  public:
@@ skipping 46 matching lines @@
     //   STARTED->STOPPING
     //   STOPPING_TO_RESTART->STARTED
     //   STOPPING_TO_RESTART->STOPPING
     //   STOPPING->STOPPED
     //   STOPPED->STARTED
     //
     // |host_| must be nullptr in INITIALIZING and STOPPED states and not
     // 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;
 
   void StartOnNetworkThread();
 
 #if defined(OS_POSIX)
   // Callback passed to RegisterSignalHandler() to handle SIGTERM events.
   void SigTermHandler(int signal_number);
 #endif
 
@@ skipping 12 matching lines @@
 
   // Tear down resources that run on the UI thread.
   void ShutdownOnUiThread();
 
   // Applies the host config, returning true if successful.
   bool ApplyConfig(const base::DictionaryValue& config);
 
   // 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);
   bool OnUsernamePolicyUpdate(base::DictionaryValue* policies);
   bool OnNatPolicyUpdate(base::DictionaryValue* policies);
   bool OnRelayPolicyUpdate(base::DictionaryValue* policies);
   bool OnUdpPortPolicyUpdate(base::DictionaryValue* policies);
   bool OnCurtainPolicyUpdate(base::DictionaryValue* policies);
   bool OnHostTalkGadgetPrefixPolicyUpdate(base::DictionaryValue* policies);
   bool OnHostTokenUrlPolicyUpdate(base::DictionaryValue* policies);
   bool OnPairingPolicyUpdate(base::DictionaryValue* policies);
   bool OnGnubbyAuthPolicyUpdate(base::DictionaryValue* policies);
 
   scoped_ptr<HostSignalingManager> CreateHostSignalingManager();
 
   void StartHostIfReady();
   void StartHost();
 
   // Overrides for HostSignalingManager::Listener interface.
   void OnHeartbeatSuccessful() override;
   void OnUnknownHostIdError() override;
   void OnAuthFailed() override;
 
   void RestartHost();
 
   // 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
   // network thread.
   void InitializePairingRegistry(
       IPC::PlatformFileForTransit privileged_key,
       IPC::PlatformFileForTransit unprivileged_key);
 #endif  // defined(OS_WIN)
 
   // Crashes the process in response to a daemon's request. The daemon passes
@@ skipping 27 matching lines @@
   scoped_refptr<RsaKeyPair> key_pair_;
   std::string oauth_refresh_token_;
   std::string serialized_config_;
   std::string host_owner_;
   std::string host_owner_email_;
   bool use_service_account_;
   bool enable_vp9_;
   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_;
   bool allow_relay_;
   uint16 min_udp_port_;
   uint16 max_udp_port_;
   std::string talkgadget_prefix_;
   bool allow_pairing_;
 
   bool curtain_required_;
@@ skipping 35 matching lines @@
 };
 
 HostProcess::HostProcess(scoped_ptr<ChromotingHostContext> context,
                          int* exit_code_out,
                          ShutdownWatchdog* shutdown_watchdog)
     : context_(context.Pass()),
       state_(HOST_INITIALIZING),
       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),
       min_udp_port_(0),
       max_udp_port_(0),
       allow_pairing_(true),
       curtain_required_(false),
       enable_gnubby_auth_(false),
       enable_window_capture_(false),
       window_id_(0),
@@ skipping 149 matching lines @@
     ShutdownHost(kInvalidHostConfigurationExitCode);
     return;
   }
 
   if (!ApplyConfig(*config)) {
     LOG(ERROR) << "Failed to apply the configuration.";
     ShutdownHost(kInvalidHostConfigurationExitCode);
     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();
 
     // TODO(sergeyu): Here we assume that PIN is the only part of the config
     // that may change while the service is running. Change ApplyConfig() to
     // detect other changes in the config and restart host if necessary here.
     CreateAuthenticatorFactory();
   }
 }
 
@@ skipping 404 matching lines @@
   // Note: UsernamePolicyUpdate must run after OnCurtainPolicyUpdate.
   restart_required |= OnUsernamePolicyUpdate(policies.get());
   restart_required |= OnNatPolicyUpdate(policies.get());
   restart_required |= OnRelayPolicyUpdate(policies.get());
   restart_required |= OnUdpPortPolicyUpdate(policies.get());
   restart_required |= OnHostTalkGadgetPrefixPolicyUpdate(policies.get());
   restart_required |= OnHostTokenUrlPolicyUpdate(policies.get());
   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)
       RestartHost();
   }
 }
 
 void HostProcess::OnPolicyError() {
   if (!context_->network_task_runner()->BelongsToCurrentThread()) {
     context_->network_task_runner()->PostTask(
         FROM_HERE, base::Bind(&HostProcess::OnPolicyError, this));
     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() {
   if (state_ != HOST_STARTED)
     return;
 
   HOST_LOG << "Policy sets host domain: " << host_domain_;
 
   if (!host_domain_.empty()) {
     // If the user does not have a Google email, their client JID will not be
@@ skipping 281 matching lines @@
 }
 
 scoped_ptr<HostSignalingManager> HostProcess::CreateHostSignalingManager() {
   DCHECK(!host_id_.empty());  // |ApplyConfig| should already have been run.
 
   scoped_ptr<OAuthTokenGetter::OAuthCredentials> oauth_credentials(
       new OAuthTokenGetter::OAuthCredentials(xmpp_server_config_.username,
                                              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() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
   DCHECK(!host_);
   DCHECK(!host_signaling_manager_);
 
   DCHECK(state_ == HOST_INITIALIZING || state_ == HOST_STOPPING_TO_RESTART ||
          state_ == HOST_STOPPED)
       << "state_ = " << state_;
@@ skipping 80 matching lines @@
 
 void HostProcess::OnAuthFailed() {
   ShutdownHost(kInvalidOauthCredentialsExitCode);
 }
 
 void HostProcess::RestartHost() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
   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:
       state_ = HOST_STOPPING;
       break;
 
     case HOST_STOPPING:
     case HOST_STOPPED:
       // Host is already stopped or being stopped. No action is required.
       break;
   }
 }
 
-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;
 
     shutdown_watchdog_->SetExitCode(*exit_code_out_);
     shutdown_watchdog_->Arm();
 
     config_watcher_.reset();
 
     // Complete the rest of shutdown on the main thread.
     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();
   }
 }
 
+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) {
   char message[1024];
   base::snprintf(message, sizeof(message),
                  "Requested by %s at %s, line %d.",
                  function_name.c_str(), file_name.c_str(), line_number);
   base::debug::Alias(message);
 
   // The daemon requested us to crash the process.
@@ skipping 45 matching lines @@
       base::TimeDelta::FromSeconds(kShutdownTimeoutSeconds));
   new HostProcess(context.Pass(), &exit_code, &shutdown_watchdog);
 
   // Run the main (also UI) message loop until the host no longer needs it.
   message_loop.Run();
 
   return exit_code;
 }
 
 }  // namespace remoting