993259 - RestyleTracker::DoProcessRestyles() keeps getting called while idling at SMS thread UI

Status: NEW

(Core :: CSS Parsing and Computation, defect)

Description

[Ting-Yu Chou [:ting] (away)]

STR:
  1. $ cd gaia; make reference-workload-heavy
  2. Launch Messages application, enter BIG-THREAD-MIXED
  3. Do nothing but RestyleTracker::DoProcessRestyles() keeps getting called

Expected result:
  Restyle should be unnecessary if user is just idling

Notes:
  1. gaia  = d9ac78148591a50356dd5a82d2d295eeb7c234a6
     gecko = 04cea893d42ad3291e2e75b1d66a6441ec8f2716
  2. The parameters pass to RestyleTracker::ProcessOneRestyle():
       aRestyleHint = aRestyleHint 
       aChangeHint  = 0
     BTW, the tag name I got from aElement is PROGRESS:
       if (aRestyleHint & (eRestyle_Self | eRestyle_Subtree)) {
         nsAutoString tag;
         aElement->GetTagName(tag);
         ...

Comment 1

[Ting-Yu Chou [:ting] (away)]

Attachment: profile_sms.html

Captured a profile while idling at SMS thread UI, the RefreshDriver keeps ticking and goes to restyle.

Comment 2

[Gabriele Svelto [:gsvelto]]

I can see CSS recalculations in there, this looks like a dup of bug 903763.

Comment 3

[Ting-Yu Chou [:ting] (away)]

Attachment: EnsureTimerStarted.bt

While idling, nsRefreshDriver::EnsureTimerStarted() keeps getting called from nsRefreshDriver::MostRecentRefresh() and nsRefreshDriver::AddStyleFlushObserver(), I captured the backtrace.

Besides, mPendingRestyles.Count() at RestyleTracker::DoProcessRestyles() statys still between invocations.

Comment 4

[Gabriele Svelto [:gsvelto]]

(In reply to Ting-Yu Chou [:ting] from comment #3)
> Created attachment 8403854 [details]
> EnsureTimerStarted.bt
> 
> While idling, nsRefreshDriver::EnsureTimerStarted() keeps getting called
> from nsRefreshDriver::MostRecentRefresh() and
> nsRefreshDriver::AddStyleFlushObserver(), I captured the backtrace.
> 
> Besides, mPendingRestyles.Count() at RestyleTracker::DoProcessRestyles()
> statys still between invocations.

Thanks for this; on second thought this doesn't look like bug 903763, it looks to be animation-related (at least on the surface), see this snippet of the trace:

#6  0x410c7b38 in ElementAnimations::PostRestyleForAnimation (this=0x4349f380, 
    aFlags=mozilla::css::CommonAnimationManager::Can_Throttle)
    at /home/ting/w/fx/os/tarako/gecko/layout/style/nsAnimationManager.h:174

Julien, Rick, do we have CSS animations in the SMS app that could be causing this? I seem to remember that the Communications app had to implement a workaround when using CSS animations to prevent Gecko from continuously running refreshes even after the animation has stopped. I'll try to dig out that bug and its gecko brethren.

Comment 5

[Ting-Yu Chou [:ting] (away)]

Correction:

(In reply to Ting-Yu Chou [:ting] from comment #0)
> STR:
>   2. Launch Messages application, enter BIG-THREAD-MIXED

    2. Launch Messages application, enter BIG-THREAD-MIXED, wait until all the messages are loaded (I added a log in ThreadUI.onMessagesRendered() to monitor this)

>   2. The parameters pass to RestyleTracker::ProcessOneRestyle():
>        aRestyleHint = aRestyleHint 

         aRestyleHint = eRestyle_Self

Comment 6

[Julien Wajsberg [:julienw]]

The only animations I'm thinking of are the "pending message" animation and the "deleting messages" animation (for the last one, I'm not even sure we have something).

We do use some transitions too, but I don't think this would provoke this.

Comment 7

[Ting-Yu Chou [:ting] (away)]

Attachment: nsAnimationManager::AddElementData.bt

Captured one backtrace of nsAnimationManager::AddElementData() right after enter BIG-THREAD-MIXED thread.

Comment 8

[Ting-Yu Chou [:ting] (away)]

Attachment: nsAnimationManager::AddElementData.bt.noscrollintoview

I disabled the scrollIntoView at thread_ui.js, but still can reproduce the issue, captured another backtrace to nsAnimationManager::AddElementData() after enter the thread.

Comment 9

[Ting-Yu Chou [:ting] (away)]

I checked the animation name from BuildAnimations(), it is "rotate".

Comment 10

[Gabriele Svelto [:gsvelto]]

(In reply to Ting-Yu Chou [:ting] from comment #9)
> I checked the animation name from BuildAnimations(), it is "rotate".

That must be the progress spinner, can you try disabling it in shared/style/progress_activity.css ?

Comment 11

[Ting-Yu Chou [:ting] (away)]

Gabriele, you're right, I can't reproduce the issue with following change:

diff --git a/shared/style_unstable/progress_activity.css b/shared/style_unstable
index 6f9d0d8..fbcc6bb 100644
--- a/shared/style_unstable/progress_activity.css
+++ b/shared/style_unstable/progress_activity.css
@@ -5,7 +5,7 @@
 /* Spinner */
 progress:not([value]) {
   background: url(progress_activity/images/ui/default.png) no-repeat center cen
-  animation: 0.9s rotate infinite steps(30);
+  /*animation: 0.9s rotate infinite steps(30);*/
   width: 2.9rem;
   height: 2.9rem;
   border: none;

Comment 12

[Julien Wajsberg [:julienw]]

So now we need to know why the spinner triggers processStyles even when it's not displayed :)

Comment 13

[Ting-Yu Chou [:ting] (away)]

dbaron, sorry to bother, but do you know will an animation be done if its ancestor node has display property "none"? If the answer is no, could you please point me out where to find the related code? Thanks.

Comment 14

[Gabriele Svelto [:gsvelto]]

I think that the most bizarre aspect of this bug is that the spinner is never made visible in the provided STR yet it starts consuming CPU time when one enters the message view. This sounds like some kind of regression.

Comment 15

[Gabriele Svelto [:gsvelto]]

I've just tested this on 1.4 and it reproduces there too, this is very bad news.

Comment 16

[Gabriele Svelto [:gsvelto]]

Scratch my previous comment, I was using only gaia's 1.4 branch, I was still on gecko master. I'm now trying to test with gecko 1.4 branch.

Comment 17

[Gabriele Svelto [:gsvelto]]

Confirmed, this is happening on v1.4 too. I'm testing v1.3 now.

Comment 18

[Gabriele Svelto [:gsvelto]]

I'm reproducing this on v1.3 too, this is really nasty.

Comment 19

[Ting-Yu Chou [:ting] (away)]

Attachment: min_repro.tar.gz

Created a smaller content based on Messages app for reproducing the bug, can also reproduce on mozilla-central.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> do you know will an animation be done if its ancestor node has display property "none"?

No.  That's because we never even compute style for descendants of display:none nodes, so we never find out they have an animation property set.

An animation _does_ run on an element which is display:none itself but has a non-display:none parent.  See some discussion in bug 962594.

Which of those two situations are we in here?

Comment 21

[Ting-Yu Chou [:ting] (away)]

(In reply to Boris Zbarsky [:bz] from comment #20)
> No.  That's because we never even compute style for descendants of
> display:none nodes, so we never find out they have an animation property set.

This one.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Though the minimal testcase attached does have the element with an animation as a child of a display:none frame.  I'm not sure why that's animating, if so. :(

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I think we end up running the animation if there's ever something that requests the computed style for that node (e.g., getComputedStyle).

We should probably suppress updates of animations that are display:none in a similar way to the way we suppress updates to animations that are running on the compositor thread.

I wonder if we should mark this a duplicate of bug 962594.

Comment 24

[Julien Wajsberg [:julienw]]

As a work around I guess we can use the same css class that toggles the element to toggle the animation ?