Bug 1691516 - Experiment Store Sync cache should store each feature pref separately r=k88hudson

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

toolkit/components/messaging-system/experiments/ExperimentStore.jsm

@@ -22,7 +22,7 @@ const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 const IS_MAIN_PROCESS =
   Services.appinfo.processType === Services.appinfo.PROCESS_TYPE_DEFAULT;
 
-const SYNC_DATA_PREF = "messaging-system.syncdatastore.data";
+const SYNC_DATA_PREF_BRANCH = "nimbus.syncdatastore.";
 let tryJSONParse = data => {
   try {
     return JSON.parse(data);
@@ -30,16 +30,36 @@ let tryJSONParse = data => {
 
   return {};
 };
-XPCOMUtils.defineLazyPreferenceGetter(
-  this,
-  "syncDataStore",
-  SYNC_DATA_PREF,
-  {},
-  // aOnUpdate
-  (data, prev, latest) => tryJSONParse(latest),
-  // aTransform
-  tryJSONParse
-);
+XPCOMUtils.defineLazyGetter(this, "syncDataStore", () => {
+  // Pref name changed in bug 1691516 and we want to clear the old pref for users
+  try {
+    const previousPrefName = "messaging-system.syncdatastore.data";
+    Services.prefs.clearUserPref(previousPrefName);
+  } catch (e) {}
+
+  let prefBranch = Services.prefs.getBranch(SYNC_DATA_PREF_BRANCH);
+  return {
+    get(featureId) {
+      try {
+        return tryJSONParse(prefBranch.getStringPref(featureId));
+      } catch (e) {
+        /* This is expected if we don't have anything stored */
+      }
+
+      return null;
+    },
+    set(featureId, value) {
+      try {
+        prefBranch.setStringPref(featureId, JSON.stringify(value));
+      } catch (e) {
+        Cu.reportError(e);
+      }
+    },
+    delete(featureId) {
+      prefBranch.clearUserPref(featureId);
+    },
+  };
+});
 
 const DEFAULT_STORE_ID = "ExperimentStoreData";
 // Experiment feature configs that should be saved to prefs for
@@ -61,11 +81,14 @@ class ExperimentStore extends SharedDataMap {
    * @memberof ExperimentStore
    */
   getExperimentForFeature(featureId) {
-    return this.getAllActive().find(
-      experiment =>
-        experiment.featureIds?.includes(featureId) ||
-        // Supports <v1.3.0, which was when .featureIds was added
-        experiment.branch?.feature?.featureId === featureId
+    return (
+      this.getAllActive().find(
+        experiment =>
+          experiment.featureIds?.includes(featureId) ||
+          // Supports <v1.3.0, which was when .featureIds was added
+          experiment.branch?.feature?.featureId === featureId
+        // Default to the pref store if data is not yet ready
+      ) || syncDataStore.get(featureId)
     );
   }
 
@@ -90,7 +113,7 @@ class ExperimentStore extends SharedDataMap {
   getAll() {
     let data = [];
     try {
-      data = Object.values(this._data || syncDataStore);
+      data = Object.values(this._data || {});
     } catch (e) {
       Cu.reportError(e);
     }
@@ -132,19 +155,14 @@ class ExperimentStore extends SharedDataMap {
    * @param {Enrollment} experiment
    */
   _updateSyncStore(experiment) {
-    if (SYNC_ACCESS_FEATURES.includes(experiment.branch.feature?.featureId)) {
+    let featureId = experiment.branch.feature?.featureId;
+    if (SYNC_ACCESS_FEATURES.includes(featureId)) {
       if (!experiment.active) {
-        // Remove experiments on un-enroll, otherwise nothing to do
-        if (syncDataStore[experiment.slug]) {
-          delete syncDataStore[experiment.slug];
-        }
+        // Remove experiments on un-enroll, no need to check if it exists
+        syncDataStore.delete(featureId);
       } else {
-        syncDataStore[experiment.slug] = experiment;
+        syncDataStore.set(featureId, experiment);
       }
-      Services.prefs.setStringPref(
-        SYNC_DATA_PREF,
-        JSON.stringify(syncDataStore)
-      );
     }
   }
 

toolkit/components/messaging-system/test/MSTestUtils.jsm

@@ -91,7 +91,7 @@ const ExperimentFakes = {
       branch: {
         slug: "treatment",
         feature: {
-          featureId: "aboutwelcome",
+          featureId: "test-feature",
           enabled: true,
           value: { title: "hello" },
         },
@@ -118,13 +118,13 @@ const ExperimentFakes = {
         {
           slug: "control",
           ratio: 1,
-          feature: { featureId: "aboutwelcome", enabled: true, value: null },
+          feature: { featureId: "test-feature", enabled: true, value: null },
         },
         {
           slug: "treatment",
           ratio: 1,
           feature: {
-            featureId: "aboutwelcome",
+            featureId: "test-feature",
             enabled: true,
             value: { title: "hello" },
           },

toolkit/components/messaging-system/test/unit/test_ExperimentManager_enroll.js

@@ -14,6 +14,16 @@ const { ClientEnvironment } = ChromeUtils.import(
 );
 const { OS } = ChromeUtils.import("resource://gre/modules/osfile.jsm");
 
+// Experiment store caches in prefs Enrollments for fast sync access
+function cleanupStorePrefCache() {
+  const SYNC_DATA_PREF_BRANCH = "nimbus.syncdatastore.";
+  try {
+    Services.prefs.deleteBranch(SYNC_DATA_PREF_BRANCH);
+  } catch (e) {
+    // Expected if nothing is cached
+  }
+}
+
 /**
  * The normal case: Enrollment of a new experiment
  */
@@ -71,6 +81,7 @@ add_task(
  * - slug conflict
  * - group conflict
  */
+
 add_task(async function test_failure_name_conflict() {
   const manager = ExperimentFakes.manager();
   const sandbox = sinon.createSandbox();
@@ -200,8 +211,9 @@ add_task(async function test_sampling_check() {
 });
 
 add_task(async function enroll_in_reference_aw_experiment() {
-  const SYNC_DATA_PREF = "messaging-system.syncdatastore.data";
-  Services.prefs.clearUserPref(SYNC_DATA_PREF);
+  cleanupStorePrefCache();
+
+  const SYNC_DATA_PREF_BRANCH = "nimbus.syncdatastore.";
   let dir = await OS.File.getCurrentDirectory();
   let src = OS.Path.join(dir, "reference_aboutwelcome_experiment_content.json");
   let bytes = await OS.File.read(src);
@@ -222,7 +234,9 @@ add_task(async function enroll_in_reference_aw_experiment() {
   await manager.enroll(recipe);
 
   Assert.ok(manager.store.get("reference-aw"), "Successful onboarding");
-  let prefValue = Services.prefs.getStringPref(SYNC_DATA_PREF);
+  let prefValue = Services.prefs.getStringPref(
+    `${SYNC_DATA_PREF_BRANCH}aboutwelcome`
+  );
   Assert.ok(
     prefValue,
     "aboutwelcome experiment enrollment should be stored to prefs"

toolkit/components/messaging-system/test/unit/test_ExperimentStore.js

@@ -1,6 +1,6 @@
 "use strict";
 
-const SYNC_DATA_PREF = "messaging-system.syncdatastore.data";
+const SYNC_DATA_PREF_BRANCH = "nimbus.syncdatastore.";
 
 const { ExperimentFakes } = ChromeUtils.import(
   "resource://testing-common/MSTestUtils.jsm"
@@ -17,6 +17,15 @@ add_task(async function test_sharedDataMap_key() {
   Assert.ok(store._sharedDataKey, "Make sure it's defined");
 });
 
+// Experiment store caches in prefs Enrollments for fast sync access
+function cleanupStorePrefCache() {
+  try {
+    Services.prefs.deleteBranch(SYNC_DATA_PREF_BRANCH);
+  } catch (e) {
+    // Expected if nothing is cached
+  }
+}
+
 add_task(async function test_usageBeforeInitialization() {
   const store = ExperimentFakes.store();
   const experiment = ExperimentFakes.experiment("foo", {
@@ -66,7 +75,7 @@ add_task(async function test_event_updates_main() {
   await store.init();
 
   // Set update cb
-  store.on("update:aboutwelcome", updateEventCbStub);
+  store.on(`update:${experiment.branch.feature.featureId}`, updateEventCbStub);
 
   store.addExperiment(experiment);
   store.updateExperiment("foo", { active: false });
@@ -209,7 +218,8 @@ add_task(async function test_updateExperiment() {
 });
 
 add_task(async function test_sync_access_before_init() {
-  Services.prefs.clearUserPref(SYNC_DATA_PREF);
+  cleanupStorePrefCache();
+
   let store = ExperimentFakes.store();
 
   Assert.equal(store.getAll().length, 0, "Start with an empty store");
@@ -220,23 +230,31 @@ add_task(async function test_sync_access_before_init() {
   await store.init();
   store.addExperiment(syncAccessExp);
 
-  let prefValue = JSON.parse(Services.prefs.getStringPref(SYNC_DATA_PREF));
+  let prefValue;
+  try {
+    prefValue = JSON.parse(
+      Services.prefs.getStringPref(`${SYNC_DATA_PREF_BRANCH}newtab`)
+    );
+  } catch (e) {
+    Assert.ok(false, "Failed to parse pref value");
+  }
 
-  Assert.ok(Object.keys(prefValue).length === 1, "Parsed stored experiment");
-  Assert.equal(
-    prefValue.foo.slug,
-    syncAccessExp.slug,
-    "Got back the experiment"
-  );
+  Assert.ok(prefValue, "Parsed stored experiment");
+  Assert.equal(prefValue.slug, syncAccessExp.slug, "Got back the experiment");
 
   // New un-initialized store that should read the pref value
   store = ExperimentFakes.store();
 
-  Assert.equal(store.getAll().length, 1, "Returns experiment from pref");
+  Assert.equal(
+    store.getExperimentForFeature("newtab").slug,
+    "foo",
+    "Returns experiment from pref"
+  );
 });
 
 add_task(async function test_sync_access_update() {
-  Services.prefs.clearUserPref(SYNC_DATA_PREF);
+  cleanupStorePrefCache();
+
   let store = ExperimentFakes.store();
   let experiment = ExperimentFakes.experiment("foo", {
     feature: { featureId: "aboutwelcome", enabled: true },
@@ -253,14 +271,19 @@ add_task(async function test_sync_access_update() {
   });
 
   store = ExperimentFakes.store();
-  let experiments = store.getAll();
+  let cachedExperiment = store.getExperimentForFeature("aboutwelcome");
 
-  Assert.equal(experiments.length, 1, "Got back 1 experiment");
-  Assert.equal(experiments[0].branch.feature.value, "bar", "Got updated value");
+  Assert.ok(cachedExperiment, "Got back 1 experiment");
+  Assert.equal(
+    cachedExperiment.branch.feature.value,
+    "bar",
+    "Got updated value"
+  );
 });
 
 add_task(async function test_sync_features_only() {
-  Services.prefs.clearUserPref(SYNC_DATA_PREF);
+  cleanupStorePrefCache();
+
   let store = ExperimentFakes.store();
   let experiment = ExperimentFakes.experiment("foo", {
     feature: { featureId: "cfr", enabled: true },
@@ -275,7 +298,8 @@ add_task(async function test_sync_features_only() {
 });
 
 add_task(async function test_sync_access_unenroll() {
-  Services.prefs.clearUserPref(SYNC_DATA_PREF);
+  cleanupStorePrefCache();
+
   let store = ExperimentFakes.store();
   let experiment = ExperimentFakes.experiment("foo", {
     feature: { featureId: "aboutwelcome", enabled: true },
@@ -294,10 +318,11 @@ add_task(async function test_sync_access_unenroll() {
 });
 
 add_task(async function test_sync_access_unenroll_2() {
-  Services.prefs.clearUserPref(SYNC_DATA_PREF);
+  cleanupStorePrefCache();
+
   let store = ExperimentFakes.store();
   let experiment1 = ExperimentFakes.experiment("foo", {
-    feature: { featureId: "aboutwelcome", enabled: true },
+    feature: { featureId: "newtab", enabled: true },
   });
   let experiment2 = ExperimentFakes.experiment("bar", {
     feature: { featureId: "aboutwelcome", enabled: true },
@@ -310,24 +335,39 @@ add_task(async function test_sync_access_unenroll_2() {
 
   Assert.equal(store.getAll().length, 2, "2/2 experiments");
 
-  store.updateExperiment("bar", { active: false });
   let other_store = ExperimentFakes.store();
-  Assert.equal(
-    other_store.getAll().length,
-    1,
-    "Unenrolled from 1/2 experiments"
+
+  Assert.ok(
+    other_store.getExperimentForFeature("aboutwelcome"),
+    "Fetches experiment from pref cache even before init (aboutwelcome)"
+  );
+
+  store.updateExperiment("bar", { active: false });
+
+  Assert.ok(
+    other_store.getExperimentForFeature("newtab").slug,
+    "Fetches experiment from pref cache even before init (newtab)"
+  );
+  Assert.ok(
+    !other_store.getExperimentForFeature("aboutwelcome")?.slug,
+    "Experiment was updated and should not be found"
   );
 
   store.updateExperiment("foo", { active: false });
-  Assert.equal(
-    other_store.getAll().length,
-    0,
+  Assert.ok(
+    !other_store.getExperimentForFeature("newtab")?.slug,
     "Unenrolled from 2/2 experiments"
   );
 
   Assert.equal(
-    Services.prefs.getStringPref(SYNC_DATA_PREF),
-    "{}",
-    "Empty store"
+    Services.prefs.getStringPref(`${SYNC_DATA_PREF_BRANCH}newtab`, "").length,
+    0,
+    "Cleared pref 1"
+  );
+  Assert.equal(
+    Services.prefs.getStringPref(`${SYNC_DATA_PREF_BRANCH}aboutwelcome`, "")
+      .length,
+    0,
+    "Cleared pref 2"
   );
 });