Issue 1793 - check whether the frame is whitelisted before injecting CSS

src/plugin/PluginTabBase.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 206 matching lines @@
     // https://msdn.microsoft.com/en-us/library/aa752136(v=vs.85).aspx
     if (parentDispatch.IsEqualObject(&browser))
     {
       return nullptr;
     }
     ATL::CComQIPtr<IServiceProvider> parentDocumentServiceProvider = parentDispatch;
     if (!parentDocumentServiceProvider)
     {
       return nullptr;
     }
-    ATL::CComPtr<IWebBrowser2> webBrowser;

[sergei, 2015/11/18 15:06:37]

Changed to use QueryService obtaining IWebBrowser2 without intermediate
IWebBrowserApp.

[Eric, 2015/11/20 15:43:51]

For what it's worth, I suggest naming this variable 'parentBrowser' in analogy
to 'parentDispatch' above.

-    if (FAILED(parentDocumentServiceProvider->QueryService(IID_IWebBrowserApp, &webBrowser)))

[Eric, 2015/11/18 15:25:22]

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.

[Oleksandr, 2015/11/20 03:03:14]

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).

[Eric, 2015/11/20 15:43:51]

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?

[sergei, 2015/11/30 15:29:32]

Yes, it's a common technique for type-safeness, we are already using it also for
QueryInterface.

[Eric, 2015/11/30 17:23:26]

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.

+    ATL::CComPtr<IWebBrowser2> parentBrowser;
+    if (FAILED(parentDocumentServiceProvider->QueryService(SID_SWebBrowserApp, &parentBrowser)))
     {
       return nullptr;
     }
-    return webBrowser;
+    return parentBrowser;
   }
 
   bool IsFrameWhiteListed(ATL::CComPtr<IWebBrowser2> frame)
   {
     if (!frame)
     {
       return false;
     }
     auto url = GetLocationUrl(*frame);
     std::vector<std::string> frameHierarchy;
@@ skipping 189 matching lines @@
           LogQueue::LogPluginError(pluginError.GetErrorCode(), pluginError.GetErrorId(), pluginError.GetErrorSubid(), pluginError.GetErrorDescription(), true, pluginError.GetProcessId(), pluginError.GetThreadId());
         }
 
         // Non-hanging sleep
         Sleep(50);
       }
 
       tabLoopIteration++;
   }
 }