Opened on 12/19/2018 at 06:10:54 AM

Closed on 12/21/2018 at 01:36:07 PM

Last modified on 03/12/2019 at 03:12:01 AM

#7178 closed change (fixed)

Remove filter order randomization

Reported by: mjethani Assignee: mjethani
Priority: P3 Milestone:
Module: Core Keywords:
Cc: kzar, sergz, hfiguiere, greiner, sebastian Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

Description (last modified by mjethani)


In #4067 the order of all the filters was randomized. This was done mainly for element hiding so the website could ostensibly not read the "filter IDs" on Firefox (I have never understood this).

Anyway, in #6562 we stopped using filter keys for element hiding filters. This doesn't change the fact that the order in which the CSS selectors appear is deterministic (insertion order), which means the randomization in lib/filterListener.js would still make sense. Also, some Firefox-specific functionality for element hiding was removed in #4140.

Given that we now use user style sheets on Firefox (#5090) and the website has no way of reading this style sheet, it should be safe to remove the randomization, whatever its original purpose.

What to change

In lib/filterListener.js, add filters in the same order in which they occur in the subscription, without any randomization.

Hints for testers

Make sure of the following:

  1. When a subscription is added, all the filters in the subscription get added and become active (i.e. if there's a filter then it should hide the element <div class="foo">Ad</div> on the page)
  2. When a subscription is disabled and then reenabled, all the filters in the subscription should become active again
  3. When a subscription is updated, any new filters added to the subscription should become active. e.g. let's say a subscription is loaded remotely from and it contains at first, then you add the filter to it and then manually update the subscription on the options page; the new filter should hide the element <div class="bar">Ad</div> on the page (after refreshing the page of course)
  4. Any user-defined filter that is added should become active, i.e. it should work on the page

Attachments (0)

Change History (14)

comment:1 Changed on 12/19/2018 at 06:14:05 AM by mjethani

What do you think, Dave?

comment:2 Changed on 12/19/2018 at 06:44:08 AM by mjethani

  • Description modified (diff)

comment:3 Changed on 12/19/2018 at 10:04:28 AM by kzar

Sounds OK to me, Manish, but I was not the mastermind behind the randomisation. The best person to ask would be Wladimir, alternatively I recommend having a look back through the commit history of the adblockplus repository for clues about why the randomisation was necessary.

Last edited on 12/19/2018 at 10:04:47 AM by kzar

comment:4 Changed on 12/19/2018 at 02:17:20 PM by mjethani

Thanks, Dave. I asked you because you reviewed the change, I figured you'd have some background (but you've probably forgotten because it was a long time ago).

Even if I were to go through the entire commit history, I would not really understand why this randomization was done. But I can try.

On the other hand, given how things work now (with user style sheets and everything), I can't think of any reason for the filter order to be randomized. If no one here has any ideas, let's just go ahead and simplify this code.

comment:5 Changed on 12/19/2018 at 03:43:20 PM by kzar

Yea, Manish, I remember reviewing that one.

I checked the code worked like he specified in the issue, but I didn't fully understand why the randomisation was necessary. Or, perhaps I asked him in IRC at the time, rather than in the code review, I can't remember! Like you say it was some time ago!

To understand why the randomisation was necessary in the first place we'd have to look through the adblockplus history, or ask Wladimir himself. The change you linked was not where the randomisation was originally introduced, but rather a necessary change for the getSelectorsByDomain optimisations I was working on at the time.

Sorry I couldn't be more help, FWIW going ahead with this sounds OK to me.

comment:6 Changed on 12/19/2018 at 09:06:09 PM by mjethani

  • Review URL(s) modified (diff)

comment:7 Changed on 12/19/2018 at 09:06:31 PM by mjethani

  • Priority changed from Unknown to P3
  • Ready set

comment:8 Changed on 12/19/2018 at 09:06:51 PM by mjethani

  • Owner set to mjethani

comment:9 Changed on 12/19/2018 at 09:06:58 PM by mjethani

  • Status changed from new to reviewing

comment:10 Changed on 12/19/2018 at 10:38:21 PM by sebastian

I don't get all the details together either, but the randomization had something to do with how information of user stylesheets got leaked in legacy Gecko extensions. It should no longer be necessary for Web Extensions.

comment:11 Changed on 12/21/2018 at 01:27:02 PM by abpbot

A commit referencing this issue has landed:
Issue 7178 - Remove filter order randomization

comment:12 Changed on 12/21/2018 at 01:36:07 PM by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:13 Changed on 02/07/2019 at 03:23:57 AM by abpbot

A commit referencing this issue has landed:
Issue 7178 - Remove filter order randomization

comment:14 Changed on 03/12/2019 at 03:12:01 AM by rscott

  • Verified working set

Add Comment

Modify Ticket

Change Properties
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.