Issue 4023 - Use localforage (IndexedDB) instead of localStorage

lib/adblockplus.js

Index: lib/adblockplus.js
===================================================================
--- a/lib/adblockplus.js
+++ b/lib/adblockplus.js
@@ -357,60 +357,44 @@
     return keyPrefix + (file instanceof FakeFile ? file.path : file.spec);
   }
 
-  function loadFile(file)
-  {
-    return new Promise(function(resolve, reject)
-    {
-      var key = fileToKey(file);
-      ext.storage.get([key], function(items)
-      {
-        var entry = items[key];
-        if (!entry)
-        {
-          try
-          {
-            entry = JSON.parse(window.localStorage.getItem(key));
-          }
-          catch (err)
-          {}
-        }
-        if (entry)
-        {
-          resolve(entry);
-        }
+  function loadFile(file, successCallback, errorCallback)
+  {
+    var key = fileToKey(file);
+    var entry = localStorage.getItem(key);
+    if (entry)
+    {
+      try
+      {
+        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
+    {
+      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
-        {
-          reject(new Error("File doesn't exist"));
-        }
+          successCallback(value);
       });
-    }.bind(this));
+    }
   }
+
   function saveFile(file, data, callback)
   {
-    var entry = {};
     var key = fileToKey(file);
+    var entry = {
+      lastModified: Date.now(),
+      content: data
+    };
 
-    if (typeof browser == "undefined")
-    {
-      entry[key] = {
-        lastModified: Date.now(),
-        content: data
-      };
-      ext.storage.set(entry, callback);
-    }
-    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);
-    }
-    callback();
+    localStorage.removeItem(key);
+    localforage.setItem(key, entry, callback);
   }
   exports.IO = {
     resolveFilePath: function(path)
@@ -423,10 +407,6 @@
       {
         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];
@@ -435,7 +415,7 @@
         }
         callback(null);
       }
-      loadFile(file).then(onLoaded, callback);
+      loadFile(file, onLoaded, callback);
     },
     writeToFile: function(file, data, callback)
     {
@@ -447,7 +427,7 @@
       {
         saveFile(toFile, entry.content, callback);
       }
-      loadFile(file).then(onLoaded, callback);
+      loadFile(file, onLoaded, callback);
     },
     renameFile: function(fromFile, newName, callback)
     {
@@ -458,7 +438,7 @@
           ext.storage.set(keyPrefix + newName, entry, callback);
         });
       }
-      loadFile(file).then(onLoaded, callback);
+      loadFile(file, onLoaded, callback);
     },
     removeFile: function(file, callback)
     {
@@ -474,7 +454,7 @@
           lastModified: entry.lastModified
         });
       }
-      loadFile(file).then(onLoaded, callback);
+      loadFile(file, onLoaded, callback);
     }
   };
   return exports;