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)

Product:
Core
Component:
DOM: Animation
Version:
58 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: dw, Assigned: hiro)

References

Details

Attachments

(3 files)

bug_1443358_testcase.zip
1.12 KB, application/zip
Details
Bug 1443358 - Introduce a new function that is able to count restyle in a given window.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1443358 - Consider that the target frame is scrolled out if scrollable parent frame size is empty.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
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)
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)
Hey ovidiu,

I upgraded to Firefox 60 today (60.0beta4) and immediately noticed this very same issue actually.
I.e. "Macbook = Hairdryer" 
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.
Thanks leonard for the performance profile. 

Mike, can you please take a look at this profile? Thanks
Flags: needinfo?(mconley)
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)
Whoops - ni?d wrong person.
Flags: needinfo?(dw) → needinfo?(leonard)
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)
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. :)
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)
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. :)
(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.
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.
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
(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).
(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).
(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.
See Also: → 1442654
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)
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)
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 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+
(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: nobody → hikezoe
Status: NEW → ASSIGNED
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
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
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
Filed a new bug, bug 1452641.
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".
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.