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 195 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);

[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
   // 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(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) {
     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();
+    } 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() {
   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(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:
       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(
+    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;
 
     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());

[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) {
   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