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-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 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;
 
+

[sergei, 2016/02/01 15:50:43]

Why not to put it into anonymous namespace?

[Eric, 2016/02/03 17:17:05]

OK.

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

[sergei, 2016/02/01 15:50:43]

Off topic: Still a global variable. When will we stop it?...

[Eric, 2016/02/03 17:17:05]

The map from threads to BHO instances is inherently a singleton, and thus will
always act much like a global. Just can't get around it.

Global variables are not inherently bad. Global variables that could be scoped
smaller are what's bad. With the state that 'CPluginClass' is in right now,
combining BHO and UI responsibilities, we can't do incrementally better.

+
 /*
  * 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 19 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(this);
-
   Dictionary::Create(GetBrowserLanguage());
 }
 
 CPluginClass::~CPluginClass()
 {
   delete m_tab;
 }
 
 HWND CPluginClass::GetBrowserHWND() const
 {
@@ skipping 88 matching lines @@
 
       /*
        * 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*");
       }
 
+      /*
+       * 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/01 15:50:43]

That looks suspicious. It can happen that two instances are instantiated in one
thread and it should be fine for us.
By throwing the exception here, we are basically breaking the functionality for
any other instance created using the same thread.

[Eric, 2016/02/03 17:17:05]

Actually, that cannot happen. 

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

[sergei, 2016/02/01 15:50:43]

Pay attention that we are overriding the value of `CPluginClass*` for
`::GetCurrentThreadId()`. But now it does not happen. See also the comment above
in SetSite.
As it has been said in comments earlier, it looks logical to set the current
CPluginClass* from current UI thread.

[Eric, 2016/02/03 17:17:05]

I don't know what you mean here.
It's not logical; it's incorrect.

-        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;
 }
 
 
 STDMETHODIMP CPluginClass::QueryStatus(const GUID* pguidCmdGroup, ULONG cCmds, OLECMD prgCmds[], OLECMDTEXT* pCmdText)
 {
   if (cCmds == 0) return E_INVALIDARG;
   if (prgCmds == 0) return E_POINTER;
 
   prgCmds[0].cmdf = OLECMDF_ENABLED;
 
@@ skipping 360 matching lines @@
   return result;
 
 }
 
 
 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 142 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.Move(rect.left + (rect.right - rect.left) / 2, rect.top + (rect.bottom - rect.top) / 2); 
       }
       if (LOWORD(wParam) == UIS_CLEAR)
       {
         pClass->notificationMessage.Hide();
@@ skipping 107 matching lines @@
   s_criticalSectionLocal.Unlock();
 
   return icon;
 }
 
 ATOM CPluginClass::GetAtomPaneClass()
 {
   return s_atomPaneClass;
 }