Issue 5059 - Simplify I/O API and FilterStorage implementation

lib/filterStorage.js

Index: lib/filterStorage.js
===================================================================
--- a/lib/filterStorage.js
+++ b/lib/filterStorage.js
@@ -17,26 +17,22 @@
 
 "use strict";
 
 /**
  * @fileOverview FilterStorage class responsible for managing user's
  *               subscriptions and filters.
  */
 
-const {FileUtils} = Cu.import("resource://gre/modules/FileUtils.jsm", {});
-const {Services} = Cu.import("resource://gre/modules/Services.jsm", {});
-
 const {IO} = require("io");
 const {Prefs} = require("prefs");
 const {Filter, ActiveFilter} = require("filterClasses");
 const {Subscription, SpecialSubscription,
        ExternalSubscription} = require("subscriptionClasses");
 const {FilterNotifier} = require("filterNotifier");
-const {Utils} = require("utils");
 
 /**
  * Version number of the filter storage file format.
  * @type {number}
  */
 let formatVersion = 4;
 
 /**
@@ -57,59 +53,22 @@ let FilterStorage = exports.FilterStorag
    * @type {number}
    */
   get formatVersion()
   {
     return formatVersion;
   },
 
   /**
-   * File that the filter list has been loaded from and should be saved to
-   * @type {nsIFile}
+   * File containing the filter list
+   * @type {string}
    */
   get sourceFile()
   {
-    let file = null;
-    if (Prefs.patternsfile)
-    {
-      // Override in place, use it instead of placing the file in the
-      // regular data dir
-      file = IO.resolveFilePath(Prefs.patternsfile);
-    }
-    if (!file)
-    {
-      // Place the file in the data dir
-      file = IO.resolveFilePath(Prefs.data_directory);
-      if (file)
-        file.append("patterns.ini");
-    }
-    if (!file)
-    {
-      // Data directory pref misconfigured? Try the default value
-      try
-      {
-        let dir = Services.prefs.getDefaultBranch("extensions.adblockplus.")
-                          .getCharPref("data_directory");
-        file = IO.resolveFilePath(dir);
-        if (file)
-          file.append("patterns.ini");
-      }
-      catch (e) {}
-    }
-
-    if (!file)
-    {
-      Cu.reportError("Adblock Plus: Failed to resolve filter file location " +
-                     "from extensions.adblockplus.patternsfile preference");
-    }
-
-    // Property is configurable because of the test suite.
-    Object.defineProperty(this, "sourceFile",
-                          {value: file, configurable: true});
-    return file;
+    return "patterns.ini";

[Wladimir Palant, 2017/04/10 14:59:15]

We are removing configurability here, there is no point any more.

   },
 
   /**
    * Will be set to true if no patterns.ini file exists.
    * @type {boolean}
    */
   firstRun: false,
 
@@ -404,23 +363,25 @@ let FilterStorage = exports.FilterStorag
 
   /**
    * @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.
    */
-  importData()
+  importData(silent)
   {
     let parser = new INIParser();
     return line =>
     {
       parser.process(line);
       if (line === null)
       {
         // Old special groups might have been converted, remove them if
@@ -447,105 +408,97 @@ let FilterStorage = exports.FilterStorag
         Subscription.knownSubscriptions = parser.knownSubscriptions;
 
         if (parser.userFilters)
         {
           for (let filter of parser.userFilters)
             this.addFilter(Filter.fromText(filter), null, undefined, true);
         }
 
-        FilterNotifier.triggerListeners("load");
+        if (!silent)
+          FilterNotifier.triggerListeners("load");
       }
     };
   },
 
   /**
-   * Loads all subscriptions from the disk
+   * Loads all subscriptions from the disk.
+   * @return {Promise} promise resolved or rejected when loading is complete
    */
   loadFromDisk()
   {
-    let readFile = () =>
-    {
-      let parser = {
-        process: this.importData()
-      };
-      IO.readFromFile(this.sourceFile, parser, readFromFileException =>
-      {
-        this.initialized = true;
-
-        if (!readFromFileException && this.subscriptions.length == 0)
-        {
-          // No filter subscriptions in the file, this isn't right.
-          readFromFileException = new Error("No data in the file");
-        }
-
-        if (readFromFileException)
-          Cu.reportError(readFromFileException);
-
-        if (readFromFileException)
-          tryBackup(1);
-      });
-    };
-
     let tryBackup = backupIndex =>
     {
-      this.restoreBackup(backupIndex).then(() =>
+      return this.restoreBackup(backupIndex, true).then(() =>
       {
         if (this.subscriptions.length == 0)
-          tryBackup(backupIndex + 1);
+          return tryBackup(backupIndex + 1);
       }).catch(error =>
       {
         // Give up
       });
     };
 
-    IO.statFile(this.sourceFile, (statError, statData) =>
+    return IO.statFile(this.sourceFile).then(statData =>
     {
-      if (statError || !statData.exists)
+      if (!statData.exists)
       {
         this.firstRun = true;
-        this.initialized = true;
-        FilterNotifier.triggerListeners("load");
+        return;
       }
-      else
-        readFile();
+
+      let parser = this.importData(true);
+      return IO.readFromFile(this.sourceFile, parser).then(() =>
+      {
+        parser(null);
+        if (this.subscriptions.length == 0)
+        {
+          // No filter subscriptions in the file, this isn't right.
+          throw new Error("No data in the file");
+        }
+      });
+    }).catch(error =>
+    {
+      Cu.reportError(error);
+      return tryBackup(1);
+    }).then(() =>
+    {
+      this.initialized = true;
+      FilterNotifier.triggerListeners("load");

[Wladimir Palant, 2017/04/10 14:59:15]

There is a change here: load notification is fired and initialized flag is set
only after we tried backups. This restores the logic we had before
https://issues.adblockplus.org/ticket/5052.

     });
   },
 
   /**
+   * 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
+   */
+  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
    */
-  restoreBackup(backupIndex)
+  restoreBackup(backupIndex, silent)
   {
-    return new Promise((resolve, reject) =>
+    let backupFile = this.getBackupName(backupIndex);
+    let parser = this.importData(silent);
+    return IO.readFromFile(backupFile, parser).then(() =>
     {
-      // Attempt to load a backup
-      let [, part1, part2] = /^(.*)(\.\w+)$/.exec(
-        this.sourceFile.leafName
-      ) || [null, this.sourceFile.leafName, ""];
-
-      let backupFile = this.sourceFile.clone();
-      backupFile.leafName = (part1 + "-backup" + backupIndex + part2);
-
-      let parser = {
-        process: this.importData()
-      };
-      IO.readFromFile(backupFile, parser, error =>
-      {
-        if (error)
-          reject(error);
-        else
-        {
-          this.saveToDisk();
-          resolve();
-        }
-      });
+      parser(null);
+      return this.saveToDisk();
     });
   },
 
   /**
    * Generator serializing filter data and yielding it line by line.
    */
   *exportData()
   {
@@ -603,121 +556,100 @@ let FilterStorage = exports.FilterStorag
    * 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
    */
   saveToDisk()
   {
     if (this._saving)
     {
       this._needsSave = true;
       return;
     }
 
-    // Make sure the file's parent directory exists
-    let targetFile = this.sourceFile;
-    try
-    {
-      targetFile.parent.create(Ci.nsIFile.DIRECTORY_TYPE,
-                               FileUtils.PERMS_DIRECTORY);
-    }
-    catch (e) {}

[Wladimir Palant, 2017/04/10 14:59:15]

This code block is removed, now the IO module is responsible for creating the
directory (which in practice no implementations need to do any more).

-
-    let writeFilters = () =>
-    {
-      IO.writeToFile(targetFile, this.exportData(), e =>
-      {
-        this._saving = false;
-
-        if (e)
-          Cu.reportError(e);
-
-        if (this._needsSave)
-        {
-          this._needsSave = false;
-          this.saveToDisk();
-        }
-        else
-          FilterNotifier.triggerListeners("save");
-      });
-    };
-
-    let checkBackupRequired = (callbackNotRequired, callbackRequired) =>
-    {
-      if (Prefs.patternsbackups <= 0)
-        callbackNotRequired();
-      else
-      {
-        IO.statFile(targetFile, (statFileException, statData) =>
-        {
-          if (statFileException || !statData.exists)
-            callbackNotRequired();
-          else
-          {
-            let [, part1, part2] = /^(.*)(\.\w+)$/.exec(targetFile.leafName) ||
-              [null, targetFile.leafName, ""];
-            let newestBackup = targetFile.clone();
-            newestBackup.leafName = part1 + "-backup1" + part2;
-            IO.statFile(
-              newestBackup,
-              (statBackupFileException, statBackupData) =>
-              {
-                if (!statBackupFileException && (!statBackupData.exists ||
-                           (Date.now() - statBackupData.lastModified) /
-                             3600000 >= Prefs.patternsbackupinterval))
-                {
-                  callbackRequired(part1, part2);
-                }
-                else
-                  callbackNotRequired();
-              }
-            );
-          }
-        });
-      }
-    };
-
-    let removeLastBackup = (part1, part2) =>
-    {
-      let file = targetFile.clone();
-      file.leafName = part1 + "-backup" + Prefs.patternsbackups + part2;
-      IO.removeFile(
-        file, e => renameBackup(part1, part2, Prefs.patternsbackups - 1)
-      );
-    };
-
-    let renameBackup = (part1, part2, index) =>
-    {
-      if (index > 0)
-      {
-        let fromFile = targetFile.clone();
-        fromFile.leafName = part1 + "-backup" + index + part2;
-
-        let toName = part1 + "-backup" + (index + 1) + part2;
-
-        IO.renameFile(fromFile, toName, e => renameBackup(part1, part2,
-                                                          index - 1));
-      }
-      else
-      {
-        let toFile = targetFile.clone();
-        toFile.leafName = part1 + "-backup" + (index + 1) + part2;
-
-        IO.copyFile(targetFile, toFile, writeFilters);
-      }
-    };
-
     this._saving = true;
 
-    checkBackupRequired(writeFilters, removeLastBackup);
+    return Promise.resolve().then(() =>
+    {
+      // First check whether we need to create a backup
+      if (Prefs.patternsbackups <= 0)
+        return false;
+
+      return IO.statFile(this.sourceFile).then(statData =>
+      {
+        if (!statData.exists)
+          return false;
+
+        return IO.statFile(this.getBackupName(1)).then(backupStatData =>
+        {
+          if (backupStatData.exists &&
+              (Date.now() - backupStatData.lastModified) / 3600000 <
+                Prefs.patternsbackupinterval)
+          {
+            return false;
+          }
+          return true;
+        });
+      });
+    }).then(backupRequired =>
+    {
+      if (!backupRequired)
+        return;
+
+      let ignoreErrors = error =>
+      {
+        // Expected error, backup file doesn't exist.
+      };
+
+      let renameBackup = index =>
+      {
+        if (index > 0)
+        {
+          return IO.renameFile(this.getBackupName(index),
+                               this.getBackupName(index + 1))
+                   .catch(ignoreErrors)
+                   .then(() => renameBackup(index - 1));
+        }
+
+        return IO.renameFile(this.sourceFile, this.getBackupName(1))
+                 .catch(ignoreErrors);

[Wladimir Palant, 2017/04/10 14:59:15]

The logic is slightly simplified by not removing the last backup explicitly any
more. The assumption is that IO.renameFile() will always overwrite existing
files (which it does for the remaining implementations).

+      };
+
+      // Rename existing files
+      return renameBackup(Prefs.patternsbackups - 1);
+    }).catch(error =>
+    {
+      // Errors during backup creation shouldn't prevent writing filters.
+      Cu.reportError(error);
+    }).then(() =>
+    {
+      return IO.writeToFile(this.sourceFile, this.exportData());
+    }).then(() =>
+    {
+      FilterNotifier.triggerListeners("save");
+    }).catch(error =>
+    {
+      // If saving failed, report error but continue - we still have to process
+      // flags.
+      Cu.reportError(error);
+    }).then(() =>
+    {
+      this._saving = false;
+      if (this._needsSave)
+      {
+        this._needsSave = false;
+        this.saveToDisk();
+      }
+    });
   },
 
   /**
    * @typedef FileInfo
    * @type {object}
    * @property {nsIFile} file
    * @property {number} lastModified
    */
@@ -725,40 +657,33 @@ let FilterStorage = exports.FilterStorag
   /**
    * Returns a promise resolving in a list of existing backup files.
    * @return {Promise.<FileInfo[]>}
    */
   getBackupFiles()
   {
     let backups = [];
 
-    let [, part1, part2] = /^(.*)(\.\w+)$/.exec(
-      FilterStorage.sourceFile.leafName
-    ) || [null, FilterStorage.sourceFile.leafName, ""];
-
     function checkBackupFile(index)
     {
-      return new Promise((resolve, reject) =>
+      return IO.statFile(this.getBackupName(index)).then(statData =>
       {
-        let file = FilterStorage.sourceFile.clone();
-        file.leafName = part1 + "-backup" + index + part2;
+        if (!statData.exists)
+          return backups;
 
-        IO.statFile(file, (error, result) =>
-        {
-          if (!error && result.exists)
-          {
-            backups.push({
-              index,
-              lastModified: result.lastModified
-            });
-            resolve(checkBackupFile(index + 1));
-          }
-          else
-            resolve(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);
   }
 };
 
 /**
@@ -789,17 +714,17 @@ function removeSubscriptionFilters(subsc
   {
     let i = filter.subscriptions.indexOf(subscription);
     if (i >= 0)
       filter.subscriptions.splice(i, 1);
   }
 }
 
 /**
- * IO.readFromFile() listener to parse filter data.
+ * Listener returned by FilterStorage.importData(), parses filter data.
  * @constructor
  */
 function INIParser()
 {
   this.fileProperties = this.curObj = {};
   this.subscriptions = [];
   this.knownFilters = Object.create(null);
   this.knownSubscriptions = Object.create(null);
@@ -892,16 +817,10 @@ INIParser.prototype =
       else if (this.wantObj === false && val)
         this.curObj.push(val.replace(/\\\[/g, "["));
     }
     finally
     {
       Filter.knownFilters = origKnownFilters;
       Subscription.knownSubscriptions = origKnownSubscriptions;
     }
-
-    // Allow events to be processed every now and then.
-    // Note: IO.readFromFile() will deal with the potential reentrance here.
-    this.linesProcessed++;
-    if (this.linesProcessed % 1000 == 0)
-      return Utils.yield();
   }
 };