Element hiding completely breaks if there are any syntax errors in element hiding filters

[alucardo]

Environment

OS : Windows 7
Browser : Google Chrome Version 63
Adblock Plus version 3.0.1.1939

How to reproduce

1. Add Mapx's broken filter subscription ​http://axistrivia.altervista.org/list/testwrongfilter.txt
2. Browse to ​https://adblockplus.org/
3. Click the ABP logo next to your address bar
4. Select "Block element"
5. Hover the ABP logo on the webpage at the top left corner
6. The image becomes yellowish
7. Click on it
8. A popup appears asking for confirmation.
9. Click Add

Observed behaviour

The selected logo doesn't disappear.

Exceptions are shown in the developer console.

Expected behaviour

The selected logo disappears.

[kzar]

I can't reproduce this with Chrome 49.0.2623.75 (64-bit) or Version 56.0.2924.87 (64-bit). I don't have more time at my computer today, guys could you try to reproduce this too before putting out 1.13.2? I'd like to avoid a third emergency release...

alucardo do you have any other extensions installed? If so which? Also what filter subscriptions and other custom filters do you have? Do you have Acceptable Ads enabled?

[trev]

I can reproduce in Chrome 56, the logo doesn't disappear immediately but it does after refreshing the page. So the issue here is new element hiding filters not applying immediately.

[mapx]

tested in Version 58.0.3029.19 beta (64-bit), the logo disappears immediately, no issue for me

[trev]

Actually, I cannot reproduce the issue reliably. There seems to be a race condition here.

[wspee]

Works for me in:

Linux 57.0.2987.98 (64-bit)
Windows 10 56.0.2924.87 (64-bit)
Windows 10 49.0.2623.75 (64-bit)
Windows 10 49.0.2623.75 (32-bit)
Windows XP (32-Bit) 49.0.2623.75 (32-bit)

I tried to do it multiple times, in case it's a sporadic behavior, worked for me every time.

Edit: Also tried if it makes a difference whether or not the abp panel is open, it doesn't.

[trev]

Actually, the title appears to be misleading. Maybe the OP could confirm that this is actually about filters not applying immediately rather than personal filters being really broken - i.e. that reloading the page produces the expected results.

[alucardo]

Even after refreshing, the filter doesn't apply.

I have 2 filter-lists :

• EasyList
• Prebake (Filter Obtrusive Cookie Notices)

And [Allow some non-intrusive advertising] is UNCHECKED

I will try disabling those lists and my other extenstions and i will let you know.

[alucardo]

Found it, it's EasyList !
If i disable EasyList, personal filters work again.
Maybe it is due to the certificate of ​https://easylist.to/ which gives an SSL error on Windows XP : ERR_SSL_VERSION_OR_CIPHER_MISMATCH

[mapx]

Did you update your easylist ? 2-3 days ago a typo in easylist provoked a real mess (seen in the chrome console)

[alucardo]

Updated EasyList, didn't change anything.
Removed it, added it back => problem solved!
Sorry guys for the hassle. Thanks a lot for the help and quick feedback.

[sebastian]

While it turned out that a bad subscription caused these custom filters to generally not apply for alucardo, I'm still wondering why trev occasionally has to reload the page before filters added through the "Block element" dialog show any effect. These issues are most likely unrelated, and while we can ignore the former (assuming that it was a data corruption or user's fault), the latter might still be a bug.

[mapx]

you should insert a typo in easylist (copy of easylist) like here:
​https://forums.lanik.us/viewtopic.php?p=114966#p114966

and see if you can reproduce the "issue".

[trev]

Replying to sebastian:

While it turned out that a bad subscription caused these custom filters to generally not apply for alucardo, I'm still wondering why trev occasionally has to reload the page before filters added through the "Block element" dialog show any effect.

Yes, it's a good question. I checked the code yesterday and couldn't see where a race condition would possibly come from. Unfortunately, the issue was so rare that I couldn't figure out whether it was caused by messaging hickups or DOM modification quirks.

[trev]

Replying to mapx:

you should insert a typo in easylist (copy of easylist) like here:
​https://forums.lanik.us/viewtopic.php?p=114966#p114966

and see if you can reproduce the "issue".

This is unlikely to be the cause, and even if it is - selectors are added in groups of 200 on Chrome. We can spend lots of time trying to reproduce and the broken selector will still be in a different group, not causing any visible issues.

[mapx]

It's not unlikely, it's exactly the cause of the issue.

I prepared for you the case I specified above.
Subscribe
​http://axistrivia.altervista.org/list/easylist2.txt

and you'll get the wrong behaviour.

See also the messages in the console.

ABP should check (on first load of the day) the filters and isolate the wrong filters.

[mapx]

the wrong filter in the list easylist2

##a[href^="https://understandsolar.com/signup/?lead_source="][href*=&tracking_code="]

[mapx]

Replying to sebastian:

While it turned out that a bad subscription caused these custom filters to generally not apply for alucardo, I'm still wondering why trev occasionally has to reload the page before filters added through the "Block element" dialog show any effect. These issues are most likely unrelated, and while we can ignore the former (assuming that it was a data corruption or user's fault), the latter might still be a bug.

Simply when trev did his tests easylist was still broken. Minutes after easylist was updated in his ABP and the wrong behaviour disappeared.

[mapx]

tested in uBo: no strange behaviour (on load uBo (re)organize / check the filters and remove / flag the wrong filters). It's what ABP needs too.

[sebastian]

We already have performance problems when loading/parsing subscriptions in less powerful environments (i.e. on mobile). Adding logic to validate the selector of each element hiding filter would make it even worse. And even if we do, the invalid selectors still wouldn't work as intended by the filter list author. So it is the responsibility of filter list authors to test their filters before adding them.

As for uBlock (Origin), it uses a whole different approach, instead of injecting CSS (in batches), they hide each element individually, so that a single invalid selector doesn't have any implications on other filters. However, as I explained in 242#comment:27, this approach isn't an option for us.

[trev]

I can confirm that this invalid filter causes the issue - while the rules aren't in the same group, a single failing insertRule call causes Adblock Plus to stop. So maybe we should at least catch exceptions.

What I was seeing seems to have been a different issue, hard to tell however.

[mapx]

Replying to trev:

a single failing insertRule call causes Adblock Plus to stop. So maybe we should at least catch exceptions.

well, in that case at least ABP could dismiss the wrong filters group (200 filters) and continue, without stopping / crashing.

[sebastian]

Well, the reason we inject CSS selectors in batches, was a limitation in Blink that caused it to struggle when inserting a long list of selectors at once, but this limit has been raised significantly a long time ago. Then there was Safari which still has a similar limit, but is no longer supported by this code. It seems inserting selectors in batches is no longer necessary. So once we implement #242 and use chrome.tabs.insertCSS() we might just insert all selectors at once, neither does insertCSS() give us an error when there are any invalid selectors. Furthermore, Chrome is used to deoptimize the whole function when using try..catch, and this code path is critical as it blocks page load. Recently, the deoptimization was removed from V8, but I'm not sure which Chrome versions are still effected.

However, the issue trev originally reproduced still seems to be another one. As he told me on IRC, he reproduced it ~3 times out of ~100. So the theory that there was an invalid selector in the subscriptions, which then automatically got updated before reloading the page, doesn't seem plausible.

[mapx]

well, it's something ABP could still improve (without performance issues as the check will be run only once every 4-5 days):

The filter lists are compiled by uBO, and invalid filters are thrown out at compile time. The parsing/validating is performed once only when a filter list is updated, afterward the compiled version is used to load filters with zero parsing/validating overhead

from:
​https://github.com/gorhill/uBlock/issues/2465#issuecomment-288071596

Using this simple approach a single filter wont send in crash ABP anymore.

[kzar]

I agree we should handle this situation better. Like Wladimir mentions we can catch exceptions thrown by insertRule. When an exception is thrown we can insert a rule for each selector in the batch separately, so that only the broken ones are skipped.

[sebastian]

Do you really think this is worth it, for the time being? We have to regress here anyway once we move to tabs.insertCSS(), not to mention potential performance implications, see comment:24.

[kzar]

Why do we have to regress here anyway when moving to tabs.insertCSS()? We can catch the exception there too. Also see the codereview for a discussion about the performance of my suggested change.

The fact that element hiding completely breaks in this situation is pretty bad IMO, mistakes with filters will happen from time to time.

[sebastian]

Replying to kzar:

Why do we have to regress here anyway when moving to tabs.insertCSS()? We can catch the exception there too.

We cannot catch the exception with tabs.insertCSS() because there is none, see comment:24.

Also see the codereview for a discussion about the performance of my suggested change.

Even if deoptimization of try..catch isn't an issue we have to bother about anymore, we shouldn't insert selectors in batches of 200 anymore but rather insert all selectors at once, which makes the fallback you are suggesting more problematic, see my comment on the review.

The fact that element hiding completely breaks in this situation is pretty bad IMO, mistakes with filters will happen from time to time.

If that can be avoided, that would be nice. But so far nobody came up with a sustainable solution without major downsides (if there is any).

[mapx]

Did you read my comment ?
https://issues.adblockplus.org/ticket/5015#comment:25

The check should be done only when the subscription is updated. No performance issues and only once every 4-5 days.

[kzar]

We cannot catch the exception with tabs.insertCSS() because there is none

Wow I found that surprising, but tested myself and can confirm. I've filed ​Chromium issue 704454 for that.

Even if deoptimization of try..catch isn't an issue we have to bother about anymore...

Well I split the part with try...catch into a separate function for that reason.

...we shouldn't insert selectors in batches of 200 anymore but rather insert all selectors at once, which makes the fallback you are suggesting more problematic, see my comment on the review.

OK, I'll respond there.

[sebastian]

Replying to mapx:

Did you read my comment ?
https://issues.adblockplus.org/ticket/5015#comment:25

The check should be done only when the subscription is updated. No performance issues and only once every 4-5 days.

This would at least work regardless whether the selectors are inject with a content script or by chrome.tabs.insertCSS(), however, the way Adblock Plus works internally this would have to be done not only when the filter lists are updated, but also every time the extension is loaded (e.g. on browser start). Even if we do significant changes to strip invalid filters as they are downloaded (which by the way still is potentially more often than every 4-5 days, as well as on first run), this would still be a potential performance issue on old computers and mobile devices, see comment:21.

Replying to kzar:

We cannot catch the exception with tabs.insertCSS() because there is none

Wow I found that surprising, but tested myself and can confirm. I've filed ​Chromium issue 704454 for that.

I guess, if that issue gets addressed by Chrome, and there will be a way to detect failure with chrome.tabs.insertCSS(), this approach could work with that API as well. I wouldn't hold back #242 just because of that limitation though. Not sure if the old/existing element hiding code is still worth being changed, in particular as long as it isn't clear yet whether we will able to the same with insertCSS().

[kzar]

Unassigning myself from this for now.

[mapx]

happened again:
https://issues.adblockplus.org/ticket/6097

No rethinking on checking the filters on loading them in ABP ?

[sebastian]

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the ​new issue tracker.

[cekk2123]

spam