Bug 1614738, remove racy promiseContentSearchChange function in favour of a version that adds the listener first before performing the engine modification action, r=adw

This fixes intermittent test failures that occur more frequently with the JSWindowActor based content search module.

Depends on D68237

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

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

browser/base/content/test/about/browser_aboutHome_search_POST.js

@@ -7,16 +7,17 @@ ignoreAllUncaughtExceptions();
 add_task(async function() {
   info("Check POST search engine support");
 
-  await BrowserTestUtils.withNewTab({ gBrowser, url: "about:home" }, function(
-    browser
-  ) {
-    return new Promise(resolve => {
+  let currEngine = await Services.search.getDefault();
+
+  await BrowserTestUtils.withNewTab(
+    { gBrowser, url: "about:home" },
+    async browser => {
+      let observerPromise = new Promise(resolve => {
         let searchObserver = async function search_observer(
           subject,
           topic,
           data
         ) {
-        let currEngine = await Services.search.getDefault();
           let engine = subject.QueryInterface(Ci.nsISearchEngine);
           info("Observer: " + data + " for " + engine.name);
 
@@ -33,13 +34,31 @@ add_task(async function() {
             "browser-search-engine-modified"
           );
 
+          resolve(engine);
+        };
+
+        Services.obs.addObserver(
+          searchObserver,
+          "browser-search-engine-modified"
+        );
+      });
+
+      let engine;
+      await promiseContentSearchChange(browser, async () => {
+        Services.search.addEngine(
+          "http://test:80/browser/browser/base/content/test/about/POSTSearchEngine.xml",
+          null,
+          false
+        );
+
+        engine = await observerPromise;
+        Services.search.setDefault(engine);
+        return engine.name;
+      });
+
       // Ready to execute the tests!
       let needle = "Search for something awesome.";
 
-        await Promise.all([
-          promiseContentSearchChange(browser, engine.name),
-          Services.search.setDefault(engine),
-        ]);
       let promise = BrowserTestUtils.browserLoaded(browser);
       await SpecialPowers.spawn(browser, [{ needle }], async function(args) {
         let doc = content.document;
@@ -65,17 +84,6 @@ add_task(async function() {
       try {
         await Services.search.removeEngine(engine);
       } catch (ex) {}
-        resolve();
-      };
-      Services.obs.addObserver(
-        searchObserver,
-        "browser-search-engine-modified"
-      );
-      Services.search.addEngine(
-        "http://test:80/browser/browser/base/content/test/about/POSTSearchEngine.xml",
-        null,
-        false
+    }
   );
 });
-  });
-});

browser/base/content/test/about/browser_aboutHome_search_composing.js

@@ -12,10 +12,13 @@ add_task(async function() {
     async function(browser) {
       // Add a test engine that provides suggestions and switch to it.
       let currEngine = await Services.search.getDefault();
-      let engine = await promiseNewEngine("searchSuggestionEngine.xml");
-      let p = promiseContentSearchChange(browser, engine.name);
+
+      let engine;
+      await promiseContentSearchChange(browser, async () => {
+        engine = await promiseNewEngine("searchSuggestionEngine.xml");
         await Services.search.setDefault(engine);
-      await p;
+        return engine.name;
+      });
 
       // Clear any search history results
       await new Promise((resolve, reject) => {

browser/base/content/test/about/browser_aboutHome_search_suggestion.js

@@ -13,11 +13,13 @@ add_task(async function() {
     async function(browser) {
       // Add a test engine that provides suggestions and switch to it.
       let currEngine = await Services.search.getDefault();
-      let engine = await promiseNewEngine("searchSuggestionEngine.xml");
-      await Promise.all([
-        promiseContentSearchChange(browser, engine.name),
-        Services.search.setDefault(engine),
-      ]);
+
+      let engine;
+      await promiseContentSearchChange(browser, async () => {
+        engine = await promiseNewEngine("searchSuggestionEngine.xml");
+        await Services.search.setDefault(engine);
+        return engine.name;
+      });
 
       await SpecialPowers.spawn(browser, [], async function() {
         // Type an X in the search input.

browser/base/content/test/about/browser_aboutHome_search_telemetry.js

@@ -13,17 +13,19 @@ add_task(async function() {
     { gBrowser, url: "about:home" },
     async function(browser) {
       let currEngine = await Services.search.getDefault();
-      let engine = await promiseNewEngine("searchSuggestionEngine.xml");
+
+      let engine;
+      await promiseContentSearchChange(browser, async () => {
+        engine = await promiseNewEngine("searchSuggestionEngine.xml");
+        await Services.search.setDefault(engine);
+        return engine.name;
+      });
+
       // Make this actually work in healthreport by giving it an ID:
       Object.defineProperty(engine.wrappedJSObject, "identifier", {
         value: "org.mozilla.testsearchsuggestions",
       });
 
-      await Promise.all([
-        promiseContentSearchChange(browser, engine.name),
-        Services.search.setDefault(engine),
-      ]);
-
       await SpecialPowers.spawn(
         browser,
         [{ expectedName: engine.name }],

browser/base/content/test/about/head.js

@@ -158,28 +158,56 @@ function promiseTabLoadEvent(tab, url) {
 }
 
 /**
- * Wait for the search engine to change.
+ * Wait for the search engine to change. searchEngineChangeFn is a function
+ * that will be called to change the search engine.
  */
-function promiseContentSearchChange(browser, newEngineName) {
-  // Callers of this depend on very specific, very racy timing, and fail
-  // if we introduce the trip through SpecialPowersParent that
-  // SpecialPowers.spawn requires.
-  return ContentTask.spawn(browser, { newEngineName }, async function(args) {
-    return new Promise(resolve => {
-      content.addEventListener("ContentSearchService", function listener(
-        aEvent
-      ) {
-        if (
-          aEvent.detail.type == "CurrentState" &&
-          content.wrappedJSObject.gContentSearchController.defaultEngine.name ==
-            args.newEngineName
-        ) {
-          content.removeEventListener("ContentSearchService", listener);
-          resolve();
+async function promiseContentSearchChange(browser, searchEngineChangeFn) {
+  // Add an event listener manually then perform the action, rather than using
+  // BrowserTestUtils.addContentEventListener as that doesn't add the listener
+  // early enough.
+  await SpecialPowers.spawn(browser, [], async () => {
+    // Store the results in a temporary place.
+    content._searchDetails = {
+      defaultEnginesList: [],
+      listener: event => {
+        if (event.detail.type == "CurrentState") {
+          content._searchDetails.defaultEnginesList.push(
+            content.wrappedJSObject.gContentSearchController.defaultEngine.name
+          );
         }
+      },
+    };
+
+    // Listen using the system group to ensure that it fires after
+    // the default behaviour.
+    content.addEventListener(
+      "ContentSearchService",
+      content._searchDetails.listener,
+      { mozSystemGroup: true }
+    );
   });
-    });
-  });
+
+  let expectedEngineName = await searchEngineChangeFn();
+
+  await SpecialPowers.spawn(
+    browser,
+    [expectedEngineName],
+    async expectedEngineNameChild => {
+      await ContentTaskUtils.waitForCondition(
+        () =>
+          content._searchDetails.defaultEnginesList &&
+          content._searchDetails.defaultEnginesList[
+            content._searchDetails.defaultEnginesList.length - 1
+          ] == expectedEngineNameChild
+      );
+      content.removeEventListener(
+        "ContentSearchService",
+        content._searchDetails.listener,
+        { mozSystemGroup: true }
+      );
+      delete content._searchDetails;
+    }
+  );
 }
 
 /**