Issue #2230, #3391 - Load filters on "download begin" event

Issue #2230, #3391 - Load filters on "download begin" event

Add event handler for "download begin" event in 'CPluginClass', which acts
as a facade for a parallel handler in 'CPluginTab'. Move the loading of
filters from the "before navigate" event to the "download begin" event.
"Download begin" is called for both ordinary page load as well as for
refresh, while "before navigate" is called only on ordinary page load. This
fixes #2230, because the root of that issue is that filters were not
(re)loaded on refresh.

Change the life cycle of 'CPluginFilter', whose initialization was the
opposite of encapsulated. (For example, see the now-removed member variable
'hideFiltersLoadedEvent', which was manipulated only outside of
'CPluginFilter'.) Derive 'CPluginTab' from 'DetachedInitializer', allowing
all threading and initialization to the be responsibility of
'CPluginFilter'. Perform the loading of filters in the thread spawned at
construction. Add a "domain" argument to the constructor, since it was the
only ultimate data dependency. Change the filter member of 'CPluginTab' to a
'unique_ptr' that's non-null only for its duration of use.

Refactored 'DetachedInitializer' to work for both static and member
initializations. Renamed existing class to
'DetachedInitializerStaticFunction', as it's a special case of the
now-more-general version.

--------

This review depends on others:
    Issue #2230 - Refactor filter life cycle and use
    https://codereview.adblockplus.org/29334386/

    Issue #3391 - Add detached initialization class
    https://codereview.adblockplus.org/29332660/

[Eric, 2016-01-22 18:17:10]

This is the second of two change sets fixing #2230. This one depends in an
essential way on having the new detached initialization class available.

Note that this change set depends immediately on two other pending change sets,
one of which itself depends on another. Including this one, that's a total of
four change sets to finally fix #2230, a P2 defect.

[sergei, 2016-01-29 10:03:21]

1. Here are some comments regarding detached initialization class (they are part
of https://codereview.adblockplus.org/29332660).

2. I'm not sure that the approach with DownloadBegin and DownloadComplete is
correct. In addition I have commented it already in the issue
https://issues.adblockplus.org/ticket/1201#comment:1 and
https://issues.adblockplus.org/ticket/1201#comment:2, so I would like to firstly
find the proper point when we can load them.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached...
File src/plugin/DetachedInitialization.h (left):

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached...
src/plugin/DetachedInitialization.h:30: * Detached initializers use a separate
thread to avoid delays at instantiation.
This line is OK.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached...
src/plugin/DetachedInitialization.h:32: * Thus such initialization must occur in
the constructor at the earliest.
These two lines are not related to this file.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached...
src/plugin/DetachedInitialization.h:33: *
It's important to say in the description whether the class is thread-safe, thus
whether it's supposed that all methods may be called "simultaneously" from
several threads.

BTW, what happens if somebody calls SpawnInitializer() and EnsureInitialized()
"simultaneously"? If we cannot spawn a thread (I hope it's very unlikely) then
the waiter will wait forever, right?

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached...
File src/plugin/DetachedInitialization.h (right):

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached...
src/plugin/DetachedInitialization.h:31: * Rather than constructing a singleton,
instead we execute an initialization function exactly once.
There is nothing about singleton.
"Detached initializers use a separate thread to avoid delays at instantiation."
looks completely sufficient.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached...
src/plugin/DetachedInitialization.h:32: * The multiprocessing and
synchronization issues, however, are essentially identical.
This line is not related to this file.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached...
src/plugin/DetachedInitialization.h:34: * See
https://en.wikipedia.org/wiki/Double-checked_locking and
http://preshing.com/20130922/acquire-and-release-fences/
I would say that semantically it looks completely differently. One can notice
some similarity in the implementation but I would say that comment is rather
confusing.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached...
src/plugin/DetachedInitialization.h:35: * The flag members of this class enable
fast-return paths, that is, a return without obtaining a mutex lock.
I don't think we need this line.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached...
src/plugin/DetachedInitialization.h:38: * 1) Derive from this class, defining
the factory Initializer().
I am afraid, I disagree with such approach (even beside the virtual
initializer).
The main issue is that it's very easy to forget to call `EnsureInitialized()`
from the destructor of the class (as it happens here) what leads to the race
condition, the derived class with all members can be already destroyed but the
initializing function using them is still running.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached...
src/plugin/DetachedInitialization.h:103: * If the initializer thread throws,
then so will EnsureInitialized().
Only this line is important, the rest about void and specific error semantics
looks redundant.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached...
src/plugin/DetachedInitialization.h:129: * Serialize return from
SpawnInitializer() and this function.
The comment is wrong. We need to lock mutex here mainly to protect condition
variable as well as it's a common convenient pattern of using of flag, condition
variable and mutex.
I would remove it completely.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached...
src/plugin/DetachedInitialization.h:145: return; // fast-return path
I would say we don't need these comments "fast return path".

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginCl...
File src/plugin/PluginClass.h (right):

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginCl...
src/plugin/PluginClass.h:53: //  friend class CPluginTab;
It can be removed

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginFi...
File src/plugin/PluginFilter.h (right):

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginFi...
src/plugin/PluginFilter.h:133: const std::wstring& domain;
That's very dangerous, make it non-reference.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
File src/plugin/PluginTabBase.cpp (right):

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:85: void CPluginTab::OnDownloadBegin()
DownloadComplete and DownloadBegin are fired several times on the same page, do
we really need to load filters several times?

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:85: void CPluginTab::OnDownloadBegin()
Are all DownloadBegin and DownloadComplete called only from the same thread? If
they are called from several threads then there is access violation problem,
because we can access `CPluginFilter* f` which is already destroyed.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:93: filter.reset(new CPluginFilter(domain));
That's really non-readable here that initialization is asynchronous if you are
not aware about internals of CPluginFilter.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:254: if (f)
What the trick are you trying to use here? it looks very suspicious.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:256: f->EnsureInitialized();
It would be better to have some wrapper around CPluginFilter with the getter
that waits for the finishing of the initialization or throws if the
initialization has thrown.

[Eric, 2016-01-29 14:17:36]

On 2016/01/29 10:03:21, sergei wrote:
> 1. Here are some comments regarding detached initialization class (they are
part
> of https://codereview.adblockplus.org/29332660).

If you are commenting here, it means that you'd like me to change the code in
the present review. If you want me to change the code in another review, put
your comments in that other review. If you had reviewed that code when it first
came out, you wouldn't have this problem.

I'll reply here on substantive code changes, but I'm not going to debate code
comments until after the non-comment code is OK.

> 2. I'm not sure that the approach with DownloadBegin and DownloadComplete is
> correct. In addition I have commented it already in the issue
> https://issues.adblockplus.org/ticket/1201#comment:1 and
> https://issues.adblockplus.org/ticket/1201#comment:2, so I would like to
firstly
> find the proper point when we can load them.

Your comments there are only partially relevant to the present code. In
particular, you have not made any argument that using the "download begin" event
fails in any way. You will have to make an argument that the present approach is
wrong if you want to have a valid objection here.

I have tested the present code. Specifically, I've verified in the debugger that
download-begin is fired on refresh. The present code works fine. If you can tell
me why it doesn't work, I'm all ears.

[Oleksandr, 2016-02-01 02:12:28]

On 2016/01/29 14:17:36, Eric wrote:
> I have tested the present code. Specifically, I've verified in the debugger
that
> download-begin is fired on refresh. The present code works fine. If you can
tell
> me why it doesn't work, I'm all ears.

The decision to put the filter loading code into OnNavigate instead of either
one of the DownloadBegin/DownloadComplete was made on purpose. As it has been
mentioned those events run multiple times per page. For example every frame and
XMLHttpRequest would trigger the event. And since the filter loading code is
considerably CPU intensive (and blocking) the browser becomes borderline
unusable on "heavy" pages like Google Docs or Gmail or Outlook.com etc. Ideally
it would be much better to implement a refresh detection mechanism and only
reload filters in DownloadBegin then.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
File src/plugin/PluginTabBase.cpp (right):

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:85: void CPluginTab::OnDownloadBegin()
On 2016/01/29 10:03:21, sergei wrote:
> Are all DownloadBegin and DownloadComplete called only from the same thread?
If
> they are called from several threads then there is access violation problem,
> because we can access `CPluginFilter* f` which is already destroyed.

In my tests they are called from the same thread. I think we are fine here.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:254: if (f)
On 2016/01/29 10:03:20, sergei wrote:
> What the trick are you trying to use here? it looks very suspicious.

+1. I don't understand what is this testing.

[Eric, 2016-02-03 18:03:19]

On 2016/02/01 02:12:28, Oleksandr wrote:
> The decision to put the filter loading code into OnNavigate instead of either
> one of the DownloadBegin/DownloadComplete was made on purpose. [...]
> Ideally
> it would be much better to implement a refresh detection mechanism and only
> reload filters in DownloadBegin then.

The design goal was to take no performance hit here. The idea is only to load
filters once, no matter how many times 'DownloadBegin()' is called. This was one
of the reasons to change member 'filter' from a fixed instance to a pointer to
one. The pointer is null if we still need to load filters for the present page
load, be that an initial load or a reload.

This approach reduces the problem to (1) annulling the filter pointer on initial
load, and (2) annulling the filter pointer on a reload.

That said, the present code fails in what should be a correctable way. The
'filter' member is annulled in 'DownloadComplete()', which is too often, since
that runs multiple times per page. We can annul it in 'OnNavigate()', but that
will only catch case (1). Would it be sufficient to call 'IsRootBrowser()' in
the handler for the download-begin event?

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginCl...
File src/plugin/PluginClass.h (right):

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginCl...
src/plugin/PluginClass.h:53: //  friend class CPluginTab;
On 2016/01/29 10:03:20, sergei wrote:
> It can be removed

Done.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginFi...
File src/plugin/PluginFilter.h (right):

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginFi...
src/plugin/PluginFilter.h:133: const std::wstring& domain;
On 2016/01/29 10:03:20, sergei wrote:
> That's very dangerous, make it non-reference.

Done.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
File src/plugin/PluginTabBase.cpp (right):

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:85: void CPluginTab::OnDownloadBegin()
On 2016/01/29 10:03:20, sergei wrote:
> DownloadComplete and DownloadBegin are fired several times on the same page,
do
> we really need to load filters several times?

See reply to Oleksandr's concern about the same issue.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:85: void CPluginTab::OnDownloadBegin()
On 2016/01/29 10:03:21, sergei wrote:
> Are all DownloadBegin and DownloadComplete called only from the same thread?
If
> they are called from several threads then there is access violation problem,
> because we can access `CPluginFilter* f` which is already destroyed.

On 2016/02/01 02:12:27, Oleksandr wrote:
> In my tests they are called from the same thread. I think we are fine here.

I believe this is part of the "apartment-like" threading that IE uses to make
calls into the BHO.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:93: filter.reset(new CPluginFilter(domain));
On 2016/01/29 10:03:21, sergei wrote:
> That's really non-readable here that initialization is asynchronous if you are
> not aware about internals of CPluginFilter.

It really doesn't matter here how 'CPluginFilter' initializes itself. All that
matters is whether the object exists or not already.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:254: if (f)
On 2016/01/29 10:03:20, sergei wrote:
> What the trick are you trying to use here? it looks very suspicious.

On 2016/02/01 02:12:27, Oleksandr wrote:
> +1. I don't understand what is this testing.

It's defensive programming. The declaration changed from a fixed member instance
of 'CPluginFilter' to a 'unique_ptr' to one. This is simply ensure the pointer
isn't null before indirecting it.

It should never be the case that 'OnDownloadComplete' is ever called without at
least a single call to 'DownloadBegin', so 'f' should never be null. It would
probably be better to throw 'logic_error' if 'f' is null, but I didn't go that
far here.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:256: f->EnsureInitialized();
On 2016/01/29 10:03:21, sergei wrote:
> It would be better to have some wrapper around CPluginFilter with the getter
> that waits for the finishing of the initialization or throws if the
> initialization has thrown.

It's a trade-off. You could declare two classes, as you suggest, or just expose
'EnsureInitialized()' in the one class, as done here. I don't see that the
benefit of a second class is large enough to warrant all the extra source code.

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
File src/plugin/PluginTabBase.h (right):

https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/PluginTa...
src/plugin/PluginTabBase.h:42: std::unique_ptr<CPluginFilter> filter;
As simple as this change looks, it's the core of the whole approach here. If the
member 'filter' is null, it means we need to load filters. If it's not, we skip
loading filters.

[Eric, 2016-02-03 18:05:16]

Patch set 2 addresses Sergei's comments.

[Oleksandr, 2016-02-04 04:40:52]

On 2016/02/03 18:03:19, Eric wrote:
> Would it be sufficient to call 'IsRootBrowser()' in
> the handler for the download-begin event?

It would have been, but I don't think we have a way to get the actual
IWebBrowser2 pointer in OnDownloadBegin, only in OnBeforeNavigate.

[Eric, 2016-02-04 17:38:57]

On 2016/02/03 18:03:19, Eric wrote:
> Would it be sufficient to call 'IsRootBrowser()' in
> the handler for the download-begin event?

On 2016/02/04 04:40:52, Oleksandr wrote:
> It would have been, but I don't think we have a way to get the actual
> IWebBrowser2 pointer in OnDownloadBegin, only in OnBeforeNavigate.

I looked into this, and you're right. The download-begin event has no
parameters. Furthermore, I didn't find any tricks to derive a document pointer
from data that's current during the event handler.

I've looked into reload detection, and whatever mechanism we want to use, it
would be too large to put into the present review. Most of the code here remains
part of a solution to #2230, since reducing the problem to managing a single
pointer is a great improvement to the present code. I'll alter the present
change set to retain the current point where filters are loaded, but using the
new encapsulation of filter initialization present here. It won't fix #2230, but
it moves us toward there.