Last Comment Bug 1323586 - No frame painted when switching to a remote tab that's undergoing paint suppression
: No frame painted when switching to a remote tab that's undergoing paint suppr...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Content Processes (show other bugs)
: 50 Branch
: Unspecified Unspecified
-- normal (vote)
: mozilla53
Assigned To: Nobody; OK to take it and work on it
:
: Bill McCloskey (:billm)
Mentors:
Depends on:
Blocks: 1300411
  Show dependency treegraph
 
Reported: 2016-12-14 15:06 PST by Mike Conley (:mconley)
Modified: 2016-12-21 22:38 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Disable paint unsuppression at page load (859 bytes, patch)
2016-12-14 15:08 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Bug 1323586 - Drop reference to previous widget listener in PuppetWidget when showing after having been hidden. (58 bytes, text/x-review-board-request)
2016-12-16 13:04 PST, Mike Conley (:mconley)
tnikkel: review+
gchang: approval‑mozilla‑aurora+
gchang: approval‑mozilla‑beta+
Details | Review

Description User image Mike Conley (:mconley) 2016-12-14 15:06:05 PST
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 User image Mike Conley (:mconley) 2016-12-14 15:08:17 PST
Created attachment 8818709 [details] [diff] [review]
Disable paint unsuppression at page load
Comment 2 User image Mike Conley (:mconley) 2016-12-14 15:09:18 PST
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 User image Mike Conley (:mconley) 2016-12-14 15:10:34 PST
ni'ing myself to attach a video demonstration.
Comment 5 User image Mike Conley (:mconley) 2016-12-15 07:35:59 PST
Here's a video demonstrating the bug:

http://www.screencast.com/t/DThT1iTu7YyM
Comment 6 User image Mike Conley (:mconley) 2016-12-16 10:13:33 PST
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 User image Bill McCloskey (:billm) 2016-12-16 10:18:46 PST
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 User image Mike Conley (:mconley) 2016-12-16 13:04:00 PST Comment hidden (mozreview-request)
Comment 9 User image Mike Conley (:mconley) 2016-12-16 13:05:30 PST Comment hidden (mozreview-request)
Comment 10 User image Timothy Nikkel (:tnikkel) 2016-12-16 14:09:13 PST
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 User image Pulsebot 2016-12-16 14:23:15 PST
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 User image Phil Ringnalda (:philor) 2016-12-17 14:04:38 PST
https://hg.mozilla.org/mozilla-central/rev/50971fda9e51
Comment 13 User image Mike Conley (:mconley) 2016-12-19 13:31:24 PST
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 User image Mike Conley (:mconley) 2016-12-21 07:58:03 PST
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 User image Gerry Chang [:gchang] 2016-12-21 19:05:55 PST
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 User image Iris Hsiao [:ihsiao] 2016-12-21 19:15:33 PST
Check-in: https://hg.mozilla.org/releases/mozilla-aurora/rev/8255a7049f44

Note You need to log in before you can comment on or make changes to this bug.