Merge pull request #729 from Osmose/storage-refactor

Refactor Storage.jsm into a class.

docs/dev/driver.rst

@@ -211,3 +211,8 @@ Storage
 
       :param key: Key to remove.
       :returns: A Promise that resolves when the value has been removed.
+
+   .. js:function:: clear()
+
+      Removes all stored values.
+      :returns: A Promise that resolves when the values have been removed.

recipe-client-addon/lib/NormandyDriver.jsm

@@ -123,15 +123,20 @@ this.NormandyDriver = function(sandboxManager) {
       return ret;
     },
 
-    createStorage(keyPrefix) {
-      let storage;
-      try {
-        storage = Storage.makeStorage(keyPrefix, sandbox);
-      } catch (e) {
-        log.error(e.stack);
-        throw e;
+    createStorage(prefix) {
+      const storage = new Storage(prefix);
+
+      // Wrapped methods that we expose to the sandbox. These are documented in
+      // the driver spec in docs/dev/driver.rst.
+      const storageInterface = {};
+      for (const method of ["getItem", "setItem", "removeItem", "clear"]) {
+        storageInterface[method] = sandboxManager.wrapAsync(storage[method].bind(storage), {
+          cloneArguments: true,
+          cloneInto: true,
+        });
       }
-      return storage;
+
+      return sandboxManager.cloneInto(storageInterface, {cloneFunctions: true});
     },
 
     setTimeout(cb, time) {

recipe-client-addon/lib/Storage.jsm

@@ -6,142 +6,84 @@
 
 const {utils: Cu} = Components;
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
-Cu.import("resource://shield-recipe-client/lib/LogManager.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "JSONFile", "resource://gre/modules/JSONFile.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
 
 this.EXPORTED_SYMBOLS = ["Storage"];
 
-const log = LogManager.getLogger("storage");
-let storePromise;
+// Lazy-load JSON file that backs Storage instances.
+XPCOMUtils.defineLazyGetter(this, "lazyStore", async function() {
+  const path = OS.Path.join(OS.Constants.Path.profileDir, "shield-recipe-client.json");
+  const store = new JSONFile({path});
+  await store.load();
+  return store;
+});
 
-function loadStorage() {
-  if (storePromise === undefined) {
-    const path = OS.Path.join(OS.Constants.Path.profileDir, "shield-recipe-client.json");
-    const storage = new JSONFile({path});
-    storePromise = (async function() {
-      await storage.load();
-      return storage;
-    })();
+this.Storage = class {
+  constructor(prefix) {
+    this.prefix = prefix;
   }
-  return storePromise;
-}
-
-this.Storage = {
-  makeStorage(prefix, sandbox) {
-    if (!sandbox) {
-      throw new Error("No sandbox passed");
-    }
-
-    const storageInterface = {
-      /**
-       * Sets an item in the prefixed storage.
-       * @returns {Promise}
-       * @resolves With the stored value, or null.
-       * @rejects Javascript exception.
-       */
-      getItem(keySuffix) {
-        return new sandbox.Promise((resolve, reject) => {
-          loadStorage()
-            .then(store => {
-              const namespace = store.data[prefix] || {};
-              const value = namespace[keySuffix] || null;
-              resolve(Cu.cloneInto(value, sandbox));
-            })
-            .catch(err => {
-              log.error(err);
-              reject(new sandbox.Error());
-            });
-        });
-      },
-
-      /**
-       * Sets an item in the prefixed storage.
-       * @returns {Promise}
-       * @resolves When the operation is completed succesfully
-       * @rejects Javascript exception.
-       */
-      setItem(keySuffix, value) {
-        return new sandbox.Promise((resolve, reject) => {
-          loadStorage()
-            .then(store => {
-              if (!(prefix in store.data)) {
-                store.data[prefix] = {};
-              }
-              store.data[prefix][keySuffix] = Cu.cloneInto(value, {});
-              store.saveSoon();
-              resolve();
-            })
-            .catch(err => {
-              log.error(err);
-              reject(new sandbox.Error());
-            });
-        });
-      },
-
-      /**
-       * Removes a single item from the prefixed storage.
-       * @returns {Promise}
-       * @resolves When the operation is completed succesfully
-       * @rejects Javascript exception.
-       */
-      removeItem(keySuffix) {
-        return new sandbox.Promise((resolve, reject) => {
-          loadStorage()
-            .then(store => {
-              if (!(prefix in store.data)) {
-                return;
-              }
-              delete store.data[prefix][keySuffix];
-              store.saveSoon();
-              resolve();
-            })
-            .catch(err => {
-              log.error(err);
-              reject(new sandbox.Error());
-            });
-        });
-      },
-
-      /**
-       * Clears all storage for the prefix.
-       * @returns {Promise}
-       * @resolves When the operation is completed succesfully
-       * @rejects Javascript exception.
-       */
-      clear() {
-        return new sandbox.Promise((resolve, reject) => {
-          return loadStorage()
-            .then(store => {
-              store.data[prefix] = {};
-              store.saveSoon();
-              resolve();
-            })
-            .catch(err => {
-              log.error(err);
-              reject(new sandbox.Error());
-            });
-        });
-      },
-    };
-
-    return Cu.cloneInto(storageInterface, sandbox, {
-      cloneFunctions: true,
-    });
-  },
 
   /**
    * Clear ALL storage data and save to the disk.
    */
-  clearAllStorage() {
-    return loadStorage()
-      .then(store => {
-        store.data = {};
-        store.saveSoon();
-      })
-      .catch(err => {
-        log.error(err);
-      });
-  },
+  static async clearAllStorage() {
+    const store = await lazyStore;
+    store.data = {};
+    store.saveSoon();
+  }
+
+  /**
+   * Sets an item in the prefixed storage.
+   * @returns {Promise}
+   * @resolves With the stored value, or null.
+   * @rejects Javascript exception.
+   */
+  async getItem(name) {
+    const store = await lazyStore;
+    const namespace = store.data[this.prefix] || {};
+    return namespace[name] || null;
+  }
+
+  /**
+   * Sets an item in the prefixed storage.
+   * @returns {Promise}
+   * @resolves When the operation is completed succesfully
+   * @rejects Javascript exception.
+   */
+  async setItem(name, value) {
+    const store = await lazyStore;
+    if (!(this.prefix in store.data)) {
+      store.data[this.prefix] = {};
+    }
+    store.data[this.prefix][name] = value;
+    store.saveSoon();
+  }
+
+  /**
+   * Removes a single item from the prefixed storage.
+   * @returns {Promise}
+   * @resolves When the operation is completed succesfully
+   * @rejects Javascript exception.
+   */
+  async removeItem(name) {
+    const store = await lazyStore;
+    if (this.prefix in store.data) {
+      delete store.data[this.prefix][name];
+      store.saveSoon();
+    }
+  }
+
+  /**
+   * Clears all storage for the prefix.
+   * @returns {Promise}
+   * @resolves When the operation is completed succesfully
+   * @rejects Javascript exception.
+   */
+  async clear() {
+    const store = await lazyStore;
+    store.data[this.prefix] = {};
+    store.saveSoon();
+  }
 };

recipe-client-addon/test/browser/browser_NormandyDriver.js

@@ -1,5 +1,7 @@
 "use strict";
 
+Cu.import("resource://shield-recipe-client/lib/NormandyDriver.jsm", this);
+
 add_task(withDriver(Assert, async function uuids(driver) {
   // Test that it is a UUID
   const uuid1 = driver.uuid();
@@ -42,3 +44,39 @@ add_task(withDriver(Assert, async function distribution(driver) {
   client = await driver.client();
   is(client.distribution, "funnelcake", "distribution is read from preferences");
 }));
+
+add_task(withSandboxManager(Assert, async function testCreateStorage(sandboxManager) {
+  const driver = new NormandyDriver(sandboxManager);
+  sandboxManager.cloneIntoGlobal("driver", driver, {cloneFunctions: true});
+
+  // Assertion helpers
+  sandboxManager.addGlobal("is", is);
+  sandboxManager.addGlobal("deepEqual", (...args) => Assert.deepEqual(...args));
+
+  await sandboxManager.evalInSandbox(`
+    (async function sandboxTest() {
+      const store = driver.createStorage("testprefix");
+      const otherStore = driver.createStorage("othertestprefix");
+      await store.clear();
+      await otherStore.clear();
+
+      await store.setItem("willremove", 7);
+      await otherStore.setItem("willremove", 4);
+      is(await store.getItem("willremove"), 7, "createStorage stores sandbox values");
+      is(
+        await otherStore.getItem("willremove"),
+        4,
+        "values are not shared between createStorage stores",
+      );
+
+      const deepValue = {"foo": ["bar", "baz"]};
+      await store.setItem("deepValue", deepValue);
+      deepEqual(await store.getItem("deepValue"), deepValue, "createStorage clones stored values");
+
+      await store.removeItem("willremove");
+      is(await store.getItem("willremove"), null, "createStorage removes items");
+
+      is('prefix' in store, false, "createStorage doesn't expose non-whitelist attributes");
+    })();
+  `);
+}));

recipe-client-addon/test/browser/browser_Storage.js

@@ -4,9 +4,8 @@ Cu.import("resource://shield-recipe-client/lib/Storage.jsm", this);
 Cu.import("resource://shield-recipe-client/lib/SandboxManager.jsm", this);
 
 add_task(async function() {
-  const fakeSandbox = {Promise};
-  const store1 = Storage.makeStorage("prefix1", fakeSandbox);
-  const store2 = Storage.makeStorage("prefix2", fakeSandbox);
+  const store1 = new Storage("prefix1");
+  const store2 = new Storage("prefix2");
 
   // Make sure values return null before being set
   Assert.equal(await store1.getItem("key"), null);
@@ -45,25 +44,3 @@ add_task(async function() {
   Assert.equal(await store1.getItem("removeTest"), null);
   Assert.equal(await store2.getItem("removeTest"), null);
 });
-
-add_task(async function testSandboxValueStorage() {
-  const manager1 = new SandboxManager();
-  const manager2 = new SandboxManager();
-  const store1 = Storage.makeStorage("testSandboxValueStorage", manager1.sandbox);
-  const store2 = Storage.makeStorage("testSandboxValueStorage", manager2.sandbox);
-  manager1.addGlobal("store", store1);
-  manager2.addGlobal("store", store2);
-  manager1.addHold("testing");
-  manager2.addHold("testing");
-
-  await manager1.evalInSandbox("store.setItem('foo', {foo: 'bar'});");
-  manager1.removeHold("testing");
-  await manager1.isNuked();
-
-  const objectMatches = await manager2.evalInSandbox(`
-    store.getItem("foo").then(item => item.foo === "bar");
-  `);
-  ok(objectMatches, "Values persisted in a store survive after their originating sandbox is nuked");
-
-  manager2.removeHold("testing");
-});