Issue #2230 - Refactor filter life cycle and use

Issue #2230 - Refactor filter life cycle and use

Remove "CPluginTab::m_traverser" and replace it with an object constructed at
the point of traversal. Remove "CPluginTraverserBase::ClearCache()". Because
traverser object are now used only once, there's no reason to reset their
state. Removed "isMainDoc" argument to 'TraverseDocument()', whose only
purpose was to guard a call to 'ClearCache()'.

Remove 'CAdblockPlusClient::IsElementHidden()'. It's only behavior over the
filter function of the same name was some unnecessary locking.

Change constructor argument for traverser classes from 'CPluginTab' to
'CPluginFilter'. These classes were only accessing the filter value.

Renamed the externally-visible version of 'TraverseDocument()' to
'TraverseDocumentRoot()'.

Remove 'TraverseHeader()' and 'TraverseSubdocument()', which were never
called. Remove 'm_isHeaderTraversed', now not assigned to anything after
initialization.

Remove traverser members 'm_cacheDomElementCount' and 'm_pBrowser', neither of
which was referenced.

--------

Other reviews depend on this one:
    Load filters on "download begin" event
    https://codereview.adblockplus.org/29334397/

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

This is the first of two change sets fixing #2230. This one only contains
changes that do not depend on having the new detached initialization class
available.

[sergei, 2016-02-26 13:24:58]

Working on CSS injection I have observed that some local changes are already
partially made in this codereview. Could you please split it into several
reviews, so I can base CSS injection on them?
BTW, nothing from this codereview really depends on
https://codereview.adblockplus.org/29331055/, it would be better to remove it, I
guess, because of it neither me nor Oleksandr had taken a careful look on the
current codereview because the dependency is not finished yet.


1. removing of `CPluginFilter::m_dataPath` - a trivial one, could be already
merged
2. removing of `CAdblockPlusClient::IsElementHidden` - a trivial one, could be
already merged
3. removing of `CPluginDomTraverserBase::m_pBrowser` and
`CPluginDomTraverserBase::m_cacheDomElementCount` - a trivial one, could be
already merged
4. removing of `CPluginTab::m_traverser` looks questionable because
`CPluginTab::OnDownloadComplete` is called many times and caching is probably
important there. I would propose to firstly replace the raw pointer member by
std::unique_ptr, call `m_traverser.reset()` from `CPluginTab::OnNavigate` and
then instantiate it in `CPluginTab::OnDownloadComplete` if it's `nullptr`. It's
also a trivial one.
5.[depends on #4] Afterwards, in another codereview we can consider
instantiating of it each time on `CPluginTab::OnDownloadComplete`. In addition
it seems can address #264.
6.[depends on #4] change the constructor argument of `CPluginDomTraverser` and
`CPluginDomTraverserBase` to `CPluginFilter*`. Actually, I would rather choose
`std::shared_ptr<CPluginFilter>` here instead of raw pointer because it can
happen that `CPluginFilter` is modified by initializer thread. So, we obtain a
new copy of `std::shared_ptr<CPluginFilter>` in `CPluginTab` and then pass it to
the constructor of `CPluginDomTraverser`. It seems here is an opportunity for
data race on `CPluginTab::m_filter` (whatever type it is). We can protect it
using e.g. mutex but the proper initialization of `CPluginTab::m_filter` should
be fixed in another commit after that one.
7.[depends on #5] removing of `CPluginDomTraverserBase<T>::ClearCache` also
should be in a separate commit.
8. it would be better to review removing of
`CPluginDomTraverserBase::m_isHeaderTraversed` and changes of
`CPluginDomTraverserBase::TraverseHeader`,
`CPluginDomTraverserBase::TraverseDocument` and
CPluginDomTraverserBase::TraverseDocument` in a separate codereview.

[Oleksandr, 2016-05-02 12:48:45]

I disagree that we can just remove cachiing here. It is very important on
XHR-heavy sites, like gmail or Google Docs. There can be hundreds of traversing
operations per page, which are taxing CPU and UI pretty bad.

I also agree with Sergei that we can split up this review as suggested above.

[Eric, 2016-05-17 19:55:52]

Patch set 2 is rebase only. I'm not sure there are actual changes; if not, it
will serve as a marker.

Removed the dependency on a prior review now committed.

[Eric, 2016-05-17 21:16:16]

On 2016/05/02 12:48:45, Oleksandr wrote:
> I disagree that we can just remove caching here.

I didn't intend for the change set to subvert caching, but it does. I was
focused on the lifetime of the filter object, and the cache suffered. We've got
problems with the life cycle of the filter object (in no small part because of
its detached initialization) but also visibility issues (we only need the filter
instance of the tab, not the whole tab).

I think the right solution here is to split out the cache as a separate class.
When we change the filter, we must invalidate the cache, but we also invalidate
the cache when we change pages/sites. Thus the cache lifetime is strictly
contained within that of the filter. On the other hand, multiple traversals
should use the same cache, so the cache lifetime contains within itself multiple
traversals. The current code combines these last two lifetimes, confusing
matters. Furthermore, having these things separate improves dealing with page
refresh, plugin enable/disable, etc. 

I'm going to think about this before posting more code.

[Eric, 2016-05-17 21:38:09]

On 2016/02/26 13:24:58, sergei wrote:
> Could you please split it into several
> reviews, so I can base CSS injection on them?

I'm not sure the splitting you're asking for is possible. Many of the splits
you're asking about are for changes that look simple in the review tool, but are
not correct taken in isolation. I have to think through this again to know
what's possible in this regard.

> 5.[...] In addition it seems can address #264.

Making the cache a separate class will assist in that.

> 6.[depends on #4] change the constructor argument of `CPluginDomTraverser` and
> `CPluginDomTraverserBase` to `CPluginFilter*`. Actually, I would rather choose
> `std::shared_ptr<CPluginFilter>` here instead of raw pointer because it can
> happen that `CPluginFilter` is modified by initializer thread.

That's not a good solution. It pushes a lifetime dependency into could that has
no need to know about how the filter is initialized.

There are two possible behaviors if we need to use the filter and it's not
available. If we want to block while filter initialization completes, we can do
that. If we want to not block, we can simply skip filtering altogether. I've
been assuming the correct behavior is to filter, since that's what the plugin
does. That means we need to block somehow. It's better to perform blocking
separately and then just use a plain pointer.

The current code has a completely crazy was of doing that blocking. The detached
initialization class I wrote in another review provides a much better way with
good locality of reference.

(There's actually another possible behavior. With a task queue, let the queue
wake up a blocking task after filter initialization completes. I consider this
approach beyond the scope of incremental progress at this point. Too much to
clean up first.)