Issue 5168444685156352: Issue 1201 - Enabling/disabling ABP everywhere applies only in the next opened tab (Closed)
DescriptionI have added a comment in a line that actually fixes the issue. The other stuff is just derivative from that.
Patch Set 1 #
Total comments: 11
Patch Set 2 : Only fix the issue, no refactoring #MessagesTotal messages: 7
http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:657: CPluginClientFactory::ReleaseMimeFilterClientInstance(); This fixes the issue. Just calling Unregister() here is not releasing m_spCFHTTP and m_spCFHTTPS in CPluginMimeFilterClient, which apparently means our Asynchronous Pluggable Protocols remain invoked. This renders s_mimeFilter irrelevant, so removing it.
I don't think the initial patch set works, but it's headed in the right direction. There's a comment I committed to the code in 'CPluginClass::SetSite' that calling 'ReleaseMimeFilterClientInstance' is something that ought to be done through a 'shared_ptr'. The way I'd fix this problem is to handle the life cycle of the mime filter _only_ through a 'shared_ptr' member of 'CPluginClass'. It's sufficient to release the pointer to the mime filter in the destructor for 'CPluginClass'. There's no need to do it explicitly in 'SetSite(0)'. When all the 'CPluginClass' instances disappear, then the mime-filter instance will be destroyed, all without any need for explicit control statements sprinkled throughout the code in the places where it might need to be destroyed. All this is to say that I consider the existence of 'ReleaseMimeFilterClientInstance' a design defect. We have a number of these initializations issues, and I've been working on them recently. The solution that seems correct involves getting rid of 'CPluginClientFactory' entirely. Among other things, it means that we'll need no code in 'DllCanUnloadNow'; all the work will already be done by then. In order for this to work, though, the life cycle of 'CPluginClass' has to work right. It seems that IE is not destroying 'CPluginClass' all the time, but that seems only to happen when terminating. Assuming that's the case, it doesn't matter that it's not releasing the mime-filter. I can do this code, if you like, or I can advise on doing it here. http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... File src/plugin/Plugin.cpp (right): http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... src/plugin/Plugin.cpp:82: if (_Module.GetLockCount() == 0) If I've read the ATL code right, this condition means that we have no outstanding instance produced from ATL-derived factories. http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... src/plugin/Plugin.cpp:89: CPluginClientFactory::ReleaseMimeFilterClientInstance(); I've seen a crash recently during shutdown when 's_mimeFilter' was being referenced after COM had already destroyed the instance. I'm guessing that any reference to COM objects here is defective. That includes a reference to 's_mimeFilterInstance' through 'ReleaseMimeFilterClientInstance'. The problem is that even though 's_mimeFilterInstance' is not 'nullptr', its value has been invalidated by COM. We need to manage the life cycle of the mime-filter correctly, but I don't think this is the way to do it. http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (left): http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:514: //Register a mime filter if it's not registered yet I'm assuming there was a reason to have this statement in the code before. If so, removing the call the 'GetMimeFilterClientInstance' is defective. We would need a replacement, something like 'EnsureMimeFilterRegistered'. http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... File src/plugin/PluginClientFactory.cpp (right): http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... src/plugin/PluginClientFactory.cpp:37: //if the constructor throws we do not want to alter instance I don't think I had looked at this comment before, but if the constructor throws here, you want to catch it and, at the very least, return 'nullptr'. And then disable ABP for the scope of a null return. This isn't the problem you're trying to address, I understand, but this illustrates how screwed up the mime-filter life cycle code is. http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... src/plugin/PluginClientFactory.cpp:56: if (s_mimeFilterInstance) See comment in Plugin.cpp. http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... File src/plugin/PluginMimeFilterClient.cpp (left): http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... src/plugin/PluginMimeFilterClient.cpp:95: void CPluginMimeFilterClient::Unregister() I'll say I do agree with removing any kind of separate function like Unregister(). Unregistering should be managed only through the destructor, which means better managing the life cycle of 'CPluginMimeFilterClient'.
One more point. The code for ID_MENU_DISABLE in 'DisplayPluginMenu' illustrates the problem. It's explicitly creating and destroying the mime-filter instance. This kind of interface means that everybody else has to manage it explicitly. We have two concerns here: (1) maintaining a reference from CPluginClass to the manager of a mime-filter instance (whether it exists or not) and (2) ensuring the enable/disable state of the plugin is consistent with the existence of the mime-filter instance. The existing design flaw is that there's not a separate class to act as this manager, which means we have code sprinkled throughout to substitute for it, a pattern that's not working all that well.
In general it should work, however I would say that the it seems that the fix could be merely in CPluginClass::OnBeforeNavigate2 (see comment). As well as, if we follow the proposed approach we could avoid additional registering of the factories in SetSite if CPluginClass is not the first time intantiated in the DLL instance. What concerns the design of the lifecycle of the stuff, Eric, could you please create the issue where we can discuss it? http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (left): http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:514: //Register a mime filter if it's not registered yet On 2015/03/27 13:35:50, Eric wrote: > I'm assuming there was a reason to have this statement in the code before. If > so, removing the call the 'GetMimeFilterClientInstance' is defective. We would > need a replacement, something like 'EnsureMimeFilterRegistered'. It was indeed the issue here. We disable the plugin and deregister the factories, but on the next request (JIC, we are inside CPluginClass::OnBeforeNavigate2) we register them again, so it's removed for good. http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:657: CPluginClientFactory::ReleaseMimeFilterClientInstance(); On 2015/03/27 08:02:29, Oleksandr wrote: > This fixes the issue. Just calling Unregister() here is not releasing m_spCFHTTP > and m_spCFHTTPS in CPluginMimeFilterClient, which apparently means our > Asynchronous Pluggable Protocols remain invoked. This renders s_mimeFilter > irrelevant, so removing it. No, it does not mean that our APP handlers remain invoked. If we unregistered them they are not called anymore, which is actually expected. According to the debugger, the reference counter of the factories is equal to 1, so they are not referenced by something else. http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... File src/plugin/PluginClientFactory.cpp (right): http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... src/plugin/PluginClientFactory.cpp:28: CPluginMimeFilterClient* CPluginClientFactory ::GetMimeFilterClientInstance() Right now we don't need this return value anymore, right? However if we decide to implement suggested Enable()/Disable(), it will be needed, what do you think about returning a reference instead of a pointer? http://codereview.adblockplus.org/5168444685156352/diff/5629499534213120/src/... src/plugin/PluginClientFactory.cpp:37: //if the constructor throws we do not want to alter instance On 2015/03/27 13:35:50, Eric wrote: > I don't think I had looked at this comment before, but if the constructor throws > here, you want to catch it and, at the very least, return 'nullptr'. And then > disable ABP for the scope of a null return. > > This isn't the problem you're trying to address, I understand, but this > illustrates how screwed up the mime-filter life cycle code is. Firstly, when CPluginMimeFilterClient throws the exception: - the assignment localInstance = new CPluginMimeFilterClient(); is not executed, so we can safely use `s_mimeFilterInstance = new CPluginMimeFilterClient();` and get rid of `localInstance` because the variable being assigned to will not be altered. - anyway, I would say compiler removes `localInstance` as optimization What concerns the handling of the exception, if we cannot install our protocol handlers we still can block by CSS, although it's not a full plugnin functionality. If some windows API call fails in CPluginMimeFilterClient then we could set some "global" flag in the body of CPluginMimeFilterClient methods to indicate to the user that the plugin cannot fully operate and don't throw any exception. If the exception is thrown not by us, then it should be processed by entry point wrapper, now this class has clearly visible single responsibility and I don't think we should introduce here additional logic of disabling of ABP.
I have ran more tests and I agree with Sergei that merely removing the code in BeforeNavigate2 would be enough. Since that fixes the issue I am just submitting it and moving refactoring to another codereview here: http://codereview.adblockplus.org/5032147387678720/
LGTM
LGTM I would suggest, however, changing the description of #1201 to "Disabling ABP everywhere ..." and dropping the "Enabling/" part. Enabling is not actually mentioned in the symptom descriptions, nor is it affected by the present change set. 
|
