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 342 matching lines @@
   var keyPrefix = "file:";
 
   function fileToKey(file)
   {
     return keyPrefix + (file instanceof FakeFile ? file.path : file.spec);
   }
 
   function loadFile(file, successCallback, errorCallback)
   {
     var key = fileToKey(file);
+    // Make sure we do not have subscriptions in localStorage from older versions first
     var entry = localStorage.getItem(key);
-    if (entry)
+    if (typeof entry == "string")
     {
       try
       {
         entry = JSON.parse(entry);
       }
       catch(err) 
       {
         setTimeout(errorCallback(new Error("File is corrupted")));
+        return;
       }
       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
-          successCallback(value);
-      });
-    }
+      return;
+    }
+    // Now try to read from IndexedDB
+    localforage.getItem(key, function(err, value) 
+    {
+      if (err || !value)
+        errorCallback(new Error("File doesn't exist"));                
+      else
+        successCallback(value);
+    });
   }
 
   function saveFile(file, data, callback)
   {
     var key = fileToKey(file);
     var entry = {
       lastModified: Date.now(),
       content: data
     };
 
@@ skipping 6160 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;
 })();