Issue 6599 - Detect data corruption of storage.local

lib/prefs.js

Index: lib/prefs.js
===================================================================
--- a/lib/prefs.js
+++ b/lib/prefs.js
@@ -214,6 +214,31 @@
   */
 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
+   */
+  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.
    *
@@ -261,7 +286,7 @@
 
 function savePref(pref)
 {
-  ext.storage.set(prefToKey(pref), overrides[pref]);

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

As discussed in IRC let's replace ext.storage with browser.storage everywhere in
another patch set, to keep make this one easier to understand.

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

There we go: https://codereview.adblockplus.org/29760680

I now rebased this patch on top of that change.

+  return browser.storage.local.set(prefToKey(pref), overrides[pref]);
 }
 
 let customSave = new Map();
@@ -275,24 +300,25 @@
   let updateScheduled = false;
   customSave.set("blocked_total", pref =>
   {
-    if (updateScheduled)
-      return;
-
-    let callback = () =>
+    if (!updateScheduled)
     {
-      lastUpdate = performance.now();
-      updateScheduled = false;
-      savePref(pref);
-    };
+      let callback = () =>
+      {
+        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;
+      let timeElapsed = performance.now() - lastUpdate;
+      if (timeElapsed < MIN_UPDATE_INTERVAL)
+      {
+        setTimeout(callback, MIN_UPDATE_INTERVAL - timeElapsed);
+        updateScheduled = true;
+      }
+      else
+        callback();
     }
-    else
-      callback();
+    return Promise.resolve();
   });
 }
 
@@ -302,21 +328,7 @@
     get() { return (pref in overrides ? overrides : defaults)[pref]; },
     set(value)
     {
-      let defaultValue = defaults[pref];
-
-      if (typeof value != typeof defaultValue)
-        throw new Error("Attempt to change preference type");
-
-      if (value == defaultValue)
-      {
-        delete overrides[pref];
-        ext.storage.remove(prefToKey(pref));
-      }
-      else
-      {
-        overrides[pref] = value;
-        (customSave.get(pref) || savePref)(pref);
-      }
+      Prefs.set(pref, value);
     },
     enumerable: true
   });
@@ -327,15 +339,10 @@
   let prefs = Object.keys(defaults);
   prefs.forEach(addPreference);
 
-  let localLoaded = new Promise(resolve =>
+  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.

   {
-    ext.storage.get(prefs.map(prefToKey), items =>
-    {
-      for (let key in items)
-        overrides[keyToPref(key)] = items[key];
-
-      resolve();
-    });
+    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 =>