Chromoting: stopping the service if a permanent error is encountered (such as the host ID is permanently not recognized by the could or invalid host configuration).

BUG=123715,112160

Review URL: http://codereview.chromium.org/10106013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@132675 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 125 insertions, 49 deletions

diff --git a/remoting/host/constants.h b/remoting/host/constants.h
new file mode 100644
index 0000000..6f36b94
--- /dev/null
+++ b/remoting/host/constants.h
@@ -0,0 +1,26 @@
+// 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.
+
+#ifndef REMOTING_HOST_CONSTANTS_H_
+#define REMOTING_HOST_CONSTANTS_H_
+
+namespace remoting {
+
+// Known host exit codes.
+enum HostExitCodes {
+  kSuccessExitCode = 0,
+  kReservedForX11ExitCode = 1,
+  kInvalidHostConfigurationExitCode = 2,
+  kInvalidHostIdExitCode = 3,
+  kInvalidOauthCredentialsExitCode = 4,
+
+  // The range of the exit codes that should be interpreted as a permanent error
+  // condition.
+  kMinPermanentErrorExitCode = kInvalidHostConfigurationExitCode,
+  kMaxPermanentErrorExitCode = kInvalidOauthCredentialsExitCode
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_CONSTANTS_H_
diff --git a/remoting/host/heartbeat_sender.cc b/remoting/host/heartbeat_sender.cc
index e061acc..8420acf 100644
--- a/remoting/host/heartbeat_sender.cc
+++ b/remoting/host/heartbeat_sender.cc
@@ -13,6 +13,7 @@
 #include "base/string_number_conversions.h"
 #include "base/time.h"
 #include "remoting/base/constants.h"
+#include "remoting/host/constants.h"
 #include "remoting/jingle_glue/iq_sender.h"
 #include "remoting/jingle_glue/jingle_thread.h"
 #include "remoting/jingle_glue/signal_strategy.h"
@@ -43,15 +44,15 @@ const int64 kResendDelayMs = 10 * 1000;  // 10 seconds.
 const int64 kResendDelayOnHostNotFoundMs = 10 * 1000; // 10 seconds.
 const int kMaxResendOnHostNotFoundCount = 12;  // 2 minutes (12 x 10 seconds).
 
-const int kExitCodeHostIdInvalid = 100;
-
 }  // namespace
 
 HeartbeatSender::HeartbeatSender(
+    Listener* listener,
     const std::string& host_id,
     SignalStrategy* signal_strategy,
     HostKeyPair* key_pair)
-    : host_id_(host_id),
+    : listener_(listener),
+      host_id_(host_id),
       signal_strategy_(signal_strategy),
       key_pair_(key_pair),
       interval_ms_(kDefaultHeartbeatIntervalMs),
@@ -134,11 +135,8 @@ void HeartbeatSender::ProcessResponse(IqRequest* request,
                               &HeartbeatSender::ResendStanza);
           return;
         }
-        // TODO(lambroslambrou): Trigger an application-defined callback to
-        // shut down the host properly, instead of just exiting here
-        // (http://crbug.com/112160).
-        LOG(ERROR) << "Exit: Host ID not found";
-        exit(kExitCodeHostIdInvalid);
+        listener_->OnUnknownHostIdError();
+        return;
       }
     }
 
diff --git a/remoting/host/heartbeat_sender.h b/remoting/host/heartbeat_sender.h
index f2f5a47..daa7ed8 100644
--- a/remoting/host/heartbeat_sender.h
+++ b/remoting/host/heartbeat_sender.h
@@ -79,10 +79,18 @@ class IqSender;
 //  </iq>
 class HeartbeatSender : public SignalStrategy::Listener {
  public:
-  // |signal_strategy| and |key_pair| must outlive this
+  class Listener {
+   public:
+    virtual ~Listener() { }
+    // Invoked when the host ID is permanently not recognized by the server.
+    virtual void OnUnknownHostIdError() = 0;
+  };
+
+  // |signal_strategy|, |key_pair| and |delegate| must outlive this
   // object. Heartbeats will start when the supplied SignalStrategy
   // enters the CONNECTED state.
-  HeartbeatSender(const std::string& host_id,
+  HeartbeatSender(Listener* listener,
+                  const std::string& host_id,
                   SignalStrategy* signal_strategy,
                   HostKeyPair* key_pair);
   virtual ~HeartbeatSender();
@@ -111,6 +119,7 @@ class HeartbeatSender : public SignalStrategy::Listener {
   scoped_ptr<buzz::XmlElement> CreateHeartbeatMessage();
   scoped_ptr<buzz::XmlElement> CreateSignature();
 
+  Listener* listener_;
   std::string host_id_;
   SignalStrategy* signal_strategy_;
   HostKeyPair* key_pair_;
diff --git a/remoting/host/heartbeat_sender_unittest.cc b/remoting/host/heartbeat_sender_unittest.cc
index f94726d..63b4499 100644
--- a/remoting/host/heartbeat_sender_unittest.cc
+++ b/remoting/host/heartbeat_sender_unittest.cc
@@ -47,8 +47,15 @@ ACTION_P(RemoveListener, list) {
   list->erase(arg0);
 }
 
-class HeartbeatSenderTest : public testing::Test {
+class HeartbeatSenderTest
+    : public testing::Test,
+      public HeartbeatSender::Listener {
  protected:
+  // Overridden from HeartbeatSender::Listener
+  virtual void OnUnknownHostIdError() OVERRIDE {
+    NOTREACHED();
+  }
+
   virtual void SetUp() OVERRIDE {
     ASSERT_TRUE(key_pair_.LoadFromString(kTestHostKeyPair));
 
@@ -62,7 +69,7 @@ class HeartbeatSenderTest : public testing::Test {
         .WillRepeatedly(Return(kTestJid));
 
     heartbeat_sender_.reset(
-        new HeartbeatSender(kHostId, &signal_strategy_, &key_pair_));
+        new HeartbeatSender(this, kHostId, &signal_strategy_, &key_pair_));
   }
 
   virtual void TearDown() OVERRIDE {
diff --git a/remoting/host/remoting_me2me_host.cc b/remoting/host/remoting_me2me_host.cc
index c71b8e5..cc5438d 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -26,6 +26,7 @@
 #include "net/base/network_change_notifier.h"
 #include "remoting/base/constants.h"
 #include "remoting/host/branding.h"
+#include "remoting/host/constants.h"
 #include "remoting/host/capturer.h"
 #include "remoting/host/chromoting_host.h"
 #include "remoting/host/chromoting_host_context.h"
@@ -51,9 +52,6 @@
 
 namespace {
 
-const int kSuccessExitCode = 0;
-const int kInvalidHostConfigurationExitCode = 1;
-
 // This is used for tagging system event logs.
 const char kApplicationName[] = "chromoting";
 
@@ -74,12 +72,15 @@ const int kMaxPortNumber = 12409;
 
 namespace remoting {
 
-class HostProcess : public OAuthClient::Delegate {
+class HostProcess
+    : public OAuthClient::Delegate,
+      public HeartbeatSender::Listener {
  public:
   HostProcess()
       : message_loop_(MessageLoop::TYPE_UI),
         allow_nat_traversal_(true),
-        restarting_(false) {
+        restarting_(false),
+        exit_code_(kSuccessExitCode) {
     context_.reset(
         new ChromotingHostContext(message_loop_.message_loop_proxy()));
     context_->Start();
@@ -194,7 +195,7 @@ class HostProcess : public OAuthClient::Delegate {
 #endif
     message_loop_.Run();
 
-    return kSuccessExitCode;
+    return exit_code_;
   }
 
   // Overridden from OAuthClient::Delegate
@@ -217,6 +218,13 @@ class HostProcess : public OAuthClient::Delegate {
 
   virtual void OnOAuthError() OVERRIDE {
     LOG(ERROR) << "OAuth: invalid credentials.";
+    Shutdown(kInvalidOauthCredentialsExitCode);
+  }
+
+  // Overridden from HeartbeatSender::Listener
+  virtual void OnUnknownHostIdError() OVERRIDE {
+    LOG(ERROR) << "Host ID not found.";
+    Shutdown(kInvalidHostIdExitCode);
   }
 
  private:
@@ -335,8 +343,8 @@ class HostProcess : public OAuthClient::Delegate {
         context_.get(), signal_strategy_.get(), desktop_environment_.get(),
         network_settings);
 
-    heartbeat_sender_.reset(
-        new HeartbeatSender(host_id_, signal_strategy_.get(), &key_pair_));
+    heartbeat_sender_.reset(new HeartbeatSender(
+        this, host_id_, signal_strategy_.get(), &key_pair_));
 
     log_to_server_.reset(
         new LogToServer(host_, ServerLogEntry::ME2ME, signal_strategy_.get()));
@@ -371,6 +379,11 @@ class HostProcess : public OAuthClient::Delegate {
     StartHost();
   }
 
+  void Shutdown(int exit_code) {
+    exit_code_ = exit_code;
+    message_loop_.PostTask(FROM_HERE, MessageLoop::QuitClosure());
+  }
+
   MessageLoop message_loop_;
   scoped_ptr<ChromotingHostContext> context_;
   scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
@@ -402,6 +415,8 @@ class HostProcess : public OAuthClient::Delegate {
   scoped_ptr<LogToServer> log_to_server_;
   scoped_ptr<HostEventLogger> host_event_logger_;
   scoped_refptr<ChromotingHost> host_;
+
+  int exit_code_;
 };
 
 }  // namespace remoting
diff --git a/remoting/host/simple_host_process.cc b/remoting/host/simple_host_process.cc
index d9fba33..eccc04b 100644
--- a/remoting/host/simple_host_process.cc
+++ b/remoting/host/simple_host_process.cc
@@ -89,7 +89,7 @@ const char kVideoSwitchValueVp8Rtp[] = "vp8rtp";
 
 namespace remoting {
 
-class SimpleHost {
+class SimpleHost : public HeartbeatSender::Listener {
  public:
   SimpleHost()
       : message_loop_(MessageLoop::TYPE_UI),
@@ -100,6 +100,12 @@ class SimpleHost {
     network_change_notifier_.reset(net::NetworkChangeNotifier::Create());
   }
 
+  // Overridden from HeartbeatSender::Listener
+  virtual void OnUnknownHostIdError() OVERRIDE {
+    LOG(ERROR) << "Host ID not found.";
+    Shutdown();
+  }
+
   int Run() {
     FilePath config_path = GetConfigPath();
     JsonHostConfig config(config_path);
@@ -244,8 +250,8 @@ class SimpleHost {
           signal_strategy_.get(), &key_pair_,
           base::Bind(&SimpleHost::SetIT2MeAccessCode, host_, &key_pair_)));
     } else {
-      heartbeat_sender_.reset(
-          new HeartbeatSender(host_id_, signal_strategy_.get(), &key_pair_));
+      heartbeat_sender_.reset(new HeartbeatSender(
+          this, host_id_, signal_strategy_.get(), &key_pair_));
     }
 
     host_->Start();
@@ -260,6 +266,10 @@ class SimpleHost {
     }
   }
 
+  void Shutdown() {
+    message_loop_.PostTask(FROM_HERE, MessageLoop::QuitClosure());
+  }
+
   MessageLoop message_loop_;
   ChromotingHostContext context_;
   scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
diff --git a/remoting/host/url_request_context.cc b/remoting/host/url_request_context.cc
index 83451a7..0e23350 100644
--- a/remoting/host/url_request_context.cc
+++ b/remoting/host/url_request_context.cc
@@ -14,22 +14,22 @@
 #include "net/http/http_server_properties_impl.h"
 #include "net/proxy/proxy_config_service.h"
 #include "net/proxy/proxy_service.h"
+#include "remoting/host/vlog_net_log.h"
 
 namespace remoting {
 
 // TODO(willchan): This is largely copied from service_url_request_context.cc,
 // which is in turn copied from some test code. Move it somewhere reusable.
 URLRequestContext::URLRequestContext(
-    net::ProxyConfigService* proxy_config_service)
+    scoped_ptr<net::ProxyConfigService> proxy_config_service)
     : ALLOW_THIS_IN_INITIALIZER_LIST(storage_(this)) {
-  net_log_.reset(new VlogNetLog());
-
+  scoped_ptr<VlogNetLog> net_log(new VlogNetLog());
   storage_.set_host_resolver(
       net::CreateSystemHostResolver(net::HostResolver::kDefaultParallelism,
                                     net::HostResolver::kDefaultRetryAttempts,
-                                    net_log_.get()));
+                                    net_log.get()));
   storage_.set_proxy_service(net::ProxyService::CreateUsingSystemProxyResolver(
-      proxy_config_service, 0u, net_log_.get()));
+      proxy_config_service.release(), 0u, net_log.get()));
   storage_.set_cert_verifier(net::CertVerifier::CreateDefault());
   storage_.set_ssl_config_service(new net::SSLConfigServiceDefaults);
   storage_.set_http_auth_handler_factory(
@@ -43,11 +43,12 @@ URLRequestContext::URLRequestContext(
   session_params.ssl_config_service = ssl_config_service();
   session_params.http_auth_handler_factory = http_auth_handler_factory();
   session_params.http_server_properties = http_server_properties();
-  session_params.net_log = net_log_.get();
+  session_params.net_log = net_log.get();
   scoped_refptr<net::HttpNetworkSession> network_session(
       new net::HttpNetworkSession(session_params));
   storage_.set_http_transaction_factory(
       new net::HttpNetworkLayer(network_session));
+  storage_.set_net_log(net_log.release());
 }
 
 URLRequestContext::~URLRequestContext() {
@@ -63,9 +64,10 @@ URLRequestContextGetter::URLRequestContextGetter(
 }
 
 net::URLRequestContext* URLRequestContextGetter::GetURLRequestContext() {
-  if (!url_request_context_)
+  if (!url_request_context_) {
     url_request_context_ =
-        new URLRequestContext(proxy_config_service_.get());
+        new URLRequestContext(proxy_config_service_.Pass());
+  }
   return url_request_context_;
 }
 
diff --git a/remoting/host/url_request_context.h b/remoting/host/url_request_context.h
index 1a5cecb..372d5ee 100644
--- a/remoting/host/url_request_context.h
+++ b/remoting/host/url_request_context.h
@@ -14,7 +14,6 @@
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_context_storage.h"
-#include "remoting/host/vlog_net_log.h"
 
 namespace base {
 class MessageLoopProxy;
@@ -27,13 +26,13 @@ namespace remoting {
 // remoting Me2Me host process where the profile is not available.
 class URLRequestContext : public net::URLRequestContext {
  public:
-  explicit URLRequestContext(net::ProxyConfigService* net_proxy_config_service);
+  explicit URLRequestContext(
+      scoped_ptr<net::ProxyConfigService> proxy_config_service);
 
  private:
   virtual ~URLRequestContext();
 
   net::URLRequestContextStorage storage_;
-  scoped_ptr<VlogNetLog> net_log_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequestContext);
 };
diff --git a/remoting/host/wts_session_process_launcher_win.cc b/remoting/host/wts_session_process_launcher_win.cc
index c417144..fe1d7d7f 100644
--- a/remoting/host/wts_session_process_launcher_win.cc
+++ b/remoting/host/wts_session_process_launcher_win.cc
@@ -27,7 +27,7 @@
 #include "ipc/ipc_channel_proxy.h"
 #include "ipc/ipc_message.h"
 #include "ipc/ipc_message_macros.h"
-
+#include "remoting/host/constants.h"
 #include "remoting/host/chromoting_messages.h"
 #include "remoting/host/sas_injector.h"
 #include "remoting/host/wts_console_monitor_win.h"
@@ -37,10 +37,6 @@ using base::TimeDelta;
 
 namespace {
 
-// The exit code returned by the host process when its configuration is not
-// valid.
-const int kInvalidHostConfigurationExitCode = 1;
-
 // The minimum and maximum delays between attempts to inject host process into
 // a session.
 const int kMaxLaunchDelaySeconds = 60;
@@ -352,9 +348,11 @@ void WtsSessionProcessLauncher::OnObjectSignaled(HANDLE object) {
 
   // Stop trying to restart the host if its process exited due to
   // misconfiguration.
-  DWORD exit_code;
-  bool stop_trying = GetExitCodeProcess(process_.handle(), &exit_code) &&
-                     exit_code == kInvalidHostConfigurationExitCode;
+  int exit_code;
+  bool stop_trying =
+      base::WaitForExitCodeWithTimeout(process_.handle(), &exit_code, 0) &&
+      kMinPermanentErrorExitCode <= exit_code &&
+      exit_code <= kMaxPermanentErrorExitCode;
 
   // The host process has been terminated for some reason. The handle can now be
   // closed.
diff --git a/remoting/remoting.gyp b/remoting/remoting.gyp
index 040554d..6f501e8 100644
--- a/remoting/remoting.gyp
+++ b/remoting/remoting.gyp
@@ -308,6 +308,7 @@
             'host/branding.h',
             'host/chromoting_messages.cc',
             'host/chromoting_messages.h',
+            'host/constants.h',
             'host/host_service.rc',
             'host/host_service_resource.h',
             'host/host_service_win.cc',
@@ -818,6 +819,7 @@
         'host/chromoting_host_context.h',
         'host/client_session.cc',
         'host/client_session.h',
+        'host/constants.h',
         'host/continue_window.h',
         'host/continue_window_mac.mm',
         'host/continue_window_linux.cc',
@@ -1033,8 +1035,8 @@
         'host/branding.cc',
         'host/branding.h',
         'host/host_event_logger.h',
-	'host/sighup_listener_mac.cc',
-	'host/sighup_listener_mac.h',
+        'host/sighup_listener_mac.cc',
+        'host/sighup_listener_mac.h',
         'host/remoting_me2me_host.cc',
       ],
       'conditions': [
diff --git a/remoting/tools/me2me_virtual_host.py b/remoting/tools/me2me_virtual_host.py
index 69994d0..f797648 100755
--- a/remoting/tools/me2me_virtual_host.py
+++ b/remoting/tools/me2me_virtual_host.py
@@ -731,14 +731,24 @@ def main():
       logging.info("Host process terminated")
       desktop.host_proc = None
 
-      # The exit-code must match the one used in HeartbeatSender.
-      if os.WEXITSTATUS(status) == 100:
+      # These exit-codes must match the ones used by the host.
+      # See remoting/host/constants.h.
+      # Delete the host or auth configuration depending on the returned error
+      # code, so the next time this script is run, a new configuration
+      # will be created and registered.
+      if os.WEXITSTATUS(status) == 2:
+        logging.info("Host configuration is invalid - exiting.")
+        os.remove(auth.config_file)
+        os.remove(host.config_file)
+        return 0
+      elif os.WEXITSTATUS(status) == 3:
         logging.info("Host ID has been deleted - exiting.")
-        # Host config is no longer valid.  Delete it, so the next time this
-        # script is run, a new Host ID will be created and registered.
         os.remove(host.config_file)
         return 0
-
+      elif os.WEXITSTATUS(status) == 4:
+        logging.info("OAuth credentials are invalid - exiting.")
+        os.remove(auth.config_file)
+        return 0
 
 if __name__ == "__main__":
   logging.basicConfig(level=logging.DEBUG)