1428692 - We observed 2 restyles in the first frame right after re-attaching to the document in test_restyles.html after bug 1193394

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P2)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

We got this new failure in today's try:
https://treeherder.mozilla.org/logviewer.html#?job_id=154670351&repo=try&lineNumber=4268

TEST-UNEXPECTED-FAIL | dom/animation/test/mozilla/test_restyles.html | We should observe one restyle in the first frame right after re-attaching to the document - got 2, expected 1

Comment 1

[Intermittent Failures Robot]

21 failures in 152 pushes (0.138 failures/push) were associated with this bug yesterday.    

Repository breakdown:
* try: 21

Platform breakdown:
* linux64: 6
* windows10-64: 4
* windows7-32: 3
* android-4-3-armv7-api16: 2
* windows10-64-ccov: 1
* osx-10-10: 1
* linux64-jsdcov: 1
* linux64-ccov: 1
* linux32: 1
* android-api-16-gradle: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1428692&startday=2018-01-10&endday=2018-01-10&tree=all

Comment 2

[Intermittent Failures Robot]

26 failures in 788 pushes (0.033 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* try: 26

Platform breakdown:
* linux64: 6
* android-4-3-armv7-api16: 5
* windows10-64: 4
* windows7-32: 3
* osx-10-10: 2
* linux32: 2
* windows10-64-ccov: 1
* linux64-jsdcov: 1
* linux64-ccov: 1
* android-api-16-gradle: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1428692&startday=2018-01-08&endday=2018-01-14&tree=all

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Take this bug to followup.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Thank you, Bevis.  Please feel free pinging me on IRC if you have any question. :)

Comment 5

[Intermittent Failures Robot]

3 failures in 657 pushes (0.005 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* try: 2
* mozilla-inbound: 1

Platform breakdown:
* android-4-3-armv7-api16: 2
* linux64-ccov: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1428692&startday=2018-01-15&endday=2018-01-21&tree=all

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I will check why the test started failing.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

As per a try [1], it seems that the failure happens only on stylo. sigh.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d024d36eaf64dcbcdf0039f80ce0267f738c86ec

Comment 8

[Hiroyuki Ikezoe (:hiro)]

OK, now I understand what was wrong.   I made a mistake to fix bug 1425009, in this commit;

 https://hg.mozilla.org/mozilla-central/rev/3ede11fe526e

We can't use tweakExpectedRestyleCount() for the failure test case.  In the case where we have the conformant Promise handling,   tweakExpectedRestyleCount() adjusts a given expected restyle count in the last frame we observes.  That's because the Promise in the last requestAnimationFrame callback is processed after restyling in the last frame.  Whereas, in this failure test, we just want to know the animation has begun at the current timeline time instead (i.e. the animation start time is clamped so that we don't post request restyle at the moment).

I try to explain to make what happens clearer.

 - Tick
  - Animation::Tick happens
   - In RequestRestyle() the animation element is appended into mElementsToRestyle *if the animation does not begin at the current timeline time*
   - resolve animation.ready
    - the animation element is detached from the document, at this moment mElementsToRestyle may have the animation element if it's the case

Some amount of frames later;

 - Tick
  - the animation element is attached to the document
  - in PreTraverse() for the *first* animation restyle we post restyle request for the animation in the case where mElementsToRestyle have the element
  - in normal traversal we create a sequential task for CascadeResults (bug 1388560)
  - in PreTraverse() for the *second* animation restyle we post restyle request

So if the animation begins at the current timeline time, we got 1 restyles, and if it's not, we got 2 restyles there.

A try based on bug 1193394;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a7044314a3c6e66ceb8694460011b87a7d33177
In this try, test cases for unthrottling transform animation failed intermittently, I think we also need startsRightNow() check there.

A try based on current m-c;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=68f639513c4bc32efedc4363bb5e39e25c820583

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I try to explain another way;

From the commit what I was wrong <https://hg.mozilla.org/mozilla-central/rev/3ede11fe526e#l1.58>;

-      is(markers.length, 2,
+      is(markers.length, expectedRestyleCount,

What we should have done here is that 

+      is(markers.length, 2 (- 1 if the animation begun at the current timeline time),

That's because if the animation begun at the current timeline time, we don't post the superfluous restyle.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1428692 - Don't use tweakExpectedRestyleCount in the case where we don't use the returned count for observeStyling called right after.

tweakExpectedRestyleCount() is supposed to work with observeStyle() which is
called right after tweakExpectedRestyleCount().  That's because
tweakExpectedRestyleCount() adjusts the expected restyle count which will
happen in continuous frames that begins from the startsRightNow() call,
especially a redundant restyle might be observed in the last frame if the
animation did not begin at the current timeline time and if the Promise inside
the last requestAnimationFrame is fulfilled after restyling happened.

Instead of using tweakExpectedRestyleCount(), we need to check whether the
animation begun at the current timeline time when the animation started, and if
the animation begun at the current timeline time, we don't observe the
superfluous restyle in a frame after the animation element was re-attached.

Review commit: https://reviewboard.mozilla.org/r/216168/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216168/

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 8946176 [details]
Bug 1428692 - Don't use tweakExpectedRestyleCount in the case where we don't use the returned count for observeStyling called right after.

https://reviewboard.mozilla.org/r/216168/#review221998

::: dom/animation/test/mozilla/file_restyles.html:1005
(Diff revision 1)
>      div.remove();
>  
>      // It's possible that the animation begins at this moment.  If this is the
>      // case, an additional superfluous restyle request that we will check later
>      // can't be observed.
> -    const expectedRestyleCount = tweakExpectedRestyleCount(animation, 1);
> +    let superfluousRestyle = startsRightNow(animation) ? 0 : 1;

(Any reason for let instead of const? This doesn't get re-assigned does it?)

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, busy 1/29~2/5) from comment #11)
> Comment on attachment 8946176 [details]
> Bug 1428692 - Don't use tweakExpectedRestyleCount in the case where we don't
> use the returned count for observeStyling called right after.
> 
> https://reviewboard.mozilla.org/r/216168/#review221998
> 
> ::: dom/animation/test/mozilla/file_restyles.html:1005
> (Diff revision 1)
> >      div.remove();
> >  
> >      // It's possible that the animation begins at this moment.  If this is the
> >      // case, an additional superfluous restyle request that we will check later
> >      // can't be observed.
> > -    const expectedRestyleCount = tweakExpectedRestyleCount(animation, 1);
> > +    let superfluousRestyle = startsRightNow(animation) ? 0 : 1;
> 
> (Any reason for let instead of const? This doesn't get re-assigned does it?)

Good catch. :)  Indeed, I had modified the value while debugging.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8946176 [details]
Bug 1428692 - Don't use tweakExpectedRestyleCount in the case where we don't use the returned count for observeStyling called right after.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216168/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c58988f74bd4
Don't use tweakExpectedRestyleCount in the case where we don't use the returned count for observeStyling called right after. r=birtles

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/c58988f74bd4