fix(storage): Lazily open database on first access instead of requiring init (#4068)

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({

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) {

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();
   }

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":

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);

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

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();
 

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);
-  });
 });

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;

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", () => {