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);
+        for (let filter of subscription.filters)
+        {
+          if (filter.subscriptions.size == 0)
+            Filter.knownFilters.delete(filter.text);
+        }
 

[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;
       }
     }
   },
 
   /**
    * Moves a subscription in the list to a new position.
    * @param {Subscription} subscription filter subscription to be moved
    * @param {Subscription} [insertBefore] filter subscription to insert before
@@ skipping 25 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]
    *   position within the subscription at which the filter should be added
    */
   addFilter(filter, subscription, position)
   {
     if (!subscription)
     {
-      for (let currentSubscription of filter.subscriptions)
-      {
-        if (currentSubscription instanceof SpecialSubscription &&
-            !currentSubscription.disabled)
-        {
-          return;   // No need to add
-        }
+      if (filter.subscriptions.some(s => s instanceof SpecialSubscription &&
+                                         !s.disabled))
+      {
+        return;   // No need to add
       }
       subscription = FilterStorage.getGroupForFilter(filter);
     }
     if (!subscription)
     {
       // No group for this filter exists, create one
       subscription = SpecialSubscription.createForFilter(filter);
       this.addSubscription(subscription);
       return;
     }
 
     if (typeof position == "undefined")
       position = subscription.filters.length;
 
-    filter.subscriptions.add(subscription);
+    if (filter.subscriptions.indexOf(subscription) < 0)
+      filter.subscriptions.push(subscription);
     subscription.filters.splice(position, 0, filter);
     FilterNotifier.triggerListeners("filter.added", filter, subscription,
                                     position);
   },
 
   /**
    * Removes a user-defined filter from the list
    * @param {Filter} filter
    * @param {SpecialSubscription} [subscription] a particular filter group that
    *      the filter should be removed from (if ommited will be removed from all
    *      subscriptions)
    * @param {number} [position]  position inside the filter group at which the
    *      filter should be removed (if ommited all instances will be removed)
    */
   removeFilter(filter, subscription, position)
   {
     let subscriptions = (
-      subscription ? [subscription] : filter.subscriptions
+      subscription ? [subscription] : filter.subscriptions.slice()
     );
-    for (let currentSubscription of subscriptions)
-    {
+    for (let i = 0; i < subscriptions.length; i++)
+    {
+      let currentSubscription = subscriptions[i];
       if (currentSubscription instanceof SpecialSubscription)
       {
         let positions = [];
         if (typeof position == "undefined")
         {
           let index = -1;
           do
           {
             index = currentSubscription.filters.indexOf(filter, index + 1);
             if (index >= 0)
               positions.push(index);
           } while (index >= 0);
         }
         else
           positions.push(position);
 
         for (let j = positions.length - 1; j >= 0; j--)
         {
           let currentPosition = positions[j];
           if (currentSubscription.filters[currentPosition] == filter)
           {
             currentSubscription.filters.splice(currentPosition, 1);
             if (currentSubscription.filters.indexOf(filter) < 0)
-              filter.subscriptions.delete(currentSubscription);
+            {
+              let index = filter.subscriptions.indexOf(currentSubscription);
+              if (index >= 0)
+                filter.subscriptions.splice(index, 1);
+            }
             FilterNotifier.triggerListeners(
               "filter.removed", filter, currentSubscription, currentPosition
             );
           }
         }
       }
     }
   },
 
   /**
@@ skipping 362 matching lines @@
  * Joins subscription's filters to the subscription without any notifications.
  * @param {Subscription} subscription
  *   filter subscription that should be connected to its filters
  */
 function addSubscriptionFilters(subscription)
 {
   if (!FilterStorage.knownSubscriptions.has(subscription.url))
     return;
 
   for (let filter of subscription.filters)
-    filter.subscriptions.add(subscription);
+    filter.subscriptions.push(subscription);
 }
 
 /**
  * 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.

+    let i = filter.subscriptions.indexOf(subscription);
+    if (i >= 0)
+      filter.subscriptions.splice(i, 1);
   }
 }
 
 /**
  * Listener returned by FilterStorage.importData(), parses filter data.
  * @constructor
  */
 function INIParser()
 {
   this.fileProperties = this.curObj = {};
@@ skipping 43 matching lines @@
             case "subscription filters":
               if (this.subscriptions.length)
               {
                 let subscription = this.subscriptions[
                   this.subscriptions.length - 1
                 ];
                 for (let text of this.curObj)
                 {
                   let filter = Filter.fromText(text);
                   subscription.filters.push(filter);
-                  filter.subscriptions.add(subscription);
+                  filter.subscriptions.push(subscription);
                 }
               }
               break;
           }
         }
 
         if (val === null)
           return;
 
         this.curSection = match[1].toLowerCase();
@@ skipping 16 matching lines @@
       else if (this.wantObj === false && val)
         this.curObj.push(val.replace(/\\\[/g, "["));
     }
     finally
     {
       Filter.knownFilters = origKnownFilters;
       Subscription.knownSubscriptions = origKnownSubscriptions;
     }
   }
 };