The Chromoting service should not start automatically unless it was configured from the webapp to do so. This way the service will not try to load unconfigured host after reboot. The installer should also respect this preserving the service start type when upgrading the installation.

The service should also stop automatically if a configuration error was detected. The UI will restart the service once configuration is recreated.


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

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

7 files changed, 171 insertions, 31 deletions

diff --git a/remoting/host/elevated_controller_win.cc b/remoting/host/elevated_controller_win.cc
index 1a8bec6..86c0aa4d 100644
--- a/remoting/host/elevated_controller_win.cc
+++ b/remoting/host/elevated_controller_win.cc
@@ -228,6 +228,26 @@ STDMETHODIMP ElevatedControllerWin::StartDaemon() {
     return hr;
   }
 
+  // Change the service start type to 'auto'.
+  if (!::ChangeServiceConfigW(service,
+                              SERVICE_NO_CHANGE,
+                              SERVICE_AUTO_START,
+                              SERVICE_NO_CHANGE,
+                              NULL,
+                              NULL,
+                              NULL,
+                              NULL,
+                              NULL,
+                              NULL,
+                              NULL)) {
+    DWORD error = GetLastError();
+    LOG_GETLASTERROR(ERROR)
+        << "Failed to change the '" << kWindowsServiceName
+        << "'service start type to 'auto'";
+    return HRESULT_FROM_WIN32(error);
+  }
+
+  // Start the service.
   if (!StartService(service, 0, NULL)) {
     DWORD error = GetLastError();
     if (error != ERROR_SERVICE_ALREADY_RUNNING) {
@@ -248,6 +268,26 @@ STDMETHODIMP ElevatedControllerWin::StopDaemon() {
     return hr;
   }
 
+  // Change the service start type to 'manual'.
+  if (!::ChangeServiceConfigW(service,
+                              SERVICE_NO_CHANGE,
+                              SERVICE_DEMAND_START,
+                              SERVICE_NO_CHANGE,
+                              NULL,
+                              NULL,
+                              NULL,
+                              NULL,
+                              NULL,
+                              NULL,
+                              NULL)) {
+    DWORD error = GetLastError();
+    LOG_GETLASTERROR(ERROR)
+        << "Failed to change the '" << kWindowsServiceName
+        << "'service start type to 'manual'";
+    return HRESULT_FROM_WIN32(error);
+  }
+
+  // Stop the service.
   SERVICE_STATUS status;
   if (!ControlService(service, SERVICE_CONTROL_STOP, &status)) {
     DWORD error = GetLastError();
@@ -279,9 +319,11 @@ HRESULT ElevatedControllerWin::OpenService(ScopedScHandle* service_out) {
     return HRESULT_FROM_WIN32(error);
   }
 
+  DWORD desired_access = SERVICE_CHANGE_CONFIG | SERVICE_QUERY_STATUS |
+                         SERVICE_START | SERVICE_STOP;
   ScopedScHandle service(
       ::OpenServiceW(scmanager, UTF8ToUTF16(kWindowsServiceName).c_str(),
-                     SERVICE_QUERY_STATUS | SERVICE_START | SERVICE_STOP));
+                     desired_access));
   if (!service.IsValid()) {
     error = GetLastError();
     LOG_GETLASTERROR(ERROR)
diff --git a/remoting/host/host_service_win.cc b/remoting/host/host_service_win.cc
index 52cb909..6a455df 100644
--- a/remoting/host/host_service_win.cc
+++ b/remoting/host/host_service_win.cc
@@ -112,11 +112,20 @@ HostService::~HostService() {
 }
 
 void HostService::AddWtsConsoleObserver(WtsConsoleObserver* observer) {
+  DCHECK(message_loop_->message_loop_proxy()->BelongsToCurrentThread());
+
   console_observers_.AddObserver(observer);
 }
 
 void HostService::RemoveWtsConsoleObserver(WtsConsoleObserver* observer) {
+  DCHECK(message_loop_->message_loop_proxy()->BelongsToCurrentThread());
+
   console_observers_.RemoveObserver(observer);
+
+  // Stop the service if there are no more observers.
+  if (!console_observers_.might_have_observers()) {
+    message_loop_->PostTask(FROM_HERE, MessageLoop::QuitClosure());
+  }
 }
 
 void HostService::OnSessionChange() {
@@ -357,12 +366,15 @@ void HostService::RunMessageLoop() {
   base::Thread io_thread(kIoThreadName);
   base::Thread::Options io_thread_options(MessageLoop::TYPE_IO, 0);
   if (!io_thread.StartWithOptions(io_thread_options)) {
+    LOG(FATAL) << "Failed to start the I/O thread";
     shutting_down_ = true;
     stopped_event_.Signal();
     return;
   }
 
-  WtsSessionProcessLauncher launcher(this, host_binary_, &io_thread);
+  WtsSessionProcessLauncher launcher(this, host_binary_,
+                                     message_loop_->message_loop_proxy(),
+                                     io_thread.message_loop_proxy());
 
   // Run the service.
   message_loop_->Run();
diff --git a/remoting/host/host_service_win.h b/remoting/host/host_service_win.h
index f39bf7c..e83988c 100644
--- a/remoting/host/host_service_win.h
+++ b/remoting/host/host_service_win.h
@@ -98,9 +98,6 @@ class HostService : public WtsConsoleMonitor {
   // The service name.
   string16 service_name_;
 
-  // The service status structure.
-  SERVICE_STATUS service_status_;
-
   // The service status handle.
   SERVICE_STATUS_HANDLE service_status_handle_;
 
diff --git a/remoting/host/installer/chromoting.wxs b/remoting/host/installer/chromoting.wxs
index d546a44..4fc1c89 100644
--- a/remoting/host/installer/chromoting.wxs
+++ b/remoting/host/installer/chromoting.wxs
@@ -108,7 +108,7 @@
                         DisplayName="@[#remoting_service.exe],-101"
                         Description="@[#remoting_service.exe],-102"
                         Arguments="--host-binary=&quot;[#remoting_me2me_host.exe]&quot; --auth-config=&quot;[config_files]host.json&quot; --host-config=&quot;[config_files]host.json&quot;"
-                        Start="auto"
+                        Start="demand"
                         Account="LocalSystem"
                         ErrorControl="ignore"
                         Interactive="no" />
@@ -287,6 +287,35 @@
       </Component>
     </DirectoryRef>
 
+    <!-- The service is always installed in the stopped state with start type
+         set to 'manual'. This becomes a problem when upgrading an existing
+         installation that is configured to start the service automatically.
+
+         Here we check the startup type before making any changes, then restart
+         the service and change its startup type as needed once the installation
+         is finished. -->
+    <Property Id="CHROMOTING_SERVICE_START_TYPE">
+      <RegistrySearch Id="chromoting_service_start_type"
+                      Root="HKLM"
+                      Key="SYSTEM\CurrentControlSet\services\$(var.ServiceName)"
+                      Name="Start"
+                      Type="raw" />
+    </Property>
+
+    <CustomAction Id="query_auto_start_service"
+                  Property="auto_start_service"
+                  Value="[CHROMOTING_SERVICE_START_TYPE]" />
+
+    <CustomAction Id="set_auto_start_service"
+                  Impersonate="no"
+                  Execute="deferred"
+                  Script="jscript">
+      <![CDATA[
+        var controller = new ActiveXObject("$(var.ControllerProgid)");
+        controller.StartDaemon();
+      ]]>
+    </CustomAction>
+
     <UIRef Id="WixUI_ErrorProgressText" />
 
     <Feature Id="chromoting_host" Level="1" Title="$(var.ChromotingHost)">
@@ -296,5 +325,12 @@
       <ComponentRef Id="service_controller"/>
     </Feature>
 
+  <InstallExecuteSequence>
+    <Custom Action="query_auto_start_service" Before="InstallInitialize"/>
+    <Custom Action="set_auto_start_service" After="StartServices">
+      <![CDATA[NOT REMOVE AND (auto_start_service = "#2")]]>
+    </Custom>
+  </InstallExecuteSequence>
+
   </Product>
 </Wix>
diff --git a/remoting/host/remoting_me2me_host.cc b/remoting/host/remoting_me2me_host.cc
index 6e1d57a..ac73694 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -50,6 +50,9 @@
 
 namespace {
 
+const int kSuccessExitCode = 0;
+const int kInvalidHostConfigurationExitCode = 1;
+
 // This is used for tagging system event logs.
 const char kApplicationName[] = "chromoting";
 
@@ -138,7 +141,7 @@ class HostProcess : public OAuthClient::Delegate {
   int Run() {
     bool tokens_pending = false;
     if (!LoadConfig(file_io_thread_.message_loop_proxy(), &tokens_pending)) {
-      return 1;
+      return kInvalidHostConfigurationExitCode;
     }
     if (tokens_pending) {
       // If we have an OAuth refresh token, then XmppSignalStrategy can't
@@ -159,7 +162,7 @@ class HostProcess : public OAuthClient::Delegate {
 #endif
     message_loop_.Run();
 
-    return 0;
+    return kSuccessExitCode;
   }
 
   // Overridden from OAuthClient::Delegate
diff --git a/remoting/host/wts_session_process_launcher_win.cc b/remoting/host/wts_session_process_launcher_win.cc
index 95b5b5b..645fd03 100644
--- a/remoting/host/wts_session_process_launcher_win.cc
+++ b/remoting/host/wts_session_process_launcher_win.cc
@@ -11,13 +11,15 @@
 #include <sddl.h>
 #include <limits>
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
+#include "base/message_loop_proxy.h"
 #include "base/process_util.h"
 #include "base/rand_util.h"
 #include "base/string16.h"
 #include "base/stringprintf.h"
-#include "base/threading/thread.h"
 #include "base/utf_string_conversions.h"
 #include "base/win/scoped_handle.h"
 #include "base/win/scoped_process_information.h"
@@ -34,6 +36,10 @@ 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;
@@ -247,9 +253,11 @@ namespace remoting {
 WtsSessionProcessLauncher::WtsSessionProcessLauncher(
     WtsConsoleMonitor* monitor,
     const FilePath& host_binary,
-    base::Thread* io_thread)
+    scoped_refptr<base::MessageLoopProxy> main_message_loop,
+    scoped_refptr<base::MessageLoopProxy> ipc_message_loop)
     : host_binary_(host_binary),
-      io_thread_(io_thread),
+      main_message_loop_(main_message_loop),
+      ipc_message_loop_(ipc_message_loop),
       monitor_(monitor),
       state_(StateDetached) {
   monitor_->AddWtsConsoleObserver(this);
@@ -261,11 +269,13 @@ WtsSessionProcessLauncher::~WtsSessionProcessLauncher() {
   DCHECK(process_.handle() == NULL);
   DCHECK(process_watcher_.GetWatchedObject() == NULL);
   DCHECK(chromoting_channel_.get() == NULL);
-
-  monitor_->RemoveWtsConsoleObserver(this);
+  if (monitor_ != NULL) {
+    monitor_->RemoveWtsConsoleObserver(this);
+  }
 }
 
 void WtsSessionProcessLauncher::LaunchProcess() {
+  DCHECK(main_message_loop_->BelongsToCurrentThread());
   DCHECK(state_ == StateStarting);
   DCHECK(!timer_.IsRunning());
   DCHECK(process_.handle() == NULL);
@@ -282,7 +292,7 @@ void WtsSessionProcessLauncher::LaunchProcess() {
         IPC::ChannelHandle(pipe.Get()),
         IPC::Channel::MODE_SERVER,
         this,
-        io_thread_->message_loop_proxy().get()));
+        ipc_message_loop_));
 
     // Create the host process command line passing the name of the IPC channel
     // to use and copying known switches from the service's command line.
@@ -320,16 +330,46 @@ void WtsSessionProcessLauncher::LaunchProcess() {
 }
 
 void WtsSessionProcessLauncher::OnObjectSignaled(HANDLE object) {
-  DCHECK(state_ == StateAttached);
+  if (!main_message_loop_->BelongsToCurrentThread()) {
+    main_message_loop_->PostTask(
+        FROM_HERE, base::Bind(&WtsSessionProcessLauncher::OnObjectSignaled,
+                              base::Unretained(this), object));
+    return;
+  }
+
+  // It is possible that OnObjectSignaled() task will be queued by another
+  // thread right before |process_watcher_| was stopped. It such a case it is
+  // safe to ignore this notification.
+  if (state_ != StateAttached) {
+    return;
+  }
+
   DCHECK(!timer_.IsRunning());
   DCHECK(process_.handle() != NULL);
   DCHECK(process_watcher_.GetWatchedObject() == NULL);
   DCHECK(chromoting_channel_.get() != NULL);
 
+  // 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;
+
   // The host process has been terminated for some reason. The handle can now be
   // closed.
   process_.Close();
   chromoting_channel_.reset();
+  state_ = StateStarting;
+
+  if (stop_trying) {
+    OnSessionDetached();
+
+    // N.B. The service will stop once the last observer is removed from
+    // the list.
+    monitor_->RemoveWtsConsoleObserver(this);
+    monitor_ = NULL;
+    return;
+  }
 
   // Expand the backoff interval if the process has died quickly or reset it if
   // it was up longer than the maximum backoff delay.
@@ -345,7 +385,6 @@ void WtsSessionProcessLauncher::OnObjectSignaled(HANDLE object) {
   }
 
   // Try to restart the host.
-  state_ = StateStarting;
   timer_.Start(FROM_HERE, launch_backoff_,
                this, &WtsSessionProcessLauncher::LaunchProcess);
 }
@@ -361,6 +400,13 @@ bool WtsSessionProcessLauncher::OnMessageReceived(const IPC::Message& message) {
 }
 
 void WtsSessionProcessLauncher::OnSendSasToConsole() {
+  if (!main_message_loop_->BelongsToCurrentThread()) {
+    main_message_loop_->PostTask(
+        FROM_HERE, base::Bind(&WtsSessionProcessLauncher::OnSendSasToConsole,
+                              base::Unretained(this)));
+    return;
+  }
+
   if (state_ == StateAttached) {
     if (sas_injector_.get() == NULL) {
       sas_injector_ = SasInjector::Create();
@@ -373,6 +419,7 @@ void WtsSessionProcessLauncher::OnSendSasToConsole() {
 }
 
 void WtsSessionProcessLauncher::OnSessionAttached(uint32 session_id) {
+  DCHECK(main_message_loop_->BelongsToCurrentThread());
   DCHECK(state_ == StateDetached);
   DCHECK(!timer_.IsRunning());
   DCHECK(process_.handle() == NULL);
@@ -410,6 +457,7 @@ void WtsSessionProcessLauncher::OnSessionAttached(uint32 session_id) {
 }
 
 void WtsSessionProcessLauncher::OnSessionDetached() {
+  DCHECK(main_message_loop_->BelongsToCurrentThread());
   DCHECK(state_ == StateDetached ||
          state_ == StateStarting ||
          state_ == StateAttached);
@@ -423,7 +471,6 @@ void WtsSessionProcessLauncher::OnSessionDetached() {
       break;
 
     case StateStarting:
-      DCHECK(timer_.IsRunning());
       DCHECK(process_.handle() == NULL);
       DCHECK(process_watcher_.GetWatchedObject() == NULL);
       DCHECK(chromoting_channel_.get() == NULL);
diff --git a/remoting/host/wts_session_process_launcher_win.h b/remoting/host/wts_session_process_launcher_win.h
index efcb93a..ea241e9 100644
--- a/remoting/host/wts_session_process_launcher_win.h
+++ b/remoting/host/wts_session_process_launcher_win.h
@@ -10,6 +10,8 @@
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/compiler_specific.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/process.h"
 #include "base/time.h"
 #include "base/timer.h"
@@ -20,16 +22,12 @@
 #include "remoting/host/wts_console_observer_win.h"
 
 namespace base {
-
-class Thread;
-
+class MessageLoopProxy;
 } // namespace base
 
 namespace IPC {
-
 class ChannelProxy;
 class Message;
-
 } // namespace IPC
 
 namespace remoting {
@@ -42,12 +40,15 @@ class WtsSessionProcessLauncher
       public IPC::Channel::Listener,
       public WtsConsoleObserver {
  public:
-  // Constructs a WtsSessionProcessLauncher object. |monitor| and |io_thread|
-  // must outlive this object. |host_binary| is the name of the executable to
-  // be launched in the console session.
-  WtsSessionProcessLauncher(WtsConsoleMonitor* monitor,
-                            const FilePath& host_binary,
-                            base::Thread* io_thread);
+  // Constructs a WtsSessionProcessLauncher object. |host_binary| is the name of
+  // the executable to be launched in the console session. All interaction with
+  // |monitor| should happen on |main_message_loop|. |ipc_message_loop| has
+  // to be an I/O message loop.
+  WtsSessionProcessLauncher(
+      WtsConsoleMonitor* monitor,
+      const FilePath& host_binary,
+      scoped_refptr<base::MessageLoopProxy> main_message_loop,
+      scoped_refptr<base::MessageLoopProxy> ipc_message_loop);
 
   virtual ~WtsSessionProcessLauncher();
 
@@ -83,9 +84,11 @@ class WtsSessionProcessLauncher
   // Timer used to schedule the next attempt to launch the process.
   base::OneShotTimer<WtsSessionProcessLauncher> timer_;
 
-  // The I/O thread hosts the Chromoting IPC channel and any other code
-  // requiring an I/O message loop.
-  base::Thread* io_thread_;
+  // The main service message loop.
+  scoped_refptr<base::MessageLoopProxy> main_message_loop_;
+
+  // Message loop used by the IPC channel.
+  scoped_refptr<base::MessageLoopProxy> ipc_message_loop_;
 
   // This pointer is used to unsubscribe from session attach and detach events.
   WtsConsoleMonitor* monitor_;