Bug 1717894 - Register Normandy's UI ready observer before awaiting to avoid races r=andreio

Without this change, Normandy migrations could take longer to run than the UI took to start up, and so the UI ready notification would be sent before the listener was set up. This caused Normandy to sometimes never initialize, especially right after an upgrade.

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

toolkit/components/normandy/Normandy.jsm

@@ -11,6 +11,7 @@ const { XPCOMUtils } = ChromeUtils.import(
 const { PromiseUtils } = ChromeUtils.import(
   "resource://gre/modules/PromiseUtils.jsm"
 );
+const { setTimeout } = ChromeUtils.import("resource://gre/modules/Timer.jsm");
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   AddonRollouts: "resource://normandy/lib/AddonRollouts.jsm",
@@ -48,9 +49,13 @@ var Normandy = {
   studyPrefsChanged: {},
   rolloutPrefsChanged: {},
   defaultPrefsHaveBeenApplied: PromiseUtils.defer(),
+  uiAvailableNotificationObserved: PromiseUtils.defer(),
 
+  /** Initialization that needs to happen before the first paint on startup. */
   async init({ runAsync = true } = {}) {
-    // Initialization that needs to happen before the first paint on startup.
+    // It is important to register the listener for the UI before the first
+    // await, to avoid missing it.
+    Services.obs.addObserver(this, UI_AVAILABLE_NOTIFICATION);
 
     // Listen for when Telemetry is disabled or re-enabled.
     Services.obs.addObserver(
@@ -70,22 +75,27 @@ var Normandy = {
 
     await NormandyMigrations.applyAll();
 
+    // Wait for the UI to be ready, or time out after 5 minutes.
     if (runAsync) {
-      Services.obs.addObserver(this, UI_AVAILABLE_NOTIFICATION);
-    } else {
-      // Remove any observers, if present.
-      try {
-        Services.obs.removeObserver(this, UI_AVAILABLE_NOTIFICATION);
-      } catch (e) {}
-
-      await this.finishInit();
+      await Promise.race([
+        this.uiAvailableNotificationObserved,
+        new Promise(resolve => setTimeout(resolve, 5 * 60 * 1000)),
+      ]);
     }
+
+    // Remove observer for UI notifications. It will error if the notification
+    // was already removed, which is fine.
+    try {
+      Services.obs.removeObserver(this, UI_AVAILABLE_NOTIFICATION);
+    } catch (e) {}
+
+    await this.finishInit();
   },
 
   async observe(subject, topic, data) {
     if (topic === UI_AVAILABLE_NOTIFICATION) {
       Services.obs.removeObserver(this, UI_AVAILABLE_NOTIFICATION);
-      this.finishInit();
+      this.uiAvailableNotificationObserved.resolve();
     } else if (topic === TelemetryUtils.TELEMETRY_UPLOAD_DISABLED_TOPIC) {
       await Promise.all(
         [

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

@@ -136,17 +136,26 @@ decorate_task(
 decorate_task(
   withStub(Normandy, "finishInit"),
   async function testStartupDelayed({ finishInitStub }) {
-    await Normandy.init();
+    let originalDeferred = Normandy.uiAvailableNotificationObserved;
+    let mockUiAvailableDeferred = PromiseUtils.defer();
+    Normandy.uiAvailableNotificationObserved = mockUiAvailableDeferred;
+
+    let initPromise = Normandy.init();
+    await null;
+
     ok(
       !finishInitStub.called,
       "When initialized, do not call finishInit immediately."
     );
 
     Normandy.observe(null, "sessionstore-windows-restored");
+    await initPromise;
     ok(
       finishInitStub.called,
       "Once the sessionstore-windows-restored event is observed, finishInit should be called."
     );
+
+    Normandy.uiAvailableNotificationObserved = originalDeferred;
   }
 );
 

toolkit/components/normandy/test/unit/test_Normandy.js

@@ -7,6 +7,14 @@ const { Normandy } = ChromeUtils.import("resource://normandy/Normandy.jsm");
 const { NormandyMigrations } = ChromeUtils.import(
   "resource://normandy/NormandyMigrations.jsm"
 );
+const { PromiseUtils } = ChromeUtils.import(
+  "resource://gre/modules/PromiseUtils.jsm"
+);
+ChromeUtils.defineModuleGetter(
+  this,
+  "TestUtils",
+  "resource://testing-common/TestUtils.jsm"
+);
 
 /* import-globals-from utils.js */
 load("utils.js");
@@ -53,3 +61,35 @@ decorate_task(
     await Normandy.uninit();
   }
 );
+
+// Normandy's initialization function should register the observer for UI
+// startup before it's first await.
+decorate_task(
+  NormandyTestUtils.withStub(Normandy, "finishInit"),
+  NormandyTestUtils.withStub(NormandyMigrations, "applyAll"),
+  async function test_normandy_init_applies_startup_prefs_synchronously({
+    applyAllStub,
+  }) {
+    let originalDeferred = Normandy.uiAvailableNotificationObserved;
+    let mockUiAvailableDeferred = PromiseUtils.defer();
+    Normandy.uiAvailableNotificationObserved = mockUiAvailableDeferred;
+
+    let applyAllDeferred = PromiseUtils.defer();
+    applyAllStub.returns(applyAllStub);
+
+    let promiseResolvedCount = 0;
+    mockUiAvailableDeferred.promise.then(() => promiseResolvedCount++);
+
+    let initPromise = Normandy.init();
+
+    Assert.equal(promiseResolvedCount, 0);
+    Normandy.observe(null, "sessionstore-windows-restored");
+    await TestUtils.waitForCondition(() => promiseResolvedCount === 1);
+
+    applyAllDeferred.resolve();
+
+    await initPromise;
+    await Normandy.uninit();
+    Normandy.uiAvailableNotificationObserved = originalDeferred;
+  }
+);