1247137 - New tab page briefly flashes when closing tab

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

STR:
1. Set browser.tabs.animate to false.
2. Open 5 or so tabs with some somewhat heavy stuff (treeherder for example).
3. Start closing the tabs.

I don't always see the new tab page, but it usually appears once when closing 5 or 10 tabs.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Any chance you can verify this STR Mike?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Filed bug 1247443 to see how many people have this pref set.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Yes! I can see it now. Thanks, billm!

Comment 4

[Dão Gottwald [:dao]]

(In reply to Benjamin Smedberg  [:bsmedberg] from bug 1247443 comment #6)
> This pref is not exposed anywhere in our preferences UI, correct? Do we
> intend to keep supporting the non-default config?

Certain animations are very distracting for some people. Being able to disable them is considered an accessibility feature. I'm not 100% sure that applies to this case, though.

Note that removeTab may also be called without the caller explicitly enabling the animation, in which case we won't animate, for backwards-compatility and to support callers expecting the tab to go away synchronously. We do this a lot in tests and some add-ons might depend on it.

Last but not least, regardless of how many users are affected, it would be really nice to understand what's going on, since it feels like something fundamental is broken here and flipping the pref just happens to expose it.

Comment 6

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Dão Gottwald [:dao] from comment #4)
> (In reply to Benjamin Smedberg  [:bsmedberg] from bug 1247443 comment #6)

It's also performance boost feature.

Comment 7

[Barbara Bermes [:barbara] - NI please!]

I'd suggest this blocks release.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

I think this bug really only presents if browser.tabs.animate is set to false (which is not the default).

A Telemetry probe landed (bug 1247443) to see how many of our users have that pref set to false. In https://bugzilla.mozilla.org/show_bug.cgi?id=1247443#c14, chutten noted that on Nightly, only a small proportion of our users reported having that pref set that way (0.7%). We might want to do the same analysis on our beta population before putting time into trying to fix this right now.

I've filed bug 1269377 to try to find that out.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #8)
> I think this bug really only presents if browser.tabs.animate is set to
> false (which is not the default).

I think it also happens when closing the last tab. More specifically, it happens when this condition is true:
http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2265-2272

Comment 10

[Jim Mathies [:jimm]]

Chris, can you help pull this probe data out of beta 47 so we can take a look at it?

Comment 11

[Chris H-C :chutten]

I've taken bug 1269377 to run the analysis. We'll see what comes.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #8)
> I think this bug really only presents if browser.tabs.animate is set to
> false (which is not the default).
> 
> A Telemetry probe landed (bug 1247443) to see how many of our users have
> that pref set to false. In
> https://bugzilla.mozilla.org/show_bug.cgi?id=1247443#c14, chutten noted that
> on Nightly, only a small proportion of our users reported having that pref
> set that way (0.7%). We might want to do the same analysis on our beta
> population before putting time into trying to fix this right now.
> 
> I've filed bug 1269377 to try to find that out.

I'm not sure what kind of results you expect that could tell us anything we don't already know. It's clear that hidden prefs have low usage, and that the usage will be lower the closer you get to the release channel. As I've tried to explain in comment 4, I think the question of how much this pref is used is a distraction and frankly a waste of time.

We just shouldn't be willing to tolerate this kind of bug in a crucial part of the browser, even if currently only a minority is exposed to it.

My guess would be that this is a regression from async tab switching. In any case, somebody needs to dive into this and figure out where exactly our code is broken, the sooner the better.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Alright, I know what's happening, at least.

When we close a tab without animation (and call _endRemoveTab right away), we destroy and remove the <xul:browser> from the DOM before we have a chance of switching to the next appropriate tab (since switching is asynchronous in an e10s world, and the layer tree isn't ready right away).

The <deck> that all of the browsers are in has a spot for the "preloaded browser" that we use for quickly opening the newtab page with. This preloaded <browser> is right at the end of the <deck>. When we remove the last <browser> element, because the layer tree for the tab-to-be-switched-to isn't ready right away, the <deck> defaults to showing the next element instead, which happens to be the preloaded browser. Once the layer tree for the switched-to tab becomes available, we then switch to that element in the deck.

So that's what's going on.

I have an idea that seems to work locally - Dao, how do you feel about setting visibility: hidden on the preloaded browser until it's needed?

Comment 14

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
> I have an idea that seems to work locally - Dao, how do you feel about
> setting visibility: hidden on the preloaded browser until it's needed?

Sounds like a not so great hack to me. I think we should switch to the right browser despite its layer tree not being ready (like we do when the layer tree update takes too long). Either that or we should keep the closing tab's browser around a little bit longer, but that feels more like a more invasive change that could have undesirable side effects.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1247137 - Show spinner if switching away from browser that goes away immediately. r?billm

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

Comment 16

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
> > I have an idea that seems to work locally - Dao, how do you feel about
> > setting visibility: hidden on the preloaded browser until it's needed?
> 
> Sounds like a not so great hack to me. I think we should switch to the right
> browser despite its layer tree not being ready (like we do when the layer
> tree update takes too long).

Ah, so instead of newtab flashes, it'll be spinner flashes. :/ Not amazing, but I agree it makes more sense than flashing the newtab page.

The patch I just posted is the least-hacky way I could find of doing that.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks Mike! I'm a little worried that we could still get into this bad situation in the non-immediate case (the one starting at [1]) as follows:

- We call _blurTab, which invokes the tab switching code. It starts loading some other tab but leaves us at the current tab until that one loads.
- We end up calling _endRemoveTab before the new tab has finished loading. I think _endRemoveTab typically triggered from [2], and the transition time seems to be 230ms (?) [3], which isn't very long.

Can we instead have _endRemoveTab check if there's an active switcher and notify it in that case? The switcher could just set lastVisibleTab to null if it's the one being closed and then call postActions. That will take care of updating the display.

[1] http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2279
[2] http://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#5646
[3] http://searchfox.org/mozilla-central/source/browser/base/content/browser.css#155

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8748828 [details]
MozReview Request: Bug 1247137 - Show spinner if switching away from browser that goes away immediately. r?billm

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

Comment 19

[Paul Pasca[Away. Please needinfo? Paul Oiegas [:pauloiegasSV]]]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160505030327

I've managed to reproduce this on the latest Nightly(49.0a1). This is only reproducible if e10s is enabled. I've also performed a regression on this issue and bellow are the results:

Last good revision: 4cb766685b73 (2014-03-03)
First bad revision: 529b86b92b1d (2014-03-04)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4cb766685b73&tochange=529b86b92b1d

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8748828 [details]
MozReview Request: Bug 1247137 - Show spinner if switching away from browser that goes away immediately. r?billm

https://reviewboard.mozilla.org/r/50547/#review47585

Thanks so much! This eliminates the problem for me.

::: browser/base/content/tabbrowser.xml:3643
(Diff revision 2)
> +            onTabRemoved: function(tab) {
> +              if (this.lastVisibleTab == tab) {
> +                // The browser that was being presented to the user is
> +                // going to be removed during this tick of the event loop.
> +                // This will cause us to show a tab spinner instead.
> +                this.lastVisibleTab = null;

Could you call preActions before this? I'm not sure if it's necessary, but it would give me some peace of mind.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8748828 [details]
MozReview Request: Bug 1247137 - Show spinner if switching away from browser that goes away immediately. r?billm

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

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60e1edb012c0

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60e1edb012c0

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Mike, this bug though severe does not manifest itself by default (unless the pref is changed). It showed up on my radar since it's a P1 regression. Should we uplift to Aurora48 and/or Beta47? Please let me know.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This bug *does* manifest with default prefs and I think it should be uplifted.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

I agree - especially considering that we're pushing Test Pilot pretty hard right now, and one of the projects there (Tab Center) exposes this bug.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8748828 [details]
MozReview Request: Bug 1247137 - Show spinner if switching away from browser that goes away immediately. r?billm

Approval Request Comment
[Feature/regressing bug #]:

e10s

[User impact if declined]:

Users with e10s enabled may see the New Tab page flash intermittently when closing the last tab, which is rather jarring.

[Describe test coverage new/current, TreeHerder]:

Manual testing, and a number of days on central.

[Risks and why]: 

Low risk. This is isolated to just e10s-specific code, so it will not affect non-e10s at all.

[String/UUID change made/needed]:

None.

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8748828 [details]
MozReview Request: Bug 1247137 - Show spinner if switching away from browser that goes away immediately. r?billm

P1 regression in e10s, Aurora48+, Beta47+

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f923e49a077

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5f542b19b619

Comment 31

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced the initial issue on 2016-02-10 Nightly on Windows 10x64.

Confirming the fix across platforms using:
- Latest 49.0a1 Nightly
- Latest 48.0a2 Aurora
- Firefox 47 beta 9