Issue 4805561958793216: Noissue - Remove dead code in CPluginClass::SetSite (Closed)
DescriptionNoissue - Remove dead code in CPluginClass::SetSite
Remove legacy code in CPluginClass::SetSite for when it was "loaded as a
toolbar handle". This branch cannot be activated, since CPluginClass is never
registered as anything but a BHO. Removed now-unused argument for InitObject.
Added a 'try' statement around the body of 'SetSite', since it's an entry
point. Removed an unused statement and a macro with empty expansion at the
beginning. Removed header "abp.h", since all that was in it was the empty
macro and it did not appear elsewhere.
Patch Set 1 #
Total comments: 13
Patch Set 2 : rebased + removed more dead code #
Total comments: 1
MessagesTotal messages: 25
The initial patch set conflicts with http://codereview.adblockplus.org/4937147073167360/ Issue 1672 - Subscribe and listen COM events using ATL::IDispEventImpl and BEGIN_SINK_MAP. There's no conceptual overlap, but it won't merge cleanly. If that review completes earlier, I'm prepared. One of the changes here is to add a try-statement around the body of 'SetSite', since it's an entry point. That change could be incorporated into the above-mentioned review, or done separately. The hardest part of this change set was the research. As it turns out, 'CPluginClass::SetSite' can't ever be called "as a toolbar handler", so the associated code for it is useless. This is evident enough from trace entries in the debug log, where the relevant entry does not appear. Digging deeper, 'SetSite' would only be called in this other context if it were registered as some other kind of browser extension or ActiveX control, and it's not, neither in the installer nor in the .RGS file (self-registration). There's an important defect I found while working with this piece of code. We're calling 'Release' on the site pointer, but never calling 'AddRef' as we're supposed to be doing. See https://msdn.microsoft.com/en-us/library/aa768219%28v=vs.85%29.aspx for details. We're using an ATL implementation base class for 'IOleCommandTarget', the interface that contains 'SetSite'; that implementation doesn't call 'AddRef' either. It also doesn't call 'Release' either, so at least it's consistent, if non-conforming, as opposed to what we've got, which is both inconsistent and non-conforming.
I have not checked he rest yet but want to comment the following On 2015/03/06 11:28:36, Eric wrote: > There's an important defect I found while working with this piece of code. We're > calling 'Release' on the site pointer, but never calling 'AddRef' as we're > supposed to be doing. See > https://msdn.microsoft.com/en-us/library/aa768219%2528v=vs.85%2529.aspx for details. > We're using an ATL implementation base class for 'IOleCommandTarget', the > interface that contains 'SetSite'; that implementation doesn't call 'AddRef' > either. It also doesn't call 'Release' either, so at least it's consistent, if > non-conforming, as opposed to what we've got, which is both inconsistent and > non-conforming. That is incorrect. Could you point to the place where we call `Release` on the site pointer and don't call `AddRef`? There is nothing with `IOleCommandTarget`. We use `ATL::IObjectWithSiteImpl`, which implements `IObjectWithSite` interface which contains `SetSite`. `ATL::IObjectWithSiteImpl` stores the site in `m_spUnkSite` which is `ATL::CComPtr<IUnknown>`, so these `AddRef` and `Release` are properly called.
On 2015/03/06 11:28:36, Eric wrote: > We're using an ATL implementation base class for 'IOleCommandTarget', the > interface that contains 'SetSite'; I misspoke here. I should have said 'IObjectWithSite'. I've been reading too many interface definitions the last couple of days. The MSDN link is correct and points to the right interface. > Could you point to the place where we call `Release` on the site pointer and > don't call `AddRef`? Sure. I've added some inline code comments. There are two locations. The defect triggers a fault when the class member m_webBrowser2 is destroyed or assigned null. At that point 'Release' is going to be called again with no previous call to 'AddRef'. > `ATL::IObjectWithSiteImpl` stores the site in `m_spUnkSite` which is > `ATL::CComPtr<IUnknown>`, so these `AddRef` and `Release` are properly called. You're right. I didn't check the definition of that member variable, instead assuming incorrectly that it was a raw pointer. This point, however, doesn't affect the flaws in our own code.
http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:131: m_webBrowser2.Release(); Explicit release. http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:263: m_webBrowser2 = unknownSite; Assigned to m_webBrowser2 here, declared as CComQIPtr<IWebBrowser2> http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:364: m_webBrowser2.Release(); Explicit release here.
http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:263: m_webBrowser2 = unknownSite; On 2015/03/06 15:58:04, Eric wrote: > Assigned to m_webBrowser2 here, declared as CComQIPtr<IWebBrowser2> Here `AddRef` is called. http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:364: m_webBrowser2.Release(); On 2015/03/06 15:58:04, Eric wrote: > Explicit release here. So, since `AddRef` is called and `m_webBrowser2` is a smart pointer, there is no inconsistency. After `m_webBrowser2.Release();` the following calls `m_webBrowser2.Release();` are safe, they do nothing.
It might be a good idea to eliminate 'm_webBrowser2' entirely, since it's only a copy of 'm_spUnkSite' in the base class. It's only used internally to 'CPluginClass', and there only to implement 'GetBrowser'. http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:364: m_webBrowser2.Release(); On 2015/03/06 16:07:19, sergei wrote: > So, since `AddRef` is called and `m_webBrowser2` is a smart pointer, there is no > inconsistency. After `m_webBrowser2.Release();` the following calls > `m_webBrowser2.Release();` are safe, they do nothing. Calling 'Release' through 'CComPtr' apparently also zeros out the pointer as well, contrary to all good design sense. I had to read the source code to see this; it's certainly not an obvious behavior. I am coming to despise ATL even more than I did before, which is quite saying something.
On 2015/03/06 16:27:22, Eric wrote: > Calling 'Release' through 'CComPtr' apparently also zeros out the pointer as > well, contrary to all good design sense. I had to read the source code to see > this; it's certainly not an obvious behavior. > > I am coming to despise ATL even more than I did before, which is quite saying > something. It's a spirit of ATL, they have some foolstable stubs and provide te implementation of a lot of common stuff.
On 2015/03/06 16:41:28, sergei wrote: > It's a spirit of ATL, they have some foolstable stubs and provide te > implementation of a lot of common stuff. I'm not arguing against that spirit; it's a good principle. The reason I despise it all has to do with the implementation and with design details, which are full of nonsense everywhere I turn. For example, MS never bothered to update ATL to a version of the compiler that did type inference for template functions. The only reason for 'CComQIPtr' is to work around that defect.
On 2015/03/06 16:53:27, Eric wrote: > For example, MS never bothered to update ATL to a version of the compiler that > did type inference for template functions. The only reason for 'CComQIPtr' is to > work around that defect. Could you explain the trouble here? ATL is being constantly updated with the compiler.
LGTM but checkout the comments. http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:244: * but it is not a proper substitute for one. I'm not sure that we need this comment except the case when SetSite is not called. Are you sure that 'SetSite(nullptr)' not called if the shutting down is graceful, thus it's not a case of the crash or something like this? Would be great if you can provide a link to the documentation. http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:283: if (GetBrowser()) We don't need this comment now.
On 2015/03/09 10:12:47, sergei wrote: > Could you explain the trouble here? ATL is being constantly updated with the > compiler. There's no specific problem with CComQIPtr. It isn't necessary, however. It could have been replaced with a template constructor with argument inference. ATL is absolutely not being "constantly updated". The most recent version of ATL shipped with VS 2005, almost a decade ago. To my knowledge, it's only had two minor changes since, one to fix a security problem (2009) and one to use static-only linking (VS 2013). Had ATL continued actual development, they could have added a template constructor at any time over the last decade.
http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:244: * but it is not a proper substitute for one. On 2015/03/09 10:23:50, sergei wrote: > I'm not sure that we need this comment except the case when SetSite is not > called. I was just rewriting the comment, mostly, with the one change you picked up on; see below. Given that the lifecycle of IE extensions is not well documented, having some kind of comment here is certainly warranted. > Are you sure that 'SetSite(nullptr)' not called if the shutting down is > graceful, thus it's not a case of the crash or something like this? Would be > great if you can provide a link to the documentation. Yes, mostly. I verified it last week. I haven't seen anything about this topic in any MS documentation. Of particular interest, I haven't seen anything about a requirement to call 'SetSite(nullptr)' before destroying a control instance. It seems best to assume that it need not be. I wrote some more detailed trace messages for this function that included values of 'this' and the site pointer. Most of them were in matched pairs, but not all. I was very reliably getting unmatched pairs. There was no obvious crashing behavior, although I can't be completely sure there wasn't any internally; this is why I said "mostly" above. The consistency in unmatched pairs seems to be something related to never having loaded a page or interrupting a page load by shutting down IE; I don't have anything definitive there. I'm going to be looking into this issue further. http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:283: if (GetBrowser()) On 2015/03/09 10:23:50, sergei wrote: > We don't need this comment now. We don't need this particular comment, but we'll need another one in the next patch set. We still need to check that the QI call on site pointer to 'IWebBrowser2' is not null. If it's not, we likely have some severe IE defect, but regardless we need to handle that case gracefully.
http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:244: * but it is not a proper substitute for one. I am pretty sure it _is_ called every time when IE is shutting down. It's that sometimes the IE shutdown from the end user perspective doesn't really shutdown the IE and the process keeps hanging. In that case it doesn't call SetSite(nullptr). Do you have any test cases when it is not called? Also, I think this comment merely reflects the documentation. I'd vote for removing it or at least making it much shorter
Sorry, I have published my old comments before refreshing the codereview. Turns out Sergei and I were concerned about the same :) http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:244: * but it is not a proper substitute for one. > I haven't seen anything about this topic in any MS documentation. Of particular interest, > I haven't seen anything about a > requirement to call 'SetSite(nullptr)' before destroying a control instance. It > seems best to assume that it need not be. https://msdn.microsoft.com/en-us/library/bb250489(v=vs.85).aspx#basics > I wrote some more detailed trace messages for this function that included values > of 'this' and the site pointer. Most of them were in matched pairs, but not all. > I was very reliably getting unmatched pairs. There was no obvious crashing > behavior, although I can't be completely sure there wasn't any internally; this > is why I said "mostly" above. > > The consistency in unmatched pairs seems to be something related to never having > loaded a page or interrupting a page load by shutting down IE; I don't have > anything definitive there. I'm going to be looking into this issue further. Could it have been the fact that the IE process did not exit?
On 2015/03/10 08:42:04, Oleksandr wrote: > https://msdn.microsoft.com/en-us/library/bb250489%28v=vs.85%29.aspx#basics I'll stipulate that this was the behavior of IE in 2006. This link is not normative documentation, though; it's a project example. It's not the sort of thing that MS keeps up to date as their own code changes. > Could it have been the fact that the IE process did not exit? I don't think so, although I didn't explicitly check. I'm on Windows 7, and I don't have anything installed that would keep it running in the background. There are several things that might be happening. I'll be doing some more research to into it shortly. None of this work affects the present change set. I am _very_ confident that we're only "loading as BHO". Possibilities: 1) We're not logging because our logging facility is shutting down early. This isn't the case, since the DEBUG_* call is one of the immediate ones; it doesn't go through a queue. 2) There's a race condition and the log entry isn't getting flushed to disk. This is extremely unlikely, given that it was easy to replicate, not an occasional occurrence. 3) The IE code cleans up BHO ordinarily, but the thread is being killed pre-emptively from elsewhere as part of shutdown. 4) The IE code has short-circuits for clean up under termination that don't bother to call 'SetSite(nullptr)'. This seems possible, but I don't give it high likelihood. 5) IE is running under multiple processes, and some processes are getting closed pre-emptively by others. This is much the same as (3) in effect. I don't think this is happening, as I've always seen all the missing calls in the same thread. 6) All the threads excepts the last thread standing complete the ordinary shutdown procedure, but as soon as one thread sees it's the only thread left, it just immediately exits, terminating the process, without further delay. Both (3) and (6) match the behavior I'm seeing adequately.
I forgot one. (7) It only fails to call 'SetSite(nullptr)' when IE is shut down while the tab is loading.
On 2015/03/09 12:26:16, Eric wrote: > On 2015/03/09 10:12:47, sergei wrote: > > Could you explain the trouble here? ATL is being constantly updated with the > > compiler. > > There's no specific problem with CComQIPtr. It isn't necessary, however. It > could have been replaced with a template constructor with argument inference. If you are talking that CComPtr could have constructor and assign operators which perform QueryInterface and in this case there no any necessity of CComQIPtr then, I guess, there is some conceptual design reasons to have it. Also take a look, CComQIPtr inherits CComPtr, so it's merely semantic feature. > ATL is absolutely not being "constantly updated". The most recent version of ATL > shipped with VS 2005, almost a decade ago. To my knowledge, it's only had two > minor changes since, one to fix a security problem (2009) and one to use > static-only linking (VS 2013). ATL is updated with each visual studio release, try to install different versions of VS and compare.
LGTM
New patch set. Need another round of review. Big-looking modifications from rebasing, but the essence of the change set hasn't changed. InitObject no longer needs an argument, since we never load as anything other than a BHO. http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:283: if (GetBrowser()) On 2015/03/09 13:31:29, Eric wrote: > On 2015/03/09 10:23:50, sergei wrote: > > We don't need this comment now. > > We don't need this particular comment, but we'll need another one in the next > patch set. We still need to check that the QI call on site pointer to > 'IWebBrowser2' is not null. If it's not, we likely have some severe IE defect, > but regardless we need to handle that case gracefully. > Done.
So what has been done here is: 1. Removed a parameter from InitObject 2. Remove a comment 3. Removed abp.h LGTM if that's it. On 2015/03/17 10:41:07, Eric wrote: > New patch set. Need another round of review. > > Big-looking modifications from rebasing, but the essence of the change set > hasn't changed. > > InitObject no longer needs an argument, since we never load as anything other > than a BHO. > > http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... > File src/plugin/PluginClass.cpp (right): > > http://codereview.adblockplus.org/4805561958793216/diff/5629499534213120/src/... > src/plugin/PluginClass.cpp:283: if (GetBrowser()) > On 2015/03/09 13:31:29, Eric wrote: > > On 2015/03/09 10:23:50, sergei wrote: > > > We don't need this comment now. > > > > We don't need this particular comment, but we'll need another one in the next > > patch set. We still need to check that the QI call on site pointer to > > 'IWebBrowser2' is not null. If it's not, we likely have some severe IE defect, > > but regardless we need to handle that case gracefully. > > > > Done.
On 2015/03/17 17:45:53, Oleksandr wrote: > So what has been done here is: > 1. Removed a parameter from InitObject > 2. Remove a comment > 3. Removed abp.h That's right; you've listed everything that wasn't part of the rebase.
LGTM, but comment is still here.
On 2015/03/30 10:54:35, sergei wrote: > but comment is still here. Which comment are you referring to?
http://codereview.adblockplus.org/4805561958793216/diff/5653164804014080/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/4805561958793216/diff/5653164804014080/src/... src/plugin/PluginClass.cpp:221: */ This comment
I've pushed with the comment in place. I've been working on these life cycle issues generally, and the code herein is due to change again soon. When that happens so will the comment. We'll need much more comprehensive documentation on life cycle, probably not in code comments but in a doxygen page. The content of this comment will move there at that point. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
