Firing event to broker without worker thread.

ceee/ie/plugin/toolband/toolband_module.cc

Index: ceee/ie/plugin/toolband/toolband_module.cc
===================================================================
--- ceee/ie/plugin/toolband/toolband_module.cc	(revision 66229)
+++ ceee/ie/plugin/toolband/toolband_module.cc	(working copy)
@@ -9,7 +9,6 @@
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/logging_win.h"
-#include "base/thread.h"
 #include "ceee/common/com_utils.h"
 #include "ceee/common/install_utils.h"
 #include "ceee/ie/broker/broker_rpc_client.h"
@@ -61,76 +60,33 @@
     return module_initialized_;
   }
 
-  // Fires an event to the broker, so that the call can be made with an
-  // instance of a broker proxy that was CoCreated in the worker thread.
+  // Fires an event to the broker.
   void FireEventToBroker(const std::string& event_name,
                          const std::string& event_args);
 
  private:
-  // TODO(vitalybuka@google.com): Fire events without this thread.
-  class ComWorkerThread : public base::Thread {
-   public:
-    ComWorkerThread();
 
-    // Called just prior to starting the message loop
-    virtual void Init();
+  // We only attach broker on first use.
+  void EnsureBrokerIsConnected();
 
-    // Called just after the message loop ends
-    virtual void CleanUp();
-
-    // Called by FireEventTask so that the broker we instantiate in the
-    // worker thread can be used.
-    void FireEventToBroker(BSTR event_name, BSTR event_args);
-   protected:
-    BrokerRpcClient broker_rpc_;
-  };
-
-  class FireEventTask : public Task {
-   public:
-    FireEventTask(ComWorkerThread* worker_thread,
-                  const std::string& event_name,
-                  const std::string& event_args)
-        : worker_thread_(worker_thread),
-          event_name_(event_name.c_str()),
-          event_args_(event_args.c_str()) {
-    }
-    virtual void Run() {
-      worker_thread_->FireEventToBroker(event_name_, event_args_);
-    }
-   private:
-    ComWorkerThread* worker_thread_;
-    CComBSTR event_name_;
-    CComBSTR event_args_;
-  };
-  // We only start the thread on first use. If we would start it on
-  // initialization, when our DLL is loaded into the broker process,
-  // it would try to start this thread which tries to CoCreate a Broker
-  // and this could cause a complex deadlock...
-  void EnsureThreadStarted();
-
-  // We use a pointer so that we can make sure we only destroy the object
-  // when the thread is properly stopped. Otherwise, we would get a DCHECK
-  // if the thread is killed before we get to Stop it when DllCanUnloadNow
-  // returns S_OK, which happens when the application quits with live objects,
-  // this causes the destructor to DCHECK.
-  ComWorkerThread* worker_thread_;
   base::AtExitManager at_exit_;
   bool module_initialized_;
   bool crash_reporting_initialized_;
 
-  int worker_thread_ref_count_;
+  int broker_ref_count_;
 
-  friend void ceee_module_util::AddRefModuleWorkerThread();
-  friend void ceee_module_util::ReleaseModuleWorkerThread();
+  BrokerRpcClient broker_rpc_client_;
 
-  void IncThreadRefCount();
-  void DecThreadRefCount();
+  friend void ceee_module_util::AddRefBroker();
+  friend void ceee_module_util::ReleaseBroker();
+
+  void IncBrokerRefCount();
+  void DecBrokerRefCount();
 };
 
 ToolbandModule::ToolbandModule()
     : crash_reporting_initialized_(false),
-      module_initialized_(false),
-      worker_thread_(NULL) {
+      module_initialized_(false) {
   wchar_t logfile_path[MAX_PATH];
   DWORD len = ::GetTempPath(arraysize(logfile_path), logfile_path);
   ::PathAppend(logfile_path, kLogFileName);
@@ -162,9 +118,7 @@
 ToolbandModule::~ToolbandModule() {
   ScriptHost::set_default_debug_application(NULL);
 
-  // Just leave thread as is. Releasing interface from this thread may hang IE.
-  DCHECK(worker_thread_ref_count_ == 0);
-  DCHECK(worker_thread_ == NULL);
+  DCHECK(!broker_rpc_client_.is_connected());
 
   // Uninitialize control hosting.
   BOOL uninitialized = AtlAxWinTerm();
@@ -175,43 +129,36 @@
 
 HRESULT ToolbandModule::DllCanUnloadNow() {
   HRESULT hr = CAtlDllModuleT<ToolbandModule>::DllCanUnloadNow();
-  if (hr == S_OK) {
-    // We must protect our data member against concurrent calls to check if we
-    // can be unloaded. We must also making the call to Term within the lock
-    // to make sure we don't try to re-initialize in case a new
-    // DllGetClassObject would occur in the mean time, in another thread.
-    m_csStaticDataInitAndTypeInfo.Lock();
-    if (module_initialized_) {
-      Term();
-    }
-    m_csStaticDataInitAndTypeInfo.Unlock();
-  }
+  if (hr == S_OK)
+    Term();
   return hr;
 }
 
 HRESULT ToolbandModule::DllGetClassObject(REFCLSID clsid, REFIID iid,
                                          void** object)  {
-  // Same comment as above in ToolbandModule::DllCanUnloadNow().
-  m_csStaticDataInitAndTypeInfo.Lock();
-  if (!module_initialized_) {
-    Init();
-  }
-  m_csStaticDataInitAndTypeInfo.Unlock();
+  Init();
   return CAtlDllModuleT<ToolbandModule>::DllGetClassObject(clsid, iid, object);
 }
 
 void ToolbandModule::Init() {
+  // We must protect our data member against concurrent calls to check if we
+  // can be unloaded. We must also making the call to Term within the lock
+  // to make sure we don't try to re-initialize in case a new
+  // DllGetClassObject would occur in the mean time, in another thread.
+  CComCritSecLock<CComCriticalSection> lock(m_csStaticDataInitAndTypeInfo);
+  if (module_initialized_)
+    return;
   crash_reporting_initialized_ = InitializeCrashReporting();
   module_initialized_ = true;
 }
 
 void ToolbandModule::Term() {
-  if (worker_thread_ != NULL) {
-    // It is OK to call Stop on a thread even when it isn't running.
-    worker_thread_->Stop();
-    delete worker_thread_;
-    worker_thread_ = NULL;
-  }
+  CComCritSecLock<CComCriticalSection> lock(m_csStaticDataInitAndTypeInfo);
+  if (!module_initialized_)
+    return;
+  if (broker_rpc_client_.is_connected())
+    broker_rpc_client_.Disconnect();
+
   if (crash_reporting_initialized_) {
     bool crash_reporting_deinitialized = ShutdownCrashReporting();
     DCHECK(crash_reporting_deinitialized);
@@ -220,101 +167,58 @@
   module_initialized_ = false;
 }
 
-void ToolbandModule::IncThreadRefCount() {
-  m_csStaticDataInitAndTypeInfo.Lock();
-  DCHECK_GE(worker_thread_ref_count_, 0);
-  worker_thread_ref_count_++;
-  m_csStaticDataInitAndTypeInfo.Unlock();
+void ToolbandModule::IncBrokerRefCount() {
+  CComCritSecLock<CComCriticalSection> lock(m_csStaticDataInitAndTypeInfo);
+  DCHECK_GE(broker_ref_count_, 0);
+  broker_ref_count_++;
 }
 
-void ToolbandModule::DecThreadRefCount() {
-  ComWorkerThread* thread = NULL;
-
-  m_csStaticDataInitAndTypeInfo.Lock();
+void ToolbandModule::DecBrokerRefCount() {
+  CComCritSecLock<CComCriticalSection> lock(m_csStaticDataInitAndTypeInfo);
   // If we're already at 0, we have a problem, so we check if we're >=.
-  DCHECK_GT(worker_thread_ref_count_, 0);
+  DCHECK_GT(broker_ref_count_, 0);
 
-  // If this was our last reference, we delete the thread. This is okay even if
-  // we increment the count again, because the thread is created on the "first"
+  // If this was our last reference, we disconnect broker. This is okay even if
+  // we increment the count again, because the broker is created on the "first"
   // FireEventToBroker, thus it will be created again if needed.
-  if (--worker_thread_ref_count_ == 0) {
-    if (worker_thread_ != NULL) {
-      // Store the worker_thread to a temporary pointer. It will be freed later.
-      thread = worker_thread_;
-      worker_thread_ = NULL;
-    }
+  if (--broker_ref_count_ == 0) {
+    if (broker_rpc_client_.is_connected())
+      broker_rpc_client_.Disconnect();
   }
-  m_csStaticDataInitAndTypeInfo.Unlock();
-
-  // Clean the thread after the unlock to be certain we don't get a deadlock
-  // (the CriticalSection could be used in the worker thread).
-  if (thread) {
-    // It is OK to call Stop on a thread even when it isn't running.
-    thread->Stop();
-    delete thread;
-  }
 }
 
-void ToolbandModule::EnsureThreadStarted() {
-  m_csStaticDataInitAndTypeInfo.Lock();
-  if (worker_thread_ == NULL) {
-    worker_thread_ = new ComWorkerThread;
-    // The COM worker thread must be a UI thread so that it can pump windows
-    // messages and allow COM to handle cross apartment calls.
-    worker_thread_->StartWithOptions(base::Thread::Options(MessageLoop::TYPE_UI,
-                                                           0));  // stack_size
+void ToolbandModule::EnsureBrokerIsConnected() {
+  CComCritSecLock<CComCriticalSection> lock(m_csStaticDataInitAndTypeInfo);
+  if (!broker_rpc_client_.is_connected()) {
+    ::CoInitializeEx(0, COINIT_MULTITHREADED);

[Sigurður Ásgeirsson, 2010/11/16 18:23:04]

I think we can skip the Coinit calls. We're running in a thread that's
guaranteed to be initialized.

[Vitaly Buka corp, 2010/11/18 23:14:21]

Done.

+    // TODO(vitalybuka@google.com): Start broker without COM after the last
+    // COM interface is removed.
+    CComPtr<ICeeeBrokerRegistrar> broker;
+    HRESULT hr = broker.CoCreateInstance(CLSID_CeeeBroker);
+    DCHECK(SUCCEEDED(hr)) << "Failed to create broker. " << com::LogHr(hr);
+    broker_rpc_client_.Connect();

[Sigurður Ásgeirsson, 2010/11/16 18:23:04]

test return value and LOG(ERROR) here?

[Vitaly Buka corp, 2010/11/18 23:14:21]

Done.

+    DCHECK(broker_rpc_client_.is_connected());
+    ::CoUninitialize();

[Sigurður Ásgeirsson, 2010/11/16 18:23:04]

Ooof.
If the ::Coinitialize call fails (e.g. due to the apartment being of the wrong
type), you may or will uninitialize COM for this thread.

[Vitaly Buka corp, 2010/11/18 23:14:21]

Done.

   }
-  m_csStaticDataInitAndTypeInfo.Unlock();
 }
 
 void ToolbandModule::FireEventToBroker(const std::string& event_name,
-                                      const std::string& event_args) {
-  EnsureThreadStarted();
-  DCHECK(worker_thread_ != NULL);
-  MessageLoop* message_loop = worker_thread_->message_loop();
-  if (message_loop) {
-    message_loop->PostTask(FROM_HERE,
-        new FireEventTask(worker_thread_, event_name, event_args));
-  } else {
-    LOG(ERROR) << "Trying to post a message before the COM worker thread is"
-                  "completely initialized and ready.";
-  }
+                                       const std::string& event_args) {
+  EnsureBrokerIsConnected();
+  CComBSTR name(event_name.c_str());

[Sigurður Ásgeirsson, 2010/11/16 18:23:04]

This looks like needless conversion and allocation to me. Can we change the RPC
client's interface to receive a const wchar_t*?

[Vitaly Buka corp, 2010/11/18 23:14:21]

Done.

+  CComBSTR args(event_args.c_str());
+  LOG_IF(ERROR, broker_rpc_client_.FireEvent(name, args)) <<

[Sigurður Ásgeirsson, 2010/11/16 18:23:04]

Fire event returns true on error?
I don't like hiding real functionality inside logging statements like this.
Please do the event firing in a separate statement and store the return value in
a local variable.

[Vitaly Buka corp, 2010/11/18 23:14:21]

Done.

+      "Failed to fire event to broker.";
 }
 
 
-ToolbandModule::ComWorkerThread::ComWorkerThread()
-    : base::Thread("CEEE-COM Worker Thread") {
-}
-
-void ToolbandModule::ComWorkerThread::Init() {
-  ::CoInitializeEx(0, COINIT_MULTITHREADED);
-  // TODO(vitalybuka@google.com): Start broker without COM.
-  CComPtr<ICeeeBroker> broker_;
-  HRESULT hr = broker_.CoCreateInstance(CLSID_CeeeBroker);
-  DCHECK(SUCCEEDED(hr)) << "Failed to create broker. " << com::LogHr(hr);
-  broker_rpc_.Connect();
-  DCHECK(broker_rpc_.is_connected());
-  ::CoUninitialize();
-}
-
-void ToolbandModule::ComWorkerThread::CleanUp() {
-  broker_rpc_.Disconnect();
-}
-
-void ToolbandModule::ComWorkerThread::FireEventToBroker(BSTR event_name,
-                                                        BSTR event_args) {
-  DCHECK(broker_rpc_.is_connected());
-  bool result = broker_rpc_.FireEvent(event_name, event_args);
-  DCHECK(result);
-}
-
 ToolbandModule module;
 
-void ceee_module_util::AddRefModuleWorkerThread() {
-  module.IncThreadRefCount();
+void ceee_module_util::AddRefBroker() {
+  module.IncBrokerRefCount();
 }
-void ceee_module_util::ReleaseModuleWorkerThread() {
-  module.DecThreadRefCount();
+void ceee_module_util::ReleaseBroker() {
+  module.DecBrokerRefCount();
 }
 
 void ceee_module_util::FireEventToBroker(const std::string& event_name,