Opened 7 months ago

Closed 7 months ago

Last modified 4 months ago

#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):

https://codereview.adblockplus.org/29965593/

Description (last modified by mjethani)

Background

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 ##.foo 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 https://example.com/foo.txt and it contains ##.foo at first, then you add the filter ##.bar 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

Change History (14)

comment:1 Changed 7 months ago by mjethani

What do you think, Dave?

comment:2 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 7 months ago 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 7 months ago by kzar (previous) (diff)

comment:4 Changed 7 months ago 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 7 months ago 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 7 months ago by mjethani

  • Review URL(s) modified (diff)

comment:7 Changed 7 months ago by mjethani

  • Priority changed from Unknown to P3
  • Ready set

comment:8 Changed 7 months ago by mjethani

  • Owner set to mjethani

comment:9 Changed 7 months ago by mjethani

  • Status changed from new to reviewing

comment:10 Changed 7 months ago 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 7 months ago by abpbot

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

comment:12 Changed 7 months ago by mjethani

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

comment:13 Changed 5 months ago by abpbot

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

comment:14 Changed 4 months ago by rscott

  • Verified working set
Note: See TracTickets for help on using tickets.