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

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

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

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.

Comment 1

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

Here's a video demonstrating what I'm seeing:

https://drive.google.com/file/d/1OQhw_b54BXXpBgmVfmlNDFbrHolRK4TU/view?usp=sharing

Comment 2

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

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

Comment 3

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

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

Comment 4

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

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.

Comment 5

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

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.

Comment 6

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

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.

Comment 7

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

Attachment: Bug 1491906 - Add async mode to tabpanels binding. r?NeilDeakin

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

[Neil Deakin]

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

Neil Deakin has approved the revision.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0851acd8fd1e
Add async mode to tabpanels binding. r=NeilDeakin

Comment 10

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 0851acd8fd1e (Bug 1491906) for clipboard failures in browser/base/content/test/general/browser_clipboard.js CLOSED TREE

Failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&classifiedState=unclassified&fromchange=1e4443c0469155afcd2cb93ca0c9a6bf9b729626&selectedJob=200187397

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=200187397&repo=autoland&lineNumber=5635

Comment 11

[Pulsebot]

Backout by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b418f5bf31cb
Add async mode to tabpanels binding. r=NeilDeakin

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b418f5bf31cb

Comment 14

[Pascal Chevrel:pascalc]

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

Comment 15

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

Yeah, I'm going to request uplift. Thanks for the reminder!

Comment 16

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

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.

Comment 17

[Pascal Chevrel:pascalc]

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

Comment 18

[Andreea Pavel [:apavel]]

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