Correctly initialize entries in the map from threads to tabs

[eric@adblockplus.org]

Background

The variable s_threadInstances maps thread identifiers to instances of CPluginClass. Its entries are currently inserted in the event handler OnWindowsStateChanged(). Inserting here is defective, since the thread that calls the event handler is not always the same as the governing thread for CPluginClass, which is the one that calls SetSite().

What to change

Change where thread entries are inserted from OnWindowStateChanged() to SetSite().

[eric@adblockplus.org]

The other aspects of the life cycle of s_threadInstances are not at issue here. As a standard container, it can be initialized statically in a DLL without a problem. It needs to be empty upon initialization, so its default constructor is adequate.

[sergz]

One can find the beginning of the discussion in codereview ​https://codereview.adblockplus.org/29330403. It would be better to discuss such conceptual things in the issue.

The argument that current implementation is defective makes no sense if Oleksandr was able to reproduce the case when SetSite and Invoke (OnWindowStateChanged) are called from different threads.

We should take into account the fact that our BHO threading model is marked as "BOTH" and the tip #6 ​https://msdn.microsoft.com/en-us/library/aa753617(v=vs.85).aspx#tip6. So, we should expect our methods to be called from different threads.
I have created the issue #3606 to change the threading model, so the proposed change should work, and in context of which we can firstly discuss whether it makes sense for us or not.

I have tested a bit the current version and taken a closer look at the code. It seems indeed already runs in a single-threaded apartment because at the beginning of SetSite we call CoInitialize which "Initializes the COM library on the current thread and identifies the concurrency model as single-thread apartment (STA)", so it seems the threading model is indeed already STA. I personally have not seen the error in the log-file related to CoInitialize, however I look there very rarely. I would propose to put there in addition an assert to be notified during the development if it's not the case. To test I have also stored the thread ID as a member of CPluginClass and I have not discovered when current thread ID is different from the stored one in OnWindowStateChanged and SetSite(nullptr). It can be also a good thing for debug build to store the thread ID as a member and check with asserts that it's still the same in each methods called by IE.

On the other hand CoInitialize is from the beginning of the repository, so, if the trouble was here then regardless of STA, we cannot expect that IE obeys the apartment threading model rules.

I have also found when OnWindowStateChanged had been added (see ​https://github.com/adblockplus/new-adblockplusie/commit/b3f5dd07fc61b8cfa6743216e694681e7321caeb). According to the commit message I would expect the issue with opening of a group of tabs, however I could not reproduce it, though I have not tested it exhaustively (I have tried merely on IE8-32bit-Vista, IE11-32bit-Win10).

BTW, there is another interesting commit ​https://github.com/adblockplus/new-adblockplusie/commit/911a189513f677134c55f362d6118da73af77ee3 which removes a lot of code related to some MainThread and MainUIThread.

Oleksandr can you still reproduce it, could you provide the steps to do it and more details?

[eric@adblockplus.org]

The whole argument about CoInitialize is basically irrelevant to the present issue. I consider it a defect in our code that we don't call CoInitializeEx with the "multithreaded" policy. Our code already assumes multithreading, we might as well tell COM we support it.

But how we declare our own threading policy doesn't affect how IE actually calls the code. However IE is implemented, it always calls SetSite, both zero and non-zero arguments, on the same thread. I have experimentally verified this; the log tracing I did this with has been committed.

[eric@adblockplus.org]

It's exactly because OnWindowStateChanged can be called from another thread that makes adding entries there incorrect. We are removing entries in SetSite, and when we add them in another thread, they don't get removed. That's a memory leak.

And it's not just a memory leak. It also generates incorrect behavior. The one important place we're using this map is in our APP protocol handler, where it uses the thread to locate the BHO instance. The thread that this call comes in on is the same as the one that calls SetSite. If the thread map is initialized in some other thread, it will incorrectly not locate the BHO instance. If this were not the case, the thread map would be completely useless and we should just stop using it there and find some other way of linking that request to a BHO instance.

we cannot expect that IE obeys the apartment threading model rules.

We can observe, however, that IE does indeed call certain functions all from within the same thread. It doesn't matter whether IE is internally using STA for these calls or not. The observed behavior is what matters. The observed behavior makes the existing code defective and the present issue the right way to fix it.

[oleksandr]

I *think* have reproduced it before just by randomly browsing. I can not reproduce it now after some rigorous tests, so I am not sure at this point if OnWindowStateChanged can indeed be called from another thread. However, as Eric mentioned, it really doesn't change much, since we still have a possible leak in the code if we store thread id in OnWindowStateChanged I think it is safe enough to move that to SetSite.