859 bytes, patch
|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
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.
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
ni'ing myself to attach a video demonstration.
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
Here's a video demonstrating the bug: http://www.screencast.com/t/DThT1iTu7YyM
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
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?
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 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+
Pushed by email@example.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
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 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 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.
You need to log in before you can comment on or make changes to this bug.