2062133 - browser_privatebrowsing_resetPBM.js awaits an array instead of Promise.all, so the tab-close wait does nothing

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Dan Mehic]

In browser/components/privatebrowsing/test/browser/browser_privatebrowsing_resetPBM.js, the test_reset_action_closes_pinned_and_selected_tabs task awaits an array instead of a promise, so the wait it is meant to perform does nothing.

Line 800:

// Create promises for tab close for all tabs in the triggering private browsing window.
let promisesTabsClosed = win.gBrowser.tabs.map(tab =>
  BrowserTestUtils.waitForTabClosing(tab)
);

info("Trigger the restart PBM action");
await ResetPBMPanel._restartPBM(win);

info("Wait for all tabs to be closed.");
await promisesTabsClosed;          // <-- awaits an Array, not a Promise

promisesTabsClosed is an Array of promises, and await on a non-thenable resolves on the next microtask without waiting for any of its elements. The info("Wait for all tabs to be closed.") line therefore guards nothing, and the Assert.equal(win.gBrowser.tabs.length, 1, ...) immediately after it can run before the tabs have actually closed.

The two sibling tasks in the same file get this right and show the intended form:

• line 565 — await Promise.all(promisesTabsClosed);
• line 640 — await Promise.all(promisesTabsClosed);

Fix: change line 800 to await Promise.all(promisesTabsClosed);.

Impact: low in practice. ResetPBMPanel._restartPBM calls removeAllTabsBut before the data-clearing await it performs internally, so by the time _restartPBM resolves the tabs are almost always already gone and the assertion passes anyway. This is a latent correctness problem in the test rather than a known intermittent — I have not linked it to any observed failure.

Found while reviewing bug 1857519, which fixes a different instance of the same class of problem in this file (awaiting something that is not ordered against the thing being asserted). Filed separately to keep that patch minimal.

This looks like a good first bug: single-line change, the correct form is already present twice in the same file, and it can be verified with ./mach mochitest browser/components/privatebrowsing/test/browser/browser_privatebrowsing_resetPBM.js.

Comment 1

[Kana]

Hi, I'm working on a patch for this and have verified the fix locally. Could this bug be assigned to me?

Comment 2

[Kana]

Attachment: Bug 2062133 - properly wait for all promises when closing tabs in test_reset_action_closes_pinned_and_selected_tabs r?timhuang

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7285c0622093
https://hg.mozilla.org/integration/autoland/rev/157cd04c4caa
properly wait for all promises when closing tabs in test_reset_action_closes_pinned_and_selected_tabs r=timhuang

Comment 4

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/157cd04c4caa