Issue 7243 - Update adblockpluscore dependency to hg:e26e122e0702

[Manish Jethani, 2019-01-29 05:06:30]

Patch Set 1

https://codereview.adblockplus.org/29991594/diff/29991595/lib/csp.js
File lib/csp.js (right):

https://codereview.adblockplus.org/29991594/diff/29991595/lib/csp.js#newcode72
lib/csp.js:72: let {blocking} = defaultMatcher.search(details.url, typeMap.CSP,
hostname,
If the filter is a blocking filter, we look up all the matching blocking
filters. This is some redundant computation but it kept the implementation
simple and generic (so later we can look up any type of filter for logging
purposes, not just CSP). There are very few CSP filters and they're kept in
their own list. In practice this is fine.

https://codereview.adblockplus.org/29991594/diff/29991595/qunit/tests/url.js
File qunit/tests/url.js (right):

https://codereview.adblockplus.org/29991594/diff/29991595/qunit/tests/url.js#...
qunit/tests/url.js:21: const {isThirdParty} =
require("../../adblockpluscore/lib/domain");
I don't think we need to keep these tests in adblockpluschrome because they are
already duplicated in adblockpluscore, but you decide.

[Manish Jethani, 2019-01-30 19:20:07]

Patch Set 2: Update dependency to hg:91cd83c4c272

[Sebastian Noack, 2019-01-30 19:54:25]

https://codereview.adblockplus.org/29991594/diff/29993600/lib/csp.js
File lib/csp.js (right):

https://codereview.adblockplus.org/29991594/diff/29993600/lib/csp.js#newcode62
lib/csp.js:62: if (cspMatch instanceof WhitelistFilter)
Why not calling search() with "all" instead?

Or if it's faster, keep calling search() with "blocking" and only call
matchesAny() if we found any blocking filters. That should at least be faster
than calling both matchesAny() and search() in the common case (i.e. if there is
no whitelist filter).

Or keep calling matchesAny() first but bail as well if no filter is found.

https://codereview.adblockplus.org/29991594/diff/29993600/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29991594/diff/29993600/lib/devtools.js#new...
lib/devtools.js:88: // avoid duplicates.
This comment is misleading. $csp filter relate to a request, but the relation is
1-to-n (instead of 1-to-1) now.

https://codereview.adblockplus.org/29991594/diff/29993600/qunit/tests/url.js
File qunit/tests/url.js (right):

https://codereview.adblockplus.org/29991594/diff/29993600/qunit/tests/url.js#...
qunit/tests/url.js:21: const {isThirdParty} =
require("../../adblockpluscore/lib/domain");
Shouldn't we rather remove the isThridParty() checks altogether now? We
generally don't test functions from core directly here. They should be tested
within adblockpluscore instead.

[Manish Jethani, 2019-01-31 01:04:15]

https://codereview.adblockplus.org/29991594/diff/29993600/lib/csp.js
File lib/csp.js (right):

https://codereview.adblockplus.org/29991594/diff/29993600/lib/csp.js#newcode62
lib/csp.js:62: if (cspMatch instanceof WhitelistFilter)
On 2019/01/30 19:54:25, Sebastian Noack wrote:
> Why not calling search() with "all" instead?

That would look for all blocking and whitelist filters, unconditionally. We
definitely don't want that in general (maybe only for DevTools logging but
that's a separate issue).

> Or if it's faster, keep calling search() with "blocking" and only call
> matchesAny() if we found any blocking filters. That should at least be faster
> than calling both matchesAny() and search() in the common case (i.e. if there
is
> no whitelist filter).

This would have the disadvantage that for whitelisted URLs we would first look
for all the blocking filters only to find out that the URL is whitelisted later.
Maybe that's not so bad, but it disadvantages whitelisted URLs.

> Or keep calling matchesAny() first but bail as well if no filter is found.

This is what we are doing now here in this patch:

 1.  Look for any filter using matchesAny()
 2.  If no filter found, bail
 3.  If filter is whitelist filter, log and bail
 4.  Look for all blocking filters using search(), log and inject headers

It's not the best but it keeps the changes to a minimum for now and since there
are only ~140 CSP filters  in EasyList it should be OK for now. We can
reconsider at a later point if it would be more optimal to do it differently.

https://codereview.adblockplus.org/29991594/diff/29993600/qunit/tests/url.js
File qunit/tests/url.js (right):

https://codereview.adblockplus.org/29991594/diff/29993600/qunit/tests/url.js#...
qunit/tests/url.js:21: const {isThirdParty} =
require("../../adblockpluscore/lib/domain");
On 2019/01/30 19:54:25, Sebastian Noack wrote:
> Shouldn't we rather remove the isThridParty() checks altogether now? We
> generally don't test functions from core directly here. They should be tested
> within adblockpluscore instead.

Done.

[Manish Jethani, 2019-01-31 01:04:40]

Patch Set 3: Remove third-party checks from QUnit tests

I will address other comments later.

[Sebastian Noack, 2019-01-31 01:12:24]

LGTM, except for the slightly misleading comment. But I will call it a day soon.
So if this is the only change in the next patch set, don't wait for my approval.

https://codereview.adblockplus.org/29991594/diff/29993600/lib/csp.js
File lib/csp.js (right):

https://codereview.adblockplus.org/29991594/diff/29993600/lib/csp.js#newcode62
lib/csp.js:62: if (cspMatch instanceof WhitelistFilter)
On 2019/01/31 01:04:14, Manish Jethani wrote:
> On 2019/01/30 19:54:25, Sebastian Noack wrote:
> > Why not calling search() with "all" instead?
> 
> That would look for all blocking and whitelist filters, unconditionally. We
> definitely don't want that in general (maybe only for DevTools logging but
> that's a separate issue).
> 
> > Or if it's faster, keep calling search() with "blocking" and only call
> > matchesAny() if we found any blocking filters. That should at least be
faster
> > than calling both matchesAny() and search() in the common case (i.e. if
there
> is
> > no whitelist filter).
> 
> This would have the disadvantage that for whitelisted URLs we would first look
> for all the blocking filters only to find out that the URL is whitelisted
later.
> Maybe that's not so bad, but it disadvantages whitelisted URLs.
> 
> > Or keep calling matchesAny() first but bail as well if no filter is found.
> 
> This is what we are doing now here in this patch:
> 
>  1.  Look for any filter using matchesAny()
>  2.  If no filter found, bail
>  3.  If filter is whitelist filter, log and bail
>  4.  Look for all blocking filters using search(), log and inject headers
> 
> It's not the best but it keeps the changes to a minimum for now and since
there
> are only ~140 CSP filters  in EasyList it should be OK for now. We can
> reconsider at a later point if it would be more optimal to do it differently.

I don't think that the whitelisted scenario is one worth optimizing for, but no
filter matching at all is the common case here. I didn't expand the code above,
so I didn't see that we are already in a branch that is only executed if there
is any filter match in the first place. So never mind, that seems good enough.

[Manish Jethani, 2019-01-31 02:54:45]

Patch Set 4: Compare CSP filters by text

You were right about the misleading comment. I have updated the code to compare
CSP filters by text now.

Let me explain.

Suppose you have the following filter in both EasyList and your own user-defined
filters:

  ||4chan.org^$csp=script-src 'self' * 'unsafe-inline' *.gstatic.com
*.google.com *.googleapis.com

In this case ABP treats it as one filter (belonging to two subscriptions) and
injected only one copy of the header (of course). For such a case, it makes
sense to have only one entry in the DevTools panel - although it should show the
filter as belonging to multiple filter lists (which is doesn't seem to right
now).

Then there's the other case where the URL pattern differs. Let's say you have
this filter in your user-defined filters:

  chan.org^$csp=script-src 'self' * 'unsafe-inline' *.gstatic.com *.google.com
*.googleapis.com

Now technically it's a different filter, therefore the same header is injected
twice. This is OK. But since the URL pattern is different, the filter list
author would like to see that their filter is getting applied (it is). In that
case, we should not consider it to be a duplicate of the filter in EasyList
based only on the CSP value. The simple thing to do is to compare the text. If
we do this (this patch), it logs both the filters separately for this case.

[Manish Jethani, 2019-01-31 02:58:45]

On 2019/01/31 02:54:45, Manish Jethani wrote:

> The simple thing to do is to compare the text.

Actually we could just compare the filter objects because two different filter
objects can't have the same text (but this may change with #7097). I guess it's
clearer to compare the text.

[Sebastian Noack, 2019-01-31 03:59:33]

LGTM