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 208 matching lines @@
    * @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);
   },
@@ skipping 50 matching lines @@
 }
 
 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();

[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.

+        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)

[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.

     {
       Prefs.set(pref, value);
     },
     enumerable: true
   });
 }
 
 function init()
 {
   let prefs = Object.keys(defaults);
@@ skipping 45 matching lines @@
           eventEmitter.emit(pref);
         }
       }
     });
   }
 
   Prefs.untilLoaded = Promise.all([localLoaded, managedLoaded]).then(onLoaded);
 }
 
 init();