1366473 - Sometimes all tabs have close buttons when overflowing

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dão Gottwald [:dao]]

I sometimes see this after restoring my session. This is likely a regression from bug 1356655. I think there's a race condition due to adjustTabstrip being called from a timeout via _handleNewTab: http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/browser/base/content/tabbrowser.xml#2358-2360

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1366473 - Explicitly hide close buttons in background tabs when overflowing.

Due to adjustTabstrip (1) returning early when overflowing and (2) being called in
setTimeout, it may miss the opportunity to set the closebuttons attribute, which would
make all tabs show their close button in overflow mode.

Review commit: https://reviewboard.mozilla.org/r/141272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141272/

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 8869695 [details]
Bug 1366473 - Explicitly hide close buttons in background tabs when overflowing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141272/diff/1-2/

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8869695 [details]
Bug 1366473 - Explicitly hide close buttons in background tabs when overflowing.

https://reviewboard.mozilla.org/r/141272/#review145098

The patch looks good (so r=me), but that setTimeout seems nasty; do we still need it? (sorry for the delay, I thought I had hit 'publish' on this review yesterday :-/)

Comment 4

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> Comment on attachment 8869695 [details]
> Bug 1366473 - Explicitly hide close buttons in background tabs when
> overflowing.
> 
> https://reviewboard.mozilla.org/r/141272/#review145098
> 
> The patch looks good (so r=me), but that setTimeout seems nasty; do we still
> need it?

Yes, because _handleNewTab needs to know whether the tab got selected after the addTab call.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8869695 [details]
Bug 1366473 - Explicitly hide close buttons in background tabs when overflowing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141272/diff/2-3/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8d70b131270
Explicitly hide close buttons in background tabs when overflowing. r=florian

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8d70b131270

Comment 8

[Oana Botisan, Desktop QA]

I managed to reproduce this bug using the Nightly from 2017-05-20 both on Windows 10 x64 and Ubuntu 16.04 x64. Then I retested using the latest Nightly and Firefox 55.0b13 by opening a lot of new tabs and restarting the browser a few times on Mac Os 10.12, Windows 10 x64 and Ubuntu 16.04 x64 and the bug is not reproducing anymore. 
I think it's fixed.

Comment 9

[Andrei Vaida [:avaida]]

(In reply to Oana Botisan from comment #8)
> I managed to reproduce this bug using the Nightly from 2017-05-20 both on
> Windows 10 x64 and Ubuntu 16.04 x64. Then I retested using the latest
> Nightly and Firefox 55.0b13 by opening a lot of new tabs and restarting the
> browser a few times on Mac Os 10.12, Windows 10 x64 and Ubuntu 16.04 x64 and
> the bug is not reproducing anymore. 
> I think it's fixed.

I can confirm Oana's findings on both 55.0b13 and 56.0a1. I believe it's safe to mark this verified, thank you Oana!