Synchronize access to s_threadInstances in CPluginClass::OnWindowStateChanged

[sergz]

Background

We are modifying it there but there is no any synchronization primitive is used.

What to change

For the present just add locks using s_criticalSectionLocal.

[oleksandr]

You probably mean OnTabChanged, not OnWindowStateChanged?

[eric@adblockplus.org]

I'm not convinced this is even the right problem to fix, but certainly just adding another lock call in a random location in the code is at best a half measure.

• There's a very basic assumption around s_threadInstances that there's only ever a single instance of CPluginClass per thread. This is inherent in the structure that maps a thread identifier to a single instance rather than to a set of instances; in other words, it's a one-to-one map and not a one-to-many relationship.

• The apparent behavior of IE is that instances of CPluginClass and tab threads correspond bijectively, that is, there's only one instance per thread and one thread per instance. If this is the case, then there's no need to synchronize s_threadInstances at all, since there's no possibility of a race because there are never two threads of control that might compete for a resource.

• On the other hand, if this assumption is wrong, then the current behavior of OnTabChanged is defective. OnTabChanged first checks to see if there's another instance of CPluginClass, and then does nothing if there is. Since s_threadInstances is used to implement GetTab(DWORD), that means that in this circumstance it may return a pointer to a different object. Putting a synchronization primitive around this defect does not fix it.

• GetTab(DWORD) is only ever called with GetCurrentThreadId() as its argument, and is also the only place that ever retrieves the mapped data (a pointer to CPluginClass). That means that s_threadInstances is only being used as an expensive way of implementing thread-local storage (TLS). Visual C++ has had an extension for TLS for a number of years now. C++11 also now has TLS. The Visual C++ extension doesn't support constructors/destructors as C++11 does, but we don't need that to store a pointer.

It would be better just to eliminate s_threadInstances and replace it with a TLS pointer. It can be assigned in the constructor and nulled out in the destructor.

As a separate fix, we should deal with the relationship between CPluginClass instances and threads. At the very least, we see a new thread in OnTabChanged, we should log that we're seeing behavior we don't expect.