Backed out changeset cdc6d97f0b14 (bug 1502182) for ESLint failures in builds/worker/checkouts/gecko/toolkit/components/normandy/lib/RecipeRunner

toolkit/components/normandy/lib/AddonStudies.jsm

@@ -72,20 +72,14 @@ 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, mode) {
-  if (!mode) {
-    throw new Error("mode is required");
-  }
-  return db.objectStore(STORE_NAME, mode);
+function getStore(db) {
+  return db.objectStore(STORE_NAME, "readwrite");
 }
 
 var AddonStudies = {
@@ -105,16 +99,21 @@ var AddonStudies = {
         const oldStudies = await AddonStudies.getAll();
         let db = await getDatabase();
         await AddonStudies.clear();
-        const store = getStore(db, "readwrite");
-        await Promise.all(studies.map(study => store.add(study)));
+        for (const study of studies) {
+          await getStore(db).add(study);
+        }
+        await AddonStudies.close();
 
         try {
           await testFunction(...args, studies);
         } finally {
           db = await getDatabase();
           await AddonStudies.clear();
-          const store = getStore(db, "readwrite");
-          await Promise.all(oldStudies.map(study => store.add(study)));
+          for (const study of oldStudies) {
+            await getStore(db).add(study);
+          }
+
+          await AddonStudies.close();
         }
       };
     };
@@ -130,6 +129,7 @@ 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,7 +146,11 @@ 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();
     }
   },
 
@@ -155,7 +159,19 @@ var AddonStudies = {
    */
   async clear() {
     const db = await getDatabase();
-    await getStore(db, "readwrite").clear();
+    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();
+    }
   },
 
   /**
@@ -165,7 +181,7 @@ var AddonStudies = {
    */
   async has(recipeId) {
     const db = await getDatabase();
-    const study = await getStore(db, "readonly").get(recipeId);
+    const study = await getStore(db).get(recipeId);
     return !!study;
   },
 
@@ -176,7 +192,7 @@ var AddonStudies = {
    */
   async get(recipeId) {
     const db = await getDatabase();
-    return getStore(db, "readonly").get(recipeId);
+    return getStore(db).get(recipeId);
   },
 
   /**
@@ -185,7 +201,7 @@ var AddonStudies = {
    */
   async getAll() {
     const db = await getDatabase();
-    return getStore(db, "readonly").getAll();
+    return getStore(db).getAll();
   },
 
   /**
@@ -194,7 +210,7 @@ var AddonStudies = {
    */
   async add(study) {
     const db = await getDatabase();
-    return getStore(db, "readwrite").add(study);
+    return getStore(db).add(study);
   },
 
   /**
@@ -204,7 +220,7 @@ var AddonStudies = {
    */
   async delete(recipeId) {
     const db = await getDatabase();
-    return getStore(db, "readwrite").delete(recipeId);
+    return getStore(db).delete(recipeId);
   },
 
   /**
@@ -221,7 +237,7 @@ var AddonStudies = {
     study.active = false;
     study.studyEndDate = new Date();
     const db = await getDatabase();
-    await getStore(db, "readwrite").put(study);
+    await getStore(db).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,20 +73,14 @@ 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, mode) {
-  if (!mode) {
-    throw new Error("mode is required");
-  }
-  return db.objectStore(STORE_NAME, mode);
+function getStore(db) {
+  return db.objectStore(STORE_NAME, "readwrite");
 }
 
 var PreferenceRollouts = {
@@ -130,7 +124,7 @@ var PreferenceRollouts = {
 
       if (changed) {
         const db = await getDatabase();
-        await getStore(db, "readwrite").put(rollout);
+        await getStore(db).put(rollout);
       }
     }
   },
@@ -152,15 +146,18 @@ var PreferenceRollouts = {
   withTestMock(testFunction) {
     return async function inner(...args) {
       let db = await getDatabase();
-      const oldData = await getStore(db, "readonly").getAll();
-      await getStore(db, "readwrite").clear();
+      const oldData = await getStore(db).getAll();
+      await getStore(db).clear();
       try {
         await testFunction(...args);
       } finally {
         db = await getDatabase();
-        await getStore(db, "readwrite").clear();
-        const store = getStore(db, "readwrite");
-        await Promise.all(oldData.map(d => store.add(d)));
+        const store = getStore(db);
+        let promises = [store.clear()];
+        for (const d of oldData) {
+          promises.push(store.add(d));
+        }
+        await Promise.all(promises);
       }
     };
   },
@@ -171,7 +168,7 @@ var PreferenceRollouts = {
    */
   async add(rollout) {
     const db = await getDatabase();
-    return getStore(db, "readwrite").add(rollout);
+    return getStore(db).add(rollout);
   },
 
   /**
@@ -184,7 +181,7 @@ var PreferenceRollouts = {
       throw new Error(`Tried to update ${rollout.slug}, but it doesn't already exist.`);
     }
     const db = await getDatabase();
-    return getStore(db, "readwrite").put(rollout);
+    return getStore(db).put(rollout);
   },
 
   /**
@@ -194,7 +191,7 @@ var PreferenceRollouts = {
    */
   async has(slug) {
     const db = await getDatabase();
-    const rollout = await getStore(db, "readonly").get(slug);
+    const rollout = await getStore(db).get(slug);
     return !!rollout;
   },
 
@@ -204,13 +201,13 @@ var PreferenceRollouts = {
    */
   async get(slug) {
     const db = await getDatabase();
-    return getStore(db, "readonly").get(slug);
+    return getStore(db).get(slug);
   },
 
   /** Get all rollouts in the database. */
   async getAll() {
     const db = await getDatabase();
-    return getStore(db, "readonly").getAll();
+    return getStore(db).getAll();
   },
 
   /** Get all rollouts in the "active" state. */
@@ -233,4 +230,17 @@ 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

@@ -248,6 +248,9 @@ 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,6 +69,30 @@ 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,6 +87,37 @@ 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,6 +128,7 @@ async function withMockActionSandboxManagers(actions, testFunction) {
 }
 
 decorate_task(
+  withSpy(AddonStudies, "close"),
   withStub(Uptake, "reportRunner"),
   withStub(NormandyApi, "fetchRecipes"),
   withStub(ActionsManager.prototype, "fetchRemoteActions"),
@@ -135,6 +136,7 @@ decorate_task(
   withStub(ActionsManager.prototype, "runRecipe"),
   withStub(ActionsManager.prototype, "finalize"),
   async function testRun(
+    closeSpy,
     reportRunnerStub,
     fetchRecipesStub,
     fetchRemoteActionsStub,
@@ -168,12 +170,16 @@ 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"};
@@ -201,6 +207,11 @@ 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();
   }
 );