Bug 1502182 - In Normandy, never close IndexedDB databases, and be explicit about objectStore modes r=Gijs,asuth

I suspect that the root cause of bug 1502182 is that we try open a database
connection before the old one is fully closed. Instead of dealing with
complicatedasync bookkeeping to make sure this doesn't happen, this patch
simply never closes the database connection.  I don't think any of the
benefits of closing IndexedDB databases apply to Normandy, and it isn't
a significant cost to simply keep them open.

Additionally, the patch distinguishes between readonly and readwrite
transactions with the database. This was originally done to try and fix
the bug. When it didn't help, I decided to leave the change in because
it is a beneficial change anyways.

Differential Revision: https://phabricator.services.mozilla.com/D10629

--HG--
extra : moz-landing-system : lando

toolkit/components/normandy/lib/AddonStudies.jsm

@@ -72,14 +72,20 @@ async function getDatabase() {
 /**
  * Get a transaction for interacting with the study store.
  *
+ * @param {IDBDatabase} db
+ * @param {String} mode Either "readonly" or "readwrite"
+ *
  * NOTE: Methods on the store returned by this function MUST be called
  * synchronously, otherwise the transaction with the store will expire.
  * This is why the helper takes a database as an argument; if we fetched the
  * database in the helper directly, the helper would be async and the
  * transaction would expire before methods on the store were called.
  */
-function getStore(db) {
-  return db.objectStore(STORE_NAME, "readwrite");
+function getStore(db, mode) {
+  if (!mode) {
+    throw new Error("mode is required");
+  }
+  return db.objectStore(STORE_NAME, mode);
 }
 
 var AddonStudies = {
@@ -99,21 +105,16 @@ var AddonStudies = {
         const oldStudies = await AddonStudies.getAll();
         let db = await getDatabase();
         await AddonStudies.clear();
-        for (const study of studies) {
-          await getStore(db).add(study);
-        }
-        await AddonStudies.close();
+        const store = getStore(db, "readwrite");
+        await Promise.all(studies.map(study => store.add(study)));
 
         try {
           await testFunction(...args, studies);
         } finally {
           db = await getDatabase();
           await AddonStudies.clear();
-          for (const study of oldStudies) {
-            await getStore(db).add(study);
-          }
-
-          await AddonStudies.close();
+          const store = getStore(db, "readwrite");
+          await Promise.all(oldStudies.map(study => store.add(study)));
         }
       };
     };
@@ -129,7 +130,6 @@ var AddonStudies = {
         await this.markAsEnded(study, "uninstalled-sideload");
       }
     }
-    await this.close();
 
     // Listen for add-on uninstalls so we can stop the corresponding studies.
     AddonManager.addAddonListener(this);
@@ -146,11 +146,7 @@ var AddonStudies = {
     const activeStudies = (await this.getAll()).filter(study => study.active);
     const matchingStudy = activeStudies.find(study => study.addonId === addon.id);
     if (matchingStudy) {
-      // Use a dedicated DB connection instead of the shared one so that we can
-      // close it without fear of affecting other users of the shared connection.
-      const db = await openDatabase();
       await this.markAsEnded(matchingStudy, "uninstalled");
-      await db.close();
     }
   },
 
@@ -159,19 +155,7 @@ var AddonStudies = {
    */
   async clear() {
     const db = await getDatabase();
-    await getStore(db).clear();
-  },
-
-  /**
-   * Close the current database connection if it is open.
-   */
-  async close() {
-    if (databasePromise) {
-      const promise = databasePromise;
-      databasePromise = null;
-      const db = await promise;
-      await db.close();
-    }
+    await getStore(db, "readwrite").clear();
   },
 
   /**
@@ -181,7 +165,7 @@ var AddonStudies = {
    */
   async has(recipeId) {
     const db = await getDatabase();
-    const study = await getStore(db).get(recipeId);
+    const study = await getStore(db, "readonly").get(recipeId);
     return !!study;
   },
 
@@ -192,7 +176,7 @@ var AddonStudies = {
    */
   async get(recipeId) {
     const db = await getDatabase();
-    return getStore(db).get(recipeId);
+    return getStore(db, "readonly").get(recipeId);
   },
 
   /**
@@ -201,7 +185,7 @@ var AddonStudies = {
    */
   async getAll() {
     const db = await getDatabase();
-    return getStore(db).getAll();
+    return getStore(db, "readonly").getAll();
   },
 
   /**
@@ -210,7 +194,7 @@ var AddonStudies = {
    */
   async add(study) {
     const db = await getDatabase();
-    return getStore(db).add(study);
+    return getStore(db, "readwrite").add(study);
   },
 
   /**
@@ -220,7 +204,7 @@ var AddonStudies = {
    */
   async delete(recipeId) {
     const db = await getDatabase();
-    return getStore(db).delete(recipeId);
+    return getStore(db, "readwrite").delete(recipeId);
   },
 
   /**
@@ -237,7 +221,7 @@ var AddonStudies = {
     study.active = false;
     study.studyEndDate = new Date();
     const db = await getDatabase();
-    await getStore(db).put(study);
+    await getStore(db, "readwrite").put(study);
 
     Services.obs.notifyObservers(study, STUDY_ENDED_TOPIC, `${study.recipeId}`);
     TelemetryEvents.sendEvent("unenroll", "addon_study", study.name, {

toolkit/components/normandy/lib/PreferenceRollouts.jsm

@@ -73,14 +73,20 @@ function getDatabase() {
 /**
  * Get a transaction for interacting with the rollout store.
  *
+ * @param {IDBDatabase} db
+ * @param {String} mode Either "readonly" or "readwrite"
+ *
  * NOTE: Methods on the store returned by this function MUST be called
  * synchronously, otherwise the transaction with the store will expire.
  * This is why the helper takes a database as an argument; if we fetched the
  * database in the helper directly, the helper would be async and the
  * transaction would expire before methods on the store were called.
  */
-function getStore(db) {
-  return db.objectStore(STORE_NAME, "readwrite");
+function getStore(db, mode) {
+  if (!mode) {
+    throw new Error("mode is required");
+  }
+  return db.objectStore(STORE_NAME, mode);
 }
 
 var PreferenceRollouts = {
@@ -124,7 +130,7 @@ var PreferenceRollouts = {
 
       if (changed) {
         const db = await getDatabase();
-        await getStore(db).put(rollout);
+        await getStore(db, "readwrite").put(rollout);
       }
     }
   },
@@ -146,18 +152,15 @@ var PreferenceRollouts = {
   withTestMock(testFunction) {
     return async function inner(...args) {
       let db = await getDatabase();
-      const oldData = await getStore(db).getAll();
-      await getStore(db).clear();
+      const oldData = await getStore(db, "readonly").getAll();
+      await getStore(db, "readwrite").clear();
       try {
         await testFunction(...args);
       } finally {
         db = await getDatabase();
-        const store = getStore(db);
-        let promises = [store.clear()];
-        for (const d of oldData) {
-          promises.push(store.add(d));
-        }
-        await Promise.all(promises);
+        await getStore(db, "readwrite").clear();
+        const store = getStore(db, "readwrite");
+        await Promise.all(oldData.map(d => store.add(d)));
       }
     };
   },
@@ -168,7 +171,7 @@ var PreferenceRollouts = {
    */
   async add(rollout) {
     const db = await getDatabase();
-    return getStore(db).add(rollout);
+    return getStore(db, "readwrite").add(rollout);
   },
 
   /**
@@ -181,7 +184,7 @@ var PreferenceRollouts = {
       throw new Error(`Tried to update ${rollout.slug}, but it doesn't already exist.`);
     }
     const db = await getDatabase();
-    return getStore(db).put(rollout);
+    return getStore(db, "readwrite").put(rollout);
   },
 
   /**
@@ -191,7 +194,7 @@ var PreferenceRollouts = {
    */
   async has(slug) {
     const db = await getDatabase();
-    const rollout = await getStore(db).get(slug);
+    const rollout = await getStore(db, "readonly").get(slug);
     return !!rollout;
   },
 
@@ -201,13 +204,13 @@ var PreferenceRollouts = {
    */
   async get(slug) {
     const db = await getDatabase();
-    return getStore(db).get(slug);
+    return getStore(db, "readonly").get(slug);
   },
 
   /** Get all rollouts in the database. */
   async getAll() {
     const db = await getDatabase();
-    return getStore(db).getAll();
+    return getStore(db, "readonly").getAll();
   },
 
   /** Get all rollouts in the "active" state. */
@@ -230,17 +233,4 @@ var PreferenceRollouts = {
       }
     }
   },
-
-  /**
-   * Close the current database connection if it is open. If it is not open,
-   * this is a no-op.
-   */
-  async closeDB() {
-    if (databasePromise) {
-      const promise = databasePromise;
-      databasePromise = null;
-      const db = await promise;
-      await db.close();
-    }
-  },
 };

toolkit/components/normandy/lib/RecipeRunner.jsm

@@ -18,7 +18,6 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   NormandyApi: "resource://normandy/lib/NormandyApi.jsm",
   ClientEnvironment: "resource://normandy/lib/ClientEnvironment.jsm",
   CleanupManager: "resource://normandy/lib/CleanupManager.jsm",
-  AddonStudies: "resource://normandy/lib/AddonStudies.jsm",
   Uptake: "resource://normandy/lib/Uptake.jsm",
   ActionsManager: "resource://normandy/lib/ActionsManager.jsm",
 });
@@ -248,9 +247,6 @@ var RecipeRunner = {
 
     await actions.finalize();
 
-    // Close storage connections
-    await AddonStudies.close();
-
     Uptake.reportRunner(Uptake.RUNNER_SUCCESS);
   },
 

toolkit/components/normandy/test/browser/browser_AddonStudies.js

@@ -69,30 +69,6 @@ decorate_task(
   }
 );
 
-decorate_task(
-  AddonStudies.withStudies(),
-  async function testCloseDatabase() {
-    await AddonStudies.close();
-    const openSpy = sinon.spy(IndexedDB, "open");
-    sinon.assert.notCalled(openSpy);
-
-    // Using studies at all should open the database, but only once.
-    await AddonStudies.has("foo");
-    await AddonStudies.get("foo");
-    sinon.assert.calledOnce(openSpy);
-
-    // close can be called multiple times
-    await AddonStudies.close();
-    await AddonStudies.close();
-
-    // After being closed, new operations cause the database to be opened again
-    await AddonStudies.has("test-study");
-    sinon.assert.calledTwice(openSpy);
-
-    openSpy.restore();
-  }
-);
-
 decorate_task(
   AddonStudies.withStudies([
     addonStudyFactory({name: "test-study1"}),

toolkit/components/normandy/test/browser/browser_PreferenceRollouts.js

@@ -87,37 +87,6 @@ decorate_task(
   }
 );
 
-decorate_task(
-  PreferenceRollouts.withTestMock,
-  async function testCloseDatabase() {
-    await PreferenceRollouts.closeDB();
-    const openSpy = sinon.spy(IndexedDB, "open");
-    sinon.assert.notCalled(openSpy);
-
-    try {
-      // Using rollouts at all should open the database, but only once.
-      await PreferenceRollouts.has("foo");
-      await PreferenceRollouts.get("foo");
-      sinon.assert.calledOnce(openSpy);
-      openSpy.reset();
-
-      // close can be called multiple times
-      await PreferenceRollouts.closeDB();
-      await PreferenceRollouts.closeDB();
-      // and don't cause the database to be opened (that would be weird)
-      sinon.assert.notCalled(openSpy);
-
-      // After being closed, new operations cause the database to be opened again, but only once
-      await PreferenceRollouts.has("foo");
-      await PreferenceRollouts.get("foo");
-      sinon.assert.calledOnce(openSpy);
-
-    } finally {
-      openSpy.restore();
-    }
-  }
-);
-
 // recordOriginalValue should update storage to note the original values
 decorate_task(
   PreferenceRollouts.withTestMock,

toolkit/components/normandy/test/browser/browser_RecipeRunner.js

@@ -128,7 +128,6 @@ async function withMockActionSandboxManagers(actions, testFunction) {
 }
 
 decorate_task(
-  withSpy(AddonStudies, "close"),
   withStub(Uptake, "reportRunner"),
   withStub(NormandyApi, "fetchRecipes"),
   withStub(ActionsManager.prototype, "fetchRemoteActions"),
@@ -136,7 +135,6 @@ decorate_task(
   withStub(ActionsManager.prototype, "runRecipe"),
   withStub(ActionsManager.prototype, "finalize"),
   async function testRun(
-    closeSpy,
     reportRunnerStub,
     fetchRecipesStub,
     fetchRemoteActionsStub,
@@ -170,16 +168,12 @@ decorate_task(
       [[Uptake.RUNNER_SUCCESS]],
       "RecipeRunner should report uptake telemetry",
     );
-
-    // Ensure storage is closed after the run.
-    ok(closeSpy.calledOnce, "Storage should be closed after the run");
   }
 );
 
 decorate_task(
   withMockNormandyApi,
   async function testRunFetchFail(mockApi) {
-    const closeSpy = sinon.spy(AddonStudies, "close");
     const reportRunner = sinon.stub(Uptake, "reportRunner");
 
     const action = {name: "action"};
@@ -207,11 +201,6 @@ decorate_task(
       sinon.assert.calledWith(reportRunner, Uptake.RUNNER_INVALID_SIGNATURE);
     });
 
-    // If the recipe fetch failed, we don't need to call close since nothing
-    // opened a connection in the first place.
-    sinon.assert.notCalled(closeSpy);
-
-    closeSpy.restore();
     reportRunner.restore();
   }
 );