tabpanels binding is switching the selectedIndex before the layers are necessarily ready

RESOLVED FIXED in Firefox 63

Status

()

defect
P1
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

({regression})

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

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 unaffected, firefox63 fixed, firefox64 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
1. On the 2018 Quantum Reference hardware, set dom.ipc.processPriorityManager.enabled to true
2. Restart the browser, and load a series of tabs. I don't think it really matters what tabs they are.
3. Switch tabs with the mouse.

ER:

Tab switches that either result in showing the content, or a tab switch spinner and _then_ the content.

AR:

I see tab switches where we sometimes show the expected content, and then a tab switch spinner, and then the content again.
(Assignee)

Comment 2

7 months ago
Here's a profile that I dumped after seeing the bug. In this profile, the tab I saw the spinner on was a bbc.com tab:

https://perfht.ml/2D3Abfp
(Assignee)

Comment 3

7 months ago
Here's a tab switch log:

START
Initial tab is loaded?: true
warmupTab 2(https://www.wikipedia.org/) 0:(-) 1:VR(AR)(+) 2:(-) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:VR(AR)(+) 2:(WR)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:VR(AR)(+) 2:(WR)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:VR(AR)(+) 2:(WR)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:VR(AR)(+) 2:(WR)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:VR(AR)(+) 2:(WR)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:VR(AR)(+) 2:(WR)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:VR(AR)(+) 2:(WR)(+?) 3:(-) 4:(-) cached: 0
requestTab 2(https://www.wikipedia.org/) 0:(-) 1:VR(AR)(+) 2:(WR)(+?) 3:(-) 4:(-) cached: 0
Loading tab 2(https://www.wikipedia.org/)
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:V(AR)(+) 2:LR(WR)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:V(AR)(+) 2:LR(WR)(+?) 3:(-) 4:(-) cached: 0
onUnloadTimeout 0:(-) 1:V(AR)(+) 2:LR(WR)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:V(AR)(+) 2:LR(R)(+?) 3:(-) 4:(-) cached: 0
onLoadTimeout 0:(-) 1:V(AR)(+) 2:LR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
Switch to tab 2 - 2(https://www.wikipedia.org/)
done 0:(-) 1:(AR)(+) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(AR)(+) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(AR)(+) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(AR)(+) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(AR)(+) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
onUnloadTimeout 0:(-) 1:(AR)(+) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(-?) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
onLayersCleared(1) 0:(-) 1:(-?) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(-) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(-) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(-) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(-) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(-) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
onUnloadTimeout 0:(-) 1:(-) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(-) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(-) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(-) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(-) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
done 0:(-) 1:(-) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
onLayersReady(2, true) 0:(-) 1:(-) 2:VR(R)(+?) 3:(-) 4:(-) cached: 0
Set requested tab docshell to active and preserveLayers to false 0:(-) 1:(-) 2:VR(AR)(+) 3:(-) 4:(-) cached: 0
Tab should be blank: false
Requested tab is remote?: true
DEBUG: spinner time = 923
DEBUG: tab switch time = 1328
FINISH
done 0:(-) 1:(-) 2:VR(AR)(+) 3:(-) 4:(-) cached: 0
(Assignee)

Comment 4

7 months ago
Oh man, this is a sad tale. In bug 1442582, we removed the tabbrowser-tabpanels binding.

I'm pretty certain part of the responsibility of that binding was to skip switching the tab panels, and have AsyncTabSwitcher do it instead.

When we removed that binding, we switched back to the old default tabpanels binding. That binding updates its selectedPanel _immediately_ when a user initiates a tab switch. I think that may have been a mistake - or, at least, something we need to understand a bit better.

In some cases, I guess the compositor still has some layers around, since we're able to switch the tabpanels and show something reasonable (the web content). But the AsyncTabSwitcher continues to wait for the MozLayerTreeReady event. Painting takes a long time on the 2018 reference hardware, so it times out, and _then_ we show the spinner.

Tab warming might be masking this issue for most of our users, but it wouldn't surprise me if this is manifesting as some flashing for our users when switching tabs. Coincidentally, I filed bug 1491873 today, which might be rooted in the same issue.
Blocks: 1442582
(Assignee)

Comment 5

7 months ago
So the obvious thing to do here is to try to make the tabpanels binding defer changing the selectedIndex to AsyncTabSwitcher like the old removed binding did.

However, I want to understand the case where the tab is being shown much more quickly than we seem to be able to paint it via renderLayers. If we can detect when a tab is in that special state, we might be able to switch tabs synchronously only in the good cases.
Component: DOM: Content Processes → Tabbed Browser
Product: Core → Firefox
Summary: Enabling background process manager on weaker hardware results in long and maybe unnecessary tab spinners → tabpanels binding is switching the selectedIndex before the layers are necessarily ready
(Assignee)

Updated

7 months ago
See Also: → 1491873
(Assignee)

Comment 6

7 months ago
Here's a profile that I just gathered on my macOS machine that, I think, helps capture the issue a little bit. In this case, I was switching from a searchfox page to a Bugzilla page:

https://perfht.ml/2NOLKeq

Notice that just after 3.24s, the screenshot shows us rendering the background colour of the Bugzilla page, _despite the fact that the Bugzilla page hasn't finished painting yet_ (it finishes just after 3.25s), and before the MozLayerTreeReady event (which fires just after 3.26s).

We're showing that blank-ish frame because, presumably, some part of our graphics infrastructure in the parent remembers the background colour of the previous tab. I want to know how that part works.
(Assignee)

Comment 7

7 months ago
We need this because the AsyncTabSwitcher is responsible for switching between the
remote browser tab panels asynchronously. Asynchronous mode for the tabpanels
binding delegates the responsibility of actually changing the index of the
underlying deck to someone else (AsyncTabSwitcher, in this case).

Comment 8

7 months ago
Comment on attachment 9010017 [details]
Bug 1491906 - Add async mode to tabpanels binding. r?NeilDeakin

Neil Deakin has approved the revision.
Attachment #9010017 - Flags: review+
Assignee: nobody → mconley
Keywords: regression
Priority: -- → P1

Comment 9

7 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0851acd8fd1e
Add async mode to tabpanels binding. r=NeilDeakin

Comment 11

7 months ago
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ed1ff14c6f2
Backed out changeset 0851acd8fd1e for clipboard failures in browser/base/content/test/general/browser_clipboard.js CLOSED TREE

Comment 12

7 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b418f5bf31cb
Add async mode to tabpanels binding. r=NeilDeakin
(Assignee)

Updated

7 months ago
Flags: needinfo?(mconley)

Comment 13

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b418f5bf31cb
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Mike, this bug is a P1 and a regression, do you intent to uplift it to 63 or can it ride the trains to 64? Thanks
Flags: needinfo?(mconley)
(Assignee)

Comment 15

7 months ago
Yeah, I'm going to request uplift. Thanks for the reminder!
Flags: needinfo?(mconley)
(Assignee)

Comment 16

7 months ago
Comment on attachment 9010017 [details]
Bug 1491906 - Add async mode to tabpanels binding. r?NeilDeakin

Approval Request Comment
[Feature/Bug causing the regression]:

Bug 1442582

[User impact if declined]:

The user might see some flashing and flickering when switching tabs, especially when changing tabs with the keyboard. I think we also risk sometimes showing random video memory in those moments.

[Is this code covered by automated tests?]:

Yes.

[Has the fix been verified in Nightly?]:

Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 

No.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

We're adding in a special-case for async tab switching for when e10s is enabled that we always had in the original binding - just ported it to the current binding.

[String changes made/needed]:

None.
Attachment #9010017 - Flags: approval-mozilla-beta?
Comment on attachment 9010017 [details]
Bug 1491906 - Add async mode to tabpanels binding. r?NeilDeakin

Fix for a P1 regression on 63, uplift approved for beta 9. Thanks
Attachment #9010017 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Updated

7 months ago
Duplicate of this bug: 1491873
You need to log in before you can comment on or make changes to this bug.