IE crashes on disposing of s_asyncWebBrowser2

[sergz]

Environment

Win8.1
IE11
ABP built from 733

How to reproduce

1. open ​http://www.bing.com/
2. click to open outlook.com in new tab
3. Close IE
4. Confirm to close all tabs

Observed behaviour

It crashed

Expected behaviour

It should not crash, should gracefully shutdown.

[sergz]

callstack

[eric@adblockplus.org]

Looking at the call stack, the problem is that we're trying to call Release() on a COM pointer to an object that no longer exists. Apparently IE is already largely shut down by the time it unloads BHO instances.

The only code in the stack trace that's in our own source code is (1) the destructor for CPluginClass, which implicitly calls the default destructors for its most all of its members and (2) the declaration of static member variable s_asyncWebBrowser2. Arguably the defective class is CComPtrBase (a base class of CComQIPtr that we use here), because its implementation of Release does not have an exception handler, which it really ought to, since it's calling code in a foreign COM object that could have arbitrary behavior.

Added this issue as blocking #74, because the reported behavior is the very essence of not-graceful shutdown.

[sergz]

Replying to eric@…:

Looking at the call stack, the problem is that we're trying to call Release() on a COM pointer to an object that no longer exists.

Yes, but just in case I would note that it's not necessary that that non-existing already object is IWebBrowser2. See below, it can be some arbitrary object from already unloaded DLL.

Apparently IE is already largely shut down by the time it unloads BHO instances.

I would not say apparently because it's not obvious here thus the statement might be incorrect.

The only code in the stack trace that's in our own source code is (1) the destructor for CPluginClass, which implicitly calls the default destructors for its most all of its members and (2) the declaration of static member variable s_asyncWebBrowser2. Arguably the defective class is CComPtrBase (a base class of CComQIPtr that we use here), because its implementation of Release does not have an exception handler, which it really ought to, since it's calling code in a foreign COM object that could have arbitrary behavior.

There is no destructor for CPluginClass. s_asyncWebBrowser2 is a static member of CPluginClass and it's being destructed during the unloading of DLL, all BHOs are already destroyed.
So, I would say it's our bug, we should clean all browser related stuff at least when the related BHO instance (instance which implements IObjectWithSite, CPluginClass) is being unbound (SetSite(nullptr) is called).

It's not in the original issue report because of several things. The main one is that I'm not sure whether we really need static s_asyncWebBrowser2. BHO is instantiated several times within one DLL instance and we should not work with IWebBrowser2 which is for another instance of BHO. I thought that while addressing this moment we will solve the second reason, it actually requires to check when is the proper place to clean the stuff, may be we should do it in DISPID_ONQUIT handler or when SetSite(nullptr) is called or somewhen else.

What concerns defective class is CComPtrBase, I personally don't think that it's defective. Expecting any kind of failure from AddRef/Release would complicate the stuff so much. So let's don't complicate the things and continue to use ATL.

Added this issue as blocking #74, because the reported behavior is the very essence of not-graceful shutdown.

But #74 is about graceful shutdown of the engine while this issue is about shutting down of the addon. I would not bind them.