Issue #3432 - Manage COM events with a resource class

src/plugin/PluginClass.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 72 matching lines @@
     }
 
     int Width() const
     {
       return right - left;
     }
   };
 }
 
 CPluginClass::CPluginClass()
-  : m_webBrowser2(nullptr)
+  : m_webBrowser2(nullptr),
+  detachedInitializationFailed(false)
 {
   DEBUG_GENERAL([this]() -> std::wstring
     {
       std::wstring s = L"CPluginClass::<constructor>, this = ";
       s += ToHexLiteral(this);
       return s;
     }());
 
   //Use this line to debug memory leaks
   //    _CrtDumpMemoryLeaks();
 
-  m_isAdvised = false;
   m_hTabWnd = NULL;
   m_hStatusBarWnd = NULL;
   m_hPaneWnd = NULL;
   m_nPaneWidth = 0;
   m_pWndProcStatus = NULL;
   m_hTheme = NULL;
   m_isInitializedOk = false;
 
 
   m_tab = new CPluginTab();
@@ skipping 64 matching lines @@
     DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserUrl - Reached with m_webBrowser2 == nullptr");
     url = m_tab->GetDocumentUrl();
   }
   return url;
 }
 
 DWORD WINAPI CPluginClass::StartInitObject(LPVOID thisPtr)
 {
   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;
 }
 
 /*
  * IE calls this when it creates a new browser window or tab, immediately after it also 
  * creates the object. The argument 'unknownSite' in is the OLE "site" of the object, 
  * which is an IWebBrowser2 interface associated with the window/tab. 
  *
  * IE also ordinarily calls this again when its window/tab is closed, in which case
  * 'unknownSite' will be null. Extraordinarily, this is sometimes _not_ called when IE
  * is shutting down. Thus 'SetSite(nullptr)' has some similarities with a destructor,
  * but it is not a proper substitute for one.
  */
 STDMETHODIMP CPluginClass::SetSite(IUnknown* unknownSite)
 {
   try
   {
     if (unknownSite)
     {
       DEBUG_GENERAL(L"================================================================================\nNEW TAB UI\n================================================================================");
 
       HRESULT hr = ::CoInitialize(NULL);
       if (FAILED(hr))
       {
         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.
        */
       m_webBrowser2 = ATL::CComQIPtr<IWebBrowser2>(unknownSite);
       if (!m_webBrowser2)
       {
         throw std::logic_error("CPluginClass::SetSite - Unable to convert site pointer to IWebBrowser2*");
       }
       DEBUG_GENERAL([this]() -> std::wstring
         {
@@ skipping 10 matching lines @@
       {
         // Always register on startup, then check if we need to unregister in a separate thread
         s_mimeFilter = CPluginClientFactory::GetMimeFilterClientInstance();
         s_asyncWebBrowser2 = unknownSite;
         s_instances.insert(this);
       }
       s_criticalSectionLocal.Unlock();
 
       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;
         ss << L"CPluginClass::SetSite, this = " << ToHexLiteral(this);
         ss << L", browser = nullptr";
         return ss.str();
       }());
 
-      Unadvise();
 
       // Destroy window
       if (m_pWndProcStatus)
       {
         ::SetWindowLongPtr(m_hStatusBarWnd, GWLP_WNDPROC, (LPARAM)(WNDPROC)m_pWndProcStatus);
 
         m_pWndProcStatus = NULL;
       }
 
       if (m_hPaneWnd)
@@ skipping 159 matching lines @@
   IDispatch* frameBrowserDisp /**< [in] */,
   VARIANT* urlVariant /**< [in] */,
   VARIANT* /**< [in] Flags*/,
   VARIANT* /**< [in] TargetFrameName*/,
   VARIANT* /**< [in] PostData*/,
   VARIANT* /**< [in] Headers*/,
   VARIANT_BOOL* /**< [in, out] Cancel*/)
 {
   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)
     {
       return;
     }
     if (!urlVariant || urlVariant->vt != VT_BSTR)
     {
       return;
     }
     std::wstring url(urlVariant->bstrVal, SysStringLen(urlVariant->bstrVal));
@@ skipping 24 matching lines @@
     }
   }
   catch (...)
   {
   }
 }
 
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnDownloadComplete()
 {
+  if (detachedInitializationFailed) return;
   try
   {
     if (!m_webBrowser2)
     {
       DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::OnDownloadComplete - Reached with m_webBrowser2 == nullptr");
       return;
     }
     DEBUG_NAVI(L"Navi::Download Complete")
     m_tab->OnDownloadComplete(m_webBrowser2);
   }
   catch (...)
   {
   }
 }
 
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnDocumentComplete(IDispatch* frameBrowserDisp, VARIANT* /*urlOrPidl*/)
 {
+  if (detachedInitializationFailed) return;
   try
   {
     DEBUG_NAVI(L"Navi::Document Complete");
     ATL::CComQIPtr<IWebBrowser2> webBrowser2 = frameBrowserDisp;
     if (!webBrowser2)
     {
       return;
     }
     std::wstring frameSrc = GetLocationUrl(*webBrowser2);
     m_tab->OnDocumentComplete(webBrowser2, frameSrc, IsRootBrowser(webBrowser2));
   }
   catch (...)
   {
   }
 }
 
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnWindowStateChanged(unsigned long flags, unsigned long validFlagsMask)
 {
+  if (detachedInitializationFailed) return;
   try
   {
     DEBUG_GENERAL(L"WindowStateChanged (check tab changed)");
     bool newtabshown = validFlagsMask == (OLECMDIDF_WINDOWSTATE_USERVISIBLE | OLECMDIDF_WINDOWSTATE_ENABLED)
       && flags == (OLECMDIDF_WINDOWSTATE_USERVISIBLE | OLECMDIDF_WINDOWSTATE_ENABLED);
     if (newtabshown)
     {
       std::map<DWORD,CPluginClass*>::const_iterator it = s_threadInstances.find(GetCurrentThreadId());
       if (it == s_threadInstances.end())
       {
@@ skipping 10 matching lines @@
     DEBUG_GENERAL(L"WindowStateChanged (check tab changed) end");
   }
   catch (...)
   {
   }
 }
 
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnCommandStateChange(long /*command*/, VARIANT_BOOL /*enable*/)
 {
+  if (detachedInitializationFailed) return;
   try
   {
     if (m_hPaneWnd == NULL)
     {
       CreateStatusBarPane();
     }
     else
     {
       if (AdblockPlus::IE::InstalledMajorVersion() > 6)
       {
         RECT rect;
         //Get the RECT for the leftmost pane (the status text pane)
         BOOL rectRes = ::SendMessage(m_hStatusBarWnd, SB_GETRECT, 0, (LPARAM)&rect);
         if (rectRes == TRUE)
         {
           MoveWindow(m_hPaneWnd, rect.right - m_nPaneWidth, 0, m_nPaneWidth, rect.bottom - rect.top, TRUE);
         }
       }
     }
   }
   catch (...)
   {
   }
 }
 
-// Entry point
-void STDMETHODCALLTYPE CPluginClass::OnOnQuit()
-{
-  try
-  {
-    Unadvise();
-  }
-  catch (...)
-  {
-  }
-}
-
 bool CPluginClass::InitObject()
 {
   DEBUG_GENERAL("InitObject - begin");
   CPluginSettings* settings = CPluginSettings::GetInstance();
 
   if (!settings->GetPluginEnabled())
   {
     s_mimeFilter->Unregister();
   }
 
@@ skipping 368 matching lines @@
     }
   }
   s_criticalSectionLocal.Unlock();
 
   return tab;
 }
 
 
 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;
 
   prgCmds[0].cmdf = OLECMDF_ENABLED;
 
   return S_OK;
 }
 
 HMENU CPluginClass::CreatePluginMenu(const std::wstring& url)
 {
@@ skipping 189 matching lines @@
   fmii.dwTypeData = const_cast<LPWSTR>(ctext.c_str());
   fmii.cch = static_cast<UINT>(ctext.size());
   ::SetMenuItemInfoW(hMenu, ID_MENU_SETTINGS, FALSE, &fmii);
 
   return true;
 }
 
 
 STDMETHODIMP CPluginClass::Exec(const GUID*, DWORD nCmdID, DWORD, VARIANTARG*, VARIANTARG*)
 {
+  if (detachedInitializationFailed) return;
   HWND hBrowserWnd = GetBrowserHWND();
   if (!hBrowserWnd)
   {
     return E_FAIL;
   }
 
   // Create menu
   HMENU hMenu = CreatePluginMenu(m_tab->GetDocumentUrl());
   if (!hMenu)
   {
@@ skipping 385 matching lines @@
   if (m_hPaneWnd == NULL)
   {
     CreateStatusBarPane();
   }
   if ((m_hPaneWnd != NULL) && !::InvalidateRect(m_hPaneWnd, NULL, FALSE))
   {
       DEBUG_ERROR_LOG(::GetLastError(), PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_INVALIDATE_STATUSBAR, "Class::Invalidate statusbar");
   }
 }
 
-
-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;
 
   s_criticalSectionLocal.Lock();
   {
     if (!s_hIcons[type])
     {
       std::wstring imageToLoad = L"#";
       imageToLoad += std::to_wstring(s_hIconTypes[type]);
       s_hIcons[type] = (HICON)::LoadImage(_Module.m_hInst, imageToLoad.c_str(), IMAGE_ICON, iconWidth, iconHeight, LR_SHARED);
       if (!s_hIcons[type])
       {
         DEBUG_ERROR_LOG(::GetLastError(), PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_LOAD_ICON, "Class::GetIcon - LoadIcon");
       }
     }
 
     icon = s_hIcons[type];
   }
   s_criticalSectionLocal.Unlock();
 
   return icon;
 }
 
 ATOM CPluginClass::GetAtomPaneClass()
 {
   return s_atomPaneClass;
 }