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 "Instances.h"
 #include <thread>
@@ -57,13 +58,11 @@
 
 ATOM CPluginClass::s_atomPaneClass = NULL;
 HINSTANCE CPluginClass::s_hUxtheme = NULL;
-std::set<CPluginClass*> CPluginClass::s_instances;
+SyncSet<CPluginClass*, nullptr> instanceSet;
 
 CComAutoCriticalSection CPluginClass::s_criticalSectionLocal;
 CComAutoCriticalSection CPluginClass::s_criticalSectionWindow;
 
-CComQIPtr<IWebBrowser2> CPluginClass::s_asyncWebBrowser2;
-
 namespace
 {
   /**
@@ -130,6 +129,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)
 {
@@ -189,19 +281,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;
@@ -250,6 +329,8 @@
 {
   try
   {
+    std::lock_guard<std::mutex> lock(siteGuard);
+
     if (unknownSite)
     {
       DEBUG_GENERAL(L"================================================================================\nNEW TAB UI\n================================================================================");
@@ -283,16 +364,18 @@
       {
         throw std::logic_error("CPluginClass::SetSite - May not overwrite existing entry in thread map");
       }
+      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();
 
@@ -357,22 +440,23 @@
       HANDLE hMainThread = NULL;
       HANDLE hTabThread = NULL;
 
+      if (!instanceSet.EraseWithCheck(this))
+      {
+        throw std::logic_error("CPluginClass::SetSite - Missing entry in instance set");
+      }
       if (!threadMap.RemoveAndCheck())
       {
         throw std::logic_error("CPluginClass::SetSite - Missing entry in thread map");
       }
-
       s_criticalSectionLocal.Lock();
       {
-        s_instances.erase(this);
-        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================================================================================")
@@ -388,116 +472,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()
   {
-    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)
+  {
+    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");
 }
 
@@ -761,7 +858,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();
     }
@@ -819,22 +916,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;
@@ -947,31 +1034,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());
+      }
+    }
+    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)
@@ -998,25 +1085,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()
@@ -1130,29 +1205,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();