Issue 7016 - Convert serialization functions into generators

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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
 /**
- * @fileOverview FilterStorage class responsible for managing user's
- *               subscriptions and filters.
+ * @fileOverview <code>filterStorage</code> object responsible for managing the
+ * user's subscriptions and filters.
  */
 
 const {IO} = require("io");
 const {Prefs} = require("prefs");
 const {Filter, ActiveFilter} = require("./filterClasses");
 const {Subscription, SpecialSubscription,
        ExternalSubscription} = require("./subscriptionClasses");
 const {filterNotifier} = require("./filterNotifier");
 const {INIParser} = require("./iniParser");
 
 /**
  * Version number of the filter storage file format.
  * @type {number}
  */
-let formatVersion = 5;
+const FORMAT_VERSION = 5;
 
 /**
- * This class reads user's filters from disk, manages them in memory
- * and writes them back.
- * @class
+ * {@link filterStorage} implementation.
  */
-let FilterStorage = exports.FilterStorage =
+class FilterStorage
 {
   /**
-   * Will be set to true after the initial loadFromDisk() call completes.
-   * @type {boolean}
-   */
-  initialized: false,
-
-  /**
-   * Version number of the patterns.ini format used.
+   * @hideconstructor
+   */
+  constructor()
+  {
+    /**
+     * Will be set to true after the initial {@link FilterStorage#loadFromDisk}
+     * call completes.
+     * @type {boolean}
+     */
+    this.initialized = false;
+
+    /**
+     * Will be set to <code>true</code> if no <code>patterns.ini</code> file
+     * exists.
+     * @type {boolean}
+     */
+    this.firstRun = false;
+
+    /**
+     * Map of properties listed in the filter storage file before the sections
+     * start. Right now this should be only the format version.
+     * @type {object}
+     */
+    this.fileProperties = Object.create(null);
+
+    /**
+     * Map of subscriptions already on the list, by their URL/identifier.
+     * @type {Map.<string,Subscription>}
+     */
+    this.knownSubscriptions = new Map();
+
+    /**
+     * Will be set to true if {@link FilterStorage#saveToDisk} is running
+     * (reentrance protection).
+     * @type {boolean}
+     * @private
+     */
+    this._saving = false;
+
+    /**
+     * Will be set to true if a {@link FilterStorage#saveToDisk} call arrives
+     * while {@link FilterStorage#saveToDisk} is already running (delayed
+     * execution).
+     * @type {boolean}
+     * @private
+     */
+    this._needsSave = false;
+  }
+
+  /**
+   * The version number of the <code>patterns.ini</code> format used.
    * @type {number}
    */
   get formatVersion()
   {
-    return formatVersion;
-  },
-
-  /**
-   * File containing the filter list
+    return FORMAT_VERSION;
+  }
+
+  /**
+   * The file containing the subscriptions.
    * @type {string}
    */
   get sourceFile()
   {
     return "patterns.ini";
-  },
-
-  /**
-   * Will be set to true if no patterns.ini file exists.
-   * @type {boolean}
-   */
-  firstRun: false,
-
-  /**
-   * Map of properties listed in the filter storage file before the sections
-   * start. Right now this should be only the format version.
-   */
-  fileProperties: Object.create(null),
-
-  /**
-   * Yields subscriptions containing all filters
+  }
+
+  /**
+   * Yields all subscriptions in the storage.
    * @yields {Subscription}
    */
   *subscriptions()
   {
     yield* this.knownSubscriptions.values();
-  },
-
-  /**
-   * Number of known subscriptions.
+  }
+
+  /**
+   * The number of subscriptions in the storage.
    * @type {number}
    */
   get subscriptionCount()
   {
     return this.knownSubscriptions.size;
-  },
-
-  /**
-   * Map of subscriptions already on the list, by their URL/identifier
-   * @type {Map.<string,Subscription>}
-   */
-  knownSubscriptions: new Map(),
+  }
 
   /**
    * Finds the filter group that a filter should be added to by default. Will
-   * return null if this group doesn't exist yet.
+   * return <code>null</code> if this group doesn't exist yet.
    * @param {Filter} filter
-   * @return {?SpecialSubscription}
+   * @returns {?SpecialSubscription}
    */
   getGroupForFilter(filter)
   {
     let generalSubscription = null;
-    for (let subscription of FilterStorage.knownSubscriptions.values())
+    for (let subscription of this.knownSubscriptions.values())
     {
       if (subscription instanceof SpecialSubscription && !subscription.disabled)
       {
         // Always prefer specialized subscriptions
         if (subscription.isDefaultFor(filter))
           return subscription;
 
         // If this is a general subscription - store it as fallback
         if (!generalSubscription &&
             (!subscription.defaults || !subscription.defaults.length))
         {
           generalSubscription = subscription;
         }
       }
     }
     return generalSubscription;
-  },
-
-  /**
-   * Adds a filter subscription to the list
-   * @param {Subscription} subscription filter subscription to be added
+  }
+
+  /**
+   * Adds a subscription to the storage.
+   * @param {Subscription} subscription The subscription to be added.
    */
   addSubscription(subscription)
   {
-    if (FilterStorage.knownSubscriptions.has(subscription.url))
+    if (this.knownSubscriptions.has(subscription.url))
       return;
 
-    FilterStorage.knownSubscriptions.set(subscription.url, subscription);
-    addSubscriptionFilters(subscription);
+    this.knownSubscriptions.set(subscription.url, subscription);
+    connectSubscriptionFilters(subscription);
 
     filterNotifier.emit("subscription.added", subscription);
-  },
-
-  /**
-   * Removes a filter subscription from the list
-   * @param {Subscription} subscription filter subscription to be removed
+  }
+
+  /**
+   * Removes a subscription from the storage.
+   * @param {Subscription} subscription The subscription to be removed.
    */
   removeSubscription(subscription)
   {
-    if (!FilterStorage.knownSubscriptions.has(subscription.url))
+    if (!this.knownSubscriptions.has(subscription.url))
       return;
 
-    removeSubscriptionFilters(subscription);
-
-    FilterStorage.knownSubscriptions.delete(subscription.url);
+    disconnectSubscriptionFilters(subscription);
+
+    this.knownSubscriptions.delete(subscription.url);
 
     // This should be the last remaining reference to the Subscription
     // object.
     Subscription.knownSubscriptions.delete(subscription.url);
 
     filterNotifier.emit("subscription.removed", subscription);
-  },
-
-  /**
-   * 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
+  }
+
+  /**
+   * Replaces the list of filters in a subscription with a new list.
+   * @param {Subscription} subscription The subscription to be updated.
+   * @param {Array.<Filter>} filters The new list of filters.
    */
   updateSubscriptionFilters(subscription, filters)
   {
-    removeSubscriptionFilters(subscription);
+    disconnectSubscriptionFilters(subscription);
     let oldFilters = subscription.filters;
     subscription.filters = filters;
-    addSubscriptionFilters(subscription);
+    connectSubscriptionFilters(subscription);
     filterNotifier.emit("subscription.updated", subscription, oldFilters);
-  },
-
-  /**
-   * Adds a user-defined filter to the list
+  }
+
+  /**
+   * Adds a user-defined filter to the storage.
    * @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
+   * @param {?SpecialSubscription} [subscription] The subscription that the
+   *   filter should be added to.
+   * @param {number} [position] The position within the subscription at which
+   *   the filter should be added. If not specified, the filter is added at the
+   *   end of the subscription.
    */
   addFilter(filter, subscription, position)
   {
     if (!subscription)
     {
       for (let currentSubscription of filter.subscriptions())
       {
         if (currentSubscription instanceof SpecialSubscription &&
             !currentSubscription.disabled)
         {
           return;   // No need to add
         }
       }
-      subscription = FilterStorage.getGroupForFilter(filter);
+      subscription = this.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.addSubscription(subscription);
     subscription.filters.splice(position, 0, filter);
     filterNotifier.emit("filter.added", filter, subscription, position);
-  },
-
-  /**
-   * Removes a user-defined filter from the list
+  }
+
+  /**
+   * Removes a user-defined filter from the storage.
    * @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)
+   * @param {?SpecialSubscription} [subscription] The subscription that the
+   *   filter should be removed from. If not specified, the filter will be
+   *   removed from all subscriptions.
+   * @param {number} [position] The position within the subscription at which
+   *   the filter should be removed. If not specified, all instances of the
+   *   filter will be removed.
    */
   removeFilter(filter, subscription, position)
   {
     let subscriptions = (
       subscription ? [subscription] : filter.subscriptions()
     );
     for (let currentSubscription of subscriptions)
     {
       if (currentSubscription instanceof SpecialSubscription)
       {
@@ skipping 18 matching lines @@
           {
             currentSubscription.filters.splice(currentPosition, 1);
             if (currentSubscription.filters.indexOf(filter) < 0)
               filter.removeSubscription(currentSubscription);
             filterNotifier.emit("filter.removed", filter, currentSubscription,
                                 currentPosition);
           }
         }
       }
     }
-  },
-
-  /**
-   * Moves a user-defined filter to a new position
+  }
+
+  /**
+   * Moves a user-defined filter to a new position.
    * @param {Filter} filter
-   * @param {SpecialSubscription} subscription filter group where the filter is
-   *                                           located
-   * @param {number} oldPosition current position of the filter
-   * @param {number} newPosition new position of the filter
+   * @param {SpecialSubscription} subscription The subscription where the
+   *   filter is located.
+   * @param {number} oldPosition The current position of the filter.
+   * @param {number} newPosition The new position of the filter.
    */
   moveFilter(filter, subscription, oldPosition, newPosition)
   {
     if (!(subscription instanceof SpecialSubscription) ||
         subscription.filters[oldPosition] != filter)
     {
       return;
     }
 
     newPosition = Math.min(Math.max(newPosition, 0),
                            subscription.filters.length - 1);
     if (oldPosition == newPosition)
       return;
 
     subscription.filters.splice(oldPosition, 1);
     subscription.filters.splice(newPosition, 0, filter);
     filterNotifier.emit("filter.moved", filter, subscription, oldPosition,
                         newPosition);
-  },
-
-  /**
-   * Increases the hit count for a filter by one
+  }
+
+  /**
+   * Increases the hit count for a filter by one.
    * @param {Filter} filter
    */
   increaseHitCount(filter)
   {
     if (!Prefs.savestats || !(filter instanceof ActiveFilter))
       return;
 
     filter.hitCount++;
     filter.lastHit = Date.now();
-  },
-
-  /**
-   * Resets hit count for some filters
-   * @param {Filter[]} filters  filters to be reset, if null all filters will
-   *                            be reset
+  }
+
+  /**
+   * Resets hit count for some filters.
+   * @param {?Array.<Filter>} [filters] The filters to be reset. If not
+   *   specified, all filters will be reset.
    */
   resetHitCounts(filters)
   {
     if (!filters)
       filters = Filter.knownFilters.values();
     for (let filter of filters)
     {
       filter.hitCount = 0;
       filter.lastHit = 0;
     }
-  },
+  }
 
   /**
    * @callback TextSink
    * @param {string?} line
    */
 
   /**
    * Allows importing previously serialized filter data.
-   * @param {boolean} silent
-   *    If true, no "load" notification will be sent out.
-   * @return {TextSink}
-   *    Function to be called for each line of data. Calling it with null as
-   *    parameter finalizes the import and replaces existing data. No changes
-   *    will be applied before finalization, so import can be "aborted" by
-   *    forgetting this callback.
+   * @param {boolean} silent If <code>true</code>, no "load" notification will
+   *   be sent out.
+   * @returns {TextSink} The function to be called for each line of data.
+   *   Calling it with <code>null</code> as the argument finalizes the import
+   *   and replaces existing data. No changes will be applied before
+   *   finalization, so import can be "aborted" by forgetting this callback.
    */
   importData(silent)
   {
     let parser = new INIParser();
     return line =>
     {
       parser.process(line);
       if (line === null)
       {
         let knownSubscriptions = new Map();
         for (let subscription of parser.subscriptions)
           knownSubscriptions.set(subscription.url, subscription);
 
         this.fileProperties = parser.fileProperties;
         this.knownSubscriptions = knownSubscriptions;
         Filter.knownFilters = parser.knownFilters;
         Subscription.knownSubscriptions = parser.knownSubscriptions;
 
         if (!silent)
           filterNotifier.emit("load");
       }
     };
-  },
-
-  /**
-   * Loads all subscriptions from the disk.
-   * @return {Promise} promise resolved or rejected when loading is complete
+  }
+
+  /**
+   * Loads all subscriptions from disk.
+   * @returns {Promise} A promise resolved or rejected when loading is complete.
    */
   loadFromDisk()
   {
     let tryBackup = backupIndex =>
     {
       return this.restoreBackup(backupIndex, true).then(() =>
       {
         if (this.knownSubscriptions.size == 0)
           return tryBackup(backupIndex + 1);
       }).catch(error =>
@@ skipping 22 matching lines @@
       });
     }).catch(error =>
     {
       Cu.reportError(error);
       return tryBackup(1);
     }).then(() =>
     {
       this.initialized = true;
       filterNotifier.emit("load");
     });
-  },
-
-  /**
-   * Constructs the file name for a patterns.ini backup.
-   * @param {number} backupIndex
-   *    number of the backup file (1 being the most recent)
-   * @return {string} backup file name
+  }
+
+  /**
+   * Constructs the file name for a <code>patterns.ini</code> backup.
+   * @param {number} backupIndex Number of the backup file (1 being the most
+   *   recent).
+   * @returns {string} Backup file name.
    */
   getBackupName(backupIndex)
   {
     let [name, extension] = this.sourceFile.split(".", 2);
     return (name + "-backup" + backupIndex + "." + extension);
-  },
+  }
 
   /**
    * Restores an automatically created backup.
-   * @param {number} backupIndex
-   *    number of the backup to restore (1 being the most recent)
-   * @param {boolean} silent
-   *    If true, no "load" notification will be sent out.
-   * @return {Promise} promise resolved or rejected when restoring is complete
+   * @param {number} backupIndex Number of the backup to restore (1 being the
+   *   most recent).
+   * @param {boolean} silent If <code>true</code>, no "load" notification will
+   *   be sent out.
+   * @returns {Promise} A promise resolved or rejected when restoration is
+   *   complete.
    */
   restoreBackup(backupIndex, silent)
   {
     let backupFile = this.getBackupName(backupIndex);
     let parser = this.importData(silent);
     return IO.readFromFile(backupFile, parser).then(() =>
     {
       parser(null);
       return this.saveToDisk();
     });
-  },
+  }
 
   /**
    * Generator serializing filter data and yielding it line by line.
+   * @yields {string}
    */
   *exportData()
   {
     // Do not persist external subscriptions
     let subscriptions = [];
     for (let subscription of this.subscriptions())
     {
       if (!(subscription instanceof ExternalSubscription) &&
           !(subscription instanceof SpecialSubscription &&
             subscription.filters.length == 0))
       {
         subscriptions.push(subscription);
       }
     }
 
     yield "# Adblock Plus preferences";
-    yield "version=" + formatVersion;
+    yield "version=" + this.formatVersion;
 
     let saved = new Set();
 
     // Save subscriptions
     for (let subscription of subscriptions)
     {
       yield* subscription.serialize();
-
-      if (subscription.filters.length)

[Manish Jethani, 2018/10/04 04:08:59]

One thing worth noting here as that before this change the serialization of a
subscription was an atomic operation. This means the entire subscription was
added to a buffer and _then_ each entry was yielded out. After this change, it
would be possible for the subscription to be modified while it is being
serialized. This could have POTENTIALLY DISASTROUS consequences, like the
generation of a patterns.ini file that the INI parser is unable to read (thus
causing Adblock Plus to no longer function).

I'd like to take my time to think about this, but at the very least it, since it
is now possible for the number of filters to go from non-zero to zero between
this length check here and the actual serialization of the filters, this is
clearly redundant. We can remove this check here. At the same time, let's make
sure that INI parser is able to read a file with a "[Subscription filters]"
section containing _no_ filters.

In general we really, really, really have to make sure that this function is
never able to generate a patterns.ini that the INI parser is unable to parse.

[Manish Jethani, 2018/10/04 04:17:06]

Additionally, it should never be possible for this function to generate a
patterns.ini that the INI parser can parse successfully but that still results
in objects (subscriptions and filters) that are in an _inconsistent_ state
(which may cause errors elsewhere).

So this change is not as simple as it may look like on the surface. We are
definitely complicating things here, but I think we could do this in a proper
way.

We will definitely have to add some more tests.

[Manish Jethani, 2018/10/04 04:36:23]

Alright, some more thinking about this. Let's do this in two steps. In the first
step, let's do this:

  yield [...subscription.serialize()];

Instead of this:

  yield* subscription.serialize();

This cancels out some of the benefit of this change, but it still makes the code
more readable and I have a hunch that it will still perform better.

In the next step (another patch, another time), let's carefully evaluate the
consequences of generating an object's serialized representation line by line.

So let's not change the behavior of the `exportData` function as part of this
patch.

Now based on everything above, we should roll the "[Subscription filters]" part
into `Subscription.prototype.serialize`, which can take an optional
`withFilters` (boolean) parameter and do this part only if it is true (it should
be false for `Subscription.prototype.toString`).

[Manish Jethani, 2018/10/04 05:10:06]

Sorry, I mean `yield*` here.

[Manish Jethani, 2018/10/04 06:10:15]

I profiled this. It performs slightly better than what we have right now.

If we change it to `yield* subscription.serialize()` then it performs
significantly better. Again, not surprising, but it's good to verify this.

If you'd like to try it out, reload the extension, open the DevTools for the
background page, go to the Performance tab and start profiling. Then run this
script, which will open up the Alexa Top 50:

  #!/bin/bash
  for domain in $(curl https://www.alexa.com/topsites | sed -nE
's/.*\/siteinfo\/([^"]*).*/\1/p')
  do
    open -a 'Google Chrome' "http://$domain"
  done

Stop profiling after approximately one minute, then go to the Bottom-up tab at
the bottom of the window (default layout) and search for "exportData".

[Jon Sonesen, 2018/10/06 00:06:32]

So, I had similar concerns. But figured we can hash it out here, and this
version was more 'to spec' in my mind.

I actually agree that this is a pretty huge change, however doing the change
which is slightly better performance wise with plans to later make the larger
change seems like it may be more of a delay than anything since we would still
need to address the same issue we face here in the future. So, I think it's
worth it to just take the time to test and properly implement the full change. 

However, I do also realize this change makes the code a bit harder to follow and
so we should consider how much that matters here. We are trying to optimize core
as much as possible (for good reason) and as long as it makes sense, then I am
up for it.

Also, if you insist we just hold off for now and use your suggestion I am
perfectly happy to do so, this was just my two rupees worth of opinion.

[Manish Jethani, 2018/10/09 15:11:03]

Alright, let's do this properly.

So the strategy is to take a snapshot of the object right at the beginning of
the serialize function, which makes it just like the current implementation in
terms of serializing a consistent state of the object.

  let {prop1, prop2, prop3} = this;
  if (prop1)
    yield prop1;
  if (prop2)
    yield prop2;
  if (prop3)
    yield prop3;

This way, if the value of a property changes in between yield statements, it
would still yield the old value.

[Jon Sonesen, 2018/10/12 03:50:05]

Spent an embarrassing amount of time trying to figure out what to change here
but realized this reply is probably meant to describe the yield approach (which
is addressed)

[Manish Jethani, 2018/10/14 20:05:36]

Ack.

Yeah, this was just a description.

-      {
-        yield "[Subscription filters]";
-        yield* subscription.serializeFilters();
-      }
+      yield* subscription.serializeFilters();
     }
 
     // Save filter data
     for (let subscription of subscriptions)
     {
       for (let filter of subscription.filters)
       {
         if (!saved.has(filter.text))
         {
           yield* filter.serialize();
           saved.add(filter.text);
         }
       }
     }
-  },
-
-  /**
-   * Will be set to true if saveToDisk() is running (reentrance protection).
-   * @type {boolean}
-   */
-  _saving: false,
-
-  /**
-   * Will be set to true if a saveToDisk() call arrives while saveToDisk() is
-   * already running (delayed execution).
-   * @type {boolean}
-   */
-  _needsSave: false,
-
-  /**
-   * Saves all subscriptions back to disk
-   * @return {Promise} promise resolved or rejected when saving is complete
+  }
+
+  /**
+   * Saves all subscriptions back to disk.
+   * @returns {Promise} A promise resolved or rejected when saving is complete.
    */
   saveToDisk()
   {
     if (this._saving)
     {
       this._needsSave = true;
       return;
     }
 
     this._saving = true;
@@ skipping 63 matching lines @@
       Cu.reportError(error);
     }).then(() =>
     {
       this._saving = false;
       if (this._needsSave)
       {
         this._needsSave = false;
         this.saveToDisk();
       }
     });
-  },
+  }
 
   /**
    * @typedef FileInfo
    * @type {object}
    * @property {number} index
    * @property {number} lastModified
    */
 
   /**
    * Returns a promise resolving in a list of existing backup files.
-   * @return {Promise.<FileInfo[]>}
+   * @returns {Promise.<Array.<FileInfo>>}
    */
   getBackupFiles()
   {
     let backups = [];
 
     let checkBackupFile = index =>
     {
       return IO.statFile(this.getBackupName(index)).then(statData =>
       {
         if (!statData.exists)
           return backups;
 
         backups.push({
           index,
           lastModified: statData.lastModified
         });
         return checkBackupFile(index + 1);
       }).catch(error =>
       {
         // Something went wrong, return whatever data we got so far.
         Cu.reportError(error);
         return backups;
       });
     };
 
     return checkBackupFile(1);
   }
-};
+}
 
 /**
- * Joins subscription's filters to the subscription without any notifications.
- * @param {Subscription} subscription
- *   filter subscription that should be connected to its filters
+ * Reads the user's filters from disk, manages them in memory, and writes them
+ * back to disk.
  */
-function addSubscriptionFilters(subscription)
+let filterStorage = new FilterStorage();
+
+exports.filterStorage = filterStorage;
+
+/**
+ * Connects a subscription to its filters without any notifications.
+ * @param {Subscription} subscription The subscription that should be
+ *   connected to its filters.
+ */
+function connectSubscriptionFilters(subscription)
 {
-  if (!FilterStorage.knownSubscriptions.has(subscription.url))
+  if (!filterStorage.knownSubscriptions.has(subscription.url))
     return;
 
   for (let filter of subscription.filters)
     filter.addSubscription(subscription);
 }
 
 /**
- * Removes subscription's filters from the subscription without any
- * notifications.
- * @param {Subscription} subscription filter subscription to be removed
+ * Disconnects a subscription from its filters without any notifications.
+ * @param {Subscription} subscription The subscription that should be
+ *   disconnected from its filters.
  */
-function removeSubscriptionFilters(subscription)
+function disconnectSubscriptionFilters(subscription)
 {
-  if (!FilterStorage.knownSubscriptions.has(subscription.url))
+  if (!filterStorage.knownSubscriptions.has(subscription.url))
     return;
 
   for (let filter of subscription.filters)
     filter.removeSubscription(subscription);
 }