Opened 6 months ago

Last modified 5 months ago

#5015 reviewing defect

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

Reported by: alucardo Assignee:
Priority: P2 Milestone:
Module: Platform Keywords:
Cc: kzar, trev, sebastian, Ross, scheer, rraceanu, mapx, wspee, greiner Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29392555/

Description (last modified by kzar)

Environment

OS : Windows XP SP3
Browser : Google Chrome Version 49.0.2623.112 m
Adblock Plus version 1.13.1

How to reproduce

  1. Add Mapx's broken filter subscription http://axistrivia.altervista.org/list/easylist2.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.

Change History (36)

comment:1 Changed 6 months ago by kzar

  • Cc kzar trev sebastian Ross scheer rraceanu added
  • Component changed from Unknown to Platform

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?

comment:2 Changed 6 months ago by 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.

comment:3 Changed 6 months ago by mapx

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

comment:4 Changed 6 months ago by mapx

  • Cc mapx added

comment:5 Changed 6 months ago by trev

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

Last edited 6 months ago by trev (previous) (diff)

comment:6 Changed 6 months ago by 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.

Last edited 6 months ago by wspee (previous) (diff)

comment:7 Changed 6 months ago by wspee

  • Cc wspee added

comment:8 Changed 6 months ago by 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.

comment:9 Changed 6 months ago by 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.

comment:10 Changed 6 months ago by 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

comment:11 Changed 6 months ago by mapx

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

comment:12 Changed 6 months ago by 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.

comment:13 follow-ups: Changed 6 months ago by 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.

Last edited 6 months ago by sebastian (previous) (diff)

comment:14 follow-up: Changed 6 months ago by 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".

comment:15 in reply to: ↑ 13 Changed 6 months ago by 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.

comment:16 in reply to: ↑ 14 Changed 6 months ago by 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.

comment:17 Changed 6 months ago by 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.

Last edited 6 months ago by mapx (previous) (diff)

comment:18 Changed 6 months ago by mapx

the wrong filter in the list easylist2

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

comment:19 in reply to: ↑ 13 Changed 6 months ago by 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.

comment:20 Changed 6 months ago by 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.

comment:21 Changed 6 months ago by 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.

comment:22 follow-up: Changed 6 months ago by 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.

comment:23 in reply to: ↑ 22 Changed 6 months ago by 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.

Last edited 6 months ago by mapx (previous) (diff)

comment:24 follow-up: Changed 6 months ago by 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.

Last edited 6 months ago by sebastian (previous) (diff)

comment:25 Changed 6 months ago by 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.

Last edited 6 months ago by mapx (previous) (diff)

comment:26 Changed 6 months ago by kzar

  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing
  • Summary changed from Personal filters not working anymore to Element hiding completely breaks if there are any syntax errors in element hiding filters

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.

comment:27 Changed 6 months ago by kzar

  • Owner set to kzar

comment:28 Changed 6 months ago by 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.

Last edited 6 months ago by sebastian (previous) (diff)

comment:29 follow-up: Changed 6 months ago by 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.

comment:30 in reply to: ↑ 24 Changed 6 months ago by sebastian

Last edited 6 months ago by sebastian (previous) (diff)

comment:31 in reply to: ↑ 29 Changed 6 months ago by 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).

Last edited 6 months ago by sebastian (previous) (diff)

comment:32 Changed 6 months ago by 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.

Last edited 6 months ago by mapx (previous) (diff)

comment:33 follow-up: Changed 6 months ago by 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.

comment:34 in reply to: ↑ 33 Changed 6 months ago by 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().

comment:35 Changed 6 months ago by greiner

  • Cc greiner added

comment:36 Changed 5 months ago by kzar

  • Owner kzar deleted

Unassigning myself from this for now.

Note: See TracTickets for help on using tickets.