Bug 1595285 - Fix TestUtils.waitForCondition to not use setInterval. r=mak
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 13 Nov 2019 18:24:02 +0000
changeset 501811 da8d5a96f3c8f93be0a4ca787eedd410dcf3b0c8
parent 501810 2284a535c8bc5a34c448665c99bdf0d7ea2d73f2
child 501812 c17276cc50c4d18a28a8653028f9489ba998d9bc
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1595285
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
testing/modules/TestUtils.jsm
--- a/testing/modules/TestUtils.jsm
+++ b/testing/modules/TestUtils.jsm
@@ -16,19 +16,17 @@
  * example LoginTestUtils.jsm.
  */
 
 "use strict";
 
 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) {
     Services.tm.dispatchToMainThread(callbackFn);
   },
 
   waitForTick() {
     return new Promise(resolve => this.executeSoon(resolve));
@@ -158,44 +156,49 @@ var TestUtils = {
    *        to 100ms.
    * @param attempts
    *        The number of times to poll before giving up and rejecting
    *        if the condition has not yet returned true. Defaults to 50
    *        (~5 seconds for 100ms intervals)
    * @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;
         }
 
         let conditionPassed = false;
         try {
           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);
     });
   },
 
   shuffle(array) {
     let results = [];
     for (let i = 0; i < array.length; ++i) {
       let randomIndex = Math.floor(Math.random() * (i + 1));
       results[i] = results[randomIndex];