Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#2503 closed defect (fixed)

Inconsistent behavior: $document flag implied for exception rules with protocol included

Reported by: passbrains Assignee: trev
Priority: P2 Milestone: Adblock-Plus-2.6.10-for-Firefox
Module: Core Keywords:
Cc: trev, sebastian Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/5672992042385408/
http://codereview.adblockplus.org/5971196453584896/

Description (last modified by trev)

Adapted from https://www.passbrains.com/dashboard/view-ticket.php?ticket_no=MAX-90

Environment

Windows + 8.1 64bit + maxthon + English

How to reproduce

  1. open the browser and go to a website such as http://www.amazon.com/
  1. click the icon ABP
  1. choose Custom filters or Adblock Plus options
  1. go to Add your own filters
  1. enter a filter i.e. @@http://www.amazon.com/
  1. Click Done and reload the page.
  1. Note the result in Maxthon ( ABP icon is still enabled and blocking some items)
  1. Open a Chrome browser and enter the same filter .
  1. Note the result in Chrome ( ABP icon is disabled and not blocking items.)

Observed behaviour

The result in Maxthon is different from that in Chrome.

Expected behaviour

The result in Maxthon shall be the same as in Chrome.

Background

We still have legacy code implying $document filter option for exception rules that have a protocol. That backwards compatibility code predates filter options.

What to change

Drop this backwards compatibility code, the format for document-wide exception rules has been @@||example.com^$document for at least five years now.

Attachments (1)

5162_1429345509_filter-exception-rule.doc (1.4 MB) - added by passbrains 5 years ago.

Download all attachments as: .zip

Change History (13)

Changed 5 years ago by passbrains

comment:1 Changed 5 years ago by vickyyu

  • Description modified (diff)
  • Priority changed from Unknown to P2

comment:2 Changed 5 years ago by mapx

  • Description modified (diff)

comment:3 Changed 5 years ago by arthur

  • Description modified (diff)

comment:4 Changed 5 years ago by arthur

  • Cc trev added

I'm not sure if that is a bug in Maxthon, I rather think it's not correct behaviour in Chrome.

Wladimir, what do you think?

comment:5 Changed 5 years ago by trev

  • Cc sebastian added
  • Component changed from Unknown to Platform

Yes, this seems to be an Adblock Plus/Chrome bug - @@http://www.amazon.com/ doesn't apply to documents, consequently it shouldn't whitelist www.amazon.com. I cannot see the problem immediately, isPageWhitelisted() does check for DOCUMENT flag correctly.

comment:6 Changed 5 years ago by trev

  • Platform changed from Maxthon to Chrome

comment:7 Changed 5 years ago by sebastian

  • Component changed from Platform to Core

Seems like following code in RegExpFilter.fromText() is causing this behavior:

  if (!blocking && (contentType == null || (contentType & RegExpFilter.typeMap.DOCUMENT)) &&
      (!options || options.indexOf("DOCUMENT") < 0) && !/^\|?[\w\-]+:/.test(text))
  {
    // Exception filters shouldn't apply to pages by default unless they start with a protocol name
    if (contentType == null)
      contentType = RegExpFilter.prototype.contentType;
    contentType &= ~RegExpFilter.typeMap.DOCUMENT;
  }

So this seems to be the intended behavior. But I wonder why it doesn't work in Firefox.

comment:8 Changed 5 years ago by trev

  • Description modified (diff)
  • Ready set
  • Summary changed from Some filter ( exception rule ) is not working the same as in Chrome to Inconsistent behavior: $document flag implied for exception rules with protocol included

You are right, Filter.fromText("@@http://www.amazon.com/").matches("http://www.amazon.com/", "DOCUMENT", "amazon.com", "false", null) returns true both in Firefox and Chrome. This is legacy code predating filter options, we should remove it.

It actually works the same in Firefox - my bad, looks like I tested on amazon.de.

comment:9 Changed 5 years ago by trev

  • Owner set to trev

comment:10 Changed 5 years ago by trev

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

comment:11 Changed 5 years ago by trev

  • Milestone set to Adblock-Plus-for-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

Fixed:

https://hg.adblockplus.org/adblockplus/rev/cc3f3887226a
https://hg.adblockplus.org/adblockplustests/rev/40ab9cba8260

This only changes the behavior for Firefox, the change will be added to Chrome later with a dependency update.

comment:12 Changed 5 years ago by sebastian

For reference, this change seems to break notifications in the current devbuild, see #2641.

Note: See TracTickets for help on using tickets.