2061837 - BrowserTestUtils.overflowTabs hangs for the whole test timeout when the tab strip does not overflow

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Dão Gottwald [:dao]]

BrowserTestUtils.overflowTabs opens tabs until the tab strip overflows, but it decides how many to open from a single measurement:

let tabCountForOverflow = Math.ceil(
  (size(arrowScrollbox) / tabMinSize) * params.overflowTabFactor
);

where tabMinSize is the selected tab's height in vertical mode and the CSS min-width otherwise. It then waits for the result with an untimed BrowserTestUtils.waitForEvent(arrowScrollbox, "overflow").

When the estimate comes up short -- overflowTabFactor: 1.1 leaves only 10% of headroom -- no overflow event is ever dispatched, so the caller hangs until the test times out, with no assertion or diagnostic pointing at the cause.

This is what backed out bug 1961577: browser_drag_tab_group_overflow.js timed out in test-verify on macOS and Windows in the vertical-tabs case. The failure screenshot shows the strip with 20 tabs, no scroll buttons, and free space below the last tab -- the requested tab count simply was not enough to overflow.

Both halves look worth fixing:

• Bound the wait, so a strip that fails to overflow reports a failure instead of burning the full timeout.
• Add tabs until the strip actually overflows (e.g. comparing arrowScrollbox.scrollSize against arrowScrollbox.scrollClientSize) instead of trusting a one-shot estimate.

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 2061837 - Keep adding tabs until the tab strip really overflows. r?#tabbrowser-reviewers

overflowTabs sizes the strip up from a single measurement and then waits for an
overflow event with no timeout, so when the estimate comes up short no event
ever arrives and the caller hangs until the test times out, with no assertion
to point at the cause. That is what backed out bug 1961577.

Top the estimate up while the scrolled content still fits, and wait for the
overflowing state with a bounded condition so a strip that can't overflow fails
with a message instead. The estimate stays as the opening batch, so callers
asking for a factor of 3 still get the same roomy strip.

Worth checking: the promises array that was awaited held tab elements, since
BrowserTestUtils.addTab returns the tab synchronously, so the overflow event
was the only thing that await actually waited for.

Comment 2

[Dão Gottwald [:dao]]

Attachment: Bug 2061837 - Let callers ask overflowTabs for a specific amount of overflow. r?#tabbrowser-reviewers

The tab count the factor produces only approximates how much room there is to
scroll, which is what a test that scrolls the strip actually cares about.
browser_drag_tab_group_overflow.js needs the strip to overflow by more tabs
than the small group it collapses and fewer than the large one, and grew its
own tab-opening loop to get that; overflowBy expresses it directly, so the
loop goes away.

Worth checking: the early return now also has to account for a strip that
overflows by less than the caller asked for.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5dd4d47bb944
https://hg.mozilla.org/integration/autoland/rev/45a92ca16be5
Keep adding tabs until the tab strip really overflows. r=tabbrowser-reviewers,nsharpley
https://github.com/mozilla-firefox/firefox/commit/103f2e252cac
https://hg.mozilla.org/integration/autoland/rev/92297113edce
Let callers ask overflowTabs for a specific amount of overflow. r=tabbrowser-reviewers,jsudiaman

Comment 4

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/45a92ca16be5
https://hg.mozilla.org/mozilla-central/rev/92297113edce