Remove Subscription object from memory when unsubscribed

[mjethani]

Background

When the user subscribes to a new filter list, a new Subscription object is created and added to Subscription.knownSubscriptions (and all over the place). When the user unsubscribes from the list, the Subscription object is removed from everywhere except Subscription.knownSubscriptions. This doesn't make sense, as the object also holds on to its Filter objects (see also #6829), even as the Filter objects release their references to the Subscription object.

When a Subscription object has definitely been removed from everywhere else, it should also be removed from Subscription.knownSubscriptions.

What to change

In FilterStorage.removeSubscription, delete the Subscription object from the Subscription.knownSubscriptions map. In order to make sure that all references to the Subscription object are lost, remove all occurrences of the object from the Filter object in removeSubscriptionFilters.

Write tests to verify this behavior.

Integration notes

Any code that uses a Subscription object should release the reference to the object after calling FilterStorage.removeSubscription. If the same subscription is to be added again, a new Subscription object should be created via Subscription.fromURL; the old Subscription object should not be reused.

Hints for testers

The change here is covered by the unit tests and it's not really feasible to verify it using the browser extension.

Here's a good test to verify that subscriptions are still working as expected:

1. Subscribe to a list
2. Verify that filters from the list are being applied
3. Unsubscribe from the list (note: don't disable the list, delete it by clicking the trash icon)
4. Verify that filters from the list are no longer being applied
5. Subscribe to the same list again
6. Verify that filters from the list are being applied once again

[abpbot]

A commit referencing this issue has landed:
​Issue 6855 - Release all references to Subscription object once removed

[sebastian]

It might be a little too late to join the discussion, but for the record: I discovered that behavior a few years ago and discussed it with Wladimir. Apparently, it was considered a feature, so that when a user accidentally removed a filter list, this could be undone without re-downloading the filter list, and without losing the information which filters have been disabled. The latter, however, isn't a feature supported by the UI anymore. So you might want to remove the disabled property of filters as well.

[mjethani]

@sebastian thanks for the additional background. I've also copied @greiner here. The more of the legacy features we can remove from core, the better it will be for memory and performance and make us look better on non-desktop platforms.

[greiner]

Similar to other features that didn't make the cut when we release our initial web extension version, we're going to reintroduce the ability to en-/disable individual filters (see ​ui#15, #6875).

If the performance argument is valid and strong and the added benefit to users of having those capabilities isn't, we could, of course, revisit this decision in the future.

[mjethani]

@greiner @sebastian alright, let's leave the feature in for now

Note that the removed subscriptions are not serialized to disk, so the benefit of them retaining state (if this works at all) is limited to the current session, which seems more like unintended behavior than a feature. This change, which deletes the Subscription object from memory, is only for removed subscriptions (clicking the trash icon), not disabled subscriptions. We are also going to delete all the associated Filter objects that don't also belong to any other subscription (#6829).

[sebastian]

It is intended more like an undo feature, like the undo button/shortcut in a text editor. After restarting your text editor (and/or reopening the file), you wouldn't expect to be able to undo changes either. That said, with this behavior removed from Core and disabled filters added in UI, we would regress compared to the legacy Firefox extension. Personally, I don't feel too strong though, but Wladimir did.

[mjethani]

If we do add the ability to undo the removal of a subscription in UI (or just the requirement that a re-added subscription must remember which of its filters were previously disabled), we can make a corresponding change in core to support that. If push comes to shove, we simply won't delete any subscription and any of its filters from memory (essentially reverting #6855 and #6829). But when we have the exact requirements we could perhaps come up with an even better solution.

Since the UI is specific to the browser extension, it could also do this on its own side. For example, it could hold on to the Subscription object with all its Filter objects and simply re-add the subscription with its previous state intact. There would have to be some coordination between core and UI to do this properly, but it's doable.

[greiner]

One more thing worth mentioning is that we'll initially only going to add this ability to the custom filter list which you cannot remove anyway so it sounds like freeing the objects shouldn't be an issue until we start using it for downloadable filter lists as well.

Even then, I agree with Sebastian that the use case for this undo behavior doesn't seem to be that strong. So if Product agrees with that then it seems there'd be no need anymore to keep those objects around.

@Jeen If the user would be able to disable some filters in EasyList through the UI and then remove and install it again, would we have to remember which filters the user disabled or should all filters be enabled?

[Ross]

Done. Doesn't look to have caused any issues or regressions.

ABP 3.3.2.2175
Firefox 62 / 51 / Windows 10
Chrome 69 / 49 / Windows 10
Opera 56 / 36 / Windows 10

[jeen]

I think this is such an extreme edge case, I feel that keeping this feature is not really necessary.

@greiner

[greiner]

Ok, thanks. Presumably, the only somewhat realistic scenario would be if someone disables Adblock Plus on a website which causes certain filters in EasyList to be disabled. Thereby Adblock Plus would get reenabled for that website when the user removes and reinstalls EasyList.