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

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM: Content Processes
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: mconley, Unassigned)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

8 months ago
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.
(Reporter)

Comment 1

8 months ago
Created attachment 8818709 [details] [diff] [review]
Disable paint unsuppression at page load
(Reporter)

Comment 2

8 months ago
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
(Reporter)

Comment 3

8 months ago
ni'ing myself to attach a video demonstration.
Flags: needinfo?(mconley)
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
See Also: → bug 1157941
(Reporter)

Comment 5

8 months ago
Here's a video demonstrating the bug:

http://www.screencast.com/t/DThT1iTu7YyM
Flags: needinfo?(mconley)
(Reporter)

Updated

8 months ago
Summary: Higher likelihood of showing tab switch spinner during paint suppression window → No frame painted when switching to a remote tab that's undergoing paint suppression
(Reporter)

Comment 6

8 months ago
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?
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

8 months ago
mozreview-review
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
Attachment #8819401 - Flags: review?(tnikkel) → review+

Comment 11

8 months ago
Pushed by mconley@mozilla.com:
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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/50971fda9e51
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Comment 13

8 months ago
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.
Flags: needinfo?(mconley)
(Reporter)

Comment 14

8 months ago
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.
Flags: needinfo?(mconley)
Attachment #8819401 - Flags: approval-mozilla-beta?
Attachment #8819401 - Flags: approval-mozilla-aurora?

Updated

8 months ago
status-firefox51: --- → affected
status-firefox52: --- → affected
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.
Attachment #8819401 - Flags: approval-mozilla-beta?
Attachment #8819401 - Flags: approval-mozilla-beta+
Attachment #8819401 - Flags: approval-mozilla-aurora?
Attachment #8819401 - Flags: approval-mozilla-aurora+

Comment 16

8 months ago
Check-in: https://hg.mozilla.org/releases/mozilla-aurora/rev/8255a7049f44
status-firefox52: affected → fixed

Comment 17

8 months ago
https://hg.mozilla.org/releases/mozilla-beta/rev/00b049c66f14
status-firefox51: affected → fixed
You need to log in before you can comment on or make changes to this bug.