Issue 4023 - Use localforage (IndexedDB) instead of localStorage

lib/adblockplus.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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
@@ skipping 339 matching lines @@
 require.scopes["io"] = (function()
 {
   var exports = {};
   var keyPrefix = "file:";
 
   function fileToKey(file)
   {
     return keyPrefix + (file instanceof FakeFile ? file.path : file.spec);
   }
 
-  function loadFile(file)
+  function loadFile(file, successCallback, errorCallback)
   {
-    return new Promise(function(resolve, reject)
+    var key = fileToKey(file);
+    var entry = localStorage.getItem(key);
+    if (entry)
     {
-      var key = fileToKey(file);
-      ext.storage.get([key], function(items)
+      try
       {
-        var entry = items[key];
-        if (!entry)
-        {
-          try
-          {
-            entry = JSON.parse(window.localStorage.getItem(key));
-          }
-          catch (err)
-          {}
-        }
-        if (entry)
-        {
-          resolve(entry);
-        }
-        else
-        {
-          reject(new Error("File doesn't exist"));
-        }
-      });
-    }.bind(this));
-  }
-  function saveFile(file, data, callback)
-  {
-    var entry = {};
-    var key = fileToKey(file);
-
-    if (typeof browser == "undefined")
-    {
-      entry[key] = {
-        lastModified: Date.now(),
-        content: data
-      };
-      ext.storage.set(entry, callback);
+        entry = JSON.parse(entry);
+      }
+      catch(err) 
+      {
+        setTimeout(errorCallback(new Error("File is corrupted")));
+      }
+      setTimeout(successCallback(entry));

[Sebastian Noack, 2016/10/10 14:58:06]

So if there is an entry in localStorage but it isn't valid JSON, we first call
the error callback and then the success callback? This seems to be wrong.

[kzar, 2016/10/11 07:22:41]

Whoops you're right, sorry I should have spotted that too.

Ollie I think you need to return after the error callback from inside the
`catch(err)` block. (As a bonus if you return after the success callback too you
could avoid needing the else {...} block for the localforage stuff.) Also
perhaps we should check for `typeof == "string"` here explicitly?

How about something like this? (untested)

// First check localStorage in case the file hasn't been migrated to IndexDB yet
var entry = localStorage.getItem(key);
if (typeof entry == "string")
{
  try
  {
    entry = JSON.parse(entry);
  }
  catch(error)
  {
    setTimeout(errorCallback(new Error("File is corrupted")));
    return;
  }
  setTimeout(successCallback(entry));
  return;
}

// Finally check IndexDB
localforage.getItem(key).then(successCallback, errorCallback);

[Oleksandr, 2016/10/11 13:52:53]

Done.

     }
     else
     {
-      var processedData = LZString.compressToUTF16(JSON.stringify(data));
-      ext.storage.remove(key);
-      entry[key] = {
-        lastModified: Date.now(),
-        content: processedData,
-        compressed: true
-      };
-      window.localStorage.setItem(key, JSON.stringify(entry[key]));
-      setTimeout(callback, 0);
+      localforage.getItem(key, function(err, value) 
+      {
+        if (err || !value)

[Sebastian Noack, 2016/10/10 14:58:06]

The check for !value seems to be wrong. So if an entry stores an empty string, a
use case that is generally supported, you treat this as an error which I think
we should not.

[Oleksandr, 2016/10/11 13:52:13]

I don't think this is generally supported. This is not a string, but object with
`lastModified` and `content` properties. `content` may very well be an empty
string, but  if a value here is falsey this is very much an error.

[Sebastian Noack, 2016/10/11 17:15:15]

Alright, I missed that the value is an object not the string with the actual
data, still I wonder whether this check is necessary, but at least it shouldn't
break anything.

[Oleksandr, 2016/10/11 19:45:54]

It does break things if we remove this check. In case the value is not in
IndexedDB database there is no error, but the value is null. We need to map that
to error. For example for cases when the code is checking if the patterns-backup
exists (which is still does).

+          errorCallback(new Error("File doesn't exist"));                
+        else
+          successCallback(value);
+      });
     }
-    callback();
+  }
+
+  function saveFile(file, data, callback)
+  {
+    var key = fileToKey(file);
+    var entry = {
+      lastModified: Date.now(),
+      content: data
+    };
+
+    localStorage.removeItem(key);
+    localforage.setItem(key, entry, callback);
   }
   exports.IO = {
     resolveFilePath: function(path)
     {
       return new FakeFile(path);
     },
     readFromFile: function(file, listener, callback)
     {
       function onLoaded(entry)
       {
         if ("content" in entry)
         {
-          if (entry["compressed"])
-          {
-            entry.content = JSON.parse(LZString.decompressFromUTF16(entry.content));
-          }
           for (var _loopIndex15 = 0; _loopIndex15 < entry.content.length; ++_loopIndex15)
           {
             var line = entry.content[_loopIndex15];
             listener.process(line);
           }
         }
         callback(null);
       }
-      loadFile(file).then(onLoaded, callback);
+      loadFile(file, onLoaded, callback);
     },
     writeToFile: function(file, data, callback)
     {
       saveFile(file, data, callback);
     },
     copyFile: function(fromFile, toFile, callback)
     {
       function onLoaded(entry)
       {
         saveFile(toFile, entry.content, callback);
       }
-      loadFile(file).then(onLoaded, callback);
+      loadFile(file, onLoaded, callback);
     },
     renameFile: function(fromFile, newName, callback)
     {
       function onLoaded()
       {
         ext.storage.remove(fileToKey(fromFile), function()
         {
           ext.storage.set(keyPrefix + newName, entry, callback);
         });
       }
-      loadFile(file).then(onLoaded, callback);
+      loadFile(file, onLoaded, callback);
     },
     removeFile: function(file, callback)
     {
       ext.storage.remove(fileToKey(file), callback);
     },
     statFile: function(file, callback)
     {
       function onLoaded(entry)
       {
         callback(null,
         {
           exists: true,
           lastModified: entry.lastModified
         });
       }
-      loadFile(file).then(onLoaded, callback);
+      loadFile(file, onLoaded, callback);
     }
   };
   return exports;
 })();
 require.scopes["downloader"] = (function()
 {
   var exports = {};
   var Utils = require("utils").Utils;
   var MILLIS_IN_SECOND = exports.MILLIS_IN_SECOND = 1000;
   var MILLIS_IN_MINUTE = exports.MILLIS_IN_MINUTE = 60 * MILLIS_IN_SECOND;
@@ skipping 6088 matching lines @@
     search.push("notificationDownloadCount=" + encodeURIComponent(downlCount));
     chrome.runtime.setUninstallURL(Utils.getDocLink("uninstalled") + "&" + search.join("&"));
   }
   if ("setUninstallURL" in chrome.runtime)
   {
     Prefs.untilLoaded.then(setUninstallURL);
     Prefs.on("notificationdata", setUninstallURL);
   }
   return exports;
 })();