Issue 5052 - Allow importing and exporting filterStorage data without file access

lib/filterStorage.js

Index: lib/filterStorage.js
===================================================================
--- a/lib/filterStorage.js
+++ b/lib/filterStorage.js
@@ -391,183 +391,200 @@ let FilterStorage = exports.FilterStorag
     }
     for (let filter of filters)
     {
       filter.hitCount = 0;
       filter.lastHit = 0;
     }
   },
 
-  _loading: false,
+  /**
+   * @callback TextSink
+   * @param {string?} line
+   */
+
+  /**
+   * Allows importing previously serialized filter data.
+   * @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()
+  {
+    let parser = new INIParser();
+    return line =>
+    {
+      parser.process(line);

[kzar, 2017/03/30 12:58:48]

I wonder why we parser.process the final null?

[Wladimir Palant, 2017/03/30 13:02:06]

The parser uses it to flush out whatever object was "in progress" right now.

[kzar, 2017/03/30 13:10:54]

Acknowledged.

+      if (line === null)
+      {
+        // Old special groups might have been converted, remove them if
+        // they are empty
+        let specialMap = new Set(["~il~", "~wl~", "~fl~", "~eh~"]);
+        let knownSubscriptions = Object.create(null);
+        for (let i = 0; i < parser.subscriptions.length; i++)
+        {
+          let subscription = parser.subscriptions[i];
+          if (subscription instanceof SpecialSubscription &&
+              subscription.filters.length == 0 &&
+              specialMap.has(subscription.url))
+          {
+            parser.subscriptions.splice(i--, 1);
+          }
+          else
+            knownSubscriptions[subscription.url] = subscription;
+        }
+
+        this.fileProperties = parser.fileProperties;
+        this.subscriptions = parser.subscriptions;
+        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");
+      }
+    };
+  },
 
   /**
    * Loads all subscriptions from the disk
-   * @param {nsIFile} [sourceFile] File to read from
    */
-  loadFromDisk(sourceFile)
+  loadFromDisk()
   {
-    if (this._loading)
-      return;
-
-    this._loading = true;
-
-    let readFile = (currentSourceFile, backupIndex) =>
+    let readFile = () =>
     {
-      let parser = new INIParser();
-      IO.readFromFile(currentSourceFile, parser, readFromFileException =>
+      let parser = {
+        process: this.importData()
+      };
+      IO.readFromFile(this.sourceFile, parser, readFromFileException =>
       {
-        if (!readFromFileException && parser.subscriptions.length == 0)
+        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 && !explicitFile)
-        {
-          // Attempt to load a backup
-          currentSourceFile = this.sourcefile;
-          if (currentSourceFile)
-          {
-            let [, part1, part2] = /^(.*)(\.\w+)$/.exec(
-              currentSourceFile.leafName
-            ) || [null, currentSourceFile.leafName, ""];
-
-            currentSourceFile = currentSourceFile.clone();
-            currentSourceFile.leafName = (
-              part1 + "-backup" + (++backupIndex) + part2
-            );
-
-            IO.statFile(currentSourceFile, (statFileException, statData) =>
-            {
-              if (!statFileException && statData.exists)
-                readFile(currentSourceFile, backupIndex);
-              else
-                doneReading(parser);
-            });
-            return;
-          }
-        }
-        doneReading(parser);
+        if (readFromFileException)
+          tryBackup(1);
       });
     };
 
-    let doneReading = parser =>
+    let tryBackup = backupIndex =>
     {
-      // Old special groups might have been converted, remove them if
-      // they are empty
-      let specialMap = {"~il~": true, "~wl~": true, "~fl~": true, "~eh~": true};
-      let knownSubscriptions = Object.create(null);
-      for (let i = 0; i < parser.subscriptions.length; i++)
+      this.restoreBackup(backupIndex).then(() =>
       {
-        let subscription = parser.subscriptions[i];
-        if (subscription instanceof SpecialSubscription &&
-            subscription.filters.length == 0 && subscription.url in specialMap)
-        {
-          parser.subscriptions.splice(i--, 1);
-        }
-        else
-          knownSubscriptions[subscription.url] = subscription;
-      }
-
-      this.fileProperties = parser.fileProperties;
-      this.subscriptions = parser.subscriptions;
-      this.knownSubscriptions = knownSubscriptions;
-      Filter.knownFilters = parser.knownFilters;
-      Subscription.knownSubscriptions = parser.knownSubscriptions;
-
-      if (parser.userFilters)
+        if (this.subscriptions.length == 0)
+          tryBackup(backupIndex + 1);
+      }).catch(error =>
       {
-        for (let i = 0; i < parser.userFilters.length; i++)
-        {
-          let filter = Filter.fromText(parser.userFilters[i]);
-          this.addFilter(filter, null, undefined, true);
-        }
-      }
-
-      this._loading = false;
-      FilterNotifier.triggerListeners("load");
-
-      if (sourceFile != this.sourceFile)
-        this.saveToDisk();
+        // Give up
+      });
     };
 
-    let explicitFile;
-    if (sourceFile)
+    IO.statFile(this.sourceFile, (statError, statData) =>
     {
-      explicitFile = true;
-      readFile(sourceFile, 0);
-    }
-    else
-    {
-      explicitFile = false;
-      ({sourceFile} = FilterStorage);
-
-      let callback = function(e, statData)
+      if (statError || !statData.exists)
       {
-        if (e || !statData.exists)
-        {
-          this.firstRun = true;
-          this._loading = false;
-          FilterNotifier.triggerListeners("load");
-        }
-        else
-          readFile(sourceFile, 0);
-      }.bind(this);
-
-      if (sourceFile)
-        IO.statFile(sourceFile, callback);
+        this.firstRun = true;
+        FilterNotifier.triggerListeners("load");
+      }
       else
-        callback(true);
-    }
+        readFile();
+    });
   },
 
-  *_generateFilterData(subscriptions)
+  /**
+   * Restores an automatically created backup.
+   * @param {number} backupIndex
+   *    number of the backup to restore (1 being the most recent)
+   * @return {Promise} promise resolved or rejected when restoring is complete
+   */
+  restoreBackup(backupIndex)
   {
+    return new Promise((resolve, reject) =>
+    {
+      // Attempt to load a backup
+      let [, part1, part2] = /^(.*)(\.\w+)$/.exec(

[kzar, 2017/03/30 12:58:48]

Well I just learned something, I didn't realise values could be dropped with
destructuring like that.

+        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();
+        }
+      });
+    });
+  },
+
+  /**
+   * Generator serializing filter data and yielding it line by line.
+   */
+  *exportData()
+  {
+    // Do not persist external subscriptions

[kzar, 2017/03/30 12:58:48]

Nit: I guess we could avoid doing this filtering until after the header lines
have been consumed. Probably doesn't matter in practice.

[Wladimir Palant, 2017/03/30 13:02:06]

Better not. This is creating a copy of the list for another reason as well - if
the list changes in the meantime we won't save bogus data. And consumption of
generator data is async.

[kzar, 2017/03/30 13:10:54]

Ah OK, makes sense.

+    let subscriptions = this.subscriptions.filter(
+      s => !(s instanceof ExternalSubscription)
+    );
+
     yield "# Adblock Plus preferences";
     yield "version=" + formatVersion;
 
-    let saved = Object.create(null);
+    let saved = new Set();
     let buf = [];
 
     // Save filter data
-    for (let i = 0; i < subscriptions.length; i++)
+    for (let subscription of subscriptions)
     {
-      let subscription = subscriptions[i];
-      for (let j = 0; j < subscription.filters.length; j++)
+      for (let filter of subscription.filters)
       {
-        let filter = subscription.filters[j];
-        if (!(filter.text in saved))
+        if (!saved.has(filter.text))
         {
           filter.serialize(buf);
-          saved[filter.text] = filter;
-          for (let k = 0; k < buf.length; k++)
-            yield buf[k];
+          saved.add(filter.text);
+          for (let line of buf)
+            yield line;
           buf.splice(0);
         }
       }
     }
 
     // Save subscriptions
-    for (let i = 0; i < subscriptions.length; i++)
+    for (let subscription of subscriptions)
     {
-      let subscription = subscriptions[i];
-
       yield "";
 
       subscription.serialize(buf);
       if (subscription.filters.length)
       {
         buf.push("", "[Subscription filters]");
         subscription.serializeFilters(buf);
       }
-      for (let k = 0; k < buf.length; k++)
-        yield buf[k];
+      for (let line of buf)
+        yield line;
       buf.splice(0);
     }
   },
 
   /**
    * Will be set to true if saveToDisk() is running (reentrance protection).
    * @type {boolean}
    */
@@ -577,66 +594,56 @@ 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
-   * @param {nsIFile} [targetFile] File to be written
    */
-  saveToDisk(targetFile)
+  saveToDisk()
   {
-    let explicitFile = true;
-    if (!targetFile)
-    {
-      targetFile = FilterStorage.sourceFile;
-      explicitFile = false;
-    }
-    if (!targetFile)
-      return;
-
-    if (!explicitFile && this._saving)
+    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._generateFilterData(subscriptions), e =>
+      IO.writeToFile(targetFile, this.exportData(), e =>
       {
-        if (!explicitFile)
-          this._saving = false;
+        this._saving = false;
 
         if (e)
           Cu.reportError(e);
 
-        if (!explicitFile && this._needsSave)
+        if (this._needsSave)
         {
           this._needsSave = false;
           this.saveToDisk();
         }
         else
           FilterNotifier.triggerListeners("save");
       });
     };
 
     let checkBackupRequired = (callbackNotRequired, callbackRequired) =>
     {
-      if (explicitFile || Prefs.patternsbackups <= 0)
+      if (Prefs.patternsbackups <= 0)
         callbackNotRequired();
       else
       {
         IO.statFile(targetFile, (statFileException, statData) =>
         {
           if (statFileException || !statData.exists)
             callbackNotRequired();
           else
@@ -689,22 +696,17 @@ let FilterStorage = exports.FilterStorag
       {
         let toFile = targetFile.clone();
         toFile.leafName = part1 + "-backup" + (index + 1) + part2;
 
         IO.copyFile(targetFile, toFile, writeFilters);
       }
     };
 
-    // Do not persist external subscriptions
-    let subscriptions = this.subscriptions.filter(
-      s => !(s instanceof ExternalSubscription)
-    );
-    if (!explicitFile)
-      this._saving = true;
+    this._saving = true;
 
     checkBackupRequired(writeFilters, removeLastBackup);
   },
 
   /**
    * @typedef FileInfo
    * @type {object}
    * @property {nsIFile} file
@@ -730,17 +732,17 @@ let FilterStorage = exports.FilterStorag
         let file = FilterStorage.sourceFile.clone();
         file.leafName = part1 + "-backup" + index + part2;
 
         IO.statFile(file, (error, result) =>
         {
           if (!error && result.exists)
           {
             backups.push({
-              file,
+              index,
               lastModified: result.lastModified
             });
             resolve(checkBackupFile(index + 1));
           }
           else
             resolve(backups);
         });
       });