1323586 - No frame painted when switching to a remote tab that's undergoing paint suppression

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

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

I've been poking at the e10s tab switch spinner problem for a while, and I think I've stumbled onto something.

We have a window of time when loading a document when it is "paint suppressed" - meaning we don't actually show its full contents until one of two things happens:

1) The page loads enough that we feel confident that we can paint it without a bunch of jitter / Flash of Unstyled Content
2) We hit a timeout, and the document doesn't have any reflows pending (if there are, once the reflows are done being processed, we will then paint).

During this time, in single process mode, if we ever wanted to paint a tab that's suppressed like this, we'd just paint the background colour, or maybe just blank.

I _think_ the async tab switching stuff that we use for e10s hasn't been made fully aware of this mechanism.

Here are some steps to reproduce:

1) In a new profile, add a new int pref called nglayout.initialpaint.delay, and set it to something high, like 5000.
2) Apply the patch I've attached to this bug. This will cause both single and multi-process mode to not unsuppress painting after document load, so we're operating strictly on the nglayout.initialpaint.delay timer.
3) Build, and start the browser. Your browser will take a little while to display its UI correctly, but just ignore that.
4) Open up a non-e10s window
5) In the e10s window, open a new tab, and send it to some web site. Switch back and forth between the initial tab and the one you just kicked off. You should see a long spinner.
6) In the non-e10s window, repeat the steps from 5. Notice that you never see any "delay" in painting - we paint the background colour immediately, even if we haven't painted the contents of the page.

So this is a case where we're showing sluggish painting, and the spinner is showing up, but the content process is not hung, meaning that it probably won't show up in our BHR stacks.

Admittedly, the scenario I set up, with the patch, is kinda artificial - but I suspect this paint suppressed window of time can still exist for users (I would imagine with a higher likelihood if they have slower network connections).

We should do our best to emulate non-e10s behaviour in this case.

Comment 1

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

Attachment: Disable paint unsuppression at page load

Comment 2

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

From #gfx:

<tnikkel> mconley: this line could be responsible for what your seeing https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/view/nsViewManager.cpp#472

Comment 3

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

ni'ing myself to attach a video demonstration.

Comment 4

[Timothy Nikkel (:tnikkel)]

Best guess:
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/view/nsViewManager.cpp#472

And InvalidateView also checks if painting is suppressed, so these might play a role:
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/base/PresShell.cpp#6338
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/painting/nsDisplayList.cpp#2099
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/view/nsViewManager.cpp#371

Comment 5

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

Here's a video demonstrating the bug:

http://www.screencast.com/t/DThT1iTu7YyM

Comment 6

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

Hey billm,

Out of curiosity, why is this conditional necessary? Is there a good reason for using WebWidget()->PaintNowIfNeeded() instead of just asking the current PresShell to paint directly?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

There's all sorts of stuff that ProcessPendingUpdates does besides painting. We need to do that stuff eventually when we switch tabs. It's better to do it right away if we can since it saves us a paint.

Comment 8

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

Attachment: Bug 1323586 - Drop reference to previous widget listener in PuppetWidget when showing after having been hidden.

The previously attached widget listener is used mainly so that we can
continue to show the previous page until the next one is ready enough
to display properly (to avoid a "flashing" effect). When the PuppetWidget
is made visible after having been hidden (perhaps during a tab switch),
the previously attached widget listener is not useful. It's actually
harmful, since PuppetWidget::Paint will attempt to paint that old
widget listener, which is not possible, since it's detatched from
a widget, so we end up not painting a frame.

Review commit: https://reviewboard.mozilla.org/r/99194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99194/

Comment 9

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

Comment on attachment 8819401 [details]
Bug 1323586 - Drop reference to previous widget listener in PuppetWidget when showing after having been hidden.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99194/diff/1-2/

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8819401 [details]
Bug 1323586 - Drop reference to previous widget listener in PuppetWidget when showing after having been hidden.

https://reviewboard.mozilla.org/r/99194/#review99506

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50971fda9e51
Drop reference to previous widget listener in PuppetWidget when showing after having been hidden. r=tnikkel

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/50971fda9e51

Comment 13

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

ni'ing myself to watch the dashboards to see how much this moves the needle. If the answer is "significantly", I'll request uplifts for as far as I can go.

Comment 14

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

Comment on attachment 8819401 [details]
Bug 1323586 - Drop reference to previous widget listener in PuppetWidget when showing after having been hidden.

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

e10s

[User impact if declined]:

Slightly higher likelihood of seeing the tab switch spinner, and so slightly worse perceived performance.

[Is this code covered by automated tests?]:

Tab switching is, yes.

[Has the fix been verified in Nightly?]:

Yes, it's been on Nightly since late last week.

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

No, I don't think so.

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

None.

[Is the change risky?]:

No, I don't think so.

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

I don't think it's risky because what this patch does is clear out some state variables when we try to show a remote browser tab. Those state variables were making it so that in certain circumstances we would fail to paint a frame, which could result in the user seeing the tab switch spinner.

[String changes made/needed]:

None.

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8819401 [details]
Bug 1323586 - Drop reference to previous widget listener in PuppetWidget when showing after having been hidden.

Reduce the possibility of seeing the tab switch spinner. Beta51+ & Aurora52+. Should be in 51 beta 10.

Comment 16

[Iris Hsiao [:ihsiao]]

Check-in: https://hg.mozilla.org/releases/mozilla-aurora/rev/8255a7049f44

Comment 17

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/releases/mozilla-beta/rev/00b049c66f14