Closed Bug 1247137 Opened 8 years ago Closed 8 years ago

New tab page briefly flashes when closing tab

Categories

(Firefox :: Tabbed Browser, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 49
Tracking Status
e10s ? ---
firefox47 --- verified
firefox48 --- verified
firefox49 --- verified

People

(Reporter: billm, Assigned: mconley)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
Any chance you can verify this STR Mike?
Flags: needinfo?(mconley)
Filed bug 1247443 to see how many people have this pref set.
Yes! I can see it now. Thanks, billm!
Flags: needinfo?(mconley)
Priority: -- → P1
(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.
Status: NEW → RESOLVED
Closed: 8 years ago
tracking-e10s: + → ---
Resolution: --- → DUPLICATE
See Also: 1237654
(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.
Blocks: 1237654
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Status: REOPENED → NEW
tracking-e10s: --- → ?
Depends on: 1247443
Flags: needinfo?(bbermes)
Severity: normal → major
Has STR: --- → yes
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
I'd suggest this blocks release.
Flags: needinfo?(bbermes)
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.
(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
Chris, can you help pull this probe data out of beta 47 so we can take a look at it?
Flags: needinfo?(chutten)
I've taken bug 1269377 to run the analysis. We'll see what comes.
Flags: needinfo?(chutten)
Depends on: 1269377
(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.
Flags: needinfo?(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?
Flags: needinfo?(mconley) → needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
(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.
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 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/
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 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.
Attachment #8748828 - Flags: review?(wmccloskey) → review+
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/
https://hg.mozilla.org/mozilla-central/rev/60e1edb012c0
Status: NEW → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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.
Flags: needinfo?(mconley)
This bug *does* manifest with default prefs and I think it should be uplifted.
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.
Flags: needinfo?(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.
Attachment #8748828 - Flags: approval-mozilla-beta?
Attachment #8748828 - Flags: approval-mozilla-aurora?
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+
Attachment #8748828 - Flags: approval-mozilla-beta?
Attachment #8748828 - Flags: approval-mozilla-beta+
Attachment #8748828 - Flags: approval-mozilla-aurora?
Attachment #8748828 - Flags: approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.