From 2c2c3c23dd0797b06737d8823fcb6e291439dd8d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Wed, 13 Nov 2019 18:24:02 +0000
Subject: [PATCH] Bug 1595285 - Fix TestUtils.waitForCondition to not use
 setInterval. r=mak

The test that is timing out with these patches does something relatively simple:

  await TestUtils.waitForCondition(async function() {
    let color = await ContentTask.spawn(browserWindow, async function() {
      /* Do stuff... */
    });
    return color == something;
  });

  await closeWindow(browserWindow);

Turns out that this can intermittently leak the window due to waitForCondition
using setInterval. setInterval can schedule multiple tasks while awaiting for
the inner ContentTask.

What this means, is that we may still have a ContentTask awaiting us when we get
to close the window. Closing the window makes the ContentTask not finish, and
thus we leak a promise keeping alive the window in gPromises:

  https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/testing/mochitest/BrowserTestUtils/ContentTask.jsm#24

Which means that we keep alive the window all the way until shutdown.

Fix it by ensuring that we only run one task at a time.

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

--HG--
extra : moz-landing-system : lando
---
 testing/modules/TestUtils.jsm | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/testing/modules/TestUtils.jsm b/testing/modules/TestUtils.jsm
index 3a650dea2196..958854e6e2b0 100644
--- a/testing/modules/TestUtils.jsm
+++ b/testing/modules/TestUtils.jsm
@@ -21,9 +21,7 @@
 var EXPORTED_SYMBOLS = ["TestUtils"];
 
 const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
-const { clearInterval, setInterval } = ChromeUtils.import(
-  "resource://gre/modules/Timer.jsm"
-);
+const { setTimeout } = ChromeUtils.import("resource://gre/modules/Timer.jsm");
 
 var TestUtils = {
   executeSoon(callbackFn) {
@@ -163,13 +161,15 @@ var TestUtils = {
    * @return Promise
    *        Resolves with the return value of the condition function.
    *        Rejects if timeout is exceeded or condition ever throws.
+   *
+   * NOTE: This is intentionally not using setInterval, using setTimeout
+   * instead. setInterval is not promise-safe.
    */
   waitForCondition(condition, msg, interval = 100, maxTries = 50) {
     return new Promise((resolve, reject) => {
       let tries = 0;
-      let intervalID = setInterval(async function() {
+      async function tryOnce() {
         if (tries >= maxTries) {
-          clearInterval(intervalID);
           msg += ` - timed out after ${maxTries} tries.`;
           reject(msg);
           return;
@@ -180,17 +180,20 @@ var TestUtils = {
           conditionPassed = await condition();
         } catch (e) {
           msg += ` - threw exception: ${e}`;
-          clearInterval(intervalID);
           reject(msg);
           return;
         }
 
         if (conditionPassed) {
-          clearInterval(intervalID);
           resolve(conditionPassed);
+          return;
         }
         tries++;
-      }, interval);
+        setTimeout(tryOnce, interval);
+      }
+
+      // FIXME(bug 1596165): This could be a direct call, ideally.
+      setTimeout(tryOnce, interval);
     });
   },