Closed Bug 1447874 Opened 2 years ago Closed 2 years ago

Wait for MozReftestInvalidate event and create animations after the event in reftest

Categories

(Core :: DOM: Animation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(2 files)

After bug 1442063, we don't get stuck after 'reftest-wait' has been removed any more.  But still, if there is an throttled animation in the first place, we get stuck in the state of  STATE_WAITING_TO_FIRE_INVALIDATE_EVENT which is the first state of reftest state machine.  The reason of the stuck is the same as I explained in bug 1442063 comment 0.

To avoid this stuck, a way I can think of is to create throttled animations after the STATE_WAITING_TO_FIRE_INVALIDATE_EVENT state has finished which means that we wait for "MozReftestInvalidate" before creating throttled animations.  This is a bit annoying way because we need to write it any time we write reftest including throttled animations.  That's said, considering webrender, we should do this way instead of tweaking our implementations.  That's because this stuck just happens reftest we shouldn't introduce any complexities in the implementation just for the case.
Brian, what do you think?
Flags: needinfo?(bbirtles)
That sounds like a fair bit of work to rewrite tests, not to mention the ongoing burden of having to remember this. It also sounds a bit brittle in that any time an animation becomes newly eligible for running on the compositor (e.g. when we increase the maximum layer size) tests will start to fail.

What's the reason why WebRender needs to always fire MozAfterPaint?

We can fairly easily tell if we have throttled animations inside reftest-content.js but then I suppose we still want to somehow wait for all non-animation related repaints before moving to STATE_WAITING_FOR_REFTEST_WAIT_REMOVAL? Is that right?

If there's some way to address this in reftest-content.js that seems preferable.
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles, public holiday Mar 21) from comment #2)
> That sounds like a fair bit of work to rewrite tests, not to mention the
> ongoing burden of having to remember this. It also sounds a bit brittle in
> that any time an animation becomes newly eligible for running on the
> compositor (e.g. when we increase the maximum layer size) tests will start
> to fail.
> 
> What's the reason why WebRender needs to always fire MozAfterPaint?

It's easier to explain why Gecko doesn't fire MozAfterPaint as well as WebRender.  That's because Gecko fires MozAfterPaint only if there are invalidation changes, whereas WebRender does not check invalidation.

So, this symptoms happens on Gecko too, if the throttled animation in question doesn't have the same 'from' and 'to' value.  Say, rotate(0deg) to rotate(360deg).  With such rotation animation reftest will end up waiting for the animation to finish unfortunately.  (To be precise it will not end up waiting for since transform animations are unthrotteld periodically)

Anyway, it's not entirely specific for WebRender.

> We can fairly easily tell if we have throttled animations inside
> reftest-content.js but then I suppose we still want to somehow wait for all
> non-animation related repaints before moving to
> STATE_WAITING_FOR_REFTEST_WAIT_REMOVAL? Is that right?

I... am not quite sure.  If I could take enough time to look into there closely, I can probably answer it with confidence.
Jeff, as you know we don't have a lot of bandwidth for platform work recently, so if someone on your team is able to help dig into this that would help.

Regarding detecting if we have compositor animations, assuming we are in a chrome context, we can use something like:

```
const hasCompositorAnimations = document.getAnimations().some(animation => animation.isRunningOnCompositor);
```

There may be some way we can use that check to avoid waiting forever while the reftest harness is in the STATE_WAITING_TO_FIRE_INVALIDATE_EVENT state.
(I am really happy that bug 1442817 has been already landed. If it hasn't, it will result another horrible situation there.)
After some more thought about this, I did recall what I was thinking.

(In reply to Brian Birtles (:birtles) from comment #2)

> We can fairly easily tell if we have throttled animations inside
> reftest-content.js but then I suppose we still want to somehow wait for all
> non-animation related repaints before moving to
> STATE_WAITING_FOR_REFTEST_WAIT_REMOVAL? Is that right?

Yes, that's right.  But we have no handy way currently for that purpose, we can't tell the pending restyle is whether for throttled animation or not when we flush styles (to be clear in STATE_WAITING_TO_FIRE_INVALIDATE_EVENT, we flush all pending style on every frame).  In bug 1425315, Ethan Lin tried to do similar way, and I believe there need lots of rest work.  The most cumbersome case is that there is a normal pending style (in every frame) and a throttled animation.
Moreover, from what I can tell in my experience, our pending style flag (e.g. nsIPresShell::NeedFlush) is complicated to use, (bug 1442020#c22 for example), and it might be still not accurate (I have fixed an issue (bug 1442861)).  So, I think waiting MozReftestInvalidate is the most reliable way for reftests which have throttled animations.
dbaron, do you have an opinion on what the right things to do here is?
Flags: needinfo?(dbaron)
Another way I came up with is to introduce a new function to nsIDOMWindowUtils to flush all pending styles, layout and reflow other than throttled animations, and use it in reftest-content.js instead of getBoundingClientRect().  The new function can be easily implemented as far as I can tell.
The try push looks good, I retriggered some of the failures just to make sure they're intermittent and not related to the patches.
Is this ready for review then?
Flags: needinfo?(hikezoe)
I'd want to wait for dbaron's insight.  Though I am also fine with the first approach.  I mean I am fine with either way.
Flags: needinfo?(hikezoe)
I think the approach in comment 9 and the patches in comment 10 seem reasonable, although it would be best to pass a state to all 3 calls of FlushRendering rather than just 2 of them.  I don't entirely understand the problem you're trying to solve, but based on my understanding, not flushing throttled animations when you're trying to wait for pending paints to settle down seems like a reasonable thing to do.
Flags: needinfo?(dbaron)
I couldn't finish up the patches before dbaron has been unavailable.  It should be okay that :birtles or :kats reviews them since dbaron commented the approach seems reasonable in comment 14?  I am fine with either person. :)  Will try both.
Comment on attachment 8964215 [details]
Bug 1447874 - Introduce DOMWindowUtils.flushLayoutWithoutThrottledAnimations.

https://reviewboard.mozilla.org/r/232958/#review238408

::: dom/interfaces/base/nsIDOMWindowUtils.idl:860
(Diff revision 2)
>     * Returns true if a flush of the given type is needed.
>     */
>    bool needsFlush(in long aFlushtype);
>  
>    /**
> +   * Flush pending laytout type notification without flushing throttled

s/laytout type notification/layout-type notifications/
Attachment #8964215 - Flags: review?(bbirtles) → review+
Comment on attachment 8964216 [details]
Bug 1447874 - Use flushLayoutWithoutThrottledAnimations in the state of STATE_WAITING_TO_FIRE_INVALIDATE_EVENT.

https://reviewboard.mozilla.org/r/232960/#review238410

::: commit-message-b4081:1
(Diff revision 2)
> +Bug 1447874 - Use flushLayoutWithoutThrottledAnimations in the sate of STATE_WAITING_TO_FIRE_INVALIDATE_EVENT. r?birtles,kats

s/sate/state/

::: layout/tools/reftest/reftest-content.js:493
(Diff revision 2)
>  // When all MozAfterPaint events and all explicit paint waits are flushed, we're
>  // done and can move to the COMPLETED state.
>  const STATE_WAITING_TO_FINISH = 4;
>  const STATE_COMPLETED = 5;
>  
> -function FlushRendering() {
> +function FlushRendering(state) {

(I'm still thinking about this but I'm wondering if it would be simpler to just pass a bool like "isWaitingForInvalidateEvent". Adding an extra state just for this feels a little awkward but perhaps that's just because it's odd to have an "Initial state" that is not really the initial state. Then again, the interpretation of the state does seem to belong to WaitForTestEnd. I probably need to look into reftest-content.js further.)

::: layout/tools/reftest/reftest-content.js:504
(Diff revision 2)
>          var afterPaintWasPending = utils.isMozAfterPaintPending;
>  
>          var root = win.document.documentElement;
>          if (root && !root.classList.contains("reftest-no-flush")) {
>              try {
> -                // Flush pending restyles and reflows for this window
> +                if (state == STATE_WAITING_TO_FIRE_INVALIDATE_EVENT) {

I think we normally prefer ===

::: layout/tools/reftest/reftest-content.js:505
(Diff revision 2)
> +                    // In the state of STATE_WAITING_TO_FIRE_INVALIDATE_EVENT,
> +                    // we shouldn't flush throttled animations since it might
> +                    // cause a new MozAfterPaint event, thus we have to wait for
> +                    // the new event and then we will get stuck in
> +                    // STATE_WAITING_TO_FIRE_INVALIDATE_EVENT.

I'm not sure I follow this comment so let me try re-writing it and you can correct it where it is wrong:

"If we are waiting for the MozReftestInvalidate event we don't want to flush throttled animations. Flushing throttled animations can continue to cause new MozAfterPaint events even when all the rendering we're concerned about should have ceased. If we wait for all these new MozAfterPaint events we'll never leave this state."

Do we wait for the MozAfterPaint events here in the test harness, or is the MozReftestInvalidate event not dispatched so long as there are pending MozAfterPaint events?
(In reply to Brian Birtles (:birtles) from comment #21)
> Comment on attachment 8964216 [details]
> Bug 1447874 - Use flushLayoutWithoutThrottledAnimations in the sate of
> STATE_WAITING_TO_FIRE_INVALIDATE_EVENT.
> 
> https://reviewboard.mozilla.org/r/232960/#review238410
> 
> ::: commit-message-b4081:1
> (Diff revision 2)
> > +Bug 1447874 - Use flushLayoutWithoutThrottledAnimations in the sate of STATE_WAITING_TO_FIRE_INVALIDATE_EVENT. r?birtles,kats
> 
> s/sate/state/
> 
> ::: layout/tools/reftest/reftest-content.js:493
> (Diff revision 2)
> >  // When all MozAfterPaint events and all explicit paint waits are flushed, we're
> >  // done and can move to the COMPLETED state.
> >  const STATE_WAITING_TO_FINISH = 4;
> >  const STATE_COMPLETED = 5;
> >  
> > -function FlushRendering() {
> > +function FlushRendering(state) {
> 
> (I'm still thinking about this but I'm wondering if it would be simpler to
> just pass a bool like "isWaitingForInvalidateEvent". Adding an extra state
> just for this feels a little awkward but perhaps that's just because it's
> odd to have an "Initial state" that is not really the initial state. Then
> again, the interpretation of the state does seem to belong to
> WaitForTestEnd. I probably need to look into reftest-content.js further.)

I do prefer to avoid boolean arguments as possible since it will be ambiguous what the boolean value means if we use boolean literal directly in call sites, e.g FlushRendering(true).  So should we introduce a new enum like value something like this;

const FLUSH_MODE = {
  FLUSH_ALL: 0,
  DONT_FLUSH_THROTTLED_ANIMATIONS: 1
};

Then use "FlushRendering(FLUSH_ALL)" outside WaitForTestEnd() and use "FlushRendering(state === STATE_WAITING_TO_FIRE_INVALIDATE_EVENT ? DONT_FLUSH_THROTTLED_ANIMATIONS : FLUSH_ALL)" ?

> ::: layout/tools/reftest/reftest-content.js:505
> (Diff revision 2)
> > +                    // In the state of STATE_WAITING_TO_FIRE_INVALIDATE_EVENT,
> > +                    // we shouldn't flush throttled animations since it might
> > +                    // cause a new MozAfterPaint event, thus we have to wait for
> > +                    // the new event and then we will get stuck in
> > +                    // STATE_WAITING_TO_FIRE_INVALIDATE_EVENT.
> 
> I'm not sure I follow this comment so let me try re-writing it and you can
> correct it where it is wrong:
> 
> "If we are waiting for the MozReftestInvalidate event we don't want to flush
> throttled animations. Flushing throttled animations can continue to cause
> new MozAfterPaint events even when all the rendering we're concerned about
> should have ceased. If we wait for all these new MozAfterPaint events we'll
> never leave this state."
> 
> Do we wait for the MozAfterPaint events here in the test harness, or is the
> MozReftestInvalidate event not dispatched so long as there are pending
> MozAfterPaint events?

The latter.  We don't dispatch MozReftestInvalidate event then get stuck.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8964216 [details]
Bug 1447874 - Use flushLayoutWithoutThrottledAnimations in the state of STATE_WAITING_TO_FIRE_INVALIDATE_EVENT.

https://reviewboard.mozilla.org/r/232960/#review238424

::: layout/tools/reftest/reftest-content.js:599
(Diff revision 3)
> +          // rendering we're concerned about should have ceased. If we wait for
> +          // all these new MozAfterPaint events we'll never leave this state.

Perhaps we should change this last sentence to something like:

"Since MozReftestInvalidate won't be sent until we finish waiting for all MozAfterPaint events, we should avoid flushing throttled animations here or else we'll never leave this state."

Whichever sentence seems most clear to you!
Attachment #8964216 - Flags: review?(bbirtles) → review+
Comment on attachment 8964215 [details]
Bug 1447874 - Introduce DOMWindowUtils.flushLayoutWithoutThrottledAnimations.

https://reviewboard.mozilla.org/r/232958/#review239280
Attachment #8964215 - Flags: review?(bugmail) → review+
Comment on attachment 8964216 [details]
Bug 1447874 - Use flushLayoutWithoutThrottledAnimations in the state of STATE_WAITING_TO_FIRE_INVALIDATE_EVENT.

https://reviewboard.mozilla.org/r/232960/#review239284

This looks good to me, thanks! (And sorry for the delay, it was a long weekend for me and I took an extra day off yesterday).
Attachment #8964216 - Flags: review?(bugmail) → review+
No worries!  Good to hear that. :)
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c3dc86c628f4
Introduce DOMWindowUtils.flushLayoutWithoutThrottledAnimations. r=birtles,kats
https://hg.mozilla.org/integration/autoland/rev/4b4230db0bdc
Use flushLayoutWithoutThrottledAnimations in the state of STATE_WAITING_TO_FIRE_INVALIDATE_EVENT. r=birtles,kats
https://hg.mozilla.org/mozilla-central/rev/c3dc86c628f4
https://hg.mozilla.org/mozilla-central/rev/4b4230db0bdc
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.