Implementation of CPluginClass::GetAsyncBrowser() makes incorrect assumption about life cycle of site objects

[eric@adblockplus.org]

Background

CPluginClass::GetAsyncBrowser() is a read-wrapper around the class variable s_asyncWebBrowser2. s_asyncWebBrowser2 is written only in SetSite() when called with a fresh site. That site can become invalid when we receive a call to SetSite() with null argument. When the site present in s_asyncWebBrowser2 becomes invalid, that variable is not changed to reflect a valid site.

This behavior is the most likely cause of #1652, although we don't have direct evidence for that.

What to change

The existing behavior of s_asyncWebBrowser2 is to return the most recently seen site pointer. There's nothing particularly special about this particular site pointer. Perhaps it was thought to be the "most likely to still exist"; it's hard to tell what the original intention is.

The functions that call GetAsyncBrowser() are all UI-related functions that do not run with any particular instance of CPluginClass. Any valid browser site could be supplied to these functions.

We have alternate sources of currently-valid browser sites. The map s_threadInstances is a map from thread identifiers to current instances of CPluginClass with non-null site pointers. The set s_instances contains a set of such instances. One or both of these structure could replace s_asyncWebBrowser2, using the member variable CPluginClass::m_webBrowser2 that contains a site pointer.