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 187 matching lines @@
   params.rgdispidNamedArgs = 0;
   hr = pWndEx->Invoke(dispid, IID_NULL, LOCALE_USER_DEFAULT, DISPATCH_PROPERTYPUT | DISPATCH_PROPERTYPUTREF, &params, 0, 0, 0);
   DEBUG_GENERAL("Invoke");
   if (FAILED(hr))
   {
     DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT_INVOKE, "CPluginTabBase::InjectABP - Failed to create Settings in JavaScript");
   }
 }
 
 namespace
 {

[Eric, 2015/11/14 20:28:13]

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.

   ATL::CComPtr<IWebBrowser2> GetParent(IWebBrowser2& browser)
   {
     ATL::CComPtr<IDispatch> parentDispatch;
     if (FAILED(browser.get_Parent(&parentDispatch)) || !parentDispatch)
     {
       return nullptr;
     }
     // The InternetExplorer application always returns a pointer to itself.
     // https://msdn.microsoft.com/en-us/library/aa752136(v=vs.85).aspx
     if (parentDispatch.IsEqualObject(&browser))
     {
       return nullptr;
     }

[Eric, 2015/11/14 20:28:13]

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.

[sergei, 2015/11/17 20:16:00]

Smart pointers are used to avoid manual calls of Release on the obtained
interfaces.

[Eric, 2015/11/17 21:00:16]

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.

[sergei, 2015/11/17 21:05:23]

We should call Release on pointers obtained either through QueryInterface or
QueryService.

[Eric, 2015/11/17 22:30:22]

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.

[sergei, 2015/11/18 10:07:50]

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.

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

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

     ATL::CComQIPtr<IServiceProvider> parentDocumentServiceProvider = parentDispatch;
     if (!parentDocumentServiceProvider)
     {
       return nullptr;
     }
-    ATL::CComPtr<IWebBrowserApp> webBrowserApp;
-    if (FAILED(parentDocumentServiceProvider->QueryService(IID_IWebBrowserApp, &webBrowserApp)) || !webBrowserApp)
+    ATL::CComPtr<IWebBrowser2> parentBrowser;
+    if (FAILED(parentDocumentServiceProvider->QueryService(SID_SWebBrowserApp, &parentBrowser)))
     {
       return nullptr;
     }
-    return ATL::CComQIPtr<IWebBrowser2>(webBrowserApp);
+    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++;
   }
 }