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

src/plugin/PluginClass.cpp

Index: src/plugin/PluginClass.cpp
===================================================================
--- a/src/plugin/PluginClass.cpp
+++ b/src/plugin/PluginClass.cpp
@@ -30,6 +30,7 @@
 #include "../shared/Dictionary.h"
 #include "IeVersion.h"
 #include "../shared/Version.h"
+#include "Instances.h"
 #include <thread>
 #include <array>
 
@@ -57,13 +58,43 @@
 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
@@ -103,10 +134,7 @@
   m_pWndProcStatus = NULL;
   m_hTheme = NULL;
   m_isInitializedOk = false;
-
-
   m_tab = new CPluginTab(this);
-
   Dictionary::Create(GetBrowserLanguage());
 }
 
@@ -215,6 +243,14 @@
         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
@@ -281,15 +317,14 @@
       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
@@ -526,16 +561,11 @@
       && 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();
@@ -954,21 +984,10 @@
   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;
 }
 
 
@@ -1349,7 +1368,7 @@
   // 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();
@@ -1512,7 +1531,7 @@
 
   case WM_UPDATEUISTATE:
     {
-      CPluginTab* tab = GetTab(::GetCurrentThreadId());
+      CPluginTab* tab = GetTabCurrentThread();
       if (tab)
       {
         tab->OnActivate();