Make sure the timer is stopped and destroyed on the thread it was created on.

remoting/host/plugin/daemon_controller_win.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.
 
 #include "remoting/host/plugin/daemon_controller.h"
 
 #include <objbase.h>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
@@ skipping 37 matching lines @@
 const char kDaemonControllerThreadName[] = "Daemon Controller thread";
 
 // The maximum interval between showing UAC prompts.
 const int kUacTimeoutSec = 15 * 60;
 
 // A base::Thread implementation that initializes COM on the new thread.
 class ComThread : public base::Thread {
  public:
   explicit ComThread(const char* name);
 
-  // Activates an elevated instance of the controller and returns the pointer
-  // to the control interface in |control_out|. This class keeps the ownership
-  // of the pointer so the caller should not call call AddRef() or Release().
-  HRESULT ActivateElevatedController(HWND window_handle,
-                                     IDaemonControl** control_out);
-
   bool Start();
 
  protected:
   virtual void Init() OVERRIDE;
   virtual void CleanUp() OVERRIDE;
 
-  void ReleaseElevatedController();
-
-  ScopedComPtr<IDaemonControl> control_;
-
-  // This timer is used to release |control_| after a timeout.
-  base::OneShotTimer<ComThread> release_timer_;
-
   DISALLOW_COPY_AND_ASSIGN(ComThread);
 };
 
 class DaemonControllerWin : public remoting::DaemonController {
  public:
   DaemonControllerWin();
   virtual ~DaemonControllerWin();
 
   virtual State GetState() OVERRIDE;
   virtual void GetConfig(const GetConfigCallback& callback) OVERRIDE;
   virtual void SetConfigAndStart(
       scoped_ptr<base::DictionaryValue> config,
       const CompletionCallback& done_callback) OVERRIDE;
   virtual void UpdateConfig(scoped_ptr<base::DictionaryValue> config,
                             const CompletionCallback& done_callback) OVERRIDE;
   virtual void Stop(const CompletionCallback& done_callback) OVERRIDE;
   virtual void SetWindow(void* window_handle) OVERRIDE;
 
  private:
-  // Converts a Windows service status code to a Daemon state.
-  static State ConvertToDaemonState(DWORD service_state);
+  // Activates an elevated instance of the controller and stores the pointer

[simonmorris, 2012/05/01 17:29:12]

"...and caches it." Or "stores" it?

[alexeypa (please no reviews), 2012/05/01 18:00:01]

Done.

+  // in |control_|.
+  HRESULT ActivateElevatedController();
 
-  // Converts HRESULT to the AsyncResult.
-  static AsyncResult HResultToAsyncResult(HRESULT hr);
+  // Releases the cached instance of the evelated controller.

[simonmorris, 2012/05/01 17:29:12]

evelated -> elevated
cached -> stored?

[alexeypa (please no reviews), 2012/05/01 18:00:01]

Done.

+  void ReleaseElevatedController();
 
   // Procedes with the daemon configuration if the installation succeeded,
   // otherwise reports the error.
   void OnInstallationComplete(scoped_ptr<base::DictionaryValue> config,
                               const CompletionCallback& done_callback,
                               HRESULT result);
 
   // Opens the Chromoting service returning its handle in |service_out|.
   DWORD OpenService(ScopedScHandle* service_out);
 
   // Converts a config dictionary to a scoped BSTR.
   static void ConfigToString(const base::DictionaryValue& config,
                              ScopedBstr* out);
 
+  // Converts a Windows service status code to a Daemon state.
+  static State ConvertToDaemonState(DWORD service_state);
+
+  // Converts HRESULT to the AsyncResult.
+  static AsyncResult HResultToAsyncResult(HRESULT hr);
+
   // The functions that actually do the work. They should be called in
   // the context of |worker_thread_|;
   void DoGetConfig(const GetConfigCallback& callback);
   void DoInstallAsNeededAndStart(scoped_ptr<base::DictionaryValue> config,
                                  const CompletionCallback& done_callback);
   void DoSetConfigAndStart(scoped_ptr<base::DictionaryValue> config,
                            const CompletionCallback& done_callback);
   void DoUpdateConfig(scoped_ptr<base::DictionaryValue> config,
                       const CompletionCallback& done_callback);
   void DoStop(const CompletionCallback& done_callback);
   void DoSetWindow(void* window_handle);
 
+  ScopedComPtr<IDaemonControl> control_;
+
+  // This timer is used to release |control_| after a timeout.
+  scoped_ptr<base::OneShotTimer<DaemonControllerWin> > release_timer_;
+
   // Handle of the plugin window.
   HWND window_handle_;
 
   // The worker thread used for servicing long running operations.
   ComThread worker_thread_;
 
   scoped_ptr<DaemonInstallerWin> installer_;
 
   DISALLOW_COPY_AND_ASSIGN(DaemonControllerWin);
 };
 
-ComThread::ComThread(const char* name) : base::Thread(name), control_(NULL) {
-}
-
-HRESULT ComThread::ActivateElevatedController(
-    HWND window_handle,
-    IDaemonControl** control_out) {
-  // Cache an instance of the Elevated Controller to prevent a UAC prompt on
-  // every operation.
-  if (control_.get() == NULL) {
-    BIND_OPTS3 bind_options;
-    memset(&bind_options, 0, sizeof(bind_options));
-    bind_options.cbStruct = sizeof(bind_options);
-    bind_options.hwnd = GetTopLevelWindow(window_handle);
-    bind_options.dwClassContext  = CLSCTX_LOCAL_SERVER;
-
-    HRESULT hr = ::CoGetObject(
-        kDaemonControllerElevationMoniker,
-        &bind_options,
-        IID_IDaemonControl,
-        control_.ReceiveVoid());
-    if (FAILED(hr)) {
-      return hr;
-    }
-
-    // Release |control_| upon expiration of the timeout.
-    release_timer_.Start(FROM_HERE,
-                         base::TimeDelta::FromSeconds(kUacTimeoutSec),
-                         this, &ComThread::ReleaseElevatedController);
-  }
-
-  *control_out = control_.get();
-  return S_OK;
+ComThread::ComThread(const char* name) : base::Thread(name) {
 }
 
 bool ComThread::Start() {
   // N.B. The single threaded COM apartment must be run on a UI message loop.
   base::Thread::Options thread_options(MessageLoop::TYPE_UI, 0);
   return StartWithOptions(thread_options);
 }
 
 void ComThread::Init() {
   CoInitialize(NULL);
 }
 
 void ComThread::CleanUp() {
-  ReleaseElevatedController();
   CoUninitialize();
 }
 
-void ComThread::ReleaseElevatedController() {
-  control_.Release();
-}
-
 DaemonControllerWin::DaemonControllerWin()
     : window_handle_(NULL),
       worker_thread_(kDaemonControllerThreadName) {
   if (!worker_thread_.Start()) {
     LOG(FATAL) << "Failed to start the Daemon Controller worker thread.";
   }
 }
 
 DaemonControllerWin::~DaemonControllerWin() {
+  // Clean up resources allocated on the worker thread.
+  worker_thread_.message_loop_proxy()->PostTask(
+      FROM_HERE,
+      base::Bind(&DaemonControllerWin::ReleaseElevatedController,
+                 base::Unretained(this)));

[simonmorris, 2012/05/01 17:29:12]

How does this guarantee that ReleaseElevatedController is run before the rest of
DaemonControllerWin's destructor destructs release_timer? Or before the next
line stops the worker thread?

[alexeypa (please no reviews), 2012/05/01 18:00:01]

worker_thread_.Stop() waits synchronously until all pending tasks were processed
by the thread.

   worker_thread_.Stop();
 }
 
 remoting::DaemonController::State DaemonControllerWin::GetState() {
   // TODO(alexeypa): Make the thread alertable, so we can switch to APC
   // notifications rather than polling.
   ScopedScHandle service;
   DWORD error = OpenService(&service);
 
   switch (error) {
@@ skipping 48 matching lines @@
           done_callback));
 }
 
 void DaemonControllerWin::SetWindow(void* window_handle) {
   worker_thread_.message_loop_proxy()->PostTask(
       FROM_HERE, base::Bind(
           &DaemonControllerWin::DoSetWindow, base::Unretained(this),
           window_handle));
 }
 
-// static
-remoting::DaemonController::State DaemonControllerWin::ConvertToDaemonState(
-    DWORD service_state) {
-  switch (service_state) {
-  case SERVICE_RUNNING:
-    return STATE_STARTED;
+HRESULT DaemonControllerWin::ActivateElevatedController() {
+  DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
 
-  case SERVICE_CONTINUE_PENDING:
-  case SERVICE_START_PENDING:
-    return STATE_STARTING;
-    break;
+  // Cache an instance of the Elevated Controller to prevent a UAC prompt on
+  // every operation.
+  if (control_.get() == NULL) {
+    BIND_OPTS3 bind_options;
+    memset(&bind_options, 0, sizeof(bind_options));
+    bind_options.cbStruct = sizeof(bind_options);
+    bind_options.hwnd = GetTopLevelWindow(window_handle_);
+    bind_options.dwClassContext  = CLSCTX_LOCAL_SERVER;
 
-  case SERVICE_PAUSE_PENDING:
-  case SERVICE_STOP_PENDING:
-    return STATE_STOPPING;
-    break;
+    HRESULT hr = ::CoGetObject(
+        kDaemonControllerElevationMoniker,
+        &bind_options,
+        IID_IDaemonControl,
+        control_.ReceiveVoid());
+    if (FAILED(hr)) {
+      return hr;
+    }
 
-  case SERVICE_PAUSED:
-  case SERVICE_STOPPED:
-    return STATE_STOPPED;
-    break;
+    // Release |control_| upon expiration of the timeout.
+    release_timer_.reset(new base::OneShotTimer<DaemonControllerWin>());

[Sergey Ulanov, 2012/05/01 17:26:05]

Maybe use DelayTimer here and reset it ever time this method is called (even
when the controller_ already exists)?
That will mean that we will drop it 15 minutes after the last use instead of 15
minutes after it is created.

[alexeypa (please no reviews), 2012/05/01 17:32:30]

I intentionally implemented it this way. I.e. it is supposed to re-prompt UAC 15
minutes after the _first_ use even if a user keeps clicking the button. It 
feels a little bit safer that the timeout is fixed.

+    release_timer_->Start(FROM_HERE,
+                          base::TimeDelta::FromSeconds(kUacTimeoutSec),
+                          this,
+                          &DaemonControllerWin::ReleaseElevatedController);
+  }
 
-  default:
-    NOTREACHED();
-    return STATE_UNKNOWN;
-  }
+  return S_OK;
 }
 
-// static
-DaemonController::AsyncResult DaemonControllerWin::HResultToAsyncResult(
-    HRESULT hr) {
-  // TODO(sergeyu): Report other errors to the webapp once it knows
-  // how to handle them.
-  return FAILED(hr) ? RESULT_FAILED : RESULT_OK;
+void DaemonControllerWin::ReleaseElevatedController() {
+  DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
+
+  control_.Release();
+  release_timer_.reset();
 }
 
 void DaemonControllerWin::OnInstallationComplete(
     scoped_ptr<base::DictionaryValue> config,
     const CompletionCallback& done_callback,
     HRESULT result) {
   DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
 
   if (SUCCEEDED(result)) {
     DoSetConfigAndStart(config.Pass(), done_callback);
@@ skipping 27 matching lines @@
       LOG_GETLASTERROR(ERROR)
           << "Failed to open to the '" << kWindowsServiceName << "' service";
     }
     return error;
   }
 
   service_out->Set(service.Take());
   return ERROR_SUCCESS;
 }
 
+// static
 void DaemonControllerWin::ConfigToString(const base::DictionaryValue& config,
                                          ScopedBstr* out) {
   std::string config_str;
   base::JSONWriter::Write(&config, &config_str);
   ScopedBstr config_scoped_bstr(UTF8ToUTF16(config_str).c_str());
   out->Swap(config_scoped_bstr);
 }
 
+// static
+remoting::DaemonController::State DaemonControllerWin::ConvertToDaemonState(
+    DWORD service_state) {
+  switch (service_state) {
+  case SERVICE_RUNNING:
+    return STATE_STARTED;
+
+  case SERVICE_CONTINUE_PENDING:
+  case SERVICE_START_PENDING:
+    return STATE_STARTING;
+    break;
+
+  case SERVICE_PAUSE_PENDING:
+  case SERVICE_STOP_PENDING:
+    return STATE_STOPPING;
+    break;
+
+  case SERVICE_PAUSED:
+  case SERVICE_STOPPED:
+    return STATE_STOPPED;
+    break;
+
+  default:
+    NOTREACHED();
+    return STATE_UNKNOWN;
+  }
+}
+
+// static
+DaemonController::AsyncResult DaemonControllerWin::HResultToAsyncResult(
+    HRESULT hr) {
+  // TODO(sergeyu): Report other errors to the webapp once it knows
+  // how to handle them.
+  return FAILED(hr) ? RESULT_FAILED : RESULT_OK;
+}
+
 void DaemonControllerWin::DoGetConfig(const GetConfigCallback& callback) {
   DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
 
   scoped_ptr<base::DictionaryValue> dictionary_null(NULL);
     // Get the name of the configuration file.
   FilePath dir = remoting::GetConfigDir();
   FilePath filename = dir.Append(kUnprivilegedConfigFileName);
 
   // Read raw data from the configuration file.
   base::win::ScopedHandle file(
@@ skipping 38 matching lines @@
   value.release();
 
   callback.Run(scoped_ptr<base::DictionaryValue>(dictionary));
 }
 
 void DaemonControllerWin::DoInstallAsNeededAndStart(
     scoped_ptr<base::DictionaryValue> config,
     const CompletionCallback& done_callback) {
   DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
 
-  IDaemonControl* control = NULL;
-  HRESULT hr = worker_thread_.ActivateElevatedController(window_handle_,
-                                                         &control);
-
-  // Just configure and start the Daemon Controller if it is installed already.
+  // Configure and start the Daemon Controller if it is installed already.
+  HRESULT hr = ActivateElevatedController();
   if (SUCCEEDED(hr)) {
     DoSetConfigAndStart(config.Pass(), done_callback);
     return;
   }
 
   // Otherwise, install it if its COM registration entry is missing.
   if (hr == CO_E_CLASSSTRING) {
     scoped_ptr<DaemonInstallerWin> installer = DaemonInstallerWin::Create(
         window_handle_,
         base::Bind(&DaemonControllerWin::OnInstallationComplete,
@@ skipping 10 matching lines @@
                << "(error: 0x" << std::hex << hr << std::dec << ").";
     done_callback.Run(HResultToAsyncResult(hr));
   }
 }
 
 void DaemonControllerWin::DoSetConfigAndStart(
     scoped_ptr<base::DictionaryValue> config,
     const CompletionCallback& done_callback) {
   DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
 
-  IDaemonControl* control = NULL;
-  HRESULT hr = worker_thread_.ActivateElevatedController(window_handle_,
-                                                         &control);
+  HRESULT hr = ActivateElevatedController();
   if (FAILED(hr)) {
     done_callback.Run(HResultToAsyncResult(hr));
     return;
   }
 
   // Set the configuration.
   ScopedBstr config_str(NULL);
   ConfigToString(*config, &config_str);
   if (config_str == NULL) {
     done_callback.Run(HResultToAsyncResult(E_OUTOFMEMORY));
     return;
   }
 
-  hr = control->SetConfig(config_str);
+  hr = control_->SetConfig(config_str);
   if (FAILED(hr)) {
     done_callback.Run(HResultToAsyncResult(hr));
     return;
   }
 
   // Start daemon.
-  hr = control->StartDaemon();
+  hr = control_->StartDaemon();
   done_callback.Run(HResultToAsyncResult(hr));
 }
 
 void DaemonControllerWin::DoUpdateConfig(
     scoped_ptr<base::DictionaryValue> config,
     const CompletionCallback& done_callback) {
   DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
 
-  IDaemonControl* control = NULL;
-  HRESULT hr = worker_thread_.ActivateElevatedController(window_handle_,
-                                                         &control);
+  HRESULT hr = ActivateElevatedController();
   if (FAILED(hr)) {
     done_callback.Run(HResultToAsyncResult(hr));
     return;
   }
 
   // Update the configuration.
   ScopedBstr config_str(NULL);
   ConfigToString(*config, &config_str);
   if (config_str == NULL) {
     done_callback.Run(HResultToAsyncResult(E_OUTOFMEMORY));
     return;
   }
 
-  hr = control->UpdateConfig(config_str);
+  hr = control_->UpdateConfig(config_str);
   done_callback.Run(HResultToAsyncResult(hr));
 }
 
 void DaemonControllerWin::DoStop(const CompletionCallback& done_callback) {
   DCHECK(worker_thread_.message_loop_proxy()->BelongsToCurrentThread());
 
-  IDaemonControl* control = NULL;
-  HRESULT hr = worker_thread_.ActivateElevatedController(window_handle_,
-                                                         &control);
+  HRESULT hr = ActivateElevatedController();
   if (FAILED(hr)) {
     done_callback.Run(HResultToAsyncResult(hr));
     return;
   }
 
-  hr = control->StopDaemon();
+  hr = control_->StopDaemon();
   done_callback.Run(HResultToAsyncResult(hr));
 }
 
 void DaemonControllerWin::DoSetWindow(void* window_handle) {
   window_handle_ = reinterpret_cast<HWND>(window_handle);
 }
 
 }  // namespace
 
 scoped_ptr<DaemonController> remoting::DaemonController::Create() {
   return scoped_ptr<DaemonController>(new DaemonControllerWin());
 }
 
 }  // namespace remoting