Issue #3432 - Manage COM events with a resource class

src/plugin/PluginClass.cpp

Index: src/plugin/PluginClass.cpp
===================================================================
--- a/src/plugin/PluginClass.cpp
+++ b/src/plugin/PluginClass.cpp
@@ -90,7 +90,8 @@
 }
 
 CPluginClass::CPluginClass()
-  : m_webBrowser2(nullptr)
+  : m_webBrowser2(nullptr),
+  detachedInitializationFailed(false)
 {
   DEBUG_GENERAL([this]() -> std::wstring
     {
@@ -102,7 +103,6 @@
   //Use this line to debug memory leaks
   //	_CrtDumpMemoryLeaks();
 
-  m_isAdvised = false;
   m_hTabWnd = NULL;
   m_hStatusBarWnd = NULL;
   m_hPaneWnd = NULL;
@@ -187,11 +187,11 @@
 {
   if (thisPtr == NULL)
     return 0;
-  if (!((CPluginClass*)thisPtr)->InitObject())
+  auto self = static_cast<CPluginClass*>(thisPtr);
+  if (!self->InitObject())
   {
-    ((CPluginClass*)thisPtr)->Unadvise();
+    self->detachedInitializationFailed = true;
   }
-
   return 0;
 }
 
@@ -218,7 +218,6 @@
       {
         DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_COINIT, "Class::SetSite - CoInitialize");
       }
-
       /*
        * We were instantiated as a BHO, so our site is always of type IWebBrowser2.
        */
@@ -249,34 +248,22 @@
 
       try
       {
-        HRESULT hr = DispEventAdvise(m_webBrowser2);
-        if (SUCCEEDED(hr))
-        {
-          m_isAdvised = true;
-          try
-          {
-            std::thread startInitObjectThread(StartInitObject, this);
-            startInitObjectThread.detach(); // TODO: but actually we should wait for the thread in the dtr.
-          }
-          catch (const std::system_error& ex)
-          {
-            DEBUG_SYSTEM_EXCEPTION(ex, PLUGIN_ERROR_THREAD, PLUGIN_ERROR_MAIN_THREAD_CREATE_PROCESS, 
-              "Class::Thread - Failed to create StartInitObject thread");
-          }
-        }
-        else
-        {
-          DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_ADVICE, "Class::SetSite - Advise");
-        }
+        std::thread startInitObjectThread(StartInitObject, this);
+        startInitObjectThread.detach(); // TODO: but actually we should wait for the thread in the dtr.
       }
-      catch (const std::runtime_error& ex)
+      catch (const std::system_error& ex)
       {
-        DEBUG_EXCEPTION(ex);
-        Unadvise();
+        detachedInitializationFailed = true;
+        DEBUG_SYSTEM_EXCEPTION(ex, PLUGIN_ERROR_THREAD, PLUGIN_ERROR_MAIN_THREAD_CREATE_PROCESS,
+          "Class::Thread - Failed to create StartInitObject thread");
       }
+
+      // Start event last, after everything is ready to go

[sergei, 2016/01/06 08:41:57]

This comment is confusing because not everything is ready yet, just a couple of
lines above we are starting some asynchronous initialization. It's clear that
before binding with event listeners we need to prepare other members, but
apparently not everything is ready to go yet.

IMO, it would be better without this comment.

[Eric, 2016/01/07 16:24:47]

Reworded.

+      browserEvents.Start(this, m_webBrowser2);
     }
     else
     {
+      browserEvents.Stop();
       DEBUG_GENERAL([this]() -> std::wstring
       {
         std::wstringstream ss;
@@ -285,7 +272,6 @@
         return ss.str();
       }());
 
-      Unadvise();
 
       // Destroy window
       if (m_pWndProcStatus)
@@ -465,6 +451,8 @@
 {
   try
   {
+    if (detachedInitializationFailed) return;

[sergei, 2016/01/06 08:41:57]

Basically all these checks in each method are indicating that something is wrong
with the design. It would be better to bind event listeners only after finishing
with the prerequisites.

[sergei, 2016/01/06 08:41:57]

it would be better to use some method `bool IsInitialized() const`, it's a bit
confusing that we are checking only result of "detached" initialization.

[Eric, 2016/01/07 16:24:47]

Something is definitely wrong with the design. It's not something, however,
that's going to be quick to change. The present change set is only incremental
progress.

The biggest single problem is that 'CPluginClass' is too much of a god class.
The current detached initialization thread is working with what should be
several different classes. Synchronization for the completion of initialization
should be isolated into these classes. Until that's (partially) accomplished,
the real issues won't be clear. 

In addition, the lack of transactional (atomic) registration already causes race
conditions we should be dealing with. See #3433 for that aspect.
That's something that may not be strictly possible. (And it might not even be
desirable.) Regardless, it's premature to talk about it at any great length
right now. The code is in too poor a shape to make that a productive
conversation yet.

[Eric, 2016/01/07 16:24:48]

I don't disagree in principle, but that change is premature. I think that
particular function may not even be what we want, not exactly. The flag
"detachedInitializationFailed" is really a proxy for something more like
"IsPluginDisabledOnTechnicalGrounds()". We're not ready to write such a function
yet. There are issue around transient vs. permanent failures, for example, that
we're nowhere close to resolving.

We have something like four different kinds of initialization for
'CPluginClass'. They are not all working well yet. We're going to have to live
with some rather more explicit checking on this issue until we've got all the
pieces in place. It's not going to happen instantly.

Anticipating this class of concerns, I created #3433 a few weeks ago on this
topic. Addressing that issue is going to be multi-commit process, I'm sure.

[sergei, 2016/02/04 13:45:49]

Acknowledged.

+
     ATL::CComQIPtr<IWebBrowser2> webBrowser = frameBrowserDisp;
     if (!webBrowser)
     {
@@ -509,6 +497,7 @@
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnDownloadComplete()
 {
+  if (detachedInitializationFailed) return;
   try
   {
     if (!m_webBrowser2)
@@ -527,6 +516,7 @@
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnDocumentComplete(IDispatch* frameBrowserDisp, VARIANT* /*urlOrPidl*/)
 {
+  if (detachedInitializationFailed) return;
   try
   {
     DEBUG_NAVI(L"Navi::Document Complete");
@@ -546,6 +536,7 @@
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnWindowStateChanged(unsigned long flags, unsigned long validFlagsMask)
 {
+  if (detachedInitializationFailed) return;
   try
   {
     DEBUG_GENERAL(L"WindowStateChanged (check tab changed)");
@@ -576,6 +567,7 @@
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnCommandStateChange(long /*command*/, VARIANT_BOOL /*enable*/)
 {
+  if (detachedInitializationFailed) return;
   try
   {
     if (m_hPaneWnd == NULL)
@@ -601,18 +593,6 @@
   }
 }
 
-// Entry point
-void STDMETHODCALLTYPE CPluginClass::OnOnQuit()
-{
-  try
-  {
-    Unadvise();
-  }
-  catch (...)
-  {
-  }
-}
-
 bool CPluginClass::InitObject()
 {
   DEBUG_GENERAL("InitObject - begin");
@@ -1001,6 +981,7 @@
 
 STDMETHODIMP CPluginClass::QueryStatus(const GUID* pguidCmdGroup, ULONG cCmds, OLECMD prgCmds[], OLECMDTEXT* pCmdText)
 {
+  if (detachedInitializationFailed) return;
   if (cCmds == 0) return E_INVALIDARG;
   if (prgCmds == 0) return E_POINTER;
 
@@ -1210,6 +1191,7 @@
 
 STDMETHODIMP CPluginClass::Exec(const GUID*, DWORD nCmdID, DWORD, VARIANTARG*, VARIANTARG*)
 {
+  if (detachedInitializationFailed) return;
   HWND hBrowserWnd = GetBrowserHWND();
   if (!hBrowserWnd)
   {
@@ -1615,29 +1597,6 @@
   }
 }
 
-
-void CPluginClass::Unadvise()
-{
-  if (!m_webBrowser2)
-  {
-    DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::Unadvise - Reached with m_webBrowser2 == nullptr");
-    return;
-  }
-  s_criticalSectionLocal.Lock();
-  {
-    if (m_isAdvised)
-    {
-      HRESULT hr = DispEventUnadvise(m_webBrowser2);
-      if (FAILED(hr))
-      {
-        DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_UNADVISE, "Class::Unadvise - Unadvise");
-      }
-      m_isAdvised = false;
-    }
-  }
-  s_criticalSectionLocal.Unlock();
-}
-
 HICON CPluginClass::GetIcon(int type)
 {
   HICON icon = NULL;