Issue 5059 - Simplify I/O API and FilterStorage implementation

lib/filterStorage.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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.
  */
 
-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;
 
 /**
  * This class reads user's filters from disk, manages them in memory
  * and writes them back.
@@ skipping 10 matching lines @@
   /**
    * Version number of the patterns.ini format used.
    * @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,
 
   /**
    * Map of properties listed in the filter storage file before the sections
@@ skipping 284 matching lines @@
     }
   },
 
   /**
    * @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
         // they are empty
         let specialMap = new Set(["~il~", "~wl~", "~fl~", "~eh~"]);
@@ skipping 16 matching lines @@
         this.knownSubscriptions = knownSubscriptions;
         Filter.knownFilters = parser.knownFilters;
         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()
   {
     // Do not persist external subscriptions
     let subscriptions = this.subscriptions.filter(
@@ skipping 47 matching lines @@
 
   /**
    * 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
    */
 
   /**
    * 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);
   }
 };
 
 /**
  * Joins subscription's filters to the subscription without any notifications.
  * @param {Subscription} subscription
@@ skipping 20 matching lines @@
 
   for (let filter of subscription.filters)
   {
     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);
 }
 INIParser.prototype =
@@ skipping 82 matching lines @@
         }
       }
       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();
   }
 };