Issue #1652, #3456 - Replace 's_asyncWebBrowser2' with thread-safe facilities

src/plugin/PluginClass.cpp

Index: src/plugin/PluginClass.cpp
===================================================================
--- a/src/plugin/PluginClass.cpp
+++ b/src/plugin/PluginClass.cpp
@@ -29,6 +29,7 @@
 #include "../shared/Utils.h"
 #include "../shared/Dictionary.h"
 #include "IeVersion.h"
+#include "Instances.h"
 #include "../shared/Version.h"
 #include <thread>
 #include <array>
@@ -56,14 +57,12 @@
 
 ATOM CPluginClass::s_atomPaneClass = NULL;
 HINSTANCE CPluginClass::s_hUxtheme = NULL;
-std::set<CPluginClass*> CPluginClass::s_instances;
+SyncSet<CPluginClass*, nullptr> instanceSet;
 std::map<DWORD, CPluginClass*> CPluginClass::s_threadInstances;
 
 CComAutoCriticalSection CPluginClass::s_criticalSectionLocal;
 CComAutoCriticalSection CPluginClass::s_criticalSectionWindow;
 
-CComQIPtr<IWebBrowser2> CPluginClass::s_asyncWebBrowser2;
-
 /*
  * Without namespace declaration, the identifier "Rectangle" is ambiguous
  * See http://msdn.microsoft.com/en-us/library/windows/desktop/dd162898(v=vs.85).aspx
@@ -89,6 +88,99 @@
   };
 }
 
+/**
+ * Wrapper class for a browser site object that ensures proper site life cycle.
+ *
+ * The browser is guaranteed to be valid for the lifetime of the object.
+ * What that means is that SetSite(0) on the containing BHO instance will not return before this object is destroyed.
+ * This property avoids a race condition where one thread is using a browser site for services while IE is simultaneously shutting it down.
+ * 
+ * This class may wrap a null browser site at the very beginning or very end of IE life time.
+ * Always check the contained value before using.
+ * If a null value is found at the beginning of IE life, it's probably a logic error, indicating that some code was launched too soon.
+ * If a null value is found at the end, it's probably harmless, since IE is shutting down.
+ *
+ * \par Invariant
+ *    - if 'browser' is nullptr then 'setSiteLock' does not own any mutex
+ *    - if 'browser' is not nullptr then 
+ *        - 'setSiteLock' owns the 'siteGuard' mutex of the CPluginClass instance from which 'browser' was copied
+ *        - 'setSiteLock' is still locked
+ */
+class AnyCurrentSiteBrowser {
+  /**
+   * The underlying browser is a raw pointer copy of 'm_webBrowser2' for some object.
+   * Synchronization ensures that it is not nullified for the life span of this object.
+   */
+  IWebBrowser2* browser;
+
+  /**
+   * Lock lasts for the duration of this object, if acquired in the constructor
+   */
+  std::unique_lock<std::mutex> setSiteLock;
+
+  /*
+   * This class is for scope variables only.
+   */
+  AnyCurrentSiteBrowser(const AnyCurrentSiteBrowser&); // = delete
+  AnyCurrentSiteBrowser& operator=(const AnyCurrentSiteBrowser&); // = delete
+  AnyCurrentSiteBrowser(AnyCurrentSiteBrowser&&); // = delete
+  AnyCurrentSiteBrowser& operator=(AnyCurrentSiteBrowser&&); // = delete
+
+public:
+  /**
+   * Default constructor copies one of the currently valid sites.
+   */
+  AnyCurrentSiteBrowser()
+  {
+    // Find a current site instance whose SetSite() lock we can acquire.
+    auto instance = instanceSet.LinearSearch(
+      [this](CPluginClass* p) -> bool
+      {
+        // Avoid deadlock with try-lock constructor
+        setSiteLock = std::unique_lock<std::mutex>(p->siteGuard, std::try_to_lock); // synchronize with return from p->SetSite()
+        if (!setSiteLock.owns_lock())
+        {
+          // SetSite(0) is currently executing in another thread, so we'll ignore this instance
+          return false;
+        }
+        // Assert 'setSiteLock' owns the mutex p->siteGuard and it is locked.
+        return true;
+      });
+    if (!instance)
+    {
+      browser = nullptr;
+    }
+    else
+    {
+      browser = instance->m_webBrowser2;
+      if (!browser)
+      {
+        throw std::logic_error("invariant violation: BHO instance without site found in a set of instances each of which must have a current site");
+      }
+    }
+  };
+  
+  /**
+   * Destructor releases any lock held and allows SetSite() to complete.
+   */
+  ~AnyCurrentSiteBrowser() {}; // = default
+
+  IWebBrowser2* operator->() const
+  {
+    return browser;
+  }
+
+  operator IWebBrowser2*() const
+  {
+    return browser;
+  }
+
+  operator bool() const
+  {
+    return browser != nullptr;
+  }
+};
+
 CPluginClass::CPluginClass()
   : m_webBrowser2(nullptr)
 {
@@ -137,19 +229,6 @@
   return m_webBrowser2.IsEqualObject(otherBrowser);
 }
 
-CComQIPtr<IWebBrowser2> CPluginClass::GetAsyncBrowser()
-{
-  CComQIPtr<IWebBrowser2> browser;
-
-  s_criticalSectionLocal.Lock();
-  {
-    browser = s_asyncWebBrowser2;
-  }
-  s_criticalSectionLocal.Unlock();
-
-  return browser;
-}
-
 std::wstring CPluginClass::GetBrowserUrl() const
 {
   std::wstring url;
@@ -195,6 +274,8 @@
 {
   try
   {
+    std::lock_guard<std::mutex> lock(siteGuard);
+
     if (unknownSite)
     {
 
@@ -214,16 +295,17 @@
       {
         throw std::logic_error("CPluginClass::SetSite - Unable to convert site pointer to IWebBrowser2*");
       }
+      if (!instanceSet.InsertIfAbsent(this))
+      {
+        throw std::logic_error("CPluginClass::SetSite - May not overwrite existing entry in instance set");
+      }
 
       //register the mimefilter
       //and only mimefilter
       //on some few computers the mimefilter does not get properly registered when it is done on another thread
       s_criticalSectionLocal.Lock();
       {
-        // 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();
 
@@ -281,23 +363,19 @@
       HANDLE hMainThread = NULL;
       HANDLE hTabThread = NULL;
 
+      if (!instanceSet.EraseWithCheck(this))
+      {
+        throw std::logic_error("CPluginClass::SetSite - Missing entry in instance set");
+      }
       s_criticalSectionLocal.Lock();
       {
-        s_instances.erase(this);
-
-        std::map<DWORD,CPluginClass*>::iterator it = s_threadInstances.find(::GetCurrentThreadId());
-        if (it != s_threadInstances.end())
-        {
-          s_threadInstances.erase(it);
-        }
-        if (s_instances.empty())
+        if (instanceSet.Empty())
         {
           // TODO: Explicitly releasing a resource when a container becomes empty looks like a job better suited for shared_ptr
           CPluginClientFactory::ReleaseMimeFilterClientInstance();
         }
       }
       s_criticalSectionLocal.Unlock();
-
       m_webBrowser2 = nullptr;
 
       DEBUG_GENERAL("================================================================================\nNEW TAB UI - END\n================================================================================")
@@ -313,116 +391,129 @@
   return S_OK;
 }
 
-bool CPluginClass::IsStatusBarEnabled()
+namespace
 {
-  DEBUG_GENERAL("IsStatusBarEnabled start");
-  HKEY pHkey;
-  HKEY pHkeySub;
-  RegOpenCurrentUser(KEY_QUERY_VALUE, &pHkey);
-  DWORD truth = 1;
-  DWORD truthSize = sizeof(truth);
-  RegOpenKey(pHkey, L"Software\\Microsoft\\Internet Explorer\\Main", &pHkeySub);
-  LONG res = RegQueryValueEx(pHkeySub, L"StatusBarWeb", NULL, NULL, (BYTE*)&truth, &truthSize);
-  RegCloseKey(pHkey);
-  if (res != ERROR_SUCCESS)
+  bool IsStatusBarDisabledInRegistry()

[Oleksandr, 2016/02/01 18:03:14]

I see no reason why IsStatusBarEnabled is refactored in this change set. It is
unrelated, IMO.

[Eric, 2016/02/03 14:57:28]

It's not entirely unrelated. These changes tag along with the changes of type of
the various browser variables from 'IWebBrowser2' or 'CComPtr<IWebBrowser2' to
'AnyCurrentSiteBrowser'. Strictly speaking, they could go into a separate change
set, but it would be a "noissue" commit.

I don't see enough value in creating two change sets here to go through the
effort of splitting the change set up. It might be worth changing the commit
message to reflect that there are "noissue" changes tagging along here to
related parts of the code.

   {
-    res = RegOpenKey(pHkey, L"Software\\Microsoft\\Internet Explorer\\MINIE", &pHkeySub);
-    if (res == ERROR_SUCCESS)
+    DEBUG_GENERAL("IsStatusBarDisabledInRegistry start");
+    HKEY pHkey;
+    HKEY pHkeySub;
+    RegOpenCurrentUser(KEY_QUERY_VALUE, &pHkey);
+    DWORD truth = 1;
+    DWORD truthSize = sizeof(truth);
+    RegOpenKey(pHkey, L"Software\\Microsoft\\Internet Explorer\\Main", &pHkeySub);
+    LONG res = RegQueryValueEx(pHkeySub, L"StatusBarWeb", NULL, NULL, (BYTE*)&truth, &truthSize);
+    RegCloseKey(pHkey);
+    if (res != ERROR_SUCCESS)
     {
-      LONG res = RegQueryValueEx(pHkeySub, L"ShowStatusBar", NULL, NULL, (BYTE*)&truth, &truthSize);
+      res = RegOpenKey(pHkey, L"Software\\Microsoft\\Internet Explorer\\MINIE", &pHkeySub);
       if (res == ERROR_SUCCESS)
       {
-        RegCloseKey(pHkey);
+        LONG res = RegQueryValueEx(pHkeySub, L"ShowStatusBar", NULL, NULL, (BYTE*)&truth, &truthSize);
+        if (res == ERROR_SUCCESS)
+        {
+          RegCloseKey(pHkey);
+        }
       }
     }
+    DEBUG_GENERAL("IsStatusBarDisabledInRegistry end");
+    return truth != 1;
   }
-  DEBUG_GENERAL("IsStatusBarEnabled end");
-  return truth == 1;
+ 
+  /**
+   * Navigate to a new tab, falling back to a new window if needed.
+   */
+  bool NavigateNewTab(IWebBrowser2* browser, const std::wstring& targetUrl)
+  {
+    ATL::CComBSTR targetUrlBstr(static_cast<int>(targetUrl.length()), targetUrl.c_str());
+    VARIANT vFlags;
+    vFlags.vt = VT_I4;
+    vFlags.intVal = navOpenInNewTab;
+    HRESULT hr = browser->Navigate(targetUrlBstr, &vFlags, NULL, NULL, NULL);
+    if (FAILED(hr))
+    {
+      vFlags.intVal = navOpenInNewWindow;
+
+      hr = browser->Navigate(targetUrlBstr, &vFlags, NULL, NULL, NULL);
+      if (FAILED(hr))
+      {
+        DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_NAVIGATION, 0, "Navigation::Failed");
+        return false;
+      }
+    }
+    return true;
+  }
 }
 
 void CPluginClass::ShowStatusBar()
 {
   DEBUG_GENERAL("ShowStatusBar start");
-
+  AnyCurrentSiteBrowser browser;
+  if (!browser)
+  {
+    DEBUG_GENERAL("ShowStatusBar abort; no browser");
+    return;
+  }
   VARIANT_BOOL isVisible;
-
-
-  CComQIPtr<IWebBrowser2> browser = GetAsyncBrowser();
-  if (browser)
+  HRESULT hr = browser->get_StatusBar(&isVisible);
+  if (FAILED(hr))
   {
-    HRESULT hr = S_OK;
-    hr = browser->get_StatusBar(&isVisible);
-    if (SUCCEEDED(hr))
-    {
-      if (!isVisible)
-      {
-        SHANDLE_PTR pBrowserHWnd;
-        browser->get_HWND((SHANDLE_PTR*)&pBrowserHWnd);
-        Dictionary* dictionary = Dictionary::GetInstance();
-
-        HKEY pHkey;
-        HKEY pHkeySub;
-        LSTATUS regRes = 0;
-        regRes = RegOpenCurrentUser(KEY_WRITE, &pHkey);
-
-        // Do we have enough rights to enable a status bar?
-        if (regRes != 0)
-        {
-          // We use the tab window here and in the next few calls, since the browser window may still not be available
-          LRESULT res = MessageBox((HWND)m_hTabWnd,
-              dictionary->Lookup("status-bar", "error-text").c_str(),
-              dictionary->Lookup("status-bar", "error-title").c_str(),
-              MB_OK);
-          return;
-        }
-        // Ask if a user wants to enable a status bar automatically
-        LRESULT res = MessageBox((HWND)m_hTabWnd,
-            dictionary->Lookup("status-bar", "question").c_str(),
-            dictionary->Lookup("status-bar", "title").c_str(),
-            MB_YESNO);
-        if (res == IDYES)
-        {
-          DWORD truth = 1;
-          regRes = RegOpenKey(pHkey, L"Software\\Microsoft\\Internet Explorer\\MINIE", &pHkeySub);
-          regRes = RegSetValueEx(pHkeySub, L"ShowStatusBar", 0, REG_DWORD, (BYTE*)&truth, sizeof(truth));
-          regRes = RegCloseKey(pHkeySub);
-          regRes = RegOpenKey(pHkey, L"Software\\Microsoft\\Internet Explorer\\Main", &pHkeySub);
-          regRes = RegSetValueEx(pHkeySub, L"StatusBarWeb", 0, REG_DWORD, (BYTE*)&truth, sizeof(truth));
-          regRes = RegCloseKey(pHkeySub);
-          hr = browser->put_StatusBar(TRUE);
-          if (FAILED(hr))
-          {
-            DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_PUT_STATUSBAR, "Class::Enable statusbar");
-          }
-          CreateStatusBarPane();
-
-          // We need to restart the tab now, to enable the status bar properly
-          VARIANT vFlags;
-          vFlags.vt = VT_I4;
-          vFlags.intVal = navOpenInNewTab;
-
-          CComBSTR curLoc;
-          browser->get_LocationURL(&curLoc);
-          HRESULT hr = browser->Navigate(curLoc, &vFlags, NULL, NULL, NULL);
-          if (FAILED(hr))
-          {
-            vFlags.intVal = navOpenInNewWindow;
-
-            hr = browser->Navigate(CComBSTR(curLoc), &vFlags, NULL, NULL, NULL);
-            if (FAILED(hr))
-            {
-              DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_NAVIGATION, PLUGIN_ERROR_NAVIGATION, "Navigation::Failed")
-            }
-          }
-          browser->Quit();
-        }
-      }
-    }
-    else
-    {
-      DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_STATUSBAR, "Class::Get statusbar state");
-    }
+    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_STATUSBAR, "CPluginClass::ShowStatusBar get_StatusBar");
+    return;
   }
+  if (!isVisible)

[Oleksandr, 2016/02/01 18:03:14]

Same here. Why is this done in the change set with the message "Replace
's_asyncWebBrowser2' with thread-safe facilities"? I don't think this is related
at all.

[Eric, 2016/02/03 14:57:28]

See above.

+  {
+    DEBUG_GENERAL("ShowStatusBar end early; browser told us status bar is visible");
+    return;
+  }
+  Dictionary* dictionary = Dictionary::GetInstance();
+  /*
+   * Do we have enough rights to enable a status bar?
+   */
+  HKEY pHkey;
+  LSTATUS regRes = RegOpenCurrentUser(KEY_WRITE, &pHkey);
+  if (regRes != 0)
+  {
+    // We use the tab window here and in the next few calls, since the browser window may still not be available
+    LRESULT res = MessageBox((HWND)m_hTabWnd,
+      dictionary->Lookup("status-bar", "error-text").c_str(),
+      dictionary->Lookup("status-bar", "error-title").c_str(),
+      MB_OK);
+    DEBUG_GENERAL("ShowStatusBar end early; user must enable status bar");
+    return;
+  }
+  /*
+   * Ask if a user wants to enable a status bar automatically
+   */
+  LRESULT res = MessageBox((HWND)m_hTabWnd,
+    dictionary->Lookup("status-bar", "question").c_str(),
+    dictionary->Lookup("status-bar", "title").c_str(),
+    MB_YESNO);
+  if (res != IDYES)
+  {
+    DEBUG_GENERAL("ShowStatusBar end early; user did not enable status bar");
+    return;
+  }
+  HKEY pHkeySub;
+  DWORD truth = 1;
+  regRes = RegOpenKey(pHkey, L"Software\\Microsoft\\Internet Explorer\\MINIE", &pHkeySub);
+  regRes = RegSetValueEx(pHkeySub, L"ShowStatusBar", 0, REG_DWORD, (BYTE*)&truth, sizeof(truth));
+  regRes = RegCloseKey(pHkeySub);
+  regRes = RegOpenKey(pHkey, L"Software\\Microsoft\\Internet Explorer\\Main", &pHkeySub);
+  regRes = RegSetValueEx(pHkeySub, L"StatusBarWeb", 0, REG_DWORD, (BYTE*)&truth, sizeof(truth));
+  regRes = RegCloseKey(pHkeySub);
+  hr = browser->put_StatusBar(TRUE);
+  if (FAILED(hr))
+  {
+    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_PUT_STATUSBAR, "Class::Enable statusbar");
+    // Ignore error
+  }
+  CreateStatusBarPane();
+  CComBSTR curLoc;
+  browser->get_LocationURL(&curLoc);
+  std::wstring currentLocation(ToWstring(curLoc));
+  NavigateNewTab(browser, currentLocation);
+  browser->Quit();
   DEBUG_GENERAL("ShowStatusBar end");
 }
 
@@ -691,7 +782,7 @@
     {
       CreateThread(NULL, NULL, (LPTHREAD_START_ROUTINE)CPluginClass::FirstRunThread, NULL, NULL, NULL);
     }
-    if (((m_hPaneWnd == NULL) || !IsStatusBarEnabled()) && isFirstRun)
+    if (((m_hPaneWnd == NULL) || IsStatusBarDisabledInRegistry()) && isFirstRun)
     {
       ShowStatusBar();
     }
@@ -749,22 +840,12 @@
         ::GetWindowThreadProcessId(hTabWnd2, &nProcessId);
         if (::GetCurrentProcessId() == nProcessId)
         {
-          bool bExistingTab = false;
-
-          s_criticalSectionLocal.Lock();
-          {
-            for (auto instance : s_instances)
+          auto instance = instanceSet.LinearSearch(
+            [hTabWnd2](CPluginClass* p)
             {
-              if (instance->m_hTabWnd == hTabWnd2)
-              {
-                bExistingTab = true;
-                break;
-              }
-            }
-          }
-          s_criticalSectionLocal.Unlock();
-
-          if (!bExistingTab)
+              return p->m_hTabWnd == hTabWnd2;
+            });
+          if (!instance)
           {
             amoundOfNewTabs ++;
             uniqueNewTab = hTabWnd2;
@@ -877,31 +958,31 @@
   return true;
 }
 
+// Entry point
 void CPluginClass::FirstRunThread()
 {
-  // Just return if the First Run Page should be suppressed
-  if (CPluginClient::GetInstance()->GetPref(L"suppress_first_run_page", false))
-    return;
+  try
+  {
+    // Just return if the First Run Page should be suppressed
+    if (CPluginClient::GetInstance()->GetPref(L"suppress_first_run_page", false))
+      return;
 
-  CoInitialize(NULL);
-  VARIANT vFlags;
-  vFlags.vt = VT_I4;
-  vFlags.intVal = navOpenInNewTab;
-
-  CComBSTR navigatePath = CComBSTR(FirstRunPageFileUrl().c_str());
-  
-  HRESULT hr = GetAsyncBrowser()->Navigate(navigatePath, &vFlags, NULL, NULL, NULL);
-  if (FAILED(hr))
+    if (SUCCEEDED(CoInitialize(NULL)))
+    {
+      AnyCurrentSiteBrowser browser;
+      if (browser)
+      {
+        NavigateNewTab(browser, FirstRunPageFileUrl());

[Oleksandr, 2016/02/01 18:03:14]

I agree this looks better with NavigateNewTab separated, but this is not
relevant to the present change at all. 

[Eric, 2016/02/03 14:57:28]

When I went through and changed types to use 'AnyCurrentSiteBrowser', I went
ahead and improved the code in its vicinity.

As I've said elsewhere, it could be split into a "noissue" commit, but I don't
see a lot of value in that at this point.

+      }
+    }
+    CoUninitialize();
+  }
+  catch (...)
   {
-    vFlags.intVal = navOpenInNewWindow;
-    hr = GetAsyncBrowser()->Navigate(navigatePath, &vFlags, NULL, NULL, NULL);
-  }
-
-  if (FAILED(hr))
-  {
-    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_NAVIGATION, PLUGIN_ERROR_NAVIGATION_WELCOME, "Navigation::Welcome page failed")
+    // Suppress errors
   }
 }
+
 void CPluginClass::CloseTheme()
 {
   if (m_hTheme)
@@ -928,25 +1009,13 @@
   }
 }
 
-
 CPluginClass* CPluginClass::FindInstance(HWND hStatusBarWnd)
 {
-  CPluginClass* result = nullptr;
-
-  s_criticalSectionLocal.Lock();
-  {
-    for (auto instance : s_instances)
+  return instanceSet.LinearSearch(
+    [hStatusBarWnd](CPluginClass* p) -> bool
     {
-      if (instance->m_hStatusBarWnd == hStatusBarWnd)
-      {
-        result = instance;
-        break;
-      }
-    }
-  }
-  s_criticalSectionLocal.Unlock();
-
-  return result;
+      return p->m_hStatusBarWnd == hStatusBarWnd;
+    });
 }
 
 CPluginTab* CPluginClass::GetTab()
@@ -1064,29 +1133,13 @@
     break;
   case ID_MENU_SETTINGS:
     {
-      CComQIPtr<IWebBrowser2> browser = GetAsyncBrowser();
+      AnyCurrentSiteBrowser browser;
       if (browser)
       {
-        VARIANT vFlags;
-        vFlags.vt = VT_I4;
-        vFlags.intVal = navOpenInNewTab;
-
-        auto userSettingsFileUrl = UserSettingsFileUrl();
-        ATL::CComBSTR urlToNavigate(static_cast<int>(userSettingsFileUrl.length()), userSettingsFileUrl.c_str());
-        HRESULT hr = browser->Navigate(urlToNavigate, &vFlags, NULL, NULL, NULL);
-        if (FAILED(hr))
-        {
-          vFlags.intVal = navOpenInNewWindow;
-
-          hr = browser->Navigate(urlToNavigate, &vFlags, NULL, NULL, NULL);
-          if (FAILED(hr))
-          {
-            DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_NAVIGATION, PLUGIN_ERROR_NAVIGATION_SETTINGS, "Navigation::Failed")
-          }
-        }
+        NavigateNewTab(browser, UserSettingsFileUrl());
       }
-      break;
     }
+    break;
   case ID_MENU_DISABLE_ON_SITE:
     {
       std::wstring urlString = GetTab()->GetDocumentUrl();