Opened 13 months ago

Last modified 7 months ago

#6091 reviewing defect

"Hide targeted messages?" notification shown for websites which don't use targeted messaging

Reported by: kzar Assignee: jsonesen
Priority: P2 Milestone:
Module: User-Interface Keywords:
Cc: mapx, jsonesen, sebastian, mjethani, Ross, greiner, arthur, SMed79 Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29785581/

Description (last modified by kzar)

Background

We're getting multiple reports from users (and some of the team) that the "Hide targeted messages?" notification is showing up whenever they open a new page.

Environment

Chrome (62.0.3202.94), Windows 10 OR Ubuntu 16.04 LTS
Adblock Plus 1.13.4 installed in a fresh Chrome profile

How to reproduce

  1. Browse to https://github.com OR ​https://happyasis.com/ OR many other websites

Observed behaviour

"Hide targeted messages?" notification is displayed.

Expected behaviour

The notification should only show up for websites that do use targeted messaging.

Notes

  • User reports: topic 54550, topic 54551, topic 54555.
  • Earliest report of the problem is yesterday (Thursday 23rd November) afternoon German time, but we're not sure when exactly the problem begun.
  • Relevant code is in adblockplusui/lib/antiadblockInit.js and string is notification_antiadblock_message.
  • Likely cause is this antiadblockfilters change which added the filter com,fr,info##div#ab-message[style^="display: block;"].

What to change

Adjust the logic in the addAntiAdblockNotification function in adblockplusui/lib/antiadblockInit.js to avoid overly generic filters from triggering the antiadblock notification. Currently filters with a domain specified trigger the notification, but we should also check that the domain is not a TLD like ".com" nor in the publicSuffixList.

Change History (40)

comment:1 Changed 13 months ago by kzar

  • Description modified (diff)

comment:2 Changed 13 months ago by arthur

  • Cc greiner added

I can reproduce it with a fresh Chrome (62.0.3202.94 (Official Build) (64-bit)) browser profile on Windows 10, installed ABP from https://adblockplus.org/. When I go to google.com, I'm redirected to google.de but I can click on "use Google.com" in the bottom right corner. The message appears there for me. Also when I just go to https://happyasis.com/.

comment:3 Changed 13 months ago by greiner

Note that this issue doesn't occur in the latest development build but can be reproduced using the latest stable build and going to github.com, for instance.

comment:4 Changed 13 months ago by kzar

  • Cc arthur added
  • Description modified (diff)

Thanks guys, I've updated the description to add those steps.

comment:5 Changed 13 months ago by kzar

  • Description modified (diff)

comment:6 Changed 13 months ago by kzar

  • Description modified (diff)

comment:7 Changed 13 months ago by kzar

  • Description modified (diff)

comment:8 follow-up: Changed 13 months ago by Ross

I haven't reproduced this yet. I can trigger the message but it only appears once, not matter if I answer yes or no.

ABP 1.13.4
Chrome 62 / Windows 10

comment:9 Changed 13 months ago by kzar

  • Description modified (diff)

comment:10 in reply to: ↑ 8 Changed 13 months ago by greiner

Replying to Ross:

I haven't reproduced this yet. I can trigger the message but it only appears once, not matter if I answer yes or no.

The issue is not that it reappears. In fact, you're describing the expected behavior because it's only supposed to reappear if you close the popup without answering the question.

The issue is that the message appears even on sites which don't contain any anti-adblock messages.

Last edited 13 months ago by greiner (previous) (diff)

comment:11 Changed 13 months ago by kzar

  • Description modified (diff)
  • Summary changed from "Hide targeted messages" notification showing up everywhere to "Hide targeted messages?" notification shown for websites which don't use targeted messaging

comment:12 Changed 13 months ago by trev

I started seeing this message myself in Thunderbird, on the xkcd.com RSS feed. I am running Adblock Plus 2.9.1.4260-beta which is quite old (released on August 22). So it's most likely a change in the Anti-Adblock list that triggered this issue.

comment:13 Changed 13 months ago by trev

Found the filter causing this:

com,fr,info##div#ab-message[style^="display: block;"]

Added here: https://hg.adblockplus.org/antiadblockfilters/rev/189f8049d8d9

comment:14 Changed 13 months ago by mapx

  • Cc SMed79 added

comment:15 Changed 13 months ago by mapx

yep, I asked hours ago smed79 why that filter ..

comment:16 Changed 13 months ago by mapx

However, even if the wrong filter but the users disabled the list and still got that message ?!

comment:17 Changed 13 months ago by trev

The messages show up only if the filter list is disabled - the point of the message is asking you whether you want to enable it.

comment:18 Changed 13 months ago by kzar

  • Description modified (diff)

comment:19 Changed 13 months ago by mapx

Well, it's one of those features I'd remove immediately (like the total ads blocked over the time)

comment:20 Changed 13 months ago by kzar

  • Component changed from Platform to User-Interface
  • Description modified (diff)

Assuming we don't remove the feature entirely I've added a suggestion of what we could do to avoid this kind of problem happening in the future.

comment:21 Changed 13 months ago by kzar

  • Description modified (diff)

comment:22 Changed 13 months ago by trev

For reference, the problematic rule has been removed: https://hg.adblockplus.org/antiadblockfilters/rev/5a6e5f1bb887. Nothing wrong with preventing this kind of issue in future however.

comment:23 follow-up: Changed 13 months ago by rhana@…

We started getting these reports on Tuesday or Wednesday.

Will refreshing the filter lists fix this?

comment:24 in reply to: ↑ 23 Changed 13 months ago by mapx

Replying to rhana@…:

We started getting these reports on Tuesday or Wednesday.

Will refreshing the filter lists fix this?

Yes.

comment:25 Changed 13 months ago by rhana@…

Oh, thank God. Thanks, mapx!

comment:26 Changed 13 months ago by jsonesen

  • Owner set to jsonesen

comment:27 Changed 12 months ago by fhd

  • Cc trev removed

comment:28 Changed 11 months ago by greiner

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

Resolving this as invalid since it turned out to be a filter issue.

comment:29 Changed 11 months ago by kzar

I don't think this should be resolved as invalid since although this is triggered by a filter the antiadblockInit code can prevent showing notifications for overly broad filters like these. (The description already explains what to change as well.)

comment:30 Changed 11 months ago by kzar

It's hardly unimportant we do something to avoid this happening again and also Jon apparently already started working on it.

comment:31 Changed 11 months ago by greiner

  • Resolution invalid deleted
  • Status changed from closed to reopened

Sorry, didn't notice the text in the "What to change" section. From the conversation it sounded like that fix would be done separately.

comment:32 Changed 11 months ago by greiner

  • Priority changed from Unknown to P2
  • Ready set

comment:33 follow-up: Changed 9 months ago by jsonesen

Hey guys,

Once I saw the ticket close a couple months ago I pretty much just moved on mentally. However, I just noticed it was reopened (my bad) I tried adding the filter to ABP but it doesn't cause the behavior to occur. Sort of stumped on how to reproduce this. Any ideas?

comment:34 in reply to: ↑ 33 ; follow-up: Changed 9 months ago by greiner

Replying to jsonesen:

Once I saw the ticket close a couple months ago I pretty much just moved on mentally. However, I just noticed it was reopened (my bad) I tried adding the filter to ABP but it doesn't cause the behavior to occur.

The issue itself is triggered by overly broad filters in the filter list which were removed in this filter list change. That's why you don't encounter the issue in the wild anymore.

The change that's proposed here, however, is meant to prevent that same issue from reappearing in case such filters make it into the filter list again.

Sort of stumped on how to reproduce this. Any ideas?

You could create a local build that uses a fake filter list instead of antiadblockfilters.txt in which you include the filter that caused this issue.

comment:35 in reply to: ↑ 34 ; follow-up: Changed 9 months ago by jsonesen

Sort of stumped on how to reproduce this. Any ideas?

You could create a local build that uses a fake filter list instead of antiadblockfilters.txt in which you include the filter that caused this issue.

Thanks, yeah I added it to the custom filter in ABP options but that had no effect. I will add my own antiadblockfilters.txt for testing.

comment:36 in reply to: ↑ 35 ; follow-up: Changed 9 months ago by greiner

Replying to jsonesen:

You could create a local build that uses a fake filter list instead of antiadblockfilters.txt in which you include the filter that caused this issue.

Thanks, yeah I added it to the custom filter in ABP options but that had no effect. I will add my own antiadblockfilters.txt for testing.

As I mentioned, you need to fake the antiadblockfilters.txt filter list so adding custom filters won't do anything.

Alternatively, you could add your own local notification using addNotification() from adblockpluscore/lib/notification.js and passing it a notification that has the "urlFilters" property which is an array that contains filter in text form that look like "||" + domain + "^$document" (see adblockplusui/lib/antiadblockInit.js).

comment:37 in reply to: ↑ 36 Changed 9 months ago by jsonesen

Replying to greiner:

As I mentioned, you need to fake the antiadblockfilters.txt filter list so adding custom filters won't do anything.

Yeah, I was just giving an example of what I had tried. Contextually I realize now that would have been better in the first comment hehe my bad. Thanks for providing options though!

comment:38 Changed 7 months ago by jsonesen

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:39 Changed 7 months ago by kzar

  • Blocked By 6690 added

comment:40 Changed 7 months ago by kzar

  • Blocked By 6690 removed
Note: See TracTickets for help on using tickets.