Bug 1540765 - PersistentCache should do JSON parsing off of the main …

…thread (mozilla#4879)

lib/PersistentCache.jsm

@@ -6,7 +6,7 @@
 const {XPCOMUtils} = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 
 ChromeUtils.defineModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
-XPCOMUtils.defineLazyGetter(this, "gTextDecoder", () => new TextDecoder());
+XPCOMUtils.defineLazyGlobalGetters(this, ["fetch"]);
 
 /**
  * A file (disk) based persistent cache of a JSON serializable object.
@@ -54,18 +54,22 @@ this.PersistentCache = class PersistentCache {
    */
   _load() {
     return this._cache || (this._cache = new Promise(async resolve => {
+      let file;
       let data = {};
+      const filepath = OS.Path.join(OS.Constants.Path.localProfileDir, this._filename);
+
       try {
-        const filepath = OS.Path.join(OS.Constants.Path.localProfileDir, this._filename);
-        const fileExists = await OS.File.exists(filepath);
-        if (fileExists) {
-          const binaryData = await OS.File.read(filepath);
-          const json = gTextDecoder.decode(binaryData);
-          data = JSON.parse(json);
+        file = await fetch(`file://${filepath}`);
+      } catch (error) {} // Cache file doesn't exist yet.
+
+      if (file) {
+        try {
+          data = await file.json();
+        } catch (error) {
+          Cu.reportError(`Failed to parse ${this._filename}: ${error.message}`);
         }
-      } catch (error) {
-        Cu.reportError(`Failed to load ${this._filename}: ${error.message}`);
       }
+
       resolve(data);
     }));
   }

test/unit/lib/DiscoveryStreamFeed.test.js

@@ -51,6 +51,7 @@ describe("DiscoveryStreamFeed", () => {
         },
       },
     });
+    global.fetch.resetHistory();
 
     sandbox.stub(feed, "_maybeUpdateCachedData").resolves();
 

test/unit/lib/PersistentCache.test.js

@@ -3,7 +3,8 @@ import {PersistentCache} from "lib/PersistentCache.jsm";
 
 describe("PersistentCache", () => {
   let fakeOS;
-  let fakeTextDecoder;
+  let fakeJsonParse;
+  let fakeFetch;
   let cache;
   let filename = "cache.json";
   let reportErrorStub;
@@ -16,17 +17,16 @@ describe("PersistentCache", () => {
     fakeOS = {
       Constants: {Path: {localProfileDir: "/foo/bar"}},
       File: {
-        exists: async () => true,
-        read: sandbox.stub().resolves({}),
         writeAtomic: sinon.stub().returns(Promise.resolve()),
       },
       Path: {join: () => filename},
     };
-    fakeTextDecoder = {decode: () => "{\"foo\": \"bar\"}"};
+    fakeJsonParse = sandbox.stub().resolves({});
+    fakeFetch = sandbox.stub().resolves({json: fakeJsonParse});
     reportErrorStub = sandbox.stub();
     globals.set("OS", fakeOS);
-    globals.set("gTextDecoder", fakeTextDecoder);
     globals.set("Cu", {reportError: reportErrorStub});
+    globals.set("fetch", fakeFetch);
 
     cache = new PersistentCache(filename);
   });
@@ -36,31 +36,28 @@ describe("PersistentCache", () => {
   });
 
   describe("#get", () => {
-    it("tries to read the file on the first get", async () => {
-      fakeOS.File.read = sinon.spy();
+    it("tries to fetch the file", async () => {
       await cache.get("foo");
-      assert.calledOnce(fakeOS.File.read);
+      assert.calledOnce(fakeFetch);
     });
-    it("doesnt try to read the file if it doesn't exist", async () => {
-      fakeOS.File.read = sinon.spy();
-      fakeOS.File.exists = async () => false;
+    it("doesnt try to parse file if it doesn't exist", async () => {
+      fakeFetch.throws();
       await cache.get("foo");
-      assert.notCalled(fakeOS.File.read);
+      assert.notCalled(fakeJsonParse);
     });
-    it("doesnt try to read the file if it was already loaded", async () => {
-      fakeOS.File.read = sinon.spy();
+    it("doesnt try to fetch the file if it was already loaded", async () => {
       await cache._load();
-      fakeOS.File.read.resetHistory();
+      fakeFetch.resetHistory();
       await cache.get("foo");
-      assert.notCalled(fakeOS.File.read);
+      assert.notCalled(fakeFetch);
     });
     it("should catch and report errors", async () => {
-      fakeOS.File.read.throws();
+      fakeJsonParse.throws();
       await cache._load();
-
       assert.calledOnce(reportErrorStub);
     });
     it("returns data for a given cache key", async () => {
+      fakeJsonParse.resolves({foo: "bar"});
       let value = await cache.get("foo");
       assert.equal(value, "bar");
     });
@@ -69,29 +66,27 @@ describe("PersistentCache", () => {
       assert.equal(value, undefined);
     });
     it("returns all the data if no cache key is specified", async () => {
+      fakeJsonParse.resolves({foo: "bar"});
       let value = await cache.get();
       assert.deepEqual(value, {foo: "bar"});
     });
   });
 
   describe("#set", () => {
-    it("tries to read the file on the first set", async () => {
-      fakeOS.File.read = sinon.spy();
+    it("tries to fetch the file on the first set", async () => {
       await cache.set("foo", {x: 42});
-      assert.calledOnce(fakeOS.File.read);
+      assert.calledOnce(fakeFetch);
     });
-    it("doesnt try to read the file if it was already loaded", async () => {
-      fakeOS.File.read = sinon.spy();
+    it("doesnt try to fetch the file if it was already loaded", async () => {
       cache = new PersistentCache(filename, true);
       await cache._load();
-      fakeOS.File.read.resetHistory();
+      fakeFetch.resetHistory();
       await cache.set("foo", {x: 42});
-      assert.notCalled(fakeOS.File.read);
+      assert.notCalled(fakeFetch);
     });
-    it("tries to read the file on the first set", async () => {
-      fakeOS.File.read = sinon.spy();
+    it("tries to fetch the file on the first set", async () => {
       await cache.set("foo", {x: 42});
-      assert.calledOnce(fakeOS.File.read);
+      assert.calledOnce(fakeFetch);
     });
     it("sets a string value", async () => {
       const key = "testkey";