Closed
Bug 1443358
Opened 5 years ago
Closed 5 years ago
Reddit page with 0 plugins or JS causes continuous style recalculation and ~50% CPU on OS X
Categories
(Core :: DOM: Animation, defect)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: dw, Assigned: hiro)
References
Details
Attachments
(3 files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:58.0) Gecko/20100101 Firefox/58.0 Build ID: 20180206200532 Steps to reproduce: 1. Have a late 2011 Macbook Pro 13" 1. Start Firefox 1. Disable all plug-ins. 1. Visit https://www.reddit.com/r/LegalAdviceUK/comments/827idq/is_the_audio_waveform_image_of_a_song_considered/ Actual results: 1. Observe plugin-container chewing mountains of CPU 1. Observe "record performance" in developer mode showing continuous "recalculate style" events 1. Observe JS flamegraph mostly empty of site JS code 1. Observe selecting "No Style" from View menu cuts CPU usage in half 1. Observe laptop has been converted into a hairdryer by software Expected results: 1. Laptop should not become hairdryer 1. Idle CPU usage of page should be 0%, not 50% 1. Style for page should not continuously recalculate(? no clue)
Comment 1•5 years ago
|
||
Hi dw, I tested this issue with Mac OS X 10.12 on MacBook Pro 13" 2013, on FF Nightly 60.0a1(2018-03-07) and I can't reproduce this issue. In my case, the CPU is at 30% for a second and then drops down to ~5-7%. What I think we can do here is to ask you to capture a performance profile. First, you need to install FF Nightly(https://nightly.mozilla.org/) and then you need to install the Cleopatra add-on. After you are done with installing, you need to start recording your profile when you reproduce this issue. You can get more info on how to install and use the Cleopatra add-on (that helps you get the performance profile) by going to: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler https://perf-html.io/
Flags: needinfo?(dw)
Comment 2•5 years ago
|
||
Hey ovidiu, I upgraded to Firefox 60 today (60.0beta4) and immediately noticed this very same issue actually. I.e. "Macbook = Hairdryer"
Comment 3•5 years ago
|
||
Well... Bugzilla cut off my comment, exactly where I put an emoji in. Here it is again without any fancy Unicode: Hey ovidiu, I upgraded to Firefox 60 today (60.0beta4) and immediately noticed this very same issue actually. I.e. "Macbook = Hairdryer" on Reddit, due to constant 20-30% CPU usage on my Macbook Pro 15" 2014. Also I'm sorry if I did anything wrong as I never profiled Firefox before nor submitted anything here yet. Here is the profiling information of /r/all: https://perfht.ml/2IAjC9x I assume that the purple dots symbolize style recalculation, right? In that case this would be a bug then, since this was not the case before (Firefox <= 59) and furthermore does not happen in Chrome.
Comment 4•5 years ago
|
||
Thanks leonard for the performance profile. Mike, can you please take a look at this profile? Thanks
Flags: needinfo?(mconley)
Comment 5•5 years ago
|
||
The profile suggests that something is frequently painting on that page. I, however, cannot reproduce this issue. Perhaps this has something to do with your Reddit user account or some reddit-specific add-ons you have enabled? Hey leonard, can you capture a quick screencast of yourself reproducing this bug and post it here? Bonus points if you do it with paint flashing enabled: https://developer.mozilla.org/en-US/docs/Tools/Paint_Flashing_Tool
Flags: needinfo?(mconley)
Comment 7•5 years ago
|
||
Mike, I've recorded a video as requested :) https://drive.google.com/file/d/1ABpWYu_QgBLvXu69QTx6wftxivJwMvLe/view?usp=sharing (I recommend downloading the video.) What you'll see in the video is that nothing gets repainted visually, except for Reddit updating the timestamps of the link list every 20 seconds. What should be noted is that I had the uBlock Origin & Bitwarden plugins enabled - there's no change if I disable them. But what's intersting is that the style recalculations stop as soon as I logout. So it's probably something specific that Reddit is doing only when you're logged in. Here are my Reddit preferences: https://drive.google.com/file/d/13JJ7gdAyVXpn2uD4JybV_qBPTzao9kG5/view?usp=sharing As you can see in my preferences I'm also part of the beta program and thus have the new chat-feature. My profile has also been migrated to the new design if that's relevant for this issue. Again Chrome does not suffer from this regression/bug and runs just fine at below 4% CPU usage in average.
Flags: needinfo?(leonard)
Comment 8•5 years ago
|
||
BEHOLD! I found a way to reproduce this reliably. My last comment gave me the idea that it must be something off-screen, like you know... The already mentioned chat box! And here you can see how opening the chat box stops the frequent style recalculations: https://drive.google.com/file/d/1KGjYKT6gEyPO6P3ETB6GK6zM_lB3L4ni/view?usp=sharing As you can see in the video, even if you close the chat box the style recalculations will have stopped permamently anyways. I hope this helps you somehow to find the cause of this. And if you need any more info/debugging/help I'll try my best. :)
Comment 9•5 years ago
|
||
Ah, thank you leonard! Very helpful, yes - I can reproduce now. The chat app appears to be a 0x0 iframe (#chat-app) on the page, and within that iframe, there's a fully accelerated CSS animation occurring for the loading image. Hey hiro, this feels very similar to bug 1237454 - is the 0x0 iframe case something that is already covered by another bug?
Flags: needinfo?(hikezoe)
Comment 10•5 years ago
|
||
Yep I was about to say the same and I made a video of it (if you still need that): https://drive.google.com/file/d/152dHvKb-3LKF3dzy6_CbznrrmC6CLG1I/view?usp=sharing And as you can see there it's not Reddit's fault that the animation is causing some kind of "obvious" CPU load, since it's not happening even if the iframe is 1x1 px. And by the way, thanks for investigating this so far. :)
Assignee | ||
Comment 11•5 years ago
|
||
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #9) > Ah, thank you leonard! Very helpful, yes - I can reproduce now. The chat app > appears to be a 0x0 iframe (#chat-app) on the page, and within that iframe, > there's a fully accelerated CSS animation occurring for the loading image. > > Hey hiro, this feels very similar to bug 1237454 - is the 0x0 iframe case > something that is already covered by another bug? I thought we do care about the case, but it seems not. Can someone post the whole html here? I don't have an account for Reddit so that I can't reproduce this issue locally.
Comment 12•5 years ago
|
||
Hey Hiroyuki, I created a heavily reduced/simplified testcase for this bug, based on Reddit's chat window. You can find a zip containing 2 files attached - bug_1443358_testcase.html is the "actual" file and bug_1443358_testcase_iframe.html contains the iframe's contents of course. When you open the former file in Firefox and start a performance analysis you should see frequent style recalculations. You can now execute the "toggle()" function I included, which will toggle between a 500x500 and 0x0 pixel iframe and those style recalculations should start/stop accordingly.
Assignee | ||
Comment 13•5 years ago
|
||
Thanks for the test case! It seems that we can't get the iframe in question in IsFrameScrolledOutOfView if the traget element is position:absolute even if we drop nsLayoutUtils::SCROLLABLE_SAME_DOC for GetNearestScrollableFrame().
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Animation
Ever confirmed: true
Flags: needinfo?(hikezoe)
Product: Firefox → Core
Assignee | ||
Comment 14•5 years ago
|
||
I hope this try fixes the issue. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0732200a77a84a54fd7b8828c9f4aad1df6c73da
Comment 15•5 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14) > I hope this try fixes the issue. > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=0732200a77a84a54fd7b8828c9f4aad1df6c73da At least with my local testing of this patch, I still see the frequent style flushes (which seems to go away after I open and then close the chat subframe).
Comment 16•5 years ago
|
||
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #15) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #14) > > I hope this try fixes the issue. > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=0732200a77a84a54fd7b8828c9f4aad1df6c73da > > At least with my local testing of this patch, I still see the frequent style > flushes (which seems to go away after I open and then close the chat > subframe). BTW: Did you use my test case attachment above for testing? The way you worded that it seems you tested it on Reddit using its chat box. Because unfortunately this is to be expected: When you open the chat box for the first time, the loading animation will soon stop and thus cannot possibly cause any style flushes anyways (since there's no active animation running).
Comment 17•5 years ago
|
||
(In reply to Leonard Hecker from comment #16) > BTW: Did you use my test case attachment above for testing? > The way you worded that it seems you tested it on Reddit using its chat box. > Because unfortunately this is to be expected: When you open the chat box for > the first time, the loading animation will soon stop and thus cannot > possibly cause any style flushes anyways (since there's no active animation > running). Ah, right! Sorry - I forgot that what we cared about here was _painting_ and not style flushing. Yes, with hiro's patches, I no longer see frequent painting (neither on Reddit, nor the test case). Thanks for the correction there, Leonard.
Comment 18•5 years ago
|
||
Hey hiro, so from my testing, it seems as if this _does_ reduce painting. Is your fix something we should consider landing?
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 19•5 years ago
|
||
Yeah, the fix can be landed with some test cases. I will try to take the time to write the test cases in this weekend.
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 20•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=863811f6c5545ee5709b85db7d03f821a73c6a37
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 23•5 years ago
|
||
mozreview-review |
Comment on attachment 8964196 [details] Bug 1443358 - Introduce a new function that is able to count restyle in a given window. https://reviewboard.mozilla.org/r/232946/#review238402 ::: dom/animation/test/mozilla/file_restyles.html:68 (Diff revision 1) > -// Returns the animation restyle markers observed during |frameCount| refresh > -// driver ticks. This function is typically used to count the number of > -// restyles that take place as part of the style update that happens on each > -// refresh driver tick, as opposed to synchronous restyles triggered by script. > +// Returns the animation restyle markers observed during |aFrameCount| refresh > +// driver ticks in |aWindow|. > +function observeStylingInTargetWindow(aWindow, aFrameCount, aOnFrame) { > + let docShell = getDocShellForObservingRestylesForWindow(aWindow); > -// > -// For the latter observeAnimSyncStyling (below) should be used. > -function observeStyling(frameCount, onFrame) { > - let docShell = getDocShellForObservingRestyles(); > > return new Promise(resolve => { > - return waitForAnimationFrames(frameCount, onFrame).then(() => { > + return waitForAnimationFrames(aFrameCount, aOnFrame).then(() => { > var markers = docShell.popProfileTimelineMarkers(); > docShell.recordProfileTimelineMarkers = false; > var stylingMarkers = markers.filter((marker, index) => { > return marker.name == 'Styles' && marker.isAnimationOnly; > }); > resolve(stylingMarkers); > }); > }); > } > > +// Returns the animation restyle markers observed during |frameCount| refresh > +// driver ticks in this `window`. This function is typically used to count the > +// number of restyles that take place as part of the style update that happens > +// on each refresh driver tick, as opposed to synchronous restyles triggered by > +// script. > +// > +// For the latter observeAnimSyncStyling (below) should be used. > +function observeStyling(frameCount, onFrame) { > + return observeStylingInTargetWindow(window, frameCount, onFrame); > +} Since we expect most call sites to use observeStyling, perhaps we should list that first. Then in the documentation for observeStylingInTargetWindow we can say, "As with observeStyling but applied to target window |aWindow|."
Attachment #8964196 -
Flags: review?(bbirtles) → review+
Comment 24•5 years ago
|
||
mozreview-review |
Comment on attachment 8964197 [details] Bug 1443358 - Consider that the target frame is scrolled out if scrollable parent frame size is empty. https://reviewboard.mozilla.org/r/232948/#review238404 ::: commit-message-7a0a7:5 (Diff revision 1) > +Bug 1443358 - Consider that the target frame is scrolled out if scrollable parent frame size is empty. r?birtles > + > +This patch adds three test cases; > + > +1) Animation on position:absolute element in a collaped iframe s/collaped/collapsed/ ::: commit-message-7a0a7:7 (Diff revision 1) > + > +This patch adds three test cases; > + > +1) Animation on position:absolute element in a collaped iframe > + This animation should be throttled. > +2) Animation on position:absolute element that has effective width and height "...that has a non-zero width and height" ::: commit-message-7a0a7:8 (Diff revision 1) > +This patch adds three test cases; > + > +1) Animation on position:absolute element in a collaped iframe > + This animation should be throttled. > +2) Animation on position:absolute element that has effective width and height > + in a collaped element s/collaped/collapsed/ (Although, see below, it might be more clear to talk about a "zero-height element" than a "collapsed eleent".) ::: commit-message-7a0a7:10 (Diff revision 1) > +1) Animation on position:absolute element in a collaped iframe > + This animation should be throttled. > +2) Animation on position:absolute element that has effective width and height > + in a collaped element > + This animation should NOT be throttled since the animation is visible > +3) Animation on position:absolute collapsed element in a collaped element s/collaped/collapsed/ ::: commit-message-7a0a7:10 (Diff revision 1) > +1) Animation on position:absolute element in a collaped iframe > + This animation should be throttled. > +2) Animation on position:absolute element that has effective width and height > + in a collaped element > + This animation should NOT be throttled since the animation is visible > +3) Animation on position:absolute collapsed element in a collaped element Also, "collapsed element in a collapsed element" is a little confusing. Perhaps just "a zero-height element whose parent also has zero height"? ::: layout/generic/nsFrame.cpp:11016 (Diff revision 1) > nsRect scrollableRect = > scrollableParent->GetVisualOverflowRectRelativeToSelf(); > + // We consider that the target is scrolled out if the scrollable frame is > + // empty. > + if (scrollableRect.IsEmpty()) { > + return true; > + } Should this whole block go before the call to nsLayoutUtils::TransformFrameRectToAncestor now? (That just seems to be a more logical order to me--the fact that it means we avoid calling nsLayoutUtils::TransformFrameRectToAncestor when scrollableRect is empty is not, I suspect, an important optimization.)
Attachment #8964197 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 25•5 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #24) > ::: layout/generic/nsFrame.cpp:11016 > (Diff revision 1) > > nsRect scrollableRect = > > scrollableParent->GetVisualOverflowRectRelativeToSelf(); > > + // We consider that the target is scrolled out if the scrollable frame is > > + // empty. > > + if (scrollableRect.IsEmpty()) { > > + return true; > > + } > > Should this whole block go before the call to > nsLayoutUtils::TransformFrameRectToAncestor now? > > (That just seems to be a more logical order to me--the fact that it means we > avoid calling nsLayoutUtils::TransformFrameRectToAncestor when > scrollableRect is empty is not, I suspect, an important optimization.) Ah right, I will move the check. Thanks!
Assignee | ||
Comment 26•5 years ago
|
||
A final try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cbed6876813f33118eb08f8973a04ab572bab25
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment 29•5 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fa028fca9307 Introduce a new function that is able to count restyle in a given window. r=birtles https://hg.mozilla.org/integration/autoland/rev/d67b34f6abae Consider that the target frame is scrolled out if scrollable parent frame size is empty. r=birtles
Comment 30•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fa028fca9307 https://hg.mozilla.org/mozilla-central/rev/d67b34f6abae
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Reporter | ||
Comment 31•5 years ago
|
||
Hi there, I am now observing this behaviour on the Google home page: https://www.google.co.uk/search?q=os%20x%20hide%20user%20from%20login%20screen Same characteristics (continuous style recalculation) No iframes on the page
Comment 32•5 years ago
|
||
Filed a new bug, bug 1452641.
Comment 34•5 years ago
|
||
This bug has been closed as a duplicate of bug 1442654 4 days ago and that exact bug (1442654) was closed as a duplicate of this one. One of the closures is wrong -> please re-open, since it is not fixed. I can easily reproduce this issue in VMs running Ubuntu (18) running in Virtualbox on MacOS (High Sierra) and running VMware Fusion on MacOS (High Sierra). Reproduced with Firefox 59.0.2 and with Firefox 61.0a1 (2018-04-19) (64-bit). To reproduce: While monitoring CPU load search for "what?" in google.com or google.nl or google.de and watch the CPU load increasing from ~0% to > 50%. Close the result page and see the opposite. The CPU load stays up for at least 10 minutes (I did not check longer times). The developer tools tell me that there seems to be a busy loop "Recalculate Style".
Assignee | ||
Comment 35•5 years ago
|
||
Are you talking about the google case? It's still open, bug 1218169.
You need to log in
before you can comment on or make changes to this bug.
Description
•