Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#1210 closed defect (fixed)

Whitelisted AFS ads are not showing up

Reported by: fhd Assignee: oleksandr
Priority: P2 Milestone: Adblock-Plus-for-Internet-Explorer-1.3
Module: Adblock-Plus-for-Internet-Explorer Keywords:
Cc: oleksandr, arthur, serggzz@… Blocked By: #1265, #1356
Blocking: Platform: Internet Explorer
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/4881420963020800/

Description (last modified by trev)

Environment

Adblock Plus for IE 1.2.690 with EasyList Germany and Acceptable Ads enabled.

How to reproduce

  1. Go to netzwelt.de
  2. Search for any term

Observed behaviour

The search ads are not showing up.

Expected behaviour

The search ads should show up.

Hints for testers

The change implemented here might affect other websites in unexpected ways, in particular by causing legitimate content to be blocked even though things worked correctly before. The chances that this actually happens seem to be rather low however.

Attachments (1)

izito_IE.png (169.5 KB) - added by arthur 5 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 5 years ago by fhd

  • Cc oleksandr added
  • Resolution set to fixed
  • Review URL(s) modified (diff)
  • Status changed from new to closed

This was committed without issue in 1.2: https://hg.adblockplus.org/adblockplusie/rev/7020c275e744

We should reland this referencing this issue.

comment:2 Changed 5 years ago by fhd

  • Milestone set to Adblock-Plus-for-Internet-Explorer-1.2

comment:3 Changed 5 years ago by trev

  • Description modified (diff)
  • Owner set to oleksandr

comment:4 Changed 5 years ago by trev

  • Resolution fixed deleted
  • Status changed from closed to reopened

The original commit was backed out in https://hg.adblockplus.org/adblockplusie/rev/b241d73f074d. Please reland with proper issue reference.

comment:5 Changed 5 years ago by sergz

The commit https://hg.adblockplus.org/adblockplusie/rev/7020c275e744 introduces another trouble.
It allows a lot of ads, which were blocked before.

Env

Acceptable ads is disabled.

How to reproduce

  1. Go to http://seasonvar.ru/
  2. Open any serial page

Observed behaviour

A lot of ads is shown, check the left side.

Expected behaviour

The ads should be blocked.

comment:6 Changed 5 years ago by fhd

That's bad, seems like that commit Wladimir reverted did have side effects :(

Can you look into this Sergey? If this is really the cause of Acceptable Ads showing up when they have been disabled, we should definitely find a proper solution quickly - or maybe just make a 1.2 fix release that reverts this.

comment:7 Changed 5 years ago by arthur

I cannot reproduce this. Are you using RU AdList+EasyList, Sergey?

comment:8 Changed 5 years ago by arthur

  • Cc arthur added

comment:9 Changed 5 years ago by oleksandr

I don't see any ads there either with RU AdList + EasyList or just plain EasyList (IE11, Windows 8.1). Works as expecteed.

comment:10 Changed 5 years ago by arthur

With EasyList only I only get the sometimes.

Changed 5 years ago by arthur

comment:11 Changed 5 years ago by arthur

Regarding #1210:

In the screenshot you can see how izito.de currently looks for me with ABP 1.2 on IE 11.

It looks like this hiding filter ##a[href^="http://www.google.com/aclk?"] is still being applied in the ad iframe although it shouldn't:

  1. The responsible whitelist for the ad iframe (@@||google.com/uds/afs$document,subdocument,domain=zoeken.nl) should make this sure via the $document option but apparently doesn't.
  2. Even if the $document option doesn't work, this element hiding whitelist in EasyList should show it: www.google.com#@#a[href^="http://www.google.com/aclk?"]

comment:12 Changed 5 years ago by sergz

After discussing it in IRC some things are clear. Seems it work, I need to perform additional testing.

Despite of it I would clarify the code, I cannot find the further usage of CFilter::contentTypeAny & ~CFilter::contentTypeSubdocument; which looks like here was an idea but then something changed. Do I correctly understand that we actually should return CFilter::contentTypeAny (which is 216-1 which is a union of all other types) without subtracting any type. We might can in theory return something like CFilter::contentTypeXmlHttpRequest & CFilter::contentTypeDocument (BTW, xmlhttprequest refers to requests started by the XMLHttpRequest object, not a request which queries for xml file, but it's another issue) and feed it to the matcher via serializing to the string like "xmlhttprequest,document". For CFilter::contentTypeAny the content type string would include all types. The matcher firstly checks whether some of these types is in the white list, if so return <not-blocked, filter-entry>, otherwise check the blocking entries. As I understand now it works similar but expects only one type in the contentType.
What do you think about it, what was the aim of CFilter::contentTypeAny, why do we subtract CFilter::contentTypeSubdocument and what happened with the idea?

comment:13 Changed 5 years ago by sergz

  • Cc serggzz@… added

comment:14 Changed 5 years ago by oleksandr

  • Blocked By 1265 added

comment:15 Changed 5 years ago by sergz

  • Blocked By 1356 added

comment:16 Changed 5 years ago by oleksandr

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

comment:17 Changed 5 years ago by oleksandr

  • Milestone changed from Adblock-Plus-1.2-for-Internet-Explorer to Adblock-Plus-for-Internet-Explorer-1.3

comment:18 Changed 5 years ago by oleksandr

This issue was fixed, since issues that were blocking it are fixed now. Current issue was just a meta-issue for those. However this one had real world manifestations, and could be observed by the user, unlike the underlying issues.

Note: See TracTickets for help on using tickets.