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("Instance without site found in set of instances with site");

[sergei, 2016/01/04 15:34:31]

"... where site is a browser (IWebBrowser2) and instance refers to an instance
of CPluginClass". Otherwise this message is completely unclear. Image that you
encountered it in a log.

[Eric, 2016/01/04 17:49:20]

When I encounter such messages in a log, I immediately use 'grep' to search the
source code for the occurrence of the statement. The exact text doesn't need to
be extensive.

More importantly, though, it's a 'logic_error', which means that your code has a
defect if it ever throws at all. If you find such a message in a log, you've got
bigger problems.

I have clarified the message, not to talk about specific classes, but to more
clearly explain the invariant that is violated if reached here.

+      }
+    }
+  };
+  
+  /**
+   * 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))

[sergei, 2016/01/04 15:34:31]

I would like to have `assert`s in addition to throwing of exceptions here and
below, otherwise the exceptions are silently eaten and we won't know about a
trouble.

[Eric, 2016/01/04 17:49:20]

'assert' is a C mechanism, and it's only active during debug.

This is very consciously a 'throw' here. While we currently have no capability
of logging in Release builds, this is the kind of exception that we want to log
to the Windows Error Reporting facility. We don't have code for that, yet.

If you want to catch them, set up Visual Studio to do so. In
Debug/Windows/Exceptions Settings, add "std::logic_error" to the list called
"Break When Thrown".

[sergei, 2016/01/29 15:28:56]

`assert` is also C++ mechanism. Yes, it's active only during debugging because
its main aim is to notify the developer about an issue. I'm not suggesting to
replace throwing, but add `assert` in addition.
That's very inconvenient because of too many false positives. I could use a
special Visual Studio profile only for adblockplusie project, but it's really
not friendly for anybody else.

[Eric, 2016/01/29 16:16:12]

You're advocating code bloat for zero-to-minimal benefit.

The presence of this check here in 'SetSite()' is purely defensive. IE creates a
new thread for each BHO instance.

Request add 'assert' here rejected.

+      {
+        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()
   {
-    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");
 }
 
@@ -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());
+      }
+    }
+    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;

[sergei, 2016/01/04 15:34:31]

Why can we not use `m_webBrowser2` here? When can it be called not between
SetSite(not nullptr) and SetSite(nullptr)?

[sergei, 2016/01/29 15:28:56]

Not answered.

[Eric, 2016/01/29 16:16:12]

This function can be called from a worker thread which is not associated with
any BHO at all.
These worker threads never call 'SetSite()' at all. (And it's never the case
that any single thread is ever reused for a second BHO instance, although that's
not particularly relevant here.)

The race condition is that we have events registered in such a way that it's
possible that a call appear here in one thread (a worker thread) where another
thread has already called 'SetSite(nullptr)' on the thread associated with this
BHO instance. Hence it's possible for 
'm_webBrowser2' to be null here.

What you're observing here is a consequence of some really awful (and quite old)
implementation decisions about 'CPluginClass'. It is handling both BHO instances
as well as UI events. They really should never have been combined into a single
class in the first place. Regardless of the past, if the UI event handlers were
in a separate class, then 'AnyCurrentSiteBrowser' is how such a class would get
a browser pointer to call 'Navigate()' on.

[Eric, 2016/01/29 16:16:12]

Sorry. Must have missed it earlier.

       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();