Opened on 06/22/2017 at 09:52:15 AM

Closed on 07/12/2017 at 09:26:31 AM

Last modified on 08/04/2017 at 06:48:59 AM

#5345 closed defect (fixed)

[abp2blocklist] $elemhide not working on walmart.com

Reported by: mjethani Assignee: mjethani
Priority: P4 Milestone:
Module: Platform Keywords: abp2blocklist, Acceptable Ads, externaldependency
Cc: sebastian, kzar, mario Blocked By:
Blocking: #5464 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29473555/

Description (last modified by mjethani)

Environment

Safari 10.1.1 on macOS 10.12.5 using Adblock Plus built from the safari bookmark (changeset 3e423a0ad97f)

How to reproduce

  1. Go to the Options page enable Safari's native content blocking
  2. In the filter list, select EasyList and "Allow some non-intrusive advertising"
  3. Go to https://www.walmart.com/search/?query=car%20rental%20cologne
  4. Scroll to the bottom and wait a few seconds

Observed behaviour

Sponsored links are not displayed

Expected behaviour

Sponsored links should be displayed because walmart.com is whitelisted

Notes

The following filter hides the ad:

##.sponsored-container-bottom

Then the following filter is supposed to prevent the previous one from hiding the ad:

@@||walmart.com^$elemhide

It turns out that this is a known issue in WebKit.

Attachments (0)

Change History (13)

comment:1 Changed on 06/22/2017 at 09:59:10 AM by mjethani

I created a rule set with just two rules to verify that this is broken:

{
  "trigger": {
    "url-filter": "^https?://"
  },
  "action": {
    "type": "css-display-none",
    "selector": ".sponsored-container-bottom"
  }
},
{
  "trigger": {
    "url-filter": "^https?://([^/]+\\.)?walmart\\.com"
  },
  "action": {
    "type": "ignore-previous-rules"
  }
}

Yep, it does not work. It's not ignoring the previous rule.

After some more analysis, I feel like this has something to do with the "specificity" (if there is such a thing for content blocker rules) of the url-filter field. The first one is more generic, it seems to be taking precedence! If I make the second one equally generic (^https?://) then it works. If the values are identical, it works. If the second one is more generic (simply swap the values), it works. I'm not one hundred percent sure what's going on, but I'm investigating.

Also, using if-domain instead of url-filter in the second rule works too. Maybe this is just a bug in Safari, and we might have to use this workaround.

By the way, this will affect $generichide and $genericblock as well.

comment:2 Changed on 06/23/2017 at 11:53:22 AM by mjethani

It turns out that this is a known issue.

comment:3 Changed on 06/23/2017 at 11:54:39 AM by mjethani

  • Description modified (diff)

comment:4 Changed on 06/23/2017 at 11:58:03 AM by kzar

Ah bugger. In that case please add the externaldependency keyword to this issue.

comment:5 Changed on 06/23/2017 at 12:48:48 PM by mjethani

  • Keywords abp2blocklist Acceptable Ads externaldependency added; ab removed

comment:6 Changed on 06/23/2017 at 01:06:35 PM by mjethani

It seems that Safari creates a separate stylesheet for "generic" rules (ones that apply to all pages) and then does not support exceptions for those rules, unless the exceptions too are generic.

From the blog post (emphasis added):

When compiling the rules, we group the CSS rules whenever we determine they will be used together. For example, if a set of rules applies on every page (by using the filter “.*”), a special stylesheet is prepared for them to be ready to use them instantly when the page has loaded.

When a “ignore-previous-rules” appears, it forces the compiler to break the stylesheet since the rules appearing before an action “ignore-previous-rules” are all dismissed when the action is activated.

I tried to fool Safari into treating the generic rule as non-generic by replacing /^https?:\/\// with /^https?:\/\/[-A-Za-z0-9._~:\/?#[\]@!$&'()*+,;=`]+$/ (valid URL characters based on RFC 3986). This did not work.

Our only option then is to make the exception too generic by using the if-domain field instead. This will unfortunately give us false negatives (since we're whitelisting the entire domain), but we might be able to fix this by using if-top-url instead. It will also continue to give us false positives inside of frames.

comment:7 Changed on 06/24/2017 at 02:54:55 PM by mjethani

  • Review URL(s) modified (diff)

comment:8 Changed on 07/11/2017 at 04:16:35 PM by kzar

  • Priority changed from Unknown to P4
  • Ready set

comment:9 Changed on 07/12/2017 at 09:24:49 AM by abpbot

comment:10 Changed on 07/12/2017 at 09:26:31 AM by mjethani

  • Resolution set to fixed
  • Status changed from new to closed

comment:11 Changed on 07/15/2017 at 10:11:23 AM by mjethani

The WebKit issue has been closed as "invalid". I've left a comment explaining the Adblock Plus issue. Meanwhile this is resolved for now using a workaround.

comment:12 Changed on 08/02/2017 at 06:30:08 AM by mjethani

  • Blocking 5464 added

comment:13 Changed on 08/04/2017 at 06:48:59 AM by mjethani

I've reported a new issue in WebKit: #175177

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from mjethani.
 
Note: See TracTickets for help on using tickets.