Issue 6829 - Free filters when subscriptions are empty

lib/filterStorage.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 127 matching lines @@
    * Removes a filter subscription from the list
    * @param {Subscription} subscription filter subscription to be removed
    */
   removeSubscription(subscription)
   {
     for (let i = 0; i < FilterStorage.subscriptions.length; i++)
     {
       if (FilterStorage.subscriptions[i].url == subscription.url)
       {
         removeSubscriptionFilters(subscription);
 

[Manish Jethani, 2018/08/25 15:03:38]

We should delete the filters here.

[Jon Sonesen, 2018/08/27 16:28:03]

Hm, because of the update function calling removSubscriptionFilters I know I
need to change it, but do you think it is possible to just change where the
update method calls remove? This way it only happens after the subscriptions
finish updating to avoid the delete/readd case? 

         FilterStorage.subscriptions.splice(i--, 1);
         FilterStorage.knownSubscriptions.delete(subscription.url);
 
         // This should be the last remaining reference to the Subscription
         // object.
         Subscription.knownSubscriptions.delete(subscription.url);
 
         FilterNotifier.triggerListeners("subscription.removed", subscription);
         return;
       }
@@ skipping 32 matching lines @@
   /**
    * Replaces the list of filters in a subscription by a new list
    * @param {Subscription} subscription filter subscription to be updated
    * @param {Filter[]} filters new filter list
    */
   updateSubscriptionFilters(subscription, filters)
   {
     removeSubscriptionFilters(subscription);
     subscription.oldFilters = subscription.filters;
     subscription.filters = filters;
     addSubscriptionFilters(subscription);

[Manish Jethani, 2018/08/25 15:03:38]

After calling addSubscriptionFilters, we should delete any filters in the
oldFilters list that don't have any subscriptions (but most of the old filters
will also be in the new filters list, so they will have a subscription).

[Jon Sonesen, 2018/08/27 16:28:02]

See comment above 

     FilterNotifier.triggerListeners("subscription.updated", subscription);
     delete subscription.oldFilters;
   },
 
   /**
    * Adds a user-defined filter to the list
    * @param {Filter} filter
    * @param {SpecialSubscription} [subscription]
    *   particular group that the filter should be added to
    * @param {number} [position]
@@ skipping 459 matching lines @@
  * Removes subscription's filters from the subscription without any
  * notifications.
  * @param {Subscription} subscription filter subscription to be removed
  */
 function removeSubscriptionFilters(subscription)
 {
   if (!FilterStorage.knownSubscriptions.has(subscription.url))
     return;
 
   for (let filter of subscription.filters)
+  {
     filter.subscriptions.delete(subscription);
+    if (filter.subscriptions.size == 0)

[Manish Jethani, 2018/08/25 15:03:38]

removeSubscriptionFilters is also called from updateSubscriptionFilters, which
means this will also remove any filters that are also in the new list of filters
(passed to updateSubscriptionFilters). I think we should do this outside of this
function, where it is being called instead.

[jsonesen, 2018/08/25 15:18:46]

The deletion should I ly occur if there are no subs riptions in the subs set,
rather than if there are disabled subscriptions so I would think it shouldn't
effect the update and in fact should only occur when the filter has no
subscriptions (hence the logical if) also the filters are already being iterated
over here so we can avoid looking at all the filters again.

[jsonesen, 2018/08/25 15:32:08]

Oh shoot nevermind, since it removes first then adds the chance of having a
filter with 0 subscriptions that is meant to be added to the update subscription
is basically 100%?

[Manish Jethani, 2018/08/26 15:56:19]

Yes, exactly. If a filter is also in a different subscription then the count
will be greater than 0, but if it's only in this subscription then it'll be 0.

+      Filter.knownFilters.delete(filter.text);

[Jon Sonesen, 2018/08/21 21:50:41]

I found that there is a pretty good reduction in the memory footprint of the
extension. With this change; after removing all subscriptions from the options
page it would return to around 60mb with nothing but the options page open in
the browser. Whereas if you test without this, the footprint never really seems
to return to as small of a footprint as when first starting the extension with
no subscriptions.

+  }
 }
 
 /**
  * Listener returned by FilterStorage.importData(), parses filter data.
  * @constructor
  */
 function INIParser()
 {
   this.fileProperties = this.curObj = {};
   this.subscriptions = [];
@@ skipping 79 matching lines @@
       else if (this.wantObj === false && val)
         this.curObj.push(val.replace(/\\\[/g, "["));
     }
     finally
     {
       Filter.knownFilters = origKnownFilters;
       Subscription.knownSubscriptions = origKnownSubscriptions;
     }
   }
 };