From 08b93a34be1688063b34f39b4b2f858726174889 Mon Sep 17 00:00:00 2001 From: ricky rosario Date: Wed, 3 Apr 2019 09:52:46 -0400 Subject: [PATCH] Bug 1540765 - PersistentCache should do JSON parsing off of the main thread (#4879) --- lib/PersistentCache.jsm | 22 ++++++---- test/unit/lib/DiscoveryStreamFeed.test.js | 1 + test/unit/lib/PersistentCache.test.js | 51 ++++++++++------------- 3 files changed, 37 insertions(+), 37 deletions(-) diff --git a/lib/PersistentCache.jsm b/lib/PersistentCache.jsm index 9dfba6ad88..e0f3b03325 100644 --- a/lib/PersistentCache.jsm +++ b/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); })); } diff --git a/test/unit/lib/DiscoveryStreamFeed.test.js b/test/unit/lib/DiscoveryStreamFeed.test.js index 8eba7ed701..55b56fe472 100644 --- a/test/unit/lib/DiscoveryStreamFeed.test.js +++ b/test/unit/lib/DiscoveryStreamFeed.test.js @@ -51,6 +51,7 @@ describe("DiscoveryStreamFeed", () => { }, }, }); + global.fetch.resetHistory(); sandbox.stub(feed, "_maybeUpdateCachedData").resolves(); diff --git a/test/unit/lib/PersistentCache.test.js b/test/unit/lib/PersistentCache.test.js index 6149bd56bf..41f715cfbd 100644 --- a/test/unit/lib/PersistentCache.test.js +++ b/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";