Issue 119 - Switch to injecting CSS for element hiding

# Currently there are a lot of unrelated changes, they should be in other codereviews and some of them are already in another codereview but there is no progress.

- add WebBrowserEventsListener.{h,cpp} which detects when the actual frame loading/reloading happened.
- add CPluginClass::m_data. It has to be a shared pointer because we capture a weak ptr on it to avoid usage of it after destroying of CPluginClass.
- add CPluginFilter::m_hideFilters with getter (it should wait for the finishing of the initialization) so we can use them to inject as CSS
- change CPluginFilter::LoadHideFilters to accept const reference instead of a copy
- add method InjectABPCSS which injects our css
- call InjectABPCSS  from CPluginTabBase::OnDocumentComplete which is called when the real page loading happened
- add methods to decide whether we need to traverse or to inject css (based on testing versions)
- do nothing in ~CPluginClass because we should do such stuff in FinalRelease

[sergei, 2014-12-10 17:16:15]

Please check the code, actually I think we should merge it gradually thus split
into several parts.

[Oleksandr, 2015-02-04 20:59:57]

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:498: {
I would prefer for this to be a separate function instead. Like
RegisterWebBrowserEventsListener(Data* data). In fact this could be a function
in CPluginClientFactory.

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:500: if (m_data->connectedWebBrowsersCache.end() ==
it)
This is Yoda condition, IMO

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:507: ATL::CComPtr<IUnknown>
listnerRefCounterGuard(listenerImpl->GetUnknown());
missing 'e'? listenerRefCounterGuard

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:509: auto onListenerDestroy = [webBrowser,
dataForCapturing]
If this whole block is moved to CPluginClientFactory, I would extract
onListenerDestroy and reloaded to separate functions too

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
File src/plugin/PluginFilter.cpp (right):

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
src/plugin/PluginFilter.cpp:754: DWORD res =
WaitForSingleObject(hideFiltersLoadedEvent, ENGINE_STARTUP_TIMEOUT);
It's hard to understand this from the scattered reviews, I just don't see it.
Where do we set this event?

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
File src/plugin/PluginFilter.h (right):

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
src/plugin/PluginFilter.h:172: const std::vector<std::wstring>& getHideFilters()
const;
GetHideFilters() to be consistent?

[sergei, 2015-04-13 08:06:58]

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:498: {
On 2015/02/04 20:59:57, Oleksandr wrote:
> I would prefer for this to be a separate function instead. Like
> RegisterWebBrowserEventsListener(Data* data). In fact this could be a function
> in CPluginClientFactory.

I've extracted it into CPluginClass::EnsureWebBrowserConnected

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:498: {
On 2015/02/04 20:59:57, Oleksandr wrote:
> I would prefer for this to be a separate function instead. Like
> RegisterWebBrowserEventsListener(Data* data). In fact this could be a function
> in CPluginClientFactory.

I guess here and below instead of CPluginClientFactory CPluginClass is meant.

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:500: if (m_data->connectedWebBrowsersCache.end() ==
it)
On 2015/02/04 20:59:57, Oleksandr wrote:
> This is Yoda condition, IMO

fixed

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:507: ATL::CComPtr<IUnknown>
listnerRefCounterGuard(listenerImpl->GetUnknown());
On 2015/02/04 20:59:57, Oleksandr wrote:
> missing 'e'? listenerRefCounterGuard

fixed

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:509: auto onListenerDestroy = [webBrowser,
dataForCapturing]
On 2015/02/04 20:59:57, Oleksandr wrote:
> If this whole block is moved to CPluginClientFactory, I would extract
> onListenerDestroy and reloaded to separate functions too

I think it will complicate the code because it is more code and anonymous
functions without any algorithmic complexity which are not relevant without the
current function only confuse. However, they could be methods of Data as well as
some other methods of CPluginClass, but let's leave it for the refactoring when
we decide that we need it.

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
File src/plugin/PluginFilter.cpp (right):

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
src/plugin/PluginFilter.cpp:754: DWORD res =
WaitForSingleObject(hideFiltersLoadedEvent, ENGINE_STARTUP_TIMEOUT);
On 2015/02/04 20:59:57, Oleksandr wrote:
> It's hard to understand this from the scattered reviews, I just don't see it.
> Where do we set this event?

`hideFiltersLoadedEvent` is not introduced in these changes, It's set in
PluginTabBase.cpp in `FilterLoader`. JIC, I'm sure that refactoring of
`hideFiltersLoadedEvent` should not be a part of this issue.

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
File src/plugin/PluginFilter.h (right):

http://codereview.adblockplus.org/6567422169448448/diff/5629499534213120/src/...
src/plugin/PluginFilter.h:172: const std::vector<std::wstring>& getHideFilters()
const;
On 2015/02/04 20:59:57, Oleksandr wrote:
> GetHideFilters() to be consistent?

fixed

[Eric, 2015-12-01 14:39:46]

Have all the prerequisites for this change set been committed? There's a new
rebased patch set. Will this apply to current tip?

If so, it's time to change the issue description as well to remove the
dependencies, which are no longer relevant.

[sergei, 2015-12-01 15:35:48]

On 2015/12/01 14:39:46, Eric wrote:
> Have all the prerequisites for this change set been committed? There's a new
> rebased patch set. Will this apply to current tip?
Yes.
> If so, it's time to change the issue description as well to remove the
> dependencies, which are no longer relevant.
Done.

[Eric, 2015-12-02 17:00:15]

This change, as written, will make all of our initialization and lifetime
problems worse. We have a number of pending reviews addressing these problems.
They should be committed first.

- Noissue - Rewrite and simplify browser-site handling in CPluginClass
    https://codereview.adblockplus.org/29323561/
    - Affects handling of m_webBrowser2, altered in this review

- NoIssue - Refactor HTTP and HTTPS namespaces registration
    https://codereview.adblockplus.org/5032147387678720/
    - Affect event registration, relevant to this review

- Issue #1467 - Rewrite the map from threads to tabs 
    https://codereview.adblockplus.org/29330403/
    - Part of the same category of issues; might not be directly relevant here


-----

A few preparatory piece of this code could be committed now.
- The code optimization for "res://" (see comment) which will make later review
easier.
- Configuration for CSS injection vs. traversal, which we'll want for
development.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/adblockplus...
File adblockplus.gyp (right):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/adblockplus...
adblockplus.gyp:153: 'src/plugin/WebBrowserEventsListener.cpp',
All the files in the passthrough directory come after any of the files in the
plugin directory. If we were going on strict alphabetical order,
"passthroughapp" would come before all the "Plugin.*" files.

Mind you, this looks like a rebase artifact, but we still ought to fix it.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
File src/plugin/PluginClass.cpp (right):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.cpp:130: m_data.reset();
'FinalRelease' is slated to disappear as part of the rationalization of
'SetSite()'. This line, I believe, will be unnecessary. 'm_webBrowser2' will be
nullified in 'SetSite(0)'. The destructor for this data member will be called in
the destructor for 'CPluginClass'.

Please see this review page for the relevant changes.
 
https://codereview.adblockplus.org/29323561/diff/29331633/src/plugin/PluginCl...

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.cpp:1114:
settings->AddWhiteListedDomain(client->GetHostFromUrl(urlString));
This change and the one immediately above are a reversion, I assume, from a
rebase operation.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
File src/plugin/PluginClass.h (left):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.h:57: CPluginTab* m_tab;
You've converted this to a 'unique_ptr' in the change set, which is an
improvement if you think it needs to be separately allocated.

I don't think it needs to be allocated at all. There's a 1-1 relationship
between BHO objects (this class 'CPluginClass') and 'CPluginTab'. There's no
good reason this shouldn't have been a ordinary, non-allocated pointer before.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.h:140: void STDMETHODCALLTYPE
OnDocumentComplete(IDispatch* frameBrowserDisp, VARIANT* /*urlOrPidl*/);
I don't think we'll need to put this event handler into a separate object.

I can tell you're working around an existing defect in our lifetime management
of objects and event registration. We already have three pending reviews on
these issues that we ought to fix first.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
File src/plugin/PluginClass.h (right):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.h:161: std::shared_ptr<Data> m_data;
This struct looks like a Very Bad Idea.

If we get our lifetime management correct, there won't be any issue about events
coming in to non-existent objects, and thus a weak pointer won't be needed at
all.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
File src/plugin/PluginTabBase.cpp (right):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginTabBase.cpp:217: }
I'd like to see a user setting for these predicates that can override what's
already here. The use cases are for developers running and debugging traverser
code. Without something we can override, it's going to be quite a bother
degrading the IE version just to run the traverser code.

I suggest two mechanisms.

(1) An in-memory static variable that can be explicitly toggled inside the VS
debugger. This can be evaluated the first time and left alone after that, which
means a second "is initialized" value somewhere.

(2) An external configuration option, either in a file or in the registry, that
configures operation when not running under the debugger.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginTabBase.cpp:398: }
This optimization, for early return on a "res://" URL, could be committed
separately and immediately. It will make the changes below easier to review. At
present it all looks different because the indentation changes, when in fact
most of it's the same.

[Eric, 2015-12-02 17:02:02]

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
File src/plugin/PluginClass.h (left):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.h:57: CPluginTab* m_tab;
On 2015/12/02 17:00:14, Eric wrote:
> ordinary, non-allocated pointer .

That should be "ordinary, non-allocated object".

[Eric, 2015-12-02 18:45:19]

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
File src/plugin/PluginClass.h (left):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.h:57: CPluginTab* m_tab;
While auditing code, I found the same problem in 'CPluginTab'. Here's what the
fix for that problem looks like:
    https://codereview.adblockplus.org/29331767/

I think we'll be able to do the same thing here, but we'll only be able to tell
that after we get all the life cycle issues sorted out.

[Eric, 2016-01-27 15:09:26]

NOT LGTM

Since I have upbraided Sergei for remaining silent, it behooves me to say what I
think. I said part of this a couple months ago, but there's been no response,
thus no opportunity to elaborate.

I don't think we'll ever be able to make incremental modifications to this code
and get it to apply. The whole idea of "class WebBrowserEventsListener" is just
plain wrong, as is "struct Data" that supports it. It's splitting "CPluginClass"
into two pieces for what seem to be life cycle issues with it. If there are
other reasons, I can't tell. Regardless, the flow control is even worse than the
incumbent code.

Review https://codereview.adblockplus.org/29334397/ allows us a hook to detect
reload events. It's not clear to me that we'll need to register for
'HTMLDocumentEvents2' as a result.

[sergei, 2016-02-25 17:48:29]

Pay attention that firstly a bug mentioned in the description should be fixed.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/adblockplus...
File adblockplus.gyp (right):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/adblockplus...
adblockplus.gyp:153: 'src/plugin/WebBrowserEventsListener.cpp',
On 2015/12/02 17:00:13, Eric wrote:
> All the files in the passthrough directory come after any of the files in the
> plugin directory. If we were going on strict alphabetical order,
> "passthroughapp" would come before all the "Plugin.*" files.
> 
> Mind you, this looks like a rebase artifact, but we still ought to fix it.

These two lines are moved before passthroughapp.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
File src/plugin/PluginClass.cpp (right):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.cpp:1114:
settings->AddWhiteListedDomain(client->GetHostFromUrl(urlString));
On 2015/12/02 17:00:14, Eric wrote:
> This change and the one immediately above are a reversion, I assume, from a
> rebase operation.

Done.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
File src/plugin/PluginClass.h (left):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.h:57: CPluginTab* m_tab;
When I was doing it I tried to avoid changes unrelated to the current issue. The
code review was already very big and cleaning changes would only complicate it.

I have changed it from raw pointer mainly because now its lifetime is scoped by
the lifetime of `struct Data`, using `std::unique_ptr` looks the most elegant
way to achieve it.

Although it seems now we can change it from a pointer to an ordinary member I
think it should be in another commit.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.h:140: void STDMETHODCALLTYPE
OnDocumentComplete(IDispatch* frameBrowserDisp, VARIANT* /*urlOrPidl*/);
On 2015/12/02 17:00:14, Eric wrote:
> I don't think we'll need to put this event handler into a separate object.
Can you give any arguments?
> 
> I can tell you're working around an existing defect in our lifetime management
> of objects and event registration. We already have three pending reviews on
> these issues that we ought to fix first.
It's not related to lifetime management.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
File src/plugin/PluginClass.h (right):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.h:155: std::map<IWebBrowser2*, WebBrowserEventsListener*>
connectedWebBrowsersCache;
These raw pointers is another point here regarding `shared_ptr/weak_ptr`. I
don't like them but I doubt that we actually want to keep strong references to
child 'WebBrowser's because in that case we can leak heavy 'WebBrowser's.

That point deserves to be deeply investigated, however the current version can
be used, so far.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.h:161: std::shared_ptr<Data> m_data;
On 2015/12/02 17:00:14, Eric wrote:
> This struct looks like a Very Bad Idea.
> 
> If we get our lifetime management correct, there won't be any issue about
events
> coming in to non-existent objects, and thus a weak pointer won't be needed at
> all.

It's not about events coming to non-existing objects.

I have observed that there is a race condition in IE regarding the destroying of
'WebBrowser's, even though `m_data` is destroyed (which happens only in
`~CPluginClass`) instances of `WebBrowserEventsListener` are still alive. It
means that our plugin is already destroyed but child 'WebBrowser's are still
alive.

Beside the comment above regarding raw pointers, we can definitely try to listen
to DISPID_ONQUIT from children and unsubscribe our `WebBrowserEventsListener` or
unsubscribe them when our plugin is being destroyed. However, it seems, in that
case the code will be merely more sophisticated. For example, it can happen that
DISPID_ONQUIT events from children come during unsubscribing operation, so we
need to serialize this part, it means again more locks and yet one "global"
variable.

So to operate safely and easier I'm using shared_ptr/weak_ptr.

We can definitely try to elaborate it, but I guess it would be better to make
later.
@Oleksandr?

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
File src/plugin/PluginTabBase.cpp (right):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginTabBase.cpp:217: }
On 2015/12/02 17:00:15, Eric wrote:
> I'd like to see a user setting for these predicates that can override what's
> already here. The use cases are for developers running and debugging traverser
> code. Without something we can override, it's going to be quite a bother
> degrading the IE version just to run the traverser code.
> 
> I suggest two mechanisms.
> 
> (1) An in-memory static variable that can be explicitly toggled inside the VS
> debugger. This can be evaluated the first time and left alone after that,
which
> means a second "is initialized" value somewhere.
> 
> (2) An external configuration option, either in a file or in the registry,
that
> configures operation when not running under the debugger.

I have created those methods exactly for that reason.
Currently developers can insert `return false;` here or add some code to read it
from some file (3 lines of code) or use something else. Not so difficult, I
guess.

I agree that we need some external flag, see #3631.

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginTabBase.cpp:398: }
On 2015/12/02 17:00:15, Eric wrote:
> This optimization, for early return on a "res://" URL, could be committed
> separately and immediately. It will make the changes below easier to review.
At
> present it all looks different because the indentation changes, when in fact
> most of it's the same.

Done. It's already not relevant.

[sergei, 2016-02-26 08:34:52]

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
File src/plugin/PluginClass.h (right):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.h:155: std::map<IWebBrowser2*, WebBrowserEventsListener*>
connectedWebBrowsersCache;
BTW, in addition we probably need to serialize the access to that map, I will
look into it.

[sergei, 2016-02-26 16:57:57]

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
File src/plugin/PluginClass.h (right):

https://codereview.adblockplus.org/6567422169448448/diff/29331703/src/plugin/...
src/plugin/PluginClass.h:155: std::map<IWebBrowser2*, WebBrowserEventsListener*>
connectedWebBrowsersCache;
On 2016/02/26 08:34:51, sergei wrote:
> BTW, in addition we probably need to serialize the access to that map, I will
> look into it.

According to my tests destructors of `WebBrowserEventsListener` are always
called from the same thread in which they have been constructed, BTW it is the
same from which `SetSite` is called.

[Oleksandr, 2016-09-30 15:29:21]

LGTM

https://codereview.adblockplus.org/6567422169448448/diff/29340865/src/plugin/...
File src/plugin/WebBrowserEventsListener.cpp (right):

https://codereview.adblockplus.org/6567422169448448/diff/29340865/src/plugin/...
src/plugin/WebBrowserEventsListener.cpp:62: void STDMETHODCALLTYPE
WebBrowserEventsListener::OnOnQuit()
OnOnQuit sounds somewhat meta. How about just OnQuit?