Issue 6599 - Detect data corruption of storage.local

lib/prefs.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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 201 matching lines @@
   * @namespace
   * @static
   */
 let Prefs = exports.Prefs = {
   /**
    * Sets the given preference.
    *
    * @param {string} preference
    * @param {any}    value
    * @return {Promise} A promise that resolves when the underlying
-                       browser.storage.local.set() operation completes
+                       browser.storage.local.set/remove() operation completes
    */
   set(preference, value)
   {
     let defaultValue = defaults[preference];
 
     if (typeof value != typeof defaultValue)
       throw new Error("Attempt to change preference type");
 
     if (value == defaultValue)
     {
       delete overrides[preference];
       return browser.storage.local.remove(prefToKey(preference));

[kzar, 2018/04/24 16:18:46]

Nit: Since we're just calling browser.storage.local.remove directly here along
with prefToKey it seems kinda inconsistent that we're then using this special
savePref function for saving which does that for us.

[Sebastian Noack, 2018/04/24 17:49:25]

Well, nothing changed in this regard. Except for returning a promise, the code
in this function is identical to the code we had in the setter before. Also note
that savePref() is also called from the customSave callback, so if we want to
get rid of it we also have to inline the code there. Plus, the one liner below
will become a little more complex (since we cannot just use || then anymore).

     }
 
     overrides[preference] = value;
     return (customSave.get(preference) || savePref)(preference);
   },
 
   /**
    * Adds a callback that is called when the
    * value of a specified preference changed.
    *
@@ skipping 34 matching lines @@
   return key.substr(keyPrefix.length);
 }
 
 function prefToKey(pref)
 {
   return keyPrefix + pref;
 }
 
 function savePref(pref)
 {
-  return browser.storage.local.set(prefToKey(pref), overrides[pref]);
+  return browser.storage.local.set({[prefToKey(pref)]: overrides[pref]});
 }
 
 let customSave = new Map();
 if (require("../buildtools/info").platform == "gecko")
 {
   // Saving one storage value causes all others to be saved as well on Gecko.
   // Make sure that updating ad counter doesn't cause the filters data to be
   // saved frequently as a side-effect.
   const MIN_UPDATE_INTERVAL = 60 * 1000;
   let lastUpdate = -MIN_UPDATE_INTERVAL;
-  let updateScheduled = false;
+  let promise = null;
   customSave.set("blocked_total", pref =>
   {
-    if (!updateScheduled)
-    {
-      let callback = () =>
+    if (!promise)
+    {
+      promise = new Promise((resolve, reject) =>
       {
-        lastUpdate = performance.now();
-        updateScheduled = false;
-        savePref(pref);
-      };
-
-      let timeElapsed = performance.now() - lastUpdate;
-      if (timeElapsed < MIN_UPDATE_INTERVAL)
-      {
-        setTimeout(callback, MIN_UPDATE_INTERVAL - timeElapsed);
-        updateScheduled = true;
-      }
-      else
-        callback();
+        setTimeout(
+          () =>
+          {
+            lastUpdate = performance.now();
+            promise = null;
+            savePref(pref).then(resolve, reject);
+          },
+          lastUpdate + MIN_UPDATE_INTERVAL - performance.now()
+        );
+      });
     }
-    return Promise.resolve();
+    return promise;
   });
 }
 
 function addPreference(pref)
 {
   Object.defineProperty(Prefs, pref, {
     get() { return (pref in overrides ? overrides : defaults)[pref]; },
     set(value)
     {
       Prefs.set(pref, value);
     },
     enumerable: true
   });
 }
 
 function init()
 {
   let prefs = Object.keys(defaults);
   prefs.forEach(addPreference);
 
-  let localLoaded = browser.storage.local.get(prefs.map(prefToKey)).then(items =>

[kzar, 2018/04/24 16:18:46]

Nit: This line is too long in Rietveld, did it pass ESLint?

[Sebastian Noack, 2018/04/24 17:49:25]

This is no longer the case after the rebase.

[kzar, 2018/04/25 10:53:51]

Acknowledged.

-  {
-    for (let key in items)
-      overrides[keyToPref(key)] = items[key];

[kzar, 2018/04/24 16:18:46]

We just had the key above, so is there not a way we can remember it to avoid
looking it up again here?

[Sebastian Noack, 2018/04/24 17:49:25]

What do you have in mind?

  let keys = prefs.map(prefToKey);
  let localLoaded = browser.storage.local.get(keys).then(items =>
  {
    for (let key of items)
      overrides[prefs[keys.indexOf(key)]] = items[key];
  });

Hardly any more readable (or faster) than what we are currently doing.

[kzar, 2018/04/25 10:53:51]

Yea, fair enough I can't see a much better way to do it either.

[kzar, 2018/04/25 10:59:07]

(I had assumed that browser.storage.local.get would return an Array of values if
you requested an Array of values, but it returns them in an Object.)

-  });
-
-  let managedLoaded = new Promise(resolve =>
-  {
-    if ("managed" in browser.storage)
-    {
-      browser.storage.managed.get(null, items =>
+  let localLoaded = browser.storage.local.get(prefs.map(prefToKey)).then(
+    items =>
+    {
+      for (let key in items)
+        overrides[keyToPref(key)] = items[key];
+    }
+  );
+
+  let managedLoaded;
+  if ("managed" in browser.storage)
+  {
+    managedLoaded = browser.storage.managed.get(null).then(
+      items =>
       {
-        // Opera doesn't support browser.storage.managed, but instead simply
-        // removing the API, Opera sets browser.runtime.lastError when using it.
-        // So we have to retrieve that error, to prevent it from showing up
-        // in the console.
-        browser.runtime.lastError;
-
         for (let key in items)
           defaults[key] = items[key];
-
-        resolve();
-      });
-    }
-    else
-    {
-      resolve();
-    }
-  });
+      },
+
+      // Opera doesn't support browser.storage.managed, but instead of simply
+      // removing the API, it gives an asynchronous error which we ignore here.
+      () => {}
+    );
+  }
+  else
+  {
+    managedLoaded = Promise.resolve();
+  }
 
   function onLoaded()
   {
-    ext.storage.onChanged.addListener(changes =>
+    browser.storage.onChanged.addListener(changes =>
     {
       for (let key in changes)
       {
         let pref = keyToPref(key);
         if (pref && pref in defaults)
         {
           let change = changes[key];
           if ("newValue" in change && change.newValue != defaults[pref])
             overrides[pref] = change.newValue;
           else
             delete overrides[pref];
 
           eventEmitter.emit(pref);
         }
       }
     });
   }
 
   Prefs.untilLoaded = Promise.all([localLoaded, managedLoaded]).then(onLoaded);
 }
 
 init();