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

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
3 months ago
13 days ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Assignee)

Description

3 months ago
Created attachment 8865228 [details]
Regression in graph (last data point)

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.
(Assignee)

Comment 1

3 months ago
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.

Comment 2

3 months ago
(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?
(Assignee)

Comment 3

3 months ago
(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.
(Assignee)

Comment 4

3 months ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

3 months ago
(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. :)

Comment 8

3 months ago
(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?
(Assignee)

Comment 9

3 months ago
(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).
(Assignee)

Updated

3 months ago
Attachment #8865279 - Flags: review?(wmccloskey)
(Assignee)

Comment 10

3 months ago
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 12

3 months ago
mozreview-review
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
(Assignee)

Comment 13

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=439720816095151f15be9946f7fb48bf56b4e059
(Assignee)

Comment 14

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=626f8c35f293dbd8b54fda2f24642f2f34765119
Is this ready for review?
Flags: needinfo?(mconley)
(Assignee)

Comment 16

3 months ago
(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)
(Assignee)

Comment 17

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=900d33f1fe747a085ee3c492d14a2d52af7644f7
(Assignee)

Comment 18

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cfa5989ab4448e54252c4052225652f18c63ebf
(Assignee)

Comment 19

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c49662025e6bd75c94d0d2de602fa0b8e912fa90
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

3 months ago
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.
(Assignee)

Comment 26

3 months ago
mozreview-review
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".
(Assignee)

Comment 27

3 months ago
mozreview-review
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 28

3 months ago
mozreview-review
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)
(Assignee)

Comment 29

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fd3de609a335a28a92141eab2d2ba19ba90f33c
(Assignee)

Comment 30

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab291e678014a931bb2db29b4cd6fe4163d51e07
(Assignee)

Comment 31

3 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 36

3 months ago
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 37

3 months ago
mozreview-review
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 38

3 months ago
mozreview-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 39

3 months ago
mozreview-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 40

3 months ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 45

3 months ago
(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)
(Assignee)

Comment 46

3 months ago
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.
(Assignee)

Comment 49

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8c5a20bbe8373317057c946553e37cae367d54a
(Assignee)

Comment 50

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=66391149e5a0e9df0c4ba3a49ff9795d8bdc9b17
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 60

3 months ago
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.
(Assignee)

Comment 61

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f0edd613e82079215ad1d992c5a983e7dff8926
(Assignee)

Updated

3 months ago
Attachment #8869783 - Flags: review?(dao+bmo) → review?(felipc)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8869783 - Flags: review?(felipc)
(Assignee)

Updated

3 months ago
Attachment #8869783 - Flags: review?(felipc)
(Assignee)

Comment 67

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fae8537ea3ff06755ddf5edab2a5c17406a442bf

Comment 68

3 months ago
mozreview-review
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+

Comment 69

3 months ago
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

Comment 70

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8b77d1448314
https://hg.mozilla.org/mozilla-central/rev/9a89e9597191
https://hg.mozilla.org/mozilla-central/rev/51a2c493c949
https://hg.mozilla.org/mozilla-central/rev/864bd43537dc
https://hg.mozilla.org/mozilla-central/rev/aa48bb3f4944
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 71

3 months ago
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.
Assignee: nobody → mconley

Updated

3 months ago
Depends on: 1367596

Comment 72

3 months ago
(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

Updated

3 months ago
Blocks: 1364454
Depends on: 1367621

Updated

3 months ago
Depends on: 1367964
Depends on: 1368074
Depends on: 1368075
(Assignee)

Updated

3 months ago
Depends on: 1368181
(Assignee)

Comment 73

3 months ago
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.
No longer depends on: 1368075

Comment 74

3 months ago
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)
(Assignee)

Comment 75

3 months ago
(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.

Updated

3 months ago
Depends on: 1370035
Depends on: 1370518
(Assignee)

Updated

2 months ago
See Also: → bug 1371884
Depends on: 1387357
You need to log in before you can comment on or make changes to this bug.