From 548c67b5f44e98e1630ea5f666a93a2c6f4f20d9 Mon Sep 17 00:00:00 2001 From: Ed Lee Date: Thu, 29 Mar 2018 11:05:31 -0700 Subject: [PATCH] fix(storage): Lazily open database on first access instead of requiring init (#4068) --- system-addon/lib/ActivityStream.jsm | 6 +++- system-addon/lib/ActivityStreamStorage.jsm | 31 ++++++------------- system-addon/lib/PrefsFeed.jsm | 7 ++--- system-addon/lib/SectionsManager.jsm | 6 +--- system-addon/lib/SnippetsFeed.jsm | 1 - system-addon/lib/TopSitesFeed.jsm | 7 ++--- .../test/unit/lib/ActivityStream.test.js | 9 +++--- .../unit/lib/ActivityStreamStorage.test.js | 27 +++++----------- .../test/unit/lib/SectionsManager.test.js | 7 ----- .../test/unit/lib/TopSitesFeed.test.js | 7 ----- 10 files changed, 30 insertions(+), 78 deletions(-) diff --git a/system-addon/lib/ActivityStream.jsm b/system-addon/lib/ActivityStream.jsm index 8c5c82d3d..7e78ad645 100644 --- a/system-addon/lib/ActivityStream.jsm +++ b/system-addon/lib/ActivityStream.jsm @@ -279,7 +279,11 @@ this.ActivityStream = class ActivityStream { try { this._updateDynamicPrefs(); this._defaultPrefs.init(); - this._storage.init(); + + // Accessing the db causes the object stores to be created / migrated. + // This needs to happen before other instances try to access the db, which + // would update only a subset of the stores to the latest version. + this._storage.db; // eslint-disable-line no-unused-expressions // Hook up the store and let all feeds and pages initialize this.store.init(this.feeds, ac.BroadcastToContent({ diff --git a/system-addon/lib/ActivityStreamStorage.jsm b/system-addon/lib/ActivityStreamStorage.jsm index 182610713..e33ff58b5 100644 --- a/system-addon/lib/ActivityStreamStorage.jsm +++ b/system-addon/lib/ActivityStreamStorage.jsm @@ -9,35 +9,26 @@ this.ActivityStreamStorage = class ActivityStreamStorage { this.dbName = "ActivityStream"; this.dbVersion = 3; this.storeName = storeName; - - this._db = null; } get db() { - if (!this._db) { - throw new Error("It looks like the db connection has not initialized yet. Are you use .init was called?"); - } - return this._db; + return this._db || (this._db = this._openDatabase()); } - get intialized() { - return this._db !== null; + async getStore() { + return (await this.db).objectStore(this.storeName, "readwrite"); } - getStore() { - return this.db.objectStore(this.storeName, "readwrite"); + async get(key) { + return (await this.getStore()).get(key); } - get(key) { - return this.getStore().get(key); + async getAll() { + return (await this.getStore()).getAll(); } - getAll() { - return this.getStore().getAll(); - } - - set(key, value) { - return this.getStore().put(value, key); + async set(key, value) { + return (await this.getStore()).put(value, key); } _openDatabase() { @@ -55,10 +46,6 @@ this.ActivityStreamStorage = class ActivityStreamStorage { } }); } - - async init() { - this._db = await this._openDatabase(); - } }; function getDefaultOptions(options) { diff --git a/system-addon/lib/PrefsFeed.jsm b/system-addon/lib/PrefsFeed.jsm index 5a4faa2d9..ee73c99f8 100644 --- a/system-addon/lib/PrefsFeed.jsm +++ b/system-addon/lib/PrefsFeed.jsm @@ -88,7 +88,7 @@ this.PrefsFeed = class PrefsFeed { } } - async init() { + init() { this._prefs.observeBranch(this); // Get the initial value of each activity stream pref @@ -103,10 +103,7 @@ this.PrefsFeed = class PrefsFeed { // Set the initial state of all prefs in redux this.store.dispatch(ac.BroadcastToContent({type: at.PREFS_INITIAL_VALUES, data: values})); - if (!this._storage.intialized) { - await this._storage.init(); - this._migratePrefs(); - } + this._migratePrefs(); this._setPrerenderPref(); this._initOnboardingPref(); } diff --git a/system-addon/lib/SectionsManager.jsm b/system-addon/lib/SectionsManager.jsm index 4e56e9d32..cfd3fc34f 100644 --- a/system-addon/lib/SectionsManager.jsm +++ b/system-addon/lib/SectionsManager.jsm @@ -80,7 +80,7 @@ const SectionsManager = { initialized: false, sections: new Map(), async init(prefs = {}) { - await this.initStorage(); + this._storage = new ActivityStreamStorage("sectionPrefs"); for (const feedPrefName of Object.keys(BUILT_IN_SECTIONS)) { const optionsPrefName = `${feedPrefName}.options`; @@ -103,10 +103,6 @@ const SectionsManager = { this.initialized = true; this.emit(this.INIT); }, - initStorage() { - this._storage = new ActivityStreamStorage("sectionPrefs"); - return this._storage.init(); - }, observe(subject, topic, data) { switch (topic) { case "nsPref:changed": diff --git a/system-addon/lib/SnippetsFeed.jsm b/system-addon/lib/SnippetsFeed.jsm index c985f15bf..c6d51b75d 100644 --- a/system-addon/lib/SnippetsFeed.jsm +++ b/system-addon/lib/SnippetsFeed.jsm @@ -169,7 +169,6 @@ this.SnippetsFeed = class SnippetsFeed { } async init() { - await this._storage.init(); this._previousSessionEnd = await this._storage.get("previousSessionEnd"); await this._refresh(); Services.prefs.addObserver(ONBOARDING_FINISHED_PREF, this._refresh); diff --git a/system-addon/lib/TopSitesFeed.jsm b/system-addon/lib/TopSitesFeed.jsm index be674b13f..d900bf488 100644 --- a/system-addon/lib/TopSitesFeed.jsm +++ b/system-addon/lib/TopSitesFeed.jsm @@ -172,11 +172,8 @@ this.TopSitesFeed = class TopSitesFeed { const links = await this.getLinksWithDefaults(); const newAction = {type: at.TOP_SITES_UPDATED, data: {links}}; - if (!this._storage.initialized) { - await this._storage.init(); - const storedPrefs = await this._storage.get(SECTION_ID) || {}; - newAction.data.pref = getDefaultOptions(storedPrefs); - } + const storedPrefs = await this._storage.get(SECTION_ID) || {}; + newAction.data.pref = getDefaultOptions(storedPrefs); if (options.broadcast) { // Broadcast an update to all open content pages diff --git a/system-addon/test/unit/lib/ActivityStream.test.js b/system-addon/test/unit/lib/ActivityStream.test.js index bfdb3a370..cf42a9b50 100644 --- a/system-addon/test/unit/lib/ActivityStream.test.js +++ b/system-addon/test/unit/lib/ActivityStream.test.js @@ -26,7 +26,6 @@ describe("ActivityStream", () => { "lib/TopSitesFeed.jsm": {TopSitesFeed: Fake}, "lib/TopStoriesFeed.jsm": {TopStoriesFeed: Fake}, "lib/HighlightsFeed.jsm": {HighlightsFeed: Fake}, - "lib/ActivityStreamStorage.jsm": {ActivityStreamStorage: Fake}, "lib/ThemeFeed.jsm": {ThemeFeed: Fake}, "lib/MessageCenterFeed.jsm": {MessageCenterFeed: Fake} })); @@ -35,7 +34,7 @@ describe("ActivityStream", () => { sandbox.stub(as.store, "uninit"); sandbox.stub(as._defaultPrefs, "init"); sandbox.stub(as._defaultPrefs, "reset"); - as._storage = {init: sandbox.stub()}; + sandbox.stub(as._storage, "_openDatabase"); }); afterEach(() => sandbox.restore()); @@ -59,14 +58,14 @@ describe("ActivityStream", () => { it("should call .store.init", () => { assert.calledOnce(as.store.init); }); - it("should call storage init", () => { - assert.calledOnce(as._storage.init); + it("should cause storage to open database", () => { + assert.calledOnce(as._storage._openDatabase); }); it("should pass to Store an INIT event with the right version", () => { as = new ActivityStream({version: "1.2.3"}); sandbox.stub(as.store, "init"); sandbox.stub(as._defaultPrefs, "init"); - as._storage = {init: sandbox.stub()}; + sandbox.stub(as._storage, "_openDatabase"); as.init(); diff --git a/system-addon/test/unit/lib/ActivityStreamStorage.test.js b/system-addon/test/unit/lib/ActivityStreamStorage.test.js index ea9ce7838..97be5e4c2 100644 --- a/system-addon/test/unit/lib/ActivityStreamStorage.test.js +++ b/system-addon/test/unit/lib/ActivityStreamStorage.test.js @@ -16,25 +16,21 @@ describe("ActivityStreamStorage", () => { afterEach(() => { sandbox.restore(); }); - it("should throw an error if you try to use it without init", () => { - assert.throws(() => storage.db); - }); - it("should not throw an error if you called init", async () => { - await storage.init(); + it("should not throw an error when accessing db", async () => { assert.ok(storage.db); }); - it("should revert key value parameters for put", () => { + it("should revert key value parameters for put", async () => { const stub = sandbox.stub(); - sandbox.stub(storage, "getStore").returns({put: stub}); + sandbox.stub(storage, "getStore").resolves({put: stub}); - storage.set("key", "value"); + await storage.set("key", "value"); assert.calledOnce(stub); assert.calledWith(stub, "value", "key"); }); it("should create a db with the correct store name", async () => { const dbStub = {createObjectStore: sandbox.stub(), objectStoreNames: {contains: sandbox.stub().returns(false)}}; - await storage.init(); + await storage.db; // call the cb with a stub indexedDB.open.args[0][2](dbStub); @@ -45,7 +41,7 @@ describe("ActivityStreamStorage", () => { it("should handle an array of object store names", async () => { storage = new ActivityStreamStorage(["store1", "store2"]); const dbStub = {createObjectStore: sandbox.stub(), objectStoreNames: {contains: sandbox.stub().returns(false)}}; - await storage.init(); + await storage.db; // call the cb with a stub indexedDB.open.args[0][2](dbStub); @@ -57,20 +53,11 @@ describe("ActivityStreamStorage", () => { it("should skip creating existing stores", async () => { storage = new ActivityStreamStorage(["store1", "store2"]); const dbStub = {createObjectStore: sandbox.stub(), objectStoreNames: {contains: sandbox.stub().returns(true)}}; - await storage.init(); + await storage.db; // call the cb with a stub indexedDB.open.args[0][2](dbStub); assert.notCalled(dbStub.createObjectStore); }); - it("should be initialized after calling init", async () => { - const dbStub = {createObjectStore: sandbox.stub(), objectStoreNames: {contains: sandbox.stub().returns(false)}}; - await storage.init(); - - // call the cb with a stub - indexedDB.open.args[0][2](dbStub); - - assert.isTrue(storage.intialized); - }); }); diff --git a/system-addon/test/unit/lib/SectionsManager.test.js b/system-addon/test/unit/lib/SectionsManager.test.js index 57edead00..9375cdf24 100644 --- a/system-addon/test/unit/lib/SectionsManager.test.js +++ b/system-addon/test/unit/lib/SectionsManager.test.js @@ -207,13 +207,6 @@ describe("SectionsManager", () => { assert.calledOnce(SectionsManager.updateSections); }); - it("should call initStorage on init", async () => { - sandbox.stub(SectionsManager, "initStorage").returns(Promise.resolve()); - - await SectionsManager.init(); - - assert.calledOnce(SectionsManager.initStorage); - }); }); describe("#_addCardTypeLinkMenuOptions", () => { const addCardTypeLinkMenuOptionsOrig = SectionsManager._addCardTypeLinkMenuOptions; diff --git a/system-addon/test/unit/lib/TopSitesFeed.test.js b/system-addon/test/unit/lib/TopSitesFeed.test.js index dbcce9ce9..c393b6a33 100644 --- a/system-addon/test/unit/lib/TopSitesFeed.test.js +++ b/system-addon/test/unit/lib/TopSitesFeed.test.js @@ -492,13 +492,6 @@ describe("Top Sites Feed", () => { assert.notCalled(feed._storage.init); }); - it("should call init storage if not initialized", async () => { - feed._storage.initialized = false; - - await feed.refresh({broadcast: false}); - - assert.calledOnce(feed._storage.init); - }); }); describe("#updateSectionPrefs", () => { it("should call updateSectionPrefs on UPDATE_SECTION_PREFS", () => {