Closed Bug 1362866 Opened 7 years ago Closed 7 years ago

Fix tab switch spinner regression (re-)introduced by bug 1054740

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(6 files)

Regression in graph (last data point)
30.70 KB, image/png
Details
Bug 1362866 - Get rid of MozTabChildNotReady event and all of its required infrastructure.
59 bytes, text/x-review-board-request
billm
: review+
Details
Bug 1362866 - Remove MozTabChildNotReady event usage from tabbrowser.xml, and replace with nsITabParent.hasPresented.
59 bytes, text/x-review-board-request
billm
: review+
Details
Bug 1362866 - Fix browser_tabSpinnerProbe.js now that tabs must be presented to spin. Also remove browser_tabSpinnerTypeProbe.js.
59 bytes, text/x-review-board-request
billm
: review+
Details
Bug 1362866 - Remove FX_TAB_SWITCH_SPINNER_TYPE probe.
59 bytes, text/x-review-board-request
billm
: review+
Details
Bug 1362866 - Rearrange tab focusing behaviour to avoid extra potential reflows.
59 bytes, text/x-review-board-request
Felipe
: review+
Details 
The patches in bug 1351677 originally landed in order to avoid an unnecessary tab switch when restoring a session. The patches did this by moving the initial browser to the selected browser slot, meaning that it was a tab move instead of a tab switch. This fixed the tab switch regression that had been introduced by bug 1256472.

Bug 1054740 ended up reverting that optimization, and the regression has returned.

The tab switch spinner tracker that I have at https://mikeconley.github.io/bug1310250/ is showing an increase in the number of clients seeing tab switch spinners by ~10%. This is almost certainly caused by the same reason that bug 1351677 tried to fix: the initial tab switch during session restore doesn't occur fast enough to avoid the tab switch spinner.

I'm happy with the simplification that Dao introduced in bug 1054740. I'm less happy with the regression in this metric, and I'm hoping we can find some kind of compromise.
I think I've found a compromise - if the tab that we're switching to is remote and has never presented, I _think_ we can treat it as "blank", and avoid the spinner.
(In reply to Mike Conley (:mconley) from comment #1)
> I think I've found a compromise - if the tab that we're switching to is
> remote and has never presented, I _think_ we can treat it as "blank", and
> avoid the spinner.

So this is mostly a cosmetic problem, not really a perf issue?
(In reply to Dão Gottwald [::dao] from comment #2)
> So this is mostly a cosmetic problem, not really a perf issue?

I guess it depends on your definition of both of those things. We're definitely doing more work in the new configuration, since we're switching away from one tab to another.

I see no evidence if this bug actually impacts, say, session restoration time. This probe doesn't seem to show much of a change when this hit Nightly: https://mzl.la/2pRxR2x . Neither does this one: https://mzl.la/2pRuZT1 .

So I have to conclude that session restoration hasn't been impacted.

What I think we've done is increased the likelihood that a tab switch spinner will display during the restoration. I guess that's more of a cosmetic / perceived performance thing.
I suspect that the hasPresented attribute that was added in bug 1342464 can be useful here; if we're async tab switching to a tab we've never seen before, we can blank it out.

This actually encapulates the case that I tried to solve in bug 1342927 (a tab can never have presented if it's TabChild was never constructed). I actually think we can strip out a lot of what I added in bug 1342927, get rid of the MozTabChildNotReady stuff, and just use hasPresented to make our decisions here. I'll try that next.
(In reply to Mike Conley (:mconley) from comment #3)
> (In reply to Dão Gottwald [::dao] from comment #2)
> > So this is mostly a cosmetic problem, not really a perf issue?
> 
> I guess it depends on your definition of both of those things. We're
> definitely doing more work in the new configuration, since we're switching
> away from one tab to another.

Except of course that we were loading about:home while restoring the session which was work too. :)
(In reply to Mike Conley (:mconley) from comment #4)
> I suspect that the hasPresented attribute that was added in bug 1342464 can
> be useful here; if we're async tab switching to a tab we've never seen
> before, we can blank it out.

Does this include links opened in background tabs?
(In reply to Dão Gottwald [::dao] from comment #8)
> 
> Does this include links opened in background tabs?

Yes. Though I believe the other browsers do the same thing (tested Chrome and Safari).
Attachment #8865279 - Flags: review?(wmccloskey)
Try build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e840ac6faa0cb854c85bce3475c8b913edaa6580 suggests I still have some front-end kinks to work out.
(In reply to Mike Conley (:mconley) from comment #9)
> (In reply to Dão Gottwald [::dao] from comment #8)
> > 
> > Does this include links opened in background tabs?
> 
> Yes. Though I believe the other browsers do the same thing (tested Chrome
> and Safari).

But these aren't "blank". And if they were, why bother hiding them with opacity:0? This is all a bit confusing...
Comment on attachment 8865279 [details]
Bug 1362866 - Remove MozTabChildNotReady event usage from tabbrowser.xml, and replace with nsITabParent.hasPresented.

https://reviewboard.mozilla.org/r/136896/#review139978

::: browser/base/content/tabbrowser.css:106
(Diff revision 1)
>  browser[pendingpaint] {
>    opacity: 0;
>  }
>  
> -tabbrowser[pendingtabchild] {
> +tabbrowser[blank] {
>    background-color: #ffffff !important;

This looks wrong too. The right color is set here: http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/browser/base/content/tabbrowser.xml#5232-5235
Is this ready for review?
Flags: needinfo?(mconley)
(In reply to Bill McCloskey (:billm) from comment #15)
> Is this ready for review?

The patch that removes the MozTabChildNotReady event, and all the required infrastructure is, yes. I still need to iron out some cases for the tabbrowser.xml changes - that should be ready soon.
Flags: needinfo?(mconley)
This ended up being a little bit of a nightmare, because we move from having a MozTabChildNotReady event firing triggering the blank, to the blank possibly occurring right away because we know the TabParent has not presented.

That caused a bunch of test failure, and a few edge cases with focus, and where timers were trying to be cleared even though they didn't exist. This configuration seemed to make try happy:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff54bcf6e0a9e33d322f6233dd213322d0cb8561

Most of these patches are really straight-forward. https://reviewboard.mozilla.org/r/136896/diff/2#index_header is the one to watch out for - this one, I'll really appreciate a fine-tuned eye to make sure my approach makes sense in the context of the async tab switcher.
(In reply to Dão Gottwald [::dao] from comment #11)
> (In reply to Mike Conley (:mconley) from comment #9)
> > (In reply to Dão Gottwald [::dao] from comment #8)
> > > 
> > > Does this include links opened in background tabs?
> > 
> > Yes. Though I believe the other browsers do the same thing (tested Chrome
> > and Safari).
> 
> But these aren't "blank". And if they were, why bother hiding them with
> opacity:0? This is all a bit confusing...

Could you please comment on this? I can't seem to figure out how initially hiding these browsers is useful. I'd like to have some basic understanding of this as a peer and triage owner.
Comment on attachment 8865279 [details]
Bug 1362866 - Remove MozTabChildNotReady event usage from tabbrowser.xml, and replace with nsITabParent.hasPresented.

https://reviewboard.mozilla.org/r/136896/#review140794

::: commit-message-edbfd:1
(Diff revision 2)
> +Bug 1362866 - Replace MozTabChildNotReady support from tabbrowser.xml, and replace with nsITabParent.hasPresented. r?billm

Nit: first "Replace" should be "Remove", and "support" should be "usage".
Comment on attachment 8865279 [details]
Bug 1362866 - Remove MozTabChildNotReady event usage from tabbrowser.xml, and replace with nsITabParent.hasPresented.

https://reviewboard.mozilla.org/r/136896/#review140834

::: browser/base/content/tabbrowser.xml:4016
(Diff revision 2)
>                // First, let's deal with blank tabs, which we show instead
>                // of the spinner when the tab is not currently set up
>                // properly in the content process.
> +              let wasBlank = false;

TODO: Add some commentary about why it may be necessary or desirable to blank out the tab.
Comment on attachment 8865279 [details]
Bug 1362866 - Remove MozTabChildNotReady event usage from tabbrowser.xml, and replace with nsITabParent.hasPresented.

https://reviewboard.mozilla.org/r/136896/#review141846

I like the overall idea here, but I have a lot of questions.

::: browser/base/content/tabbrowser.css:100
(Diff revision 2)
> -browser[pendingtabchild],
> +browser[blank],
>  browser[pendingpaint] {
>    opacity: 0;
>  }
>  
> -tabbrowser[pendingtabchild] {
> -  background-color: #ffffff !important;
> -}
> -

I don't really understand this part. Is the intent to make the blank tabs be the background chrome color instead of white? Anyway, I don't see why you go to the trouble of setting a blank attribute on tabbrowser if there's no CSS for it.

::: browser/base/content/tabbrowser.xml:3999
(Diff revision 2)
>  
> +              // Instead of showing the tab switch spinner, if a tab is
> +              // remote, we can show a blank tab instead of a spinner if
> +              // we know it has never presented before.
>                let shouldBeBlank =
> -                this.pendingTabChild.has(this.requestedTab.linkedBrowser) &&
> +                requestedBrowser.isRemoteBrowser &&

Not sure the isRemoteBrowser test is necessary if you're also checking tabParent.

::: browser/base/content/tabbrowser.xml:4039
(Diff revision 2)
>  
>                // Show or hide the spinner as needed.
>                let needSpinner = this.getTabState(showTab) != this.STATE_LOADED &&
>                                  !this.minimized &&
> -                                !shouldBeBlank;
> +                                !shouldBeBlank &&
> +                                showTab.linkedBrowser.isRemoteBrowser;

Why do we need the isRemoteBrowser check? Non-remote browsers are only ever in states STATE_LOADED or STATE_UNLOADED. Are you worried that showTab could be STATE_UNLOADED? It seems like that shouldn't happen. Maybe we should assert that, though.

::: browser/base/content/tabbrowser.xml:4058
(Diff revision 2)
>                  this.tabbrowser.setAttribute("pendingpaint", "true");
>                  this.spinnerTab.linkedBrowser.setAttribute("pendingpaint", "true");
>                }
>  
>                // Switch to the tab we've decided to make visible.
> -              if (this.visibleTab !== showTab) {
> +              if (this.visibleTab !== showTab || wasBlank) {

I don't understand why this extra condition is here. Why would we do a tab switch if wasBlank is true but visibleTab === showTab?

::: browser/base/content/tabbrowser.xml:4132
(Diff revision 2)
>                }
>                if (this.spinnerTab && !this.spinnerTab.linkedBrowser) {
>                  this.spinnerHidden();
>                  this.spinnerTab = null;
>                }
> -              if (this.loadingTab && !this.loadingTab.linkedBrowser) {
> +              if (this.loadingTab && !this.loadingTab.linkedBrowser && this.loadTimer) {

What's this for? loadingTimer should be null iff loadingTab is null.

::: browser/base/content/tabbrowser.xml:4254
(Diff revision 2)
>                this.setTabState(tab, this.STATE_LOADED);
>  
>                this.maybeFinishTabSwitch();
>  
>                if (this.loadingTab === tab) {
> +                if (this.loadTimer) {

What is this here for? loadingTimer should be null iff loadingTab is null.

::: browser/base/content/tabbrowser.xml:4417
(Diff revision 2)
> +                if (this.loadTimer && this.loadingTab === tab &&
> +                    !fl.tabParent.hasPresented) {
> +                  this.clearTimer(this.loadTimer);
> +                  this.loadTimer = null;
> +                  this.loadingTab = null;
> +                  this.assert(this.getTabState(tab) == this.STATE_LOADING);
> +                }
> +              }
> +
> +              if (this.loadTimer && !browser.isRemoteBrowser) {
> +                this.clearTimer(this.loadTimer);
> +                this.loadTimer = null;
> +                this.loadingTab = null;
> +              }

This code is very worrisome to me. What is its purpose? I can see some of it was copied from the onTabChildNotReady event, but I can't remember what it was doing there. It needs comments.
Attachment #8865279 - Flags: review?(wmccloskey)
Comment on attachment 8865279 [details]
Bug 1362866 - Remove MozTabChildNotReady event usage from tabbrowser.xml, and replace with nsITabParent.hasPresented.

https://reviewboard.mozilla.org/r/136896/#review141846

> I don't really understand this part. Is the intent to make the blank tabs be the background chrome color instead of white? Anyway, I don't see why you go to the trouble of setting a blank attribute on tabbrowser if there's no CSS for it.

> Is the intent to make the blank tabs be the background chrome color instead of white?

Yes - Dao pointed out that we already calculate that here:

http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/browser/base/content/tabbrowser.xml#5232-5235

> I don't understand why this extra condition is here. Why would we do a tab switch if wasBlank is true but visibleTab === showTab?

I was dealing with some focus issues in some tests, but I think I've found a better way to do this in the upcoming patch.
The biggest changes, along with the review feedback, involved some edge cases to satisfy from tests. Mainly:

1) We now show blank if the browser is remote but tabParent is not defined, since this can occur if the tab has crashed. TabCrashHandler is still in charge of making sure to load about:tabcrashed in the tab when needed.
2) For browser/components/newtab/tests/browser/browser_newtab_overrides.js and browser/base/content/test/general/browser_bug565575.js, in order to make sure that the right thing is focused, I now do the work of setting whether or not the URL or find bar was focused in the tab we're switching away from inside a new function, _adjustFocusBeforeTabSwitch, that occurs inside updateDisplay when we change the visibleTab.

This changes the order of things slightly for e10s windows compared to non-e10s windows, as in non-e10s windows, we do the _adjustFocusBeforeTabSwitch inside updateCurrentBrowser (which is fired after the initial requestTab call in a select event handler). With e10s now, I'm running _adjustFocusBeforeTabSwitch _and_ _adjustFocusAfterTabSwitch inside updateDisplay when shifting the visibleTab, and they're both happening before updateCurrentBrowser now.

This has made tests happy, in any case, and seems to ensure I don't accidentally focus or blur things unnecessarily (which I think I was doing in the previous patch).
Comment on attachment 8865279 [details]
Bug 1362866 - Remove MozTabChildNotReady event usage from tabbrowser.xml, and replace with nsITabParent.hasPresented.

https://reviewboard.mozilla.org/r/136896/#review143690

This looks good to me. I don't really know enough about the focus stuff to feel confident reviewing it though. Maybe someone else could take a look?

::: browser/base/content/tabbrowser.xml:3944
(Diff revision 3)
> +
> +              if (!tab.linkedBrowser.isRemoteBrowser) {
> +                // setTabState is potentially re-entrant in the non-remote case,
> +                // so we must re-get the state for this assertion.
> +                let nonRemoteState = this.getTabState(tab);
> +                // non-remote tabs can never stay in the STATE_LOADING

Please capitalize.

::: browser/base/content/tabbrowser.xml:3948
(Diff revision 3)
> +                let nonRemoteState = this.getTabState(tab);
> +                // non-remote tabs can never stay in the STATE_LOADING
> +                // or STATE_UNLOADING states. By the time this function
> +                // exits, a non-remote tab must be in STATE_LOADED or
> +                // STATE_UNLOADED, since the painting and the layer
> +                // upload happens synchronously.

happen
Attachment #8865279 - Flags: review?(wmccloskey) → review+
Comment on attachment 8865278 [details]
Bug 1362866 - Get rid of MozTabChildNotReady event and all of its required infrastructure.

https://reviewboard.mozilla.org/r/136920/#review143696
Attachment #8865278 - Flags: review?(wmccloskey) → review+
Comment on attachment 8866061 [details]
Bug 1362866 - Fix browser_tabSpinnerProbe.js now that tabs must be presented to spin. Also remove browser_tabSpinnerTypeProbe.js.

https://reviewboard.mozilla.org/r/137656/#review143698

Do we still need the FX_TAB_SWITCH_SPINNER_TYPE probe. It seems like its value will always be "seen" now. It would be nice to remove it from Histograms.json and spinnerDisplayed.
Attachment #8866061 - Flags: review?(wmccloskey) → review+
Comment on attachment 8866062 [details]
Bug 1362866 - Remove FX_TAB_SWITCH_SPINNER_TYPE probe.

https://reviewboard.mozilla.org/r/137658/#review143700

Heh heh. Okay.
Attachment #8866062 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #37)
> Comment on attachment 8865279 [details]
> Bug 1362866 - Remove MozTabChildNotReady event usage from tabbrowser.xml,
> and replace with nsITabParent.hasPresented.
> 
> https://reviewboard.mozilla.org/r/136896/#review143690
> 
> This looks good to me. I don't really know enough about the focus stuff to
> feel confident reviewing it though. Maybe someone else could take a look?
> 

Cool, I'll ask dao, thanks billm.

Hey dao, so I'm not sure how privy you are to what goes on inside of the async tab switcher, but with regard to focus, here's what I'm changing.

For the non-e10s case, I don't believe I'm changing anything. I've factored out the "pre" _adjustFocusAfterTabSwitch stuff into _adjustFocusBeforeTabSwitch, and they're still called in the same order inside updateCurrentBrowser in the non-e10s case.

In the e10s case, the order has changed slightly. When a select occurs on the tab strip, and a tab is requested, we have two possible scenarios:

1) We can perform a tab switch immediately, because we can display blank
2) We must perform the tab switch asynchronously, because we're waiting on the content process to send layers

In case (1), we'll call _adjustFocusBeforeTabSwitch, then _adjustFocusAfterTabSwitch, and then updateCurrentBrowser will run, one after another, synchronously during the select.

In case (2), since the switch is happening asynchronously, updateCurrentBrowser() will be called first (this isn't super great, since this means updating things like the URL bar before we update the visible tabs, but that's what we've been doing for a while anyways). Then, once either the layers become ready or we time out, we do _adjustFocusBeforeTabSwitch, and then _adjustFocusAfterTabSwitch.

I needed to do this focus re-ordering in order to satisfy some tests, but this actually has the added benefit of fixing bug 1364454 as well, it seems.

Do you see any problems (beyond what we already ship) in the above approach?
Flags: needinfo?(dao+bmo)
I think the universe is laughing at me. This patch breaks the browser_tabopen_reflows.js test I added in bug 1354194. I seem to have added another reflow with all of this focus business.

So clearing ni? until I can get this sorted.
Flags: needinfo?(dao+bmo)
(In reply to Mike Conley (:mconley) from comment #45)
> I needed to do this focus re-ordering in order to satisfy some tests, but
> this actually has the added benefit of fixing bug 1364454 as well, it seems.

Which I think implies that it's not just satisfying tests but actually a sensible behavior change. Sounds good to me. I actually meant to look into and potentially change the async tab switcher + focus stuff for bug 1364454.
(In reply to Dão Gottwald [::dao] from comment #47)
> (In reply to Mike Conley (:mconley) from comment #45)
> > I needed to do this focus re-ordering in order to satisfy some tests, but
> > this actually has the added benefit of fixing bug 1364454 as well, it seems.
> 
> Which I think implies that it's not just satisfying tests but actually a
> sensible behavior change. Sounds good to me. I actually meant to look into
> and potentially change the async tab switcher + focus stuff for bug 1364454.

To be clear, this is just based on your comment. I haven't actually reviewed the code changes.
Hey dao, in order to avoid some potential new spots for reflow (which would violate the tests added in bug 1354194), I did some work to re-arrange some focus steps to avoid us needlessly setting focus in one place, and re-setting it somewhere else later when style / layout may have been dirtied.

Let me know if you have any questions or concerns about this.
Attachment #8869783 - Flags: review?(dao+bmo) → review?(felipc)
Attachment #8869783 - Flags: review?(felipc)
Attachment #8869783 - Flags: review?(felipc)
Comment on attachment 8869783 [details]
Bug 1362866 - Rearrange tab focusing behaviour to avoid extra potential reflows.

https://reviewboard.mozilla.org/r/141334/#review145616
Attachment #8869783 - Flags: review?(felipc) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b77d1448314
Remove MozTabChildNotReady event usage from tabbrowser.xml, and replace with nsITabParent.hasPresented. r=billm
https://hg.mozilla.org/integration/autoland/rev/9a89e9597191
Rearrange tab focusing behaviour to avoid extra potential reflows. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/51a2c493c949
Fix browser_tabSpinnerProbe.js now that tabs must be presented to spin. Also remove browser_tabSpinnerTypeProbe.js. r=billm
https://hg.mozilla.org/integration/autoland/rev/864bd43537dc
Remove FX_TAB_SWITCH_SPINNER_TYPE probe. r=billm
https://hg.mozilla.org/integration/autoland/rev/aa48bb3f4944
Get rid of MozTabChildNotReady event and all of its required infrastructure. r=billm
Lots of white flashes when closing or switching tabs in Nightly 24-05-17. Didn't experience anything like that in 23-05-17 so I thought this bug could regress something. If needed, I might try to reproduce the bug in a new profile and record performance.
Depends on: 1367596
(In reply to ajfhajf from comment #71)
> Lots of white flashes when closing or switching tabs in Nightly 24-05-17.
> Didn't experience anything like that in 23-05-17 so I thought this bug could
> regress something. If needed, I might try to reproduce the bug in a new
> profile and record performance.

filed a new Bug 1367596
Blocks: 1364454
Depends on: 1367621
Depends on: 1367964
Depends on: 1368074
Depends on: 1368181
Bug 1368181 seems to suggest that there are a few more edge cases out there were some people are getting perma-blank tabs.

On Monday, I'll either need to write a pref to turn this feature off (and eat the regression until we can figure out the edge cases), or back this set of patches (and the other fixes I've landed for it) out.
telemetry-alerts has cottoned onto your perf work.

Looks like you drastically reduced mean spinner time (quite a lot of low-speed switches, no extra measurements): https://mzl.la/2rpUnQW
But the original regressed metric (bug 1054740) hasn't budged: https://mzl.la/2rnqPjP
Flags: needinfo?(mconley)
(In reply to Chris H-C :chutten from comment #74)
> Looks like you drastically reduced mean spinner time (quite a lot of
> low-speed switches, no extra measurements): https://mzl.la/2rpUnQW
> But the original regressed metric (bug 1054740) hasn't budged:
> https://mzl.la/2rnqPjP

I guess that's not surprising. I think we're doing a bit more work now destroying the initial browser and doing a tab switch, which would probably account for the FX_SESSION_RESTORE_RESTORE_MS bounceback.

I guess that also goes against what I told Dao in comment 3 - we _do_ have a regression here in a session restore metric, so it wasn't just cosmetic. :(
Flags: needinfo?(mconley)
That regression is being tracked in bug 1365541.
Depends on: 1370035
See Also: → 1371884
Depends on: 1387357
Depends on: 1404651
Depends on: 1404713
Depends on: 1425695
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: