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

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 11 matching lines @@
 #include "PluginSystem.h"
 #include "PluginFilter.h"
 #include "PluginMimeFilterClient.h"
 #include "AdblockPlusClient.h"
 #include "PluginClientBase.h"
 #include "PluginClientFactory.h"
 #include "PluginUtil.h"
 #include "../shared/Utils.h"
 #include "../shared/Dictionary.h"
 #include "IeVersion.h"
+#include "Instances.h"
 #include "../shared/Version.h"
 #include <thread>
 #include <array>
 
 #ifdef DEBUG_HIDE_EL
 DWORD profileTime = 0;
 #endif
 
 extern CComModule _Module;
 
 typedef HANDLE (WINAPI *OPENTHEMEDATA)(HWND, LPCWSTR);
 typedef HRESULT (WINAPI *DRAWTHEMEBACKGROUND)(HANDLE, HDC, INT, INT, LPRECT, LPRECT);
 typedef HRESULT (WINAPI *CLOSETHEMEDATA)(HANDLE);
 
 HICON CPluginClass::s_hIcons[ICON_MAX] = { NULL, NULL, NULL };
 DWORD CPluginClass::s_hIconTypes[ICON_MAX] = { IDI_ICON_DISABLED, IDI_ICON_ENABLED, IDI_ICON_DEACTIVATED };
 uint32_t iconHeight = 32;
 uint32_t iconWidth = 32;
 
 CPluginMimeFilterClient* CPluginClass::s_mimeFilter = NULL;
 
 CLOSETHEMEDATA pfnClose = NULL;
 DRAWTHEMEBACKGROUND pfnDrawThemeBackground = NULL;
 OPENTHEMEDATA pfnOpenThemeData = NULL;
 
 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
  */
 namespace AdblockPlus
 {
   /**
     * Replacement for ATL type CRect.
     */
   class Rectangle
     : public RECT
   {
   public:
     int Height() const
     {
       return bottom - top;
     }
 
     int Width() const
     {
       return right - left;
     }
   };
 }
 
+/**
+ * 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)
 {
   //Use this line to debug memory leaks
   //    _CrtDumpMemoryLeaks();
 
   m_isAdvised = false;
   m_hTabWnd = NULL;
   m_hStatusBarWnd = NULL;
   m_hPaneWnd = NULL;
@@ skipping 28 matching lines @@
     return nullptr;
   }
   return (HWND)hBrowserWndHandle;
 }
 
 bool CPluginClass::IsRootBrowser(IWebBrowser2* otherBrowser)
 {
   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;
   if (m_webBrowser2)
   {
     CComBSTR bstrURL;
     if (SUCCEEDED(m_webBrowser2->get_LocationURL(&bstrURL)) && bstrURL)
     {
       url = std::wstring(bstrURL, SysStringLen(bstrURL));
     }
@@ skipping 25 matching lines @@
  *
  * 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
   {
+    std::lock_guard<std::mutex> lock(siteGuard);
+
     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*");
       }
+      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();
 
       try
       {
         DEBUG_GENERAL("Loaded as BHO");
         HRESULT hr = DispEventAdvise(m_webBrowser2);
         if (SUCCEEDED(hr))
         {
           m_isAdvised = true;
@@ skipping 37 matching lines @@
         m_hPaneWnd = NULL;
       }
 
       m_hTabWnd = NULL;
       m_hStatusBarWnd = NULL;
 
       // Remove instance from the list, shutdown threads
       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================================================================================")
 
       ::CoUninitialize();
     }
 
   }
   catch (...)
   {
   }
   IObjectWithSiteImpl<CPluginClass>::SetSite(unknownSite);
   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");
 }
 
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnBeforeNavigate2(
   IDispatch* frameBrowserDisp /**< [in] */,
   VARIANT* urlVariant /**< [in] */,
   VARIANT* /**< [in] Flags*/,
   VARIANT* /**< [in] TargetFrameName*/,
   VARIANT* /**< [in] PostData*/,
@@ skipping 248 matching lines @@
 
     // IE6 can't be accessed from another thread, execute in current thread
     if (ieVersion < 7)
     {
       FirstRunThread();
     }
     else
     {
       CreateThread(NULL, NULL, (LPTHREAD_START_ROUTINE)CPluginClass::FirstRunThread, NULL, NULL, NULL);
     }
-    if (((m_hPaneWnd == NULL) || !IsStatusBarEnabled()) && isFirstRun)
+    if (((m_hPaneWnd == NULL) || IsStatusBarDisabledInRegistry()) && isFirstRun)
     {
       ShowStatusBar();
     }
 
     // Enable acceptable ads by default
     std::wstring aaUrl = CPluginClient::GetInstance()->GetPref(L"subscriptions_exceptionsurl", L"");
     CPluginClient::GetInstance()->AddSubscription(aaUrl);
   }
   s_criticalSectionLocal.Unlock();
 
@@ skipping 37 matching lines @@
       {
         hTabWnd2 = ::FindWindowEx(hTabWnd2, NULL, L"TabWindowClass", NULL);
       }
 
       if (hTabWnd2)
       {
         DWORD nProcessId;
         ::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;
             if (GetCurrentThreadId() == GetWindowThreadProcessId(hTabWnd2, NULL))
             {
               hBrowserWnd = hTabWnd = hTabWnd2;
               break;
             }
 
           }
@@ skipping 92 matching lines @@
     delete []pData;
   }
     
   HDC hdc = GetWindowDC(m_hStatusBarWnd);
   SendMessage(m_hStatusBarWnd, WM_PAINT, (WPARAM)hdc, 0);
   ReleaseDC(m_hStatusBarWnd, hdc);
   
   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)
   {
     if (pfnClose)
     {
       pfnClose(m_hTheme);
     }
 
     m_hTheme = NULL;
   }
 }
 
 void CPluginClass::UpdateTheme()
 {
   CloseTheme();
 
   if (pfnOpenThemeData)
   {
     m_hTheme = pfnOpenThemeData(m_hPaneWnd, L"STATUS");
     if (!m_hTheme)
     {
     }
   }
 }
 
-
 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()
 {
   return m_tab;
 }
 
 CPluginTab* CPluginClass::GetTab(DWORD dwThreadId)
 {
   CPluginTab* tab = NULL;
@@ skipping 97 matching lines @@
           s_mimeFilter = NULL;
 
           CPluginClientFactory::ReleaseMimeFilterClientInstance();
         }
       }
       s_criticalSectionLocal.Unlock();
     }
     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();
       std::string filterText = client->GetWhitelistingFilter(urlString);
       if (!filterText.empty())
       {
         client->RemoveFilter(filterText);
       }
       else
       {
@@ skipping 533 matching lines @@
   s_criticalSectionLocal.Unlock();
 
   return icon;
 }
 
 ATOM CPluginClass::GetAtomPaneClass()
 {
   return s_atomPaneClass;
 }