Lose unnecessary references to filter objects in subscription objects

[mjethani]

Background

The next step for optimizing the memory usage of Adblock Plus is to make temporary Filter objects, i.e. objects that are created, used, and discarded. In order to do this, we must first lose the unnecessary references to Filter objects that exist in other areas. The only place where Filter objects should be stored permanently is the Filter.knownFilters map.

Right now there are not-exactly-necessary references to Filter objects in at least two places:

1. The filters property of the Subscription class. We can change this property to keep the filter text instead and then convert it to a Filter object when required using Filter.fromText
2. The filtersBySelector map in lib/elemHide.js. This doesn't need to keep references to the Filter objects either, it could just store the selector and get the Filter object using Filter.fromText

We can start with the first one.

What to change

See patches submitted so far in the review URLs field.

Integration notes

With changeset ​b0170eda8137, the public filters property (array) of the Subscription class is replaced by the following:

• A filters generator method that yields Filter objects representing filters in the subscription
• A filterCount property that returns the number of filters in the subscription
• Similar to the filters generator method, a filterText generator method that yields the text property of the Filter objects
• A filterAt(index) method that returns the Filter object at a given index
• addFilter(filter), insertFilterAt(filter, index), deleteFilterAt(index), and clearFilters methods for adding and removing filters to and from the subscription
• A searchFilter(filter, fromIndex = 0) method that returns a matching Filter object in the subscription (based on the text property of the first parameter)

Please see the JSDoc documentation for the Subscription class for more details.

With changeset ​b0170eda8137, the filters() generator in the Subscription class is replaced with a filterText() generator that yields the text of the filter. The text can be converted into a Filter object using the Filter.fromText() function wherever needed. Also the filterAt() method has been replaced with filterTextAt(), which also now returns the text of the filter.

Hints for testers

Test that filters in a subscription are loaded correctly. For example, the filter /foo^ should block a request to the URL https://example.com/foo/bar (see the DevTools panel and the console to verify this). If a subscription is disabled, the filters in the subscription should no longer work (except those filters that also belong to another subscription that is not disabled). If a subscription is reenabled, the filters in the subscription should work again. Similarly for when a subscription is deleted and then re-added.

Add a custom (user-defined) filter and see that it works. For example, the filter ###foo should hide the element <div id="foo">Test</div>. Remove the filter and see that it no longer works. Add the filter again and reload the extension and see that the filter still appears in the "My filter list" box in the Advanced section of the options page and that it still works.

Add two subscriptions, each one having the same filter ###bar, then disable one of them, and see that the filter still works (i.e. an element like <div id="bar">Test</div> should be hidden). Disable both the subscriptions and see that the filter no longer works. Reenable one of the subscriptions and see that the filter works again.

[abpbot]

A commit referencing this issue has landed:
​Issue 7094 - Encapsulate management of subscription filters

[jsonesen]

Could you maybe update the tester hints?

[mjethani]

@jsonesen I've added integration notes and hints for testers.

[abpbot]

A commit referencing this issue has landed:
​Issue 7094 - Encapsulate management of subscription filters

[Ross]

Done. Looks to be working as described with no errors in the background console.

ABP 3.4.3.2253
Firefox 65.0.1 / 51 / Windows 10
Firefox Mobile 65.0.1 / Android 7.1.1
Chrome 72.0.3626.109 / 49.0.2623.75 / Windows 10
Opera 58.0.3135.65 / 36.0.2130.80 / Windows 10
Edge 44.17763.1.0 / Windows 10

[abpbot]

A commit referencing this issue has landed:
​Issue 7094 - Keep subscription filters by text only

[greiner]

Since this ticket has already been marked as verified by a tester, why are there still changes landing in here? That's making it quite hard to keep track of which changes have already landed and which ones are yet to come.

[abpbot]

A commit referencing this issue has landed:
​Issue 7094 - Link filters with subscriptions in INI parser

[mjethani]

Replying to greiner:

Since this ticket has already been marked as verified by a tester, why are there still changes landing in here? That's making it quite hard to keep track of which changes have already landed and which ones are yet to come.

This is a work-in-progress ticket, similar to #6891 and #6741. What is the proper way to land changes against such tickets? Perhaps these should be meta tickets and there should be a specific ticket for each change that lands.

[greiner]

Yes, that's how we've been handling such long-running tickets in the past and I can confirm that splitting tickets up like that would help us with keeping track of and making the necessary adjustments for any breaking changes that make it into a specific extension release.

[mjethani]

Fair enough, I am starting to close tickets like these. Moving forward I'll try to make sure that changes land against a ticket only if it is clearly defined what changes need to be made. If it sounds like a meta ticket (make such and such changes everywhere in general), we'll file specific tickets for specific changes and make them block the meta ticket until all changes are done.

[greiner]

Thanks, that's very much appreciated!

[mjethani]

Since this issue is already in a dependency update, I am limiting the scope of the changes here and closing this. I shall file a new issue for further changes (which in fact depend on other changes elsewhere (e.g. #7029)).

[Ross]

Done. Working as described. (Rechecked with #7029).

ABP 0.9.15.2340
Microsoft Edge 44.17763.1.0 / Windows 10 1809

ABP 3.5.2.2340
Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809
Firefox Mobile 68.0 / Android 7.2.2