Bug 1595285 - Fix TestUtils.waitForCondition to not use setInterval. r=mak
☠☠ backed out by 2c748c06bd9b ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 13 Nov 2019 14:39:51 +0000
changeset 501746 ffc59a6dfbd58dcc28114acc38a7ef29f55d0aab
parent 501745 149d044837d69f645972cc10319baee944cd121b
child 501747 b73d74bcafda125ebc57dcd59fb3dd00cacfed8c
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,48 @@ 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);
+      }
+
+      tryOnce();
     });
   },
 
   shuffle(array) {
     let results = [];
     for (let i = 0; i < array.length; ++i) {
       let randomIndex = Math.floor(Math.random() * (i + 1));
       results[i] = results[randomIndex];