Opened 4 years ago

Last modified 4 years ago

#2142 reviewing change

Synchronize access to s_threadInstances in CPluginClass::OnWindowStateChanged

Reported by: sergz Assignee: oleksandr
Priority: P4 Milestone:
Module: Adblock-Plus-for-Internet-Explorer Keywords: goodfirstbug
Cc: sergz Blocked By:
Blocking: Platform: Internet Explorer
Ready: no Confidential: no
Tester: Verified working:
Review URL(s):

http://codereview.adblockplus.org/5189151393579008/

Description

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.

Change History (4)

comment:1 Changed 4 years ago by oleksandr

  • Cc sergz added
  • Keywords goodfirstbug added
  • Platform changed from Unknown to Internet Explorer
  • Priority changed from Unknown to P4

You probably mean OnTabChanged, not OnWindowStateChanged?

comment:2 Changed 4 years ago by oleksandr

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:3 Changed 4 years ago by oleksandr

  • Owner set to oleksandr

comment:4 Changed 4 years ago by eric@…

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.

Last edited 4 years ago by eric@… (previous) (diff)
Note: See TracTickets for help on using tickets.