Introducing remoting::Stoppable helper base class implementing asynchronous shutdown on a specific t

remoting/host/host_service_win.cc

Index: remoting/host/host_service_win.cc
diff --git a/remoting/host/host_service_win.cc b/remoting/host/host_service_win.cc
index 41a35a3a226c391195614d1280ff1852dbb00795..241de8a1ced7f829ae06d0f2db17852e881bfb4d 100644
--- a/remoting/host/host_service_win.cc
+++ b/remoting/host/host_service_win.cc
@@ -23,8 +23,11 @@
 #include "base/threading/thread.h"
 #include "base/utf_string_conversions.h"
 #include "base/win/wrapped_window_proc.h"
+#include "remoting/base/auto_message_loop.h"
+#include "remoting/base/auto_thread.h"
 #include "remoting/base/breakpad.h"
 #include "remoting/base/scoped_sc_handle_win.h"
+#include "remoting/base/shutdownable.h"
 #include "remoting/host/branding.h"
 #include "remoting/host/host_service_resource.h"
 #include "remoting/host/usage_stats_consent.h"
@@ -82,11 +85,9 @@ namespace remoting {
 
 HostService::HostService() :
   console_session_id_(kInvalidSessionId),
-  message_loop_(NULL),
   run_routine_(&HostService::RunAsService),
   service_name_(kWindowsServiceName),
   service_status_handle_(0),
-  shutting_down_(false),
   stopped_event_(true, false) {
 }
 
@@ -94,20 +95,20 @@ HostService::~HostService() {
 }
 
 void HostService::AddWtsConsoleObserver(WtsConsoleObserver* observer) {
-  DCHECK(message_loop_->message_loop_proxy()->BelongsToCurrentThread());
+  DCHECK(message_loop_->BelongsToCurrentThread());
 
   console_observers_.AddObserver(observer);
 }
 
 void HostService::RemoveWtsConsoleObserver(WtsConsoleObserver* observer) {
-  DCHECK(message_loop_->message_loop_proxy()->BelongsToCurrentThread());
+  DCHECK(message_loop_->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::OnLauncherShutdown(Shutdownable* /* launcher */) {
+  launcher_.reset(NULL);
+  message_loop_ = NULL;
 }
 
 void HostService::OnSessionChange() {
@@ -116,10 +117,7 @@ void HostService::OnSessionChange() {
   // the console session is still the same every time a session change
   // notification event is posted. This also takes care of coalescing multiple
   // events into one since we look at the latest state.
-  uint32 console_session_id = kInvalidSessionId;
-  if (!shutting_down_) {
-    console_session_id = WTSGetActiveConsoleSessionId();
-  }
+  uint32 console_session_id = WTSGetActiveConsoleSessionId();
   if (console_session_id_ != console_session_id) {
     if (console_session_id_ != kInvalidSessionId) {
       FOR_EACH_OBSERVER(WtsConsoleObserver,
@@ -145,7 +143,9 @@ BOOL WINAPI HostService::ConsoleControlHandler(DWORD event) {
     case CTRL_CLOSE_EVENT:
     case CTRL_LOGOFF_EVENT:
     case CTRL_SHUTDOWN_EVENT:
-      self->message_loop_->PostTask(FROM_HERE, MessageLoop::QuitClosure());
+      self->message_loop_->PostTask(FROM_HERE, base::Bind(
+          &WtsSessionProcessLauncher::Shutdown,
+          base::Unretained(self->launcher_.get())));
       self->stopped_event_.Wait();
       return TRUE;
 
@@ -195,27 +195,31 @@ int HostService::Run() {
   return (this->*run_routine_)();
 }
 
-void HostService::RunMessageLoop() {
+bool HostService::BeforeMessageLoop() {
   // Launch the I/O thread.
-  base::Thread io_thread(kIoThreadName);
+  scoped_refptr<AutoThread> io_thread(
+      new AutoThread(kIoThreadName, message_loop_));

[Sergey Ulanov, 2012/07/23 22:44:37]

Why do we need thread to be refcounted?

Can we use base::SequencedWorkerPool instead as it is a preferred way to create
IO threads anyway?

[alexeypa (please no reviews), 2012/07/23 23:44:53]

To make sure it is alive until all objects depending on it being live are gone.
base::SequencedWorkerPool may be a better alternative. Let me see it is going to
work.

+
   base::Thread::Options io_thread_options(MessageLoop::TYPE_IO, 0);
-  if (!io_thread.StartWithOptions(io_thread_options)) {
+  if (!io_thread->StartWithOptions(io_thread_options)) {
     LOG(FATAL) << "Failed to start the I/O thread";
-    shutting_down_ = true;
-    stopped_event_.Signal();
-    return;
+    return false;
   }
 
-  WtsSessionProcessLauncher launcher(this, host_binary_,
-                                     message_loop_->message_loop_proxy(),
-                                     io_thread.message_loop_proxy());
-
+  // Create the session process launcher.
+  launcher_.reset(new WtsSessionProcessLauncher(
+      base::Bind(&HostService::OnLauncherShutdown, base::Unretained(this)),
+      this,
+      host_binary_,
+      message_loop_,
+      io_thread));
+  return true;
+}
+void HostService::RunMessageLoop() {
   // Run the service.
-  message_loop_->Run();
-
-  // Clean up the observers by emulating detaching from the console.
-  shutting_down_ = true;
-  OnSessionChange();
+  if (BeforeMessageLoop()) {
+    message_loop_->Run();
+  }
 
   // Release the control handler.
   stopped_event_.Signal();
@@ -237,10 +241,10 @@ int HostService::RunAsService() {
 }
 
 int HostService::RunInConsole() {
-  MessageLoop message_loop(MessageLoop::TYPE_UI);
+  AutoMessageLoop message_loop(MessageLoop::TYPE_UI);

[Sergey Ulanov, 2012/07/23 22:44:37]

You are creating ref-counted object on stack. That's never safe - the object
will try to delete itself when ref-count goes to zero.

[alexeypa (please no reviews), 2012/07/23 23:44:53]

I control the destruction logic for this object. AutoMessageLoop::OnDestruct()
will be called when the last reference goes away. It will stop the loop but do
not delete AutoMessageLoop.

Basically the use case for AutoMessageLoop and AutoThread is: the main loop is
create in the stack; all other loops are create in heap and use the main one to
destroy themselves; they a;so reference the main loop to make sure it stay alive
long enough.

 
   // Allow other threads to post to our message loop.
-  message_loop_ = &message_loop;
+  message_loop_ = scoped_refptr<AutoMessageLoop>(&message_loop);
 
   int result = kErrorExitCode;
 
@@ -304,8 +308,6 @@ cleanup:
   // we can do about it now and the program is about to exit anyway. Even if
   // it crashes nothing is going to be broken because of it.
   SetConsoleCtrlHandler(&HostService::ConsoleControlHandler, FALSE);
-
-  message_loop_ = NULL;
   return result;
 }
 
@@ -320,7 +322,9 @@ DWORD WINAPI HostService::ServiceControlHandler(DWORD control,
 
     case SERVICE_CONTROL_SHUTDOWN:
     case SERVICE_CONTROL_STOP:
-      self->message_loop_->PostTask(FROM_HERE, MessageLoop::QuitClosure());
+      self->message_loop_->PostTask(FROM_HERE, base::Bind(
+          &WtsSessionProcessLauncher::Shutdown,
+          base::Unretained(self->launcher_.get())));
       self->stopped_event_.Wait();
       return NO_ERROR;
 
@@ -335,11 +339,11 @@ DWORD WINAPI HostService::ServiceControlHandler(DWORD control,
 }
 
 VOID WINAPI HostService::ServiceMain(DWORD argc, WCHAR* argv[]) {
-  MessageLoop message_loop;
+  AutoMessageLoop message_loop(MessageLoop::TYPE_DEFAULT);

[Sergey Ulanov, 2012/07/23 22:44:37]

Why do we need MessageLoop to be refcounted here? If we just need it to be
destroyed later than ServiceMain() returns (why?) then maybe allocate it on heap
and just leak it when we return?

[alexeypa (please no reviews), 2012/07/23 23:44:53]

It doe snot have to outlive ServiceMain(). It is refcounted to express
dependencies between the message loop and objects using it. This refcounting has
nothing to do with memory allocation. The way those message loops are supposed
to be allocated is documented next to the definition of AutoThread and
AutoMessageLoop.

 
   // Allow other threads to post to our message loop.
   HostService* self = HostService::GetInstance();
-  self->message_loop_ = &message_loop;
+  self->message_loop_ = scoped_refptr<AutoMessageLoop>(&message_loop);
 
   // Register the service control handler.
   self->service_status_handle_ =
@@ -385,8 +389,6 @@ VOID WINAPI HostService::ServiceMain(DWORD argc, WCHAR* argv[]) {
         << "Failed to report service status to the service control manager";
     return;
   }
-
-  self->message_loop_ = NULL;
 }
 
 LRESULT CALLBACK HostService::SessionChangeNotificationProc(HWND hwnd,