Backing out bug 893276 for causing bug 1058840. a=lmandel
authorDave Townsend <dtownsend@oxymoronical.com>
Fri, 19 Sep 2014 11:33:47 -0700
changeset 216799 fa3e1469d0f1
parent 216798 a34329afda87
child 216800 73202bfb3f03
push id3918
push userdtownsend@mozilla.com
push date2014-09-19 18:35 +0000
treeherdermozilla-beta@fa3e1469d0f1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslmandel
bugs893276, 1058840
milestone33.0
Backing out bug 893276 for causing bug 1058840. a=lmandel
addon-sdk/source/lib/sdk/simple-storage.js
addon-sdk/source/test/test-simple-storage.js
--- a/addon-sdk/source/lib/sdk/simple-storage.js
+++ b/addon-sdk/source/lib/sdk/simple-storage.js
@@ -3,26 +3,23 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 module.metadata = {
   "stability": "stable"
 };
 
-const { Cc, Ci, Cu } = require("chrome");
+const { Cc, Ci } = require("chrome");
 const file = require("./io/file");
 const prefs = require("./preferences/service");
 const jpSelf = require("./self");
 const timer = require("./timers");
 const unload = require("./system/unload");
 const { emit, on, off } = require("./event/core");
-const { defer } = require('./core/promise');
-
-const { Task } = Cu.import("resource://gre/modules/Task.jsm", {});
 
 const WRITE_PERIOD_PREF = "extensions.addon-sdk.simple-storage.writePeriod";
 const WRITE_PERIOD_DEFAULT = 300000; // 5 minutes
 
 const QUOTA_PREF = "extensions.addon-sdk.simple-storage.quota";
 const QUOTA_DEFAULT = 5242880; // 5 MiB
 
 const JETPACK_DIR_BASENAME = "jetpack";
@@ -33,78 +30,29 @@ Object.defineProperties(exports, {
     get: function() { return manager.root; },
     set: function(value) { manager.root = value; }
   },
   quotaUsage: {
     get: function() { return manager.quotaUsage; }
   }
 });
 
-function getHash(data) {
-  let { promise, resolve } = defer();
-
-  let crypto = Cc["@mozilla.org/security/hash;1"].createInstance(Ci.nsICryptoHash);
-  crypto.init(crypto.MD5);
-
-  let listener = {
-    onStartRequest: function() { },
-
-    onDataAvailable: function(request, context, inputStream, offset, count) {
-      crypto.updateFromStream(inputStream, count);
-    },
-
-    onStopRequest: function(request, context, status) {
-      resolve(crypto.finish(false));
-    }
-  };
-
-  let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
-                  createInstance(Ci.nsIScriptableUnicodeConverter);
-  converter.charset = "UTF-8";
-  let stream = converter.convertToInputStream(data);
-  let pump = Cc["@mozilla.org/network/input-stream-pump;1"].
-             createInstance(Ci.nsIInputStreamPump);
-  pump.init(stream, -1, -1, 0, 0, true);
-  pump.asyncRead(listener, null);
-
-  return promise;
-}
-
-function writeData(filename, data) {
-  let { promise, resolve, reject } = defer();
-
-  let stream = file.open(filename, "w");
-  try {
-    stream.writeAsync(data, err => {
-      if (err)
-        reject(err);
-      else
-        resolve();
-    });
-  }
-  catch (err) {
-    // writeAsync closes the stream after it's done, so only close on error.
-    stream.close();
-    reject(err);
-  }
-
-  return promise;
-}
-
 // A generic JSON store backed by a file on disk.  This should be isolated
 // enough to move to its own module if need be...
 function JsonStore(options) {
   this.filename = options.filename;
   this.quota = options.quota;
   this.writePeriod = options.writePeriod;
   this.onOverQuota = options.onOverQuota;
   this.onWrite = options.onWrite;
-  this.hash = null;
+
   unload.ensure(this);
-  this.startTimer();
+
+  this.writeTimer = timer.setInterval(this.write.bind(this),
+                                      this.writePeriod);
 }
 
 JsonStore.prototype = {
   // The store's root.
   get root() {
     return this.isRootInited ? this._root : {};
   },
 
@@ -128,28 +76,21 @@ JsonStore.prototype = {
   // Percentage of quota used, as a number [0, Inf).  > 1 implies over quota.
   // Undefined if there is no quota.
   get quotaUsage() {
     return this.quota > 0 ?
            JSON.stringify(this.root).length / this.quota :
            undefined;
   },
 
-  startTimer: function JsonStore_startTimer() {
-    timer.setTimeout(() => {
-      this.write().then(this.startTimer.bind(this));
-    }, this.writePeriod);
-  },
-
   // Removes the backing file and all empty subdirectories.
   purge: function JsonStore_purge() {
     try {
       // This'll throw if the file doesn't exist.
       file.remove(this.filename);
-      this.hash = null;
       let parentPath = this.filename;
       do {
         parentPath = file.dirname(parentPath);
         // This'll throw if the dir isn't empty.
         file.rmdir(parentPath);
       } while (file.basename(parentPath) !== JETPACK_DIR_BASENAME);
     }
     catch (err) {}
@@ -159,35 +100,41 @@ JsonStore.prototype = {
   read: function JsonStore_read() {
     try {
       let str = file.read(this.filename);
 
       // Ideally we'd log the parse error with console.error(), but logged
       // errors cause tests to fail.  Supporting "known" errors in the test
       // harness appears to be non-trivial.  Maybe later.
       this.root = JSON.parse(str);
-      let self = this;
-      getHash(str).then(hash => this.hash = hash);
     }
     catch (err) {
       this.root = {};
-      this.hash = null;
     }
   },
 
+  // If the store is under quota, writes the root to the backing file.
+  // Otherwise quota observers are notified and nothing is written.
+  write: function JsonStore_write() {
+    if (this.quotaUsage > 1)
+      this.onOverQuota(this);
+    else
+      this._write();
+  },
+
   // Cleans up on unload.  If unloading because of uninstall, the store is
   // purged; otherwise it's written.
   unload: function JsonStore_unload(reason) {
-    timer.clearTimeout(this.writeTimer);
+    timer.clearInterval(this.writeTimer);
     this.writeTimer = null;
 
     if (reason === "uninstall")
       this.purge();
     else
-      this.write();
+      this._write();
   },
 
   // True if the root is an empty object.
   get _isEmpty() {
     if (this.root && typeof(this.root) === "object") {
       let empty = true;
       for (let key in this.root) {
         empty = false;
@@ -196,50 +143,42 @@ JsonStore.prototype = {
       return empty;
     }
     return false;
   },
 
   // Writes the root to the backing file, notifying write observers when
   // complete.  If the store is over quota or if it's empty and the store has
   // never been written, nothing is written and write observers aren't notified.
-  write: Task.async(function JsonStore_write() {
+  _write: function JsonStore__write() {
     // Don't write if the root is uninitialized or if the store is empty and the
     // backing file doesn't yet exist.
     if (!this.isRootInited || (this._isEmpty && !file.exists(this.filename)))
       return;
 
-    let data = JSON.stringify(this.root);
-
     // If the store is over quota, don't write.  The current under-quota state
     // should persist.
-    if ((this.quota > 0) && (data.length > this.quota)) {
-      this.onOverQuota(this);
-      return;
-    }
-
-    // Hash the data to compare it to any previously written data
-    let hash = yield getHash(data);
-
-    if (hash == this.hash)
+    if (this.quotaUsage > 1)
       return;
 
     // Finally, write.
+    let stream = file.open(this.filename, "w");
     try {
-      yield writeData(this.filename, data);
-
-      this.hash = hash;
-      if (this.onWrite)
-        this.onWrite(this);
+      stream.writeAsync(JSON.stringify(this.root), function writeAsync(err) {
+        if (err)
+          console.error("Error writing simple storage file: " + this.filename);
+        else if (this.onWrite)
+          this.onWrite(this);
+      }.bind(this));
     }
     catch (err) {
-      console.error("Error writing simple storage file: " + this.filename);
-      console.error(err);
+      // writeAsync closes the stream after it's done, so only close on error.
+      stream.close();
     }
-  })
+  }
 };
 
 
 // This manages a JsonStore singleton and tailors its use to simple storage.
 // The root of the JsonStore is lazy-loaded:  The backing file is only read the
 // first time the root's gotten.
 let manager = ({
   jsonStore: null,
--- a/addon-sdk/source/test/test-simple-storage.js
+++ b/addon-sdk/source/test/test-simple-storage.js
@@ -1,17 +1,16 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const file = require("sdk/io/file");
 const prefs = require("sdk/preferences/service");
 
 const QUOTA_PREF = "extensions.addon-sdk.simple-storage.quota";
-const WRITE_PERIOD_PREF = "extensions.addon-sdk.simple-storage.writePeriod";
 
 let {Cc,Ci} = require("chrome");
 
 const { Loader } = require("sdk/test/loader");
 const { id } = require("sdk/self");
 
 let storeFile = Cc["@mozilla.org/file/directory_service;1"].
                 getService(Ci.nsIProperties).
@@ -30,23 +29,22 @@ exports.testSetGet = function (assert, d
   let loader = Loader(module);
   let ss = loader.require("sdk/simple-storage");
   manager(loader).jsonStore.onWrite = function (storage) {
     assert.ok(file.exists(storeFilename), "Store file should exist");
 
     // Load the module again and make sure the value stuck.
     loader = Loader(module);
     ss = loader.require("sdk/simple-storage");
-    assert.equal(ss.storage.foo, val, "Value should persist");
     manager(loader).jsonStore.onWrite = function (storage) {
-      assert.fail("Nothing should be written since `storage` was not changed.");
+      file.remove(storeFilename);
+      done();
     };
+    assert.equal(ss.storage.foo, val, "Value should persist");
     loader.unload();
-    file.remove(storeFilename);
-    done();
   };
   let val = "foo";
   ss.storage.foo = val;
   assert.equal(ss.storage.foo, val, "Value read should be value set");
   loader.unload();
 };
 
 exports.testSetGetRootArray = function (assert, done) {
@@ -157,21 +155,20 @@ exports.testQuotaExceededHandle = functi
       let numProps = 0;
       for (let prop in ss.storage)
         numProps++;
       assert.ok(numProps, 2,
                   "Store should contain 2 values: " + ss.storage.toSource());
       assert.equal(ss.storage.x, 4, "x value should be correct");
       assert.equal(ss.storage.y, 5, "y value should be correct");
       manager(loader).jsonStore.onWrite = function (storage) {
-        assert.fail("Nothing should be written since `storage` was not changed.");
+        prefs.reset(QUOTA_PREF);
+        done();
       };
       loader.unload();
-      prefs.reset(QUOTA_PREF);
-      done();
     };
     loader.unload();
   });
   // This will be JSON.stringify()ed to: {"a":1,"b":2,"c":3} (19 bytes)
   ss.storage = { a: 1, b: 2, c: 3 };
   manager(loader).jsonStore.write();
 };
 
@@ -195,19 +192,16 @@ exports.testQuotaExceededNoHandle = func
       };
       loader.unload();
 
       loader = Loader(module);
       ss = loader.require("sdk/simple-storage");
       assert.equal(ss.storage, val,
                        "Over-quota value should not have been written, " +
                        "old value should have persisted: " + ss.storage);
-      manager(loader).jsonStore.onWrite = function (storage) {
-        assert.fail("Nothing should be written since `storage` was not changed.");
-      };
       loader.unload();
       prefs.reset(QUOTA_PREF);
       done();
     });
     manager(loader).jsonStore.write();
   };
 
   let val = "foo";
@@ -252,194 +246,16 @@ exports.testUninstall = function (assert
     loader.unload("uninstall");
     assert.ok(!file.exists(storeFilename), "Store file should be removed");
     done();
   };
   ss.storage.foo = "foo";
   loader.unload();
 };
 
-exports.testChangeInnerArray = function(assert, done) {
-  prefs.set(WRITE_PERIOD_PREF, 10);
-
-  let expected = {
-    x: [5, 7],
-    y: [7, 28],
-    z: [6, 2]
-  };
-
-  // Load the module, set a value.
-  let loader = Loader(module);
-  let ss = loader.require("sdk/simple-storage");
-  manager(loader).jsonStore.onWrite = function (storage) {
-    assert.ok(file.exists(storeFilename), "Store file should exist");
-
-    // Load the module again and check the result
-    loader = Loader(module);
-    ss = loader.require("sdk/simple-storage");
-    assert.equal(JSON.stringify(ss.storage),
-                     JSON.stringify(expected), "Should see the expected object");
-
-    // Add a property
-    ss.storage.x.push(["bar"]);
-    expected.x.push(["bar"]);
-    manager(loader).jsonStore.onWrite = function (storage) {
-      assert.equal(JSON.stringify(ss.storage),
-                       JSON.stringify(expected), "Should see the expected object");
-
-      // Modify a property
-      ss.storage.y[0] = 42;
-      expected.y[0] = 42;
-      manager(loader).jsonStore.onWrite = function (storage) {
-        assert.equal(JSON.stringify(ss.storage),
-                         JSON.stringify(expected), "Should see the expected object");
-
-        // Delete a property
-        delete ss.storage.z[1];
-        delete expected.z[1];
-        manager(loader).jsonStore.onWrite = function (storage) {
-          assert.equal(JSON.stringify(ss.storage),
-                           JSON.stringify(expected), "Should see the expected object");
-
-          // Modify the new inner-object
-          ss.storage.x[2][0] = "baz";
-          expected.x[2][0] = "baz";
-          manager(loader).jsonStore.onWrite = function (storage) {
-            assert.equal(JSON.stringify(ss.storage),
-                             JSON.stringify(expected), "Should see the expected object");
-
-            manager(loader).jsonStore.onWrite = function (storage) {
-              assert.fail("Nothing should be written since `storage` was not changed.");
-            };
-            loader.unload();
-
-            // Load the module again and check the result
-            loader = Loader(module);
-            ss = loader.require("sdk/simple-storage");
-            assert.equal(JSON.stringify(ss.storage),
-                             JSON.stringify(expected), "Should see the expected object");
-            loader.unload();
-            file.remove(storeFilename);
-            prefs.reset(WRITE_PERIOD_PREF);
-            done();
-          };
-        };
-      };
-    };
-  };
-
-  ss.storage = {
-    x: [5, 7],
-    y: [7, 28],
-    z: [6, 2]
-  };
-  assert.equal(JSON.stringify(ss.storage),
-                   JSON.stringify(expected), "Should see the expected object");
-
-  loader.unload();
-};
-
-exports.testChangeInnerObject = function(assert, done) {
-  prefs.set(WRITE_PERIOD_PREF, 10);
-
-  let expected = {
-    x: {
-      a: 5,
-      b: 7
-    },
-    y: {
-      c: 7,
-      d: 28
-    },
-    z: {
-      e: 6,
-      f: 2
-    }
-  };
-
-  // Load the module, set a value.
-  let loader = Loader(module);
-  let ss = loader.require("sdk/simple-storage");
-  manager(loader).jsonStore.onWrite = function (storage) {
-    assert.ok(file.exists(storeFilename), "Store file should exist");
-
-    // Load the module again and check the result
-    loader = Loader(module);
-    ss = loader.require("sdk/simple-storage");
-    assert.equal(JSON.stringify(ss.storage),
-                     JSON.stringify(expected), "Should see the expected object");
-
-    // Add a property
-    ss.storage.x.g = {foo: "bar"};
-    expected.x.g = {foo: "bar"};
-    manager(loader).jsonStore.onWrite = function (storage) {
-      assert.equal(JSON.stringify(ss.storage),
-                       JSON.stringify(expected), "Should see the expected object");
-
-      // Modify a property
-      ss.storage.y.c = 42;
-      expected.y.c = 42;
-      manager(loader).jsonStore.onWrite = function (storage) {
-        assert.equal(JSON.stringify(ss.storage),
-                         JSON.stringify(expected), "Should see the expected object");
-
-        // Delete a property
-        delete ss.storage.z.f;
-        delete expected.z.f;
-        manager(loader).jsonStore.onWrite = function (storage) {
-          assert.equal(JSON.stringify(ss.storage),
-                           JSON.stringify(expected), "Should see the expected object");
-
-          // Modify the new inner-object
-          ss.storage.x.g.foo = "baz";
-          expected.x.g.foo = "baz";
-          manager(loader).jsonStore.onWrite = function (storage) {
-            assert.equal(JSON.stringify(ss.storage),
-                             JSON.stringify(expected), "Should see the expected object");
-
-            manager(loader).jsonStore.onWrite = function (storage) {
-              assert.fail("Nothing should be written since `storage` was not changed.");
-            };
-            loader.unload();
-
-            // Load the module again and check the result
-            loader = Loader(module);
-            ss = loader.require("sdk/simple-storage");
-            assert.equal(JSON.stringify(ss.storage),
-                             JSON.stringify(expected), "Should see the expected object");
-            loader.unload();
-            file.remove(storeFilename);
-            prefs.reset(WRITE_PERIOD_PREF);
-            done();
-          };
-        };
-      };
-    };
-  };
-
-  ss.storage = {
-    x: {
-      a: 5,
-      b: 7
-    },
-    y: {
-      c: 7,
-      d: 28
-    },
-    z: {
-      e: 6,
-      f: 2
-    }
-  };
-  assert.equal(JSON.stringify(ss.storage),
-                   JSON.stringify(expected), "Should see the expected object");
-
-  loader.unload();
-};
-
 exports.testSetNoSetRead = function (assert, done) {
   // Load the module, set a value.
   let loader = Loader(module);
   let ss = loader.require("sdk/simple-storage");
   manager(loader).jsonStore.onWrite = function (storage) {
     assert.ok(file.exists(storeFilename), "Store file should exist");
 
     // Load the module again but don't access ss.storage.
@@ -448,23 +264,22 @@ exports.testSetNoSetRead = function (ass
     manager(loader).jsonStore.onWrite = function (storage) {
       assert.fail("Nothing should be written since `storage` was not accessed.");
     };
     loader.unload();
 
     // Load the module a third time and make sure the value stuck.
     loader = Loader(module);
     ss = loader.require("sdk/simple-storage");
-    assert.equal(ss.storage.foo, val, "Value should persist");
     manager(loader).jsonStore.onWrite = function (storage) {
-      assert.fail("Nothing should be written since `storage` was not changed.");
+      file.remove(storeFilename);
+      done();
     };
+    assert.equal(ss.storage.foo, val, "Value should persist");
     loader.unload();
-    file.remove(storeFilename);
-    done();
   };
   let val = "foo";
   ss.storage.foo = val;
   assert.equal(ss.storage.foo, val, "Value read should be value set");
   loader.unload();
 };
 
 
@@ -475,23 +290,22 @@ function setGetRoot(assert, done, val, c
   let loader = Loader(module);
   let ss = loader.require("sdk/simple-storage");
   manager(loader).jsonStore.onWrite = function () {
     assert.ok(file.exists(storeFilename), "Store file should exist");
 
     // Load the module again and make sure the value stuck.
     loader = Loader(module);
     ss = loader.require("sdk/simple-storage");
-    assert.ok(compare(ss.storage, val), "Value should persist");
-    manager(loader).jsonStore.onWrite = function (storage) {
-      assert.fail("Nothing should be written since `storage` was not changed.");
+    manager(loader).jsonStore.onWrite = function () {
+      file.remove(storeFilename);
+      done();
     };
+    assert.ok(compare(ss.storage, val), "Value should persist");
     loader.unload();
-    file.remove(storeFilename);
-    done();
   };
   ss.storage = val;
   assert.ok(compare(ss.storage, val), "Value read should be value set");
   loader.unload();
 }
 
 function setGetRootError(assert, done, val, msg) {
   let pred = new RegExp("storage must be one of the following types: " +