Bug 663268 - Simple storage overwritten with empty storage (or "delet…

…ed") if module is loaded but storage property is not accessed. r=myk, a=blocker

packages/addon-kit/lib/simple-storage.js

@@ -81,7 +81,7 @@ function JsonStore(options) {
 JsonStore.prototype = {
   // The store's root.
   get root() {
-    return this._root === undefined ? {} : this._root;
+    return this.isRootInited ? this._root : {};
   },
 
   // Performs some type checking.
@@ -95,6 +95,12 @@ JsonStore.prototype = {
     return val;
   },
 
+  // True if the root has ever been set (either via the root setter or by the
+  // backing file's having been read).
+  get isRootInited() {
+    return this._root !== undefined;
+  },
+
   // Percentage of quota used, as a number [0, Inf).  > 1 implies over quota.
   // Undefined if there is no quota.
   get quotaUsage() {
@@ -171,8 +177,9 @@ JsonStore.prototype = {
   // 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: function JsonStore__write() {
-    // If the store is empty and the file doesn't yet exist, don't write.
-    if (this._isEmpty && !file.exists(this.filename))
+    // 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;
 
     // If the store is over quota, don't write.  The current under-quota state
@@ -222,17 +229,13 @@ let manager = Trait.compose(EventEmitter, Trait.compose({
   },
 
   get root() {
-    if (!this.rootInited) {
+    if (!this.jsonStore.isRootInited)
       this.jsonStore.read();
-      this.rootInited = true;
-    }
     return this.jsonStore.root;
   },
 
   set root(val) {
-    let rv = this.jsonStore.root = val;
-    this.rootInited = true;
-    return rv;
+    return this.jsonStore.root = val;
   },
 
   unload: function manager_unload() {

packages/addon-kit/tests/test-simple-storage.js

@@ -269,6 +269,39 @@ exports.testUninstall = function (test) {
   loader.unload();
 };
 
+exports.testSetNoSetRead = function (test) {
+  test.waitUntilDone();
+
+  // Load the module, set a value.
+  let loader = newLoader(test);
+  let ss = loader.require("simple-storage");
+  manager(loader).jsonStore.onWrite = function (storage) {
+    test.assert(file.exists(storeFilename), "Store file should exist");
+
+    // Load the module again but don't access ss.storage.
+    loader = newLoader(test);
+    ss = loader.require("simple-storage");
+    manager(loader).jsonStore.onWrite = function (storage) {
+      test.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 = newLoader(test);
+    ss = loader.require("simple-storage");
+    manager(loader).jsonStore.onWrite = function (storage) {
+      file.remove(storeFilename);
+      test.done();
+    };
+    test.assertEqual(ss.storage.foo, val, "Value should persist");
+    loader.unload();
+  };
+  let val = "foo";
+  ss.storage.foo = val;
+  test.assertEqual(ss.storage.foo, val, "Value read should be value set");
+  loader.unload();
+};
+
 function manager(loader) {
   return loader.findSandboxForModule("simple-storage").globalScope.manager;
 }