Issue 5447868882092032: Issue 1793 - check whether the frame is whitelisted before injecting CSS (Closed)
Description# depends on #1794 [done] http://codereview.adblockplus.org/4806567450902528/
Patch Set 1 : update #
Total comments: 31
Patch Set 2 : rebase and address comments #
Total comments: 6
Patch Set 3 : fix according to comments #Patch Set 4 : rebase #
Total comments: 2
Patch Set 5 : add link #
Total comments: 14
Patch Set 6 : address comments #
Total comments: 7
Patch Set 7 : reduce the amount of code #
Total comments: 7
Patch Set 8 : use SID_SWebBrowserApp as service ID #Patch Set 9 : rebase and rename webBrowser to parentBrowser #
MessagesTotal messages: 30
http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/engine/Main.cpp:34: bool isWhitelisted = false; I dislike identifiers that differ only in capitalization. In any case, it's not necessary to declare this variable here to use as a return variable. It's actually not necessary at all, even later. http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/engine/Main.cpp:37: isWhitelisted = IsWhitelisted(urlArg, ""); We can simply return here and skip the subsequent 'else' http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/engine/Main.cpp:41: auto frame_ii = frameHierarchy.begin(); As much as I prefer underscores, ABP doesn't use them in identifiers. http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/engine/Main.cpp:44: while (!isWhitelisted && frame_ii != frameHierarchy.end()) We only need the second term here. http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/engine/Main.cpp:47: isWhitelisted = IsWhitelisted(url, parentUrl); if (IsWhitelisted(...)) return true; http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/engine/Main.cpp:50: } Just return false if the loop terminates having reached the end. http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... File src/plugin/PluginTabBase.cpp (right): http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:162: void InjectABPCSS(IHTMLDocument2& htmlDocument2, const std::vector<std::wstring>& hideFilters) We're not distinguishing between fatal errors and simple non-support here. If IE 9 is required, then simply check for it at the start and be done with it. Then if the QI calls fail (for example, when converting from IHTMLStyleSheet to IHTMLStyleSheet4) it can be properly designated as a fatal error by throwing an exception. I don't see anything that should fail here, assuming version prerequisites are met. Thus the 'void' return type is appropriate. What's not correct, though, is for failures to return rather than to throw. Alternatively, and it's not one I like, is to declare the function 'bool' and return success/failure. Since this is brand new code, there's no good reason not to do it right and move forward (void/throw) rather than remain mired in the past (bool/return). Overall, this function would be far easier to read with two modifications. (1) Declare all the [out] argument variables immediately before use. (2) Add a blank line separating each individual operation and its error handing. http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:260: ATL::CComPtr<IWebBrowser2> GetParent(const ATL::CComPtr<IWebBrowser2>& browser) Since you're not copying the CComPtr and incurring an Add()/Release() pair, you can just pass this argument as "IWebBrowser2*". http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:266: return retValue; "return nullptr" is much clearer here. http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:271: return retValue; As before "return nullptr" http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:276: return retValue; return nullptr http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:277: } This is the right place to declare 'retValue'. As a rule, it's best to declare variables that receive [out] arguments immediately before they're used, if only to ensure that operator& doesn't fail. http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:280: // error You've got to do something here, since a failed COM call leaves an [out] argument in an undefined state. Best would be to throw an exception. http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:294: return false; This is really a pretty serious error. Personally, I'd prefer that GetInstance() throw in case of error (and thus never return nullptr), but that's perhaps too drastic a change for the moment. At least throw here.
Not sure why I was not a reviewer here. Adding myself and my 2 cents below anyway :) http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... File src/plugin/PluginTabBase.cpp (right): http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:162: void InjectABPCSS(IHTMLDocument2& htmlDocument2, const std::vector<std::wstring>& hideFilters) On 2015/01/13 19:52:52, Eric wrote: What's not correct, though, is > for failures to return rather than to throw. I guess this code made into this codereview by accident, since InjectABPCSS is not actually used anywhere. But still I disagree with the void/throw. In this case in an unlikely scenario of anything going wrong it's better to just ignore the error and keep doing what's next in the code rather than dropping everything and unwinding the stack. This method should not throw, IMO. http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:278: if (FAILED(parentDocumentServiceProvider->QueryService(IID_IWebBrowserApp, &retValue)) || !retValue) Why is this querying for IWebBrowserApp and not IWebBrowser2? Output expects IWebBrowser2. http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:280: // error On 2015/01/13 19:52:52, Eric wrote: > You've got to do something here, since a failed COM call leaves an [out] > argument in an undefined state. > > Best would be to throw an exception. return nullptr would be better IMO
http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/engine/Main.cpp:34: bool isWhitelisted = false; On 2015/01/13 19:52:52, Eric wrote: > I dislike identifiers that differ only in capitalization. > > In any case, it's not necessary to declare this variable here to use as a return > variable. It's actually not necessary at all, even later. fixed http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/engine/Main.cpp:37: isWhitelisted = IsWhitelisted(urlArg, ""); On 2015/01/13 19:52:52, Eric wrote: > We can simply return here and skip the subsequent 'else' fixed http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/engine/Main.cpp:41: auto frame_ii = frameHierarchy.begin(); On 2015/01/13 19:52:52, Eric wrote: > As much as I prefer underscores, ABP doesn't use them in identifiers. fixed http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/engine/Main.cpp:44: while (!isWhitelisted && frame_ii != frameHierarchy.end()) On 2015/01/13 19:52:52, Eric wrote: > We only need the second term here. fixed http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/engine/Main.cpp:47: isWhitelisted = IsWhitelisted(url, parentUrl); On 2015/01/13 19:52:52, Eric wrote: > if (IsWhitelisted(...)) return true; fixed http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/engine/Main.cpp:50: } On 2015/01/13 19:52:52, Eric wrote: > Just return false if the loop terminates having reached the end. fixed http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... File src/plugin/PluginTabBase.cpp (right): http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:162: void InjectABPCSS(IHTMLDocument2& htmlDocument2, const std::vector<std::wstring>& hideFilters) On 2015/03/13 10:00:27, Oleksandr wrote: > On 2015/01/13 19:52:52, Eric wrote: > What's not correct, though, is > > for failures to return rather than to throw. > I guess this code made into this codereview by accident, since InjectABPCSS is > not actually used anywhere. But still I disagree with the void/throw. In this > case in an unlikely scenario of anything going wrong it's better to just ignore > the error and keep doing what's next in the code rather than dropping everything > and unwinding the stack. This method should not throw, IMO. I agree this function should not be in this code review. I also think that throwing would hurt more than help here, so far let's discuss it in http://codereview.adblockplus.org/6567422169448448/. http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:260: ATL::CComPtr<IWebBrowser2> GetParent(const ATL::CComPtr<IWebBrowser2>& browser) On 2015/01/13 19:52:52, Eric wrote: > Since you're not copying the CComPtr and incurring an Add()/Release() pair, you > can just pass this argument as "IWebBrowser2*". fixed to use `IWebBrowser2&`. http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:266: return retValue; On 2015/01/13 19:52:52, Eric wrote: > "return nullptr" is much clearer here. fixed http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:271: return retValue; On 2015/01/13 19:52:52, Eric wrote: > As before "return nullptr" fixed http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:276: return retValue; On 2015/01/13 19:52:52, Eric wrote: > return nullptr fixed http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:277: } On 2015/01/13 19:52:52, Eric wrote: > This is the right place to declare 'retValue'. > > As a rule, it's best to declare variables that receive [out] arguments > immediately before they're used, if only to ensure that operator& doesn't fail. fixed http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:278: if (FAILED(parentDocumentServiceProvider->QueryService(IID_IWebBrowserApp, &retValue)) || !retValue) On 2015/03/13 10:00:27, Oleksandr wrote: > Why is this querying for IWebBrowserApp and not IWebBrowser2? Output expects > IWebBrowser2. fixed http://codereview.adblockplus.org/5447868882092032/diff/5685265389584384/src/... src/plugin/PluginTabBase.cpp:280: // error On 2015/01/13 19:52:52, Eric wrote: > You've got to do something here, since a failed COM call leaves an [out] > argument in an undefined state. > > Best would be to throw an exception. I don't think we should expect undefined state of out-parameter, it will complicate the things to unusable state, check the rules of COM world https://msdn.microsoft.com/en-us/library/windows/desktop/ms686638(v=vs.85).aspx.
Just a few issues with style. http://codereview.adblockplus.org/5447868882092032/diff/5634387206995968/src/... File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5447868882092032/diff/5634387206995968/src/... src/engine/Main.cpp:46: auto GetWhitelistingFilter = [&type](const std::string& url, const std::string& parent)->std::string Code style comment. It's not a good idea to use the exact same name as the function for a local variable, even if it is substituting for a local function. I can immediately see two fixes possible: 1. Rename it to reflect the fact that it's not working with frames. 2. Extract it as its own function, with the same name and its distinct signature. Or do both. http://codereview.adblockplus.org/5447868882092032/diff/5634387206995968/src/... src/engine/Main.cpp:54: }; The whole loop below would be clearer if implemented with 'std::find_if', but I'm not sure if the overhead is worth it. You'd have to memoize the result of "filterEngine->Matches()" above to avoid a redundant call, for example. http://codereview.adblockplus.org/5447868882092032/diff/5634387206995968/src/... File src/plugin/PluginTabBase.cpp (right): http://codereview.adblockplus.org/5447868882092032/diff/5634387206995968/src/... src/plugin/PluginTabBase.cpp:184: ATL::CComPtr<IDispatch> browserParentDispatch; I'd call this 'parentDispatch' here, in analogy with the other parent below.
http://codereview.adblockplus.org/5447868882092032/diff/5634387206995968/src/... File src/engine/Main.cpp (right): http://codereview.adblockplus.org/5447868882092032/diff/5634387206995968/src/... src/engine/Main.cpp:46: auto GetWhitelistingFilter = [&type](const std::string& url, const std::string& parent)->std::string On 2015/05/14 17:07:26, Eric wrote: > Code style comment. > > It's not a good idea to use the exact same name as the function for a local > variable, even if it is substituting for a local function. I can immediately see > two fixes possible: > 1. Rename it to reflect the fact that it's not working with frames. > 2. Extract it as its own function, with the same name and its distinct > signature. > > Or do both. extracted as a separate function. http://codereview.adblockplus.org/5447868882092032/diff/5634387206995968/src/... src/engine/Main.cpp:54: }; On 2015/05/14 17:07:26, Eric wrote: > The whole loop below would be clearer if implemented with 'std::find_if', but > I'm not sure if the overhead is worth it. > > You'd have to memoize the result of "filterEngine->Matches()" above to avoid a > redundant call, for example. It seems such overhead will only complicate it. http://codereview.adblockplus.org/5447868882092032/diff/5634387206995968/src/... File src/plugin/PluginTabBase.cpp (right): http://codereview.adblockplus.org/5447868882092032/diff/5634387206995968/src/... src/plugin/PluginTabBase.cpp:184: ATL::CComPtr<IDispatch> browserParentDispatch; On 2015/05/14 17:07:26, Eric wrote: > I'd call this 'parentDispatch' here, in analogy with the other parent below. renamed
I think this makes sense overall and I have been able to manually apply it to the current tip. Can you please rebase it for final review?
On 2015/10/26 23:44:07, Oleksandr wrote: > I think this makes sense overall and I have been able to manually apply it to > the current tip. Can you please rebase it for final review? rebased
LGTM. Excited about CSS injection! https://codereview.adblockplus.org/5447868882092032/diff/29329412/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29329412/src/plugin/... src/plugin/PluginTabBase.cpp:216: // The InternetExplorer application always returns a pointer to itself. Nit: I'd include a link here: https://msdn.microsoft.com/en-us/library/aa752136(v=vs.85).aspx
https://codereview.adblockplus.org/5447868882092032/diff/29329412/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29329412/src/plugin/... src/plugin/PluginTabBase.cpp:216: // The InternetExplorer application always returns a pointer to itself. On 2015/10/28 10:12:12, Oleksandr wrote: > Nit: I'd include a link here: > https://msdn.microsoft.com/en-us/library/aa752136(v=vs.85).aspx Done.
Also, there's a typo in the review title: "whitelisted" (one lowercase-l) https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/engine/... File src/engine/Main.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/engine/... src/engine/Main.cpp:115: std::string parentUrl; This declaration can go inside the loop. https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/engine/... src/engine/Main.cpp:117: while (frameIterator != frameHierarchy.end()) Since you've already taken care of the empty case first, it would be clearer here to use a do-while loop. Alternately, use a for loop and declare-and-initialize 'url' as its first clause. https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/engine/... src/engine/Main.cpp:119: parentUrl = *frameIterator; The rvalue expression could be "*frameIterator++". There's no need for a separate increment statement at the end of the loop. https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... src/plugin/PluginTabBase.cpp:208: { I don't believe that any of the pointers in the function below need to be smart pointers. All the objects pointed to are browser objects whose lifetime is maintained by IE. None of them come into existence because we asked for them to be created, not the argument browser nor its parent browser (if any). Using smart pointers is simply excess overhead. https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... src/plugin/PluginTabBase.cpp:221: } I don't see the need for any of the code in the function body below. At this point in execution, we have received an IDispatch* from get_Parent and we need to convert it to an IWebBrowser2*. All that's needed is a single call to QueryInterface.
https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/engine/... File src/engine/Main.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/engine/... src/engine/Main.cpp:115: std::string parentUrl; On 2015/11/14 20:28:13, Eric wrote: > This declaration can go inside the loop. Done. https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/engine/... src/engine/Main.cpp:117: while (frameIterator != frameHierarchy.end()) On 2015/11/14 20:28:13, Eric wrote: > Since you've already taken care of the empty case first, it would be clearer > here to use a do-while loop. > > Alternately, use a for loop and declare-and-initialize 'url' as its first > clause. Done. https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/engine/... src/engine/Main.cpp:119: parentUrl = *frameIterator; On 2015/11/14 20:28:13, Eric wrote: > The rvalue expression could be "*frameIterator++". There's no need for a > separate increment statement at the end of the loop. Done. https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... src/plugin/PluginTabBase.cpp:221: } On 2015/11/14 20:28:13, Eric wrote: > I don't see the need for any of the code in the function body below. At this > point in execution, we have received an IDispatch* from get_Parent and we need > to convert it to an IWebBrowser2*. All that's needed is a single call to > QueryInterface. Smart pointers are used to avoid manual calls of Release on the obtained interfaces.
https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... src/plugin/PluginTabBase.cpp:221: } On 2015/11/17 20:16:00, sergei wrote: > Smart pointers are used to avoid manual calls of Release on the obtained > interfaces. I know what smart pointers for COM objects do. My point is that we don't need to call either AddRef or Release on these pointers. The lifetime of all the objects is guaranteed by _our_ caller. The objects we're asking for have life spans that completely encompass the lifetime of this function call. In other words, we don't need the overhead of smart pointers here. https://codereview.adblockplus.org/5447868882092032/diff/29330342/src/engine/... File src/engine/Main.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29330342/src/engine/... src/engine/Main.cpp:126: while (frameIterator != frameHierarchy.end()); Good. This loop reads more clearly.
https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... src/plugin/PluginTabBase.cpp:221: } On 2015/11/17 21:00:16, Eric wrote: > On 2015/11/17 20:16:00, sergei wrote: > > Smart pointers are used to avoid manual calls of Release on the obtained > > interfaces. > > I know what smart pointers for COM objects do. > > My point is that we don't need to call either AddRef or Release on these > pointers. The lifetime of all the objects is guaranteed by _our_ caller. The > objects we're asking for have life spans that completely encompass the lifetime > of this function call. In other words, we don't need the overhead of smart > pointers here. > We should call Release on pointers obtained either through QueryInterface or QueryService.
https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... src/plugin/PluginTabBase.cpp:221: } On 2015/11/17 21:05:23, sergei wrote: > We should call Release on pointers obtained either through QueryInterface or > QueryService. This is the real reason. We don't need to manage the lifetime, but QueryInterface() is going to call AddRef() anyway. That said, we simply need to ensure Release() is called once when the function goes out of scope. I don't mind using CComPtr for that. One the other hand, the point about relative lifespans remain. The return value of this function can be an ordinary pointer. Microsoft calls it the "special knowledge" rule. https://msdn.microsoft.com/en-us/library/ms810016.aspx Rule 2: Special knowledge on the part of a piece of code of the relationships of the beginnings and the endings of the lifetimes of two or more copies of an interface pointer can allow AddRef/Release pairs to be omitted. In the present case, that means that we can omit all the AddRef/Release pairs when we copy IWebBrowser2 pointers around. When you copy CComPtr, as a function return invokes the copy constructor, you're invoking an AddRef/Release pair that can be omitted. https://codereview.adblockplus.org/5447868882092032/diff/29330342/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29330342/src/plugin/... src/plugin/PluginTabBase.cpp:209: ATL::CComPtr<IWebBrowser2> GetParent(IWebBrowser2& browser) IWebBrowser2* GetParent(IWebBrowser2& browser) https://codereview.adblockplus.org/5447868882092032/diff/29330342/src/plugin/... src/plugin/PluginTabBase.cpp:221: } ATL::CComQIPtr<IWebBrowser2> parent = parentDispatch; // Call QI and ensure Release is called return parent; // invokes "operator IWebBrowser2*" All the code below this can then be omitted. That is, of course, assuming that IE acts sanely when it returns a parent browser. The reason that get_Parent() returns IDispatch is for the case when the browser is used as an ActiveX control outside of IE. We're inside IE entirely, so it seems we should be able to assume that the IDispatch we receive is an interface for a IWebBrowser2 object.
https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... src/plugin/PluginTabBase.cpp:221: } On 2015/11/17 22:30:22, Eric wrote: > On 2015/11/17 21:05:23, sergei wrote: > > We should call Release on pointers obtained either through QueryInterface or > > QueryService. > > This is the real reason. We don't need to manage the lifetime, but > QueryInterface() is going to call AddRef() anyway. That said, we simply need to > ensure Release() is called once when the function goes out of scope. I don't > mind using CComPtr for that. > > One the other hand, the point about relative lifespans remain. The return value > of this function can be an ordinary pointer. Microsoft calls it the "special > knowledge" rule. > > https://msdn.microsoft.com/en-us/library/ms810016.aspx > > Rule 2: Special knowledge on the part of a piece of code of the > relationships > of the beginnings and the endings of the lifetimes of two or more copies of > an > interface pointer can allow AddRef/Release pairs to be omitted. > > In the present case, that means that we can omit all the AddRef/Release pairs > when we copy IWebBrowser2 pointers around. When you copy CComPtr, as a function > return invokes the copy constructor, you're invoking an AddRef/Release pair that > can be omitted. I see the point. You are talking that the lifetime of the parent browser is likely not shorter than the lifetime of the child because parent probably is responsible for the lifetime of the child browser, if that's true then the child may have a pointer to the parent without increasing of the reference counter of parent's one and we may use it without touching the reference counter while child is alive. That makes sense and it can work here but we don't know anything about internals of internet explorer (recall that IE is multithreaded application and the everything can happen "between any of our instructions"), so it would be better to keep the return value with increased reference counter until we are done with it. Overhead. I would like to remind about move semantics, there should be no additional AddRef/Release for the return value, so there should not be any overhead you are talking about, we QueryInterface from webBrowserApp and then the smart pointer is "moved" without additional calls of AddRef/Release and Release method is called only once after finishing of the work with the interface. https://codereview.adblockplus.org/5447868882092032/diff/29330342/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29330342/src/plugin/... src/plugin/PluginTabBase.cpp:221: } On 2015/11/17 22:30:22, Eric wrote: > ATL::CComQIPtr<IWebBrowser2> parent = parentDispatch; // Call QI and ensure > Release is called > return parent; // invokes "operator IWebBrowser2*" > > All the code below this can then be omitted. > > That is, of course, assuming that IE acts sanely when it returns a parent > browser. The reason that get_Parent() returns IDispatch is for the case when the > browser is used as an ActiveX control outside of IE. We're inside IE entirely, > so it seems we should be able to assume that the IDispatch we receive is an > interface for a IWebBrowser2 object. Yes, this is just an assumption and it does not work because `parent` is always nullptr here. So we need the code below.
https://codereview.adblockplus.org/5447868882092032/diff/29330342/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29330342/src/plugin/... src/plugin/PluginTabBase.cpp:221: } On 2015/11/18 10:07:50, sergei wrote: > Yes, this is just an assumption and it does not work because `parent` is always > nullptr here. What do you mean "because `parent` is always nullptr"? Are you saying that the IDispatch from get_Parent() is not actually a browser?
https://codereview.adblockplus.org/5447868882092032/diff/29330342/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29330342/src/plugin/... src/plugin/PluginTabBase.cpp:221: } On 2015/11/18 14:24:02, Eric wrote: > On 2015/11/18 10:07:50, sergei wrote: > > Yes, this is just an assumption and it does not work because `parent` is > always > > nullptr here. > > What do you mean "because `parent` is always nullptr"? Are you saying that the > IDispatch from get_Parent() is not actually a browser? > In your snippet > ATL::CComQIPtr<IWebBrowser2> parent = parentDispatch; // Call QI and ensure `parent` is nullptr. I don't know whether it's a browser or not, it does not expose IWebBrowser2 through QueryInterface.
On 2015/11/18 14:28:17, sergei wrote: > In your snippet > > ATL::CComQIPtr<IWebBrowser2> parent = parentDispatch; // Call QI and ensure > `parent` is nullptr. I don't know whether it's a browser or not, it does not > expose IWebBrowser2 through QueryInterface. OK. It seems slightly absurd for the parent to be something else, but it seems that it's so.
On 2015/11/18 14:40:41, Eric wrote:
> On 2015/11/18 14:28:17, sergei wrote:
> > In your snippet
> > > ATL::CComQIPtr<IWebBrowser2> parent = parentDispatch; // Call QI and
ensure
> > `parent` is nullptr. I don't know whether it's a browser or not, it does not
> > expose IWebBrowser2 through QueryInterface.
>
> OK. It seems slightly absurd for the parent to be something else, but it seems
> that it's so.
Actually, it can be logical, the parent of the Web browser can be any object
which hosts it. In our particular case with frames it's not necessary to be
another web browser. The hierarchy can be like
WebBrowser // parent
+DOM
... some tree
+Frame
+WebBrowser // frame
According to documentation
https://msdn.microsoft.com/en-us/library/aa752136(v=vs.85).aspx
> If the WebBrowser control is in a frame, this method returns the automation
interface of the document object in the containing window.
https://codereview.adblockplus.org/5447868882092032/diff/29330395/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29330395/src/plugin/... src/plugin/PluginTabBase.cpp:227: ATL::CComPtr<IWebBrowser2> webBrowser; Changed to use QueryService obtaining IWebBrowser2 without intermediate IWebBrowserApp.
On 2015/11/18 14:57:19, sergei wrote: > According to documentation > https://msdn.microsoft.com/en-us/library/aa752136(v=vs.85).aspx > > If the WebBrowser control is in a frame, this method returns the automation > interface of the document object in the containing window. I think it would be worth putting this reference into a comment. The data structures here are not at all obvious. I've never considered ABP running under the browser as an ActiveX control, but I suppose that's relevant here.
https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29329422/src/plugin/... src/plugin/PluginTabBase.cpp:221: } On 2015/11/18 10:07:50, sergei wrote: > That makes sense and it can work here but we don't know anything about internals > of internet explorer (recall that IE is multithreaded application and the > everything can happen "between any of our instructions"), so it would be better > to keep the return value with increased reference counter until we are done with > it. I'll concede the point that this is purely defensive programming, but since that's real reason, it would be good to document it. I would find it extremely strange if IE changed these DOM objects behind the scenes, but then I can't say it's impossible. > Overhead. Frankly, I'm less worried about overhead than semantic clarity. What I do find is that when I consider overhead, it frequently clarifies issues of semantics. https://codereview.adblockplus.org/5447868882092032/diff/29330342/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29330342/src/plugin/... src/plugin/PluginTabBase.cpp:233: } Except after the call to IsEqualObject(), many of the other returns of nullptr really ought to be throwing logic_error instead. As it's written, we're suppressing notifying ourselves of deficiencies in our own code. I'm not going to insist that these be changed, though. Our exception handling is fairly poor still, and not improving the code here in this way matters little at this time. https://codereview.adblockplus.org/5447868882092032/diff/29330395/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29330395/src/plugin/... src/plugin/PluginTabBase.cpp:228: if (FAILED(parentDocumentServiceProvider->QueryService(IID_IWebBrowserApp, &webBrowser))) I believe you want "IID_IWebBrowser2" here. Otherwise you're relying on getting the same pointer for 'IWebBrowserApp' and 'IWebBrowser2'. Now that's almost certainly the case, since the second class derives from the first. Nevertheless, our discussions about COM lately have gravitated to a defensive stance.
https://codereview.adblockplus.org/5447868882092032/diff/29330395/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29330395/src/plugin/... src/plugin/PluginTabBase.cpp:228: if (FAILED(parentDocumentServiceProvider->QueryService(IID_IWebBrowserApp, &webBrowser))) On 2015/11/18 15:25:22, Eric wrote: > I believe you want "IID_IWebBrowser2" here. > > Otherwise you're relying on getting the same pointer for 'IWebBrowserApp' and > 'IWebBrowser2'. Now that's almost certainly the case, since the second class > derives from the first. Nevertheless, our discussions about COM lately have > gravitated to a defensive stance. No, I think its correct as it is now. Technically, it should be SID_SIWebBrowserApp, but that's just the same as IID_IWebBrowserApp. QueryService is not QueryInterface. It may relay the query to one of its member object, for example. So we don't have any guarantees QueryService(IID_IWebBrowser2) will work in all cases. I believe as Sergei has it here is correct: we should query for service SID_SIWebBrowserApp and then query the result for interface __uuidof(IWebBrowser2).
https://codereview.adblockplus.org/5447868882092032/diff/29330395/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29330395/src/plugin/... src/plugin/PluginTabBase.cpp:227: ATL::CComPtr<IWebBrowser2> webBrowser; For what it's worth, I suggest naming this variable 'parentBrowser' in analogy to 'parentDispatch' above. https://codereview.adblockplus.org/5447868882092032/diff/29330395/src/plugin/... src/plugin/PluginTabBase.cpp:228: if (FAILED(parentDocumentServiceProvider->QueryService(IID_IWebBrowserApp, &webBrowser))) On 2015/11/20 03:03:14, Oleksandr wrote: > I believe as Sergei has > it here is correct: we should query for service SID_SIWebBrowserApp and then > query the result for interface __uuidof(IWebBrowser2). The previous patch set did that. The present one doesn't, since there's nothing to call QI. Querying the service for IID_IWebBrowser2 does seem to work. That's how it's done in the snippet here: https://support.microsoft.com/en-us/kb/257717 So we should be able to call QS this way: QueryService(SID_SWebBrowserApp, IID_IWebBrowser2, &webBrowser) This time I looked up documentation for IServiceProvider::QueryService(). https://msdn.microsoft.com/en-us/library/cc678966(v=vs.85).aspx I didn't see any function signature holding only two arguments. Did the current patch set even compile?
On 2015/11/20 15:43:52, Eric wrote:
> The previous patch set did that. The present one doesn't, since there's
nothing
> to call QI.
---
> I didn't see any function signature holding only two arguments. Did the
current
> patch set even compile?
There's an overload of QueryService in Windows SDK:
template <class Q>
HRESULT STDMETHODCALLTYPE QueryService(_In_ REFGUID guidService, _Outptr_ Q**
pp)
{
return QueryService(guidService, __uuidof(Q), (void **)pp);
}
It accordingly queries for the service and returns the pointer to the interface
that webBrowser defines. So no need to QueryInterface afterwards. So essentially
we are already doing exactly what you are suggesting.
On 2015/11/20 15:54:24, Oleksandr wrote: > It accordingly queries for the service and returns the pointer to the interface > that webBrowser defines. So no need to QueryInterface afterwards. So essentially > we are already doing exactly what you are suggesting. Granted. Nevertheless, I would greatly prefer to use the code that's actually documented rather than a convenience method defined it a header that's not.
https://codereview.adblockplus.org/5447868882092032/diff/29330395/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29330395/src/plugin/... src/plugin/PluginTabBase.cpp:228: if (FAILED(parentDocumentServiceProvider->QueryService(IID_IWebBrowserApp, &webBrowser))) On 2015/11/20 15:43:51, Eric wrote: > On 2015/11/20 03:03:14, Oleksandr wrote: > > I believe as Sergei has > > it here is correct: we should query for service SID_SIWebBrowserApp and then > > query the result for interface __uuidof(IWebBrowser2). > > The previous patch set did that. The present one doesn't, since there's nothing > to call QI. > > Querying the service for IID_IWebBrowser2 does seem to work. That's how it's > done in the snippet here: > https://support.microsoft.com/en-us/kb/257717 > So we should be able to call QS this way: > QueryService(SID_SWebBrowserApp, IID_IWebBrowser2, &webBrowser) > > This time I looked up documentation for IServiceProvider::QueryService(). > https://msdn.microsoft.com/en-us/library/cc678966(v=vs.85).aspx > I didn't see any function signature holding only two arguments. Did the current > patch set even compile? Yes, it's a common technique for type-safeness, we are already using it also for QueryInterface.
LGTM Oleksandr, you might want to take one more look at all this before it pushes. There were significant modifications since your last approval. https://codereview.adblockplus.org/5447868882092032/diff/29330395/src/plugin/... File src/plugin/PluginTabBase.cpp (right): https://codereview.adblockplus.org/5447868882092032/diff/29330395/src/plugin/... src/plugin/PluginTabBase.cpp:228: if (FAILED(parentDocumentServiceProvider->QueryService(IID_IWebBrowserApp, &webBrowser))) On 2015/11/30 15:29:32, sergei wrote: > Yes, it's a common technique for type-safeness, we are already using it also for > QueryInterface. Actually, we're mostly _not_ using it for QI. plugin/PluginDomTraverserBase.h:184: if (FAILED(pBodyDispatch->QueryInterface(IID_IHTMLElement, (LPVOID*)&pBodyEl)) || !pBodyEl) plugin/PluginDomTraverserBase.h:364: if (SUCCEEDED(pAllCollectionDisp->QueryInterface(IID_IHTMLElementCollection, (LPVOID*)&pAllCollection)) && pAllCollection) plugin/PluginDomTraverserBase.h:424: if (SUCCEEDED(pChildCollectionDisp->QueryInterface(IID_IHTMLElementCollection, (LPVOID*)&pChildCollection)) && pChildCollection) plugin/PluginDomTraverserBase.h:441: if (SUCCEEDED(pChildElDispatch->QueryInterface(IID_IHTMLElement, (LPVOID*)&pChildEl)) && pChildEl) plugin/PluginFilter.cpp:407: hr = pPrevSiblingNode.QueryInterface(&pDomPredecessor); plugin/PluginTabBase.cpp:247: pDocDispatch->QueryInterface(IID_IOleObject, (void**)&pOleObj); plugin/PluginTabBase.cpp:255: pClientSite->QueryInterface(IID_IDocHostUIHandler, (void**)&docHostUIHandler); plugin/PluginWbPassThrough.cpp:75: HRESULT hr = internetProtocol->QueryInterface(&winInetHttpInfo); shared/MsHTMLUtils.cpp:26: if (FAILED(htmlElement.QueryInterface(&htmlElement4)) || !htmlElement4) Nine total uses. Six with explicit IID, three without. I'll sign off on this. As much as I think "__uuidof" is an abomination, it does have the merit of making QI somewhat better. To eliminate any question about consistency, I've prepared a change set eliminating all the explicit IID's used above. I'll post it later today.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
