Backed out changeset 92b9d5110599 (bug 1728843) for causing bc failures in browser_urlbar_telemetry_quicksuggest.js

CLOSED TREE

browser/actors/AboutNewTabChild.jsm

@@ -83,7 +83,7 @@ class AboutNewTabChild extends JSWindowActorChild {
 
         // Note: newtab feature info is currently being loaded in PrefsFeed.jsm,
         // But we're recording exposure events here.
-        NimbusFeatures.newtab.recordExposureEvent({ once: true });
+        NimbusFeatures.newtab.recordExposureEvent();
       }
     }
   }

browser/components/newtab/AboutNewTabService.jsm

@@ -443,7 +443,7 @@ class BaseAboutNewTabService {
      * This is calculated in the same way the default URL is.
      */
 
-    NimbusFeatures.aboutwelcome.recordExposureEvent({ once: true });
+    NimbusFeatures.aboutwelcome.recordExposureEvent();
     if (NimbusFeatures.aboutwelcome.isEnabled({ defaultValue: true })) {
       return ABOUT_WELCOME_URL;
     }

browser/components/newtab/test/browser/browser_aboutwelcome_multistage_experimentAPI.js

@@ -227,7 +227,7 @@ add_task(async function test_multistage_aboutwelcome_experimentAPI() {
     },
   ];
   const sandbox = sinon.createSandbox();
-  NimbusFeatures.aboutwelcome._didSendExposureEvent = false;
+  NimbusFeatures.aboutwelcome._sendExposureEventOnce = true;
   await setAboutWelcomePref(true);
   await ExperimentAPI.ready();
 
@@ -341,10 +341,9 @@ add_task(async function test_multistage_aboutwelcome_experimentAPI() {
     ["div.onboardingContainer"]
   );
 
-  Assert.equal(
-    ExperimentAPI.recordExposureEvent.callCount,
-    1,
-    "Called only once for exposure event"
+  Assert.ok(
+    ExperimentAPI.recordExposureEvent.called,
+    "Called for exposure event"
   );
 
   const scalars = TelemetryTestUtils.getProcessScalars("parent", true, true);

browser/components/urlbar/UrlbarProviderQuickSuggest.jsm

@@ -183,7 +183,7 @@ class ProviderQuickSuggest extends UrlbarProvider {
     if (!this._recordedExposureEvent) {
       this._recordedExposureEvent = true;
       Services.tm.idleDispatchToMainThread(() =>
-        NimbusFeatures.urlbar.recordExposureEvent({ once: true })
+        NimbusFeatures.urlbar.recordExposureEvent()
       );
     }
   }

toolkit/components/nimbus/ExperimentAPI.jsm

@@ -308,7 +308,8 @@ class _ExperimentFeature {
         `No manifest entry for ${featureId}. Please add one to toolkit/components/nimbus/FeatureManifest.js`
       );
     }
-    this._didSendExposureEvent = false;
+    // Prevent the instance from sending multiple exposure events
+    this._sendExposureEventOnce = true;
     this._onRemoteReady = null;
     this._waitForRemote = new Promise(
       resolve => (this._onRemoteReady = resolve)
@@ -513,23 +514,20 @@ class _ExperimentFeature {
     return remoteConfig;
   }
 
-  recordExposureEvent({ once = false } = {}) {
-    if (once && this._didSendExposureEvent) {
-      return;
-    }
-
-    let experimentData = ExperimentAPI.getExperiment({
-      featureId: this.featureId,
-    });
-
-    // Exposure only sent if user is enrolled in an experiment
-    if (experimentData) {
-      ExperimentAPI.recordExposureEvent({
+  recordExposureEvent() {
+    if (this._sendExposureEventOnce) {
+      let experimentData = ExperimentAPI.getExperiment({
         featureId: this.featureId,
-        experimentSlug: experimentData.slug,
-        branchSlug: experimentData.branch?.slug,
       });
-      this._didSendExposureEvent = true;
+      // Exposure only sent if user is enrolled in an experiment
+      if (experimentData) {
+        ExperimentAPI.recordExposureEvent({
+          featureId: this.featureId,
+          experimentSlug: experimentData.slug,
+          branchSlug: experimentData.branch?.slug,
+        });
+        this._sendExposureEventOnce = false;
+      }
     }
   }
 

toolkit/components/nimbus/lib/NimbusFeatures.cpp

@@ -120,13 +120,15 @@ nsresult NimbusFeatures::GetExperimentSlug(const nsACString& aFeatureId,
 
 /**
  * Sends an exposure event for aFeatureId when enrolled in an experiment.
- * By default attempt to send once per function call. For some usecases it might
- * be useful to send only once, in which case set the optional aOnce to `true`.
+ * By default we only attempt to send once. For some usecases it might be useful
+ * to send multiple times or retry to send (when for example we are enrolled
+ * after the first call to this function) in which case set the optional
+ * aForce to `true`.
  */
 nsresult NimbusFeatures::RecordExposureEvent(const nsACString& aFeatureId,
-                                             const bool aOnce) {
+                                             const bool aForce) {
   nsAutoCString featureName(aFeatureId);
-  if (!sExposureFeatureSet.EnsureInserted(featureName) && aOnce) {
+  if (!sExposureFeatureSet.EnsureInserted(featureName) && !aForce) {
     // We already sent (or tried to send) an exposure ping for this featureId
     return NS_ERROR_ABORT;
   }

toolkit/components/nimbus/lib/NimbusFeatures.h

@@ -37,7 +37,7 @@ class NimbusFeatures {
                             PrefChangedFunc aUserCallback, void* aUserData);
 
   static nsresult RecordExposureEvent(const nsACString& aFeatureId,
-                                      const bool aOnce = false);
+                                      const bool aForce = false);
 };
 
 }  // namespace mozilla

toolkit/components/nimbus/test/browser/browser_experiment_single_feature_enrollment.js

@@ -127,5 +127,5 @@ add_task(async function test_TODO() {
 
   await doExperimentCleanup();
   sandbox.restore();
-  NimbusFeatures.urlbar._didSendExposureEvent = false;
+  NimbusFeatures.urlbar._sendExposureEventOnce = true;
 });

toolkit/components/nimbus/test/gtest/NimbusFeatures_RecordExposure.cpp

@@ -29,13 +29,14 @@ TEST_F(NimbusTelemetryFixture, NimbusFeaturesTelemetry) {
       << "Should fail because not enrolled in experiment";
   // Set the experiment info for `foo`
   Preferences::SetCString(prefName.get(), prefValue);
-  ASSERT_EQ(NimbusFeatures::RecordExposureEvent("foo"_ns), NS_OK)
-      << "Should work for the 2nd call";
-  ASSERT_EQ(NimbusFeatures::RecordExposureEvent("foo"_ns, true), NS_ERROR_ABORT)
-      << "Should abort because we've sent exposure and aOnce is true";
+  ASSERT_EQ(NimbusFeatures::RecordExposureEvent("foo"_ns), NS_ERROR_ABORT)
+      << "Should fail even though enrolled because this is the 2nd call";
+  ASSERT_EQ(NimbusFeatures::RecordExposureEvent("foo"_ns, true), NS_OK)
+      << "Should work because we set aForce=true";
   ASSERT_EQ(NimbusFeatures::RecordExposureEvent("bar"_ns), NS_ERROR_UNEXPECTED)
       << "Should fail because we don't have an experiment for bar";
-
+  ASSERT_EQ(NimbusFeatures::RecordExposureEvent("foo"_ns), NS_ERROR_ABORT)
+      << "Should abort because we've already send exposure for this featureId";
   JS::RootedValue eventsSnapshot(cx.GetJSContext());
   GetEventSnapshot(cx.GetJSContext(), &eventsSnapshot);
   ASSERT_TRUE(EventPresent(cx.GetJSContext(), eventsSnapshot, "normandy"_ns,

toolkit/components/nimbus/test/unit/test_ExperimentAPI_ExperimentFeature.js

@@ -273,19 +273,16 @@ add_task(async function test_record_exposure_event_once() {
     })
   );
 
-  featureInstance.recordExposureEvent({ once: true });
-  featureInstance.recordExposureEvent({ once: true });
-  featureInstance.recordExposureEvent({ once: true });
+  featureInstance.recordExposureEvent();
+  featureInstance.recordExposureEvent();
+  featureInstance.recordExposureEvent();
 
-  Assert.ok(
-    exposureSpy.calledOnce,
-    "Should emit a single exposure event when the once param is true."
-  );
+  Assert.ok(exposureSpy.calledOnce, "Should emit a single exposure event.");
 
   sandbox.restore();
 });
 
-add_task(async function test_allow_multiple_exposure_events() {
+add_task(async function test_prevent_double_exposure() {
   const { sandbox, manager } = await setupForExperimentFeature();
 
   const featureInstance = new ExperimentFeature("foo", FAKE_FEATURE_MANIFEST);
@@ -303,11 +300,7 @@ add_task(async function test_allow_multiple_exposure_events() {
   featureInstance.recordExposureEvent();
 
   Assert.ok(exposureSpy.called, "Should emit exposure event");
-  Assert.equal(
-    exposureSpy.callCount,
-    3,
-    "Should emit an exposure event for each function call"
-  );
+  Assert.ok(exposureSpy.calledOnce, "Should emit a single exposure event");
 
   sandbox.restore();
   await doExperimentCleanup();