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/remove() operation completes
+   */
+  set(preference, value)
+  {
+    let defaultValue = defaults[preference];
+
+    if (typeof value != typeof defaultValue)
+      throw new Error("Attempt to change preference type");

[Thomas Greiner, 2018/04/27 14:50:12]

Why don't we return `Promise.reject()` here? It's quite unexpected to have to
catch errors in two different ways when expecting a promise.

[Sebastian Noack, 2018/04/27 16:01:03]

Well, if we already know immediately/synchronously about an error why signaling
it later? Also this isn't an error that would ever need to be caught, but its
more like an assertion that indicates a bug, if this error is ever thrown.
Furthermore, if using `Prefs.foo = ...` this function is called implicitly and
the promise won't be accessible.

[Thomas Greiner, 2018/04/27 16:27:33]

True, if that's a requirement we could check twice. Once before calling
`Prefs.set()` to throw the error and once inside `Prefs.set()` to return
`Promise.reject()`. That way we won't affect any code that already uses
`Prefs.set()` when we drop the setter.

[Sebastian Noack, 2018/04/27 16:33:22]

Well, compatibility with the setter property isn't the only reason to throw
synchronously here, as explained in my comment above.

[Thomas Greiner, 2018/04/27 17:03:32]

As I said, we can still throw the error in the setter while returning
`Promise.reject()` here so nothing will change how the setter is currently used
and it could therefore still fulfill the purpose of an assertion.

Going forward, however, it would allow us to use `Prefs.set().catch()` to catch
all errors, including this one, instead of having to put a try-catch around it
as well.

[Sebastian Noack, 2018/04/27 17:11:11]

Yes, we could signal the error asynchronously here and redundantly check in the
setter to signal it synchronously there, but this will involve some (undesired)
duplication. And again, I think it is totally fine if any calling code just
handles the promise rejection and ignores the error thrown here. It's just an
assertion, to avoid accidentally assigning a value of the wrong type, during
development. If this error is ever thrown, the code that triggers it should be
changed rather than handling that error.

[Thomas Greiner, 2018/04/27 17:16:06]

Note that assertions may fail asynchronously in which case such errors could
occur not only during development but also in production. So while I'm not happy
about ignoring errors, I can understand it since that appears to be a convention
here.

+
+    if (value == defaultValue)
+    {
+      delete overrides[preference];
+      return browser.storage.local.remove(prefToKey(preference));
+    }
+
+    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)
 {
-  browser.storage.local.set({[prefToKey(pref)]: overrides[pref]});
+  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();

[Thomas Greiner, 2018/04/27 14:50:12]

What if an error occurs in `savePref()`? I'd argue we'd want to catch that so
returning the promise from `savePref()` here, instead of `Promise.resolve()`
would be helpful.

[Sebastian Noack, 2018/04/27 16:01:04]

Yes, returning a resolved promise immediately is a hack, so is this whole
workaround here. Returning a promise that resolves when the deferred storage
operation completes, will make the code notably more complex, plus it might take
up to a minute for the promise to resolve, which might have weird side effects
if we'd ever rely on a promise returned here. Also note that this code path is
essentially only used for the blocked_total preference and only on Firefox.

[Sebastian Noack, 2018/04/27 16:14:52]

Never mind, it turned out returning asynchronously here is simple enough. Done.

[Thomas Greiner, 2018/04/27 16:27:33]

Thanks.

     }
-    else
-      callback();
+    return Promise.resolve();
   });
 }
 
@@ -302,21 +328,7 @@
     get() { return (pref in overrides ? overrides : defaults)[pref]; },
     set(value)

[Thomas Greiner, 2018/04/27 14:50:12]

Suggestion: What about marking this setter as deprecated to indicate that any
new code should be using `Prefs.set()`?

[Sebastian Noack, 2018/04/27 16:01:03]

We didn't decide yet, whether we want to consider using the setter properties
deprecated, or rather only use the set() method in order to detect the data
corruption during initialization.

[Thomas Greiner, 2018/04/27 16:27:33]

Acknowledged.

     {
-      let defaultValue = defaults[pref];
-
-      if (typeof value != typeof defaultValue)
-        throw new Error("Attempt to change preference type");
-
-      if (value == defaultValue)
-      {
-        delete overrides[pref];
-        browser.storage.local.remove(prefToKey(pref));
-      }
-      else
-      {
-        overrides[pref] = value;
-        (customSave.get(pref) || savePref)(pref);
-      }
+      Prefs.set(pref, value);
     },
     enumerable: true
   });
@@ -332,10 +344,6 @@
     {
       for (let key in items)
         overrides[keyToPref(key)] = items[key];
-    },
-    (error) =>
-    {
-      console.error(error);
     }
   );