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 196 matching lines @@
  * @type {number}
  */
 defaults.last_updates_page_displayed = 0;
 
 /**
   * @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
+   */
+  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.
    *
    * @param {string}   preference
    * @param {function} callback
    */
   on(preference, callback)
   {
     eventEmitter.on(preference, callback);
   },
@@ skipping 27 matching lines @@
   return key.substr(keyPrefix.length);
 }
 
 function prefToKey(pref)
 {
   return keyPrefix + pref;
 }
 
 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();
 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;
   customSave.set("blocked_total", pref =>
   {
-    if (updateScheduled)
-      return;
+    if (!updateScheduled)
+    {
+      let callback = () =>
+      {
+        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();
   });
 }
 
 function addPreference(pref)
 {
   Object.defineProperty(Prefs, pref, {
     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
   });
 }
 
 function init()
 {
   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 =>
   {
     if ("managed" in browser.storage)
     {
       browser.storage.managed.get(null, items =>
       {
         // Opera doesn't support browser.storage.managed, but instead simply
         // removing the API, Opera sets browser.runtime.lastError when using it.
@@ skipping 31 matching lines @@
           eventEmitter.emit(pref);
         }
       }
     });
   }
 
   Prefs.untilLoaded = Promise.all([localLoaded, managedLoaded]).then(onLoaded);
 }
 
 init();