Opened 12 months ago

Last modified 5 weeks ago

#5571 new defect

[abp2blocklist] Safari Technology Preview throws an error if *-domain is mixed with *-top-url

Reported by: mjethani Assignee: mjethani
Priority: P2 Milestone:
Module: Adblock-Plus-for-iOS/macOS Keywords: abp2blocklist, Acceptable Ads
Cc: sebastian, kzar, mario, BrentM Blocked By:
Blocking: Platform: Safari
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Environment

Safari Technology Preview Release 38 (Safari 11.1, WebKit 12605.1.3.1)
Adblock Plus for Safari based on the current safari bookmark (5e895ee5aee1)
macOS 10.12.5
EasyList+AA

How to reproduce

  1. Go into the options page
  2. Select EasyList+AA in your subscriptions
  3. Enable Safari's native content blocking

Observed behaviour

Error: "Rule list compilation failed: A list cannot have if-domain and unless-domain mixed with if-top-url and unless-top-url"

Expected behaviour

No error.

Notes

The Safari developers seem to have confirmed that this is intentional and not a bug in Safari. It is not allowed to mix the *-domain properties with the *-top-url properties in the same list, as we are doing.

This only affects desktop Safari for now, but it is very likely going to affect iOS as well because this restriction is most likely being imposed for performance reasons.

Change History (18)

comment:1 Changed 12 months ago by mjethani

Now we have the following options:

  1. Drop unless-top-url and accept some more false positives
  2. Use unless-domain instead of unless-top-url, which would effectively make it like $third-party
  3. Generate separate lists for Safari 11+ and older versions: always use *-top-url (use regular expressions) in Safari 11+ and never otherwise

The last one would allow us to improve rule generation on Safari 11+

comment:2 Changed 12 months ago by mjethani

  • Cc mario added

comment:3 follow-up: Changed 12 months ago by kzar

My choice would be option 3, but if we decide the effort of having two lists isn't worth it I'd go with option 2. I don't think accepting more false positives is an option, those cause us a lot of trouble.

comment:4 Changed 12 months ago by kzar

  • Platform changed from Unknown / Cross platform to Safari
  • Priority changed from Unknown to P3
  • Ready set

comment:5 Changed 11 months ago by sebastian

  • Priority changed from P3 to P4

comment:6 Changed 11 months ago by mjethani

  • Cc BrentM added

comment:7 Changed 11 months ago by sebastian

  • Priority changed from P4 to P3

comment:8 follow-ups: Changed 11 months ago by sebastian

Safari 11 came out a week ago. Did this limitation make it into the release version? If so we might want to prioritize this issue.

comment:9 in reply to: ↑ 8 Changed 10 months ago by cakefoo

Replying to sebastian:

Safari 11 came out a week ago. Did this limitation make it into the release version? If so we might want to prioritize this issue.

In Safari 11 the observed behavior is Error: "Rule list compilation failed: Too many rules in JSON array."

comment:10 Changed 10 months ago by sebastian

This is an unrelated issue. Ever since there were only up to 50k rules allowed for Content Blockers. Are you using Adblock Plus for Safari on macOS? Then you should see a warning telling you to reduce the number of filter lists.

comment:11 in reply to: ↑ 8 Changed 10 months ago by mjethani

Replying to sebastian:

Safari 11 came out a week ago. Did this limitation make it into the release version? If so we might want to prioritize this issue.

Confirmed.

When if-domain and if-top-url are used together, you get an error: "Rule list compilation failed: A trigger cannot have more than one condition (if-domain, unless-domain, if-top-url, or unless-top-url)"

comment:12 Changed 10 months ago by sebastian

  • Priority changed from P3 to P2

comment:13 Changed 7 months ago by iOSAlookBrowser

I meet the same problem. Can anyone fix this problem please?
Best Regards.

comment:14 in reply to: ↑ 3 Changed 7 months ago by mjethani

Replying to kzar:

My choice would be option 3, but if we decide the effort of having two lists isn't worth it I'd go with option 2. I don't think accepting more false positives is an option, those cause us a lot of trouble.

Unfortunately even with option 2 we'll still get some false positives. In the unit tests there are three cases: /foo, ||test.com, and http://example.com/foo. We can only use unless-domain for the second and third cases. There is no way around this.

On the other hand it seems that Safari 11 adoption has really picked up.

comment:15 Changed 7 months ago by iOSAlookBrowser

iOS 11 (with Safari 11) have been increase to 65%.

comment:16 Changed 2 months ago by mjethani

  • Milestone set to Adblock-Plus-for-Safari-next

comment:17 Changed 5 weeks ago by sebastian

  • Component changed from Platform to Adblock-Plus-for-iOS

comment:18 Changed 5 weeks ago by sebastian

  • Milestone Adblock-Plus-for-Safari-next deleted
Note: See TracTickets for help on using tickets.