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";
   },
 
   /**
    * 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);

[kzar, 2017/04/20 06:49:25]

So we return a promise here, which might recursively return more promises while
we attempt to restore from our backups. But if tryBackup gives up it will catch
the error and I think the promise here will resolve anyway. Is that what you
intended? It seems kind of counter-intuitive that the promise isn't rejected in
that case.

[Wladimir Palant, 2017/04/20 07:17:06]

Yes, this is the intended behavior - we already reported an error, and we are
attempting a fallback now. There is little point reporting another error because
the fallback failed as well.

+    }).then(() =>
+    {
+      this.initialized = true;
+      FilterNotifier.triggerListeners("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
+   */
+  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) {}
-
-    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);
+      };
+
+      // 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,42 +657,35 @@ 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)
+    let 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);
   }
 };
 
 /**
  * Joins subscription's filters to the subscription without any notifications.
  * @param {Subscription} subscription
@@ -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();
   }
 };