Issue #1467, #3397 - Rewrite the map from threads to tabs

src/plugin/PluginClass.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 12 matching lines @@
 #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 "../shared/Version.h"
+#include "Instances.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;
-std::map<DWORD, CPluginClass*> CPluginClass::s_threadInstances;
 
 CComAutoCriticalSection CPluginClass::s_criticalSectionLocal;
 CComAutoCriticalSection CPluginClass::s_criticalSectionWindow;
 
 CComQIPtr<IWebBrowser2> CPluginClass::s_asyncWebBrowser2;
 
+namespace
+{
+  /**
+   * A synchronized map whose key is always the current thread ID.
+   */
+  class CurrentThreadMap
+    : public SyncMap<DWORD, CPluginClass*, nullptr>
+  {
+    typedef SyncMap<DWORD, CPluginClass*, nullptr> Base;
+
+  public:
+    bool AddIfAbsent(CPluginClass* p)
+    {
+      return Base::AddIfAbsent(::GetCurrentThreadId(), p);
+    }
+
+    bool RemoveAndCheck()
+    {
+      return Base::RemoveAndCheck(::GetCurrentThreadId());
+    }
+
+    CPluginClass* Locate()
+    {
+      return Base::Locate(::GetCurrentThreadId());
+    }
+  };
+
+  /**
+   * Map from thread ID's to CPluginClass instances
+   */
+  CurrentThreadMap threadMap;
+}
+
 /*
  * 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
@@ skipping 34 matching lines @@
   //    _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();
-
   Dictionary::Create(GetBrowserLanguage());
 }
 
 CPluginClass::~CPluginClass()
 {
   DEBUG_GENERAL([this]() -> std::wstring
     {
       std::wstring s = L"CPluginClass::<destructor>, this = ";
       s += ToHexLiteral(this);
       return s;
@@ skipping 104 matching lines @@
         throw std::logic_error("CPluginClass::SetSite - Unable to convert site pointer to IWebBrowser2*");
       }
       DEBUG_GENERAL([this]() -> std::wstring
         {
           std::wstringstream ss;
           ss << L"CPluginClass::SetSite, this = " << ToHexLiteral(this);
           ss << L", browser = " << ToHexLiteral(m_webBrowser2);
           return ss.str();
         }());
 
+      /*
+       * Add ourselves to the thread map.
+       */
+      if (!threadMap.AddIfAbsent(this))
+      {
+        throw std::logic_error("CPluginClass::SetSite - May not overwrite existing entry in thread map");

[sergei, 2016/02/08 13:35:37]

I would also like to have asserts here and for RemoveAndCheck to be notified in
time.

[Eric, 2016/02/08 18:45:31]

No. We have error handling already. Doubling it up is just code bloat.

[Oleksandr, 2016/02/10 10:58:48]

I don't think we need asserts here as well.

+      }
+
       //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);
       }
@@ skipping 53 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 (!threadMap.RemoveAndCheck())
+      {
+        throw std::logic_error("CPluginClass::SetSite - Missing entry in thread map");
+      }
+
       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())
         {
           // 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;
 
@@ skipping 216 matching lines @@
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnWindowStateChanged(unsigned long flags, unsigned long validFlagsMask)
 {
   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())
+      if (!m_isInitializedOk)
       {
-        s_threadInstances[::GetCurrentThreadId()] = this;
-        if (!m_isInitializedOk)
-        {
-          m_isInitializedOk = true;
-          InitObject();
-          UpdateStatusBar();
-        }
+        m_isInitializedOk = true;
+        InitObject();
+        UpdateStatusBar();
       }
     }
     notificationMessage.Hide();
     DEBUG_GENERAL(L"WindowStateChanged (check tab changed) end");
   }
   catch (...)
   {
   }
 }
 
@@ skipping 398 matching lines @@
   s_criticalSectionLocal.Unlock();
 
   return result;
 }
 
 CPluginTab* CPluginClass::GetTab()
 {
   return m_tab;
 }
 
-CPluginTab* CPluginClass::GetTab(DWORD dwThreadId)
+CPluginTab* CPluginClass::GetTabCurrentThread()
 {
-  CPluginTab* tab = NULL;
-
-  s_criticalSectionLocal.Lock();
-  {
-    std::map<DWORD,CPluginClass*>::const_iterator it = s_threadInstances.find(dwThreadId);
-    if (it != s_threadInstances.end())
-    {
-      tab = it->second->m_tab;
-    }
-  }
-  s_criticalSectionLocal.Unlock();
-
-  return tab;
+  auto p = threadMap.Locate();
+  return p ? p->m_tab : nullptr;
 }
 
 // Entry point
 STDMETHODIMP CPluginClass::QueryStatus(const GUID* pguidCmdGroup, ULONG cCmds, OLECMD prgCmds[], OLECMDTEXT* pCmdText)
 {
   try
   {
     if (cCmds == 0) return E_INVALIDARG;
     if (prgCmds == 0) return E_POINTER;
 
@@ skipping 390 matching lines @@
   }
   return ::CallWindowProc(pClass->m_pWndProcStatus, hWnd, message, wParam, lParam);
 }
 
 
 HICON CPluginClass::GetStatusBarIcon(const std::wstring& url)
 {
   // use the disable icon as defualt, if the client doesn't exists
   HICON hIcon = GetIcon(ICON_PLUGIN_DEACTIVATED);
 
-  CPluginTab* tab = GetTab(::GetCurrentThreadId());
+  CPluginTab* tab = GetTabCurrentThread();
   if (tab)
   {
     CPluginClient* client = CPluginClient::GetInstance();
     if (CPluginSettings::GetInstance()->IsPluginEnabled())
     {
       if (client->IsWhitelistedUrl(url))
       {
         hIcon = GetIcon(ICON_PLUGIN_DISABLED);
       }
       else
@@ skipping 143 matching lines @@
         pClass->DisplayPluginMenu(hMenu, -1, pt, TPM_LEFTALIGN | TPM_BOTTOMALIGN);
         break;
       }
     case WM_DESTROY:
       break;
     case SC_CLOSE:
       break;
 
     case WM_UPDATEUISTATE:
       {
-        CPluginTab* tab = GetTab(::GetCurrentThreadId());
+      CPluginTab* tab = GetTabCurrentThread();
         if (tab)
         {
           tab->OnActivate();
           RECT rect;
           GetWindowRect(pClass->m_hPaneWnd, &rect);
           pClass->notificationMessage.MoveToCenter(rect);
         }
         if (LOWORD(wParam) == UIS_CLEAR)
         {
           pClass->notificationMessage.Hide();
@@ skipping 112 matching lines @@
   s_criticalSectionLocal.Unlock();
 
   return icon;
 }
 
 ATOM CPluginClass::GetAtomPaneClass()
 {
   return s_atomPaneClass;
 }