Opened on 08/11/2018 at 03:02:10 PM

Closed on 08/20/2018 at 12:53:37 PM

Last modified on 10/25/2018 at 11:06:25 AM

#6855 closed change (fixed)

Remove Subscription object from memory when unsubscribed

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

Description (last modified by mjethani)


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

Attachments (0)

Change History (21)

comment:1 Changed on 08/11/2018 at 03:02:51 PM by mjethani

  • Blocking 6829 added

comment:2 Changed on 08/11/2018 at 03:22:12 PM by mjethani

  • Description modified (diff)

comment:3 Changed on 08/11/2018 at 03:28:37 PM by mjethani

  • Owner set to mjethani
  • Review URL(s) modified (diff)

comment:4 Changed on 08/11/2018 at 03:28:44 PM by mjethani

  • Status changed from new to reviewing

comment:5 Changed on 08/12/2018 at 07:28:29 AM by mjethani

  • Description modified (diff)

comment:6 Changed on 08/16/2018 at 08:07:05 PM by abpbot

comment:7 Changed on 08/20/2018 at 12:53:09 PM by mjethani

  • Description modified (diff)

comment:8 Changed on 08/20/2018 at 12:53:37 PM by mjethani

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:9 Changed on 08/20/2018 at 01:20:56 PM by mjethani

  • Description modified (diff)

comment:10 Changed on 08/28/2018 at 03:22:34 PM by sebastian

  • Cc sebastian added

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.

comment:11 Changed on 08/29/2018 at 01:29:17 PM by mjethani

  • Cc greiner added

comment:12 Changed on 08/29/2018 at 01:31:08 PM by 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.

comment:13 Changed on 08/29/2018 at 01:32:05 PM by mjethani

  • Cc sergz added

comment:14 Changed on 08/29/2018 at 04:39:46 PM by 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.

comment:15 Changed on 08/29/2018 at 04:49:12 PM by 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).

comment:16 Changed on 08/29/2018 at 05:00:12 PM by 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.

comment:17 Changed on 08/29/2018 at 05:14:41 PM by 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.

Last edited on 08/29/2018 at 05:15:50 PM by mjethani

comment:18 Changed on 08/29/2018 at 05:17:08 PM by greiner

  • Cc jeen added

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?

comment:19 Changed on 10/23/2018 at 09:03:30 AM by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

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

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

comment:20 Changed on 10/25/2018 at 10:47:25 AM by jeen

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


comment:21 Changed on 10/25/2018 at 11:06:25 AM by 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.

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.