Closed Bug 1458841 Opened 2 years ago Closed 2 years ago

Intermittent dom/animation/test/mozilla/test_restyles.html | Visibility animation running on the main-thread on in-view element should not be throttled - got 4, expected 5

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: hiro)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell unknown])

Attachments

(3 files)

Oops.
Assignee: nobody → hikezoe
There are 31 failures in the past week.
Platforms: most of them on android-4-3-armv7-api16 opt and once on debug, and one occurrence on: windows7-32 opt, windows10-64 debug and osx-10-10 debug.

Recent log failure: https://treeherder.mozilla.org/logviewer.html#?job_id=180754656&repo=autoland&lineNumber=1595

Relevant part of the log:
[task 2018-05-29T18:28:07.086Z] 18:28:07     INFO -  405 INFO TEST-PASS | dom/animation/test/mozilla/test_restyles.html | Animation.playState should flush throttled transition style that affects the throttled animation
[task 2018-05-29T18:28:07.087Z] 18:28:07     INFO -  Buffered messages logged at 18:27:57
[task 2018-05-29T18:28:07.088Z] 18:28:07     INFO -  406 INFO AddTask.js | Leaving test restyling_for_throttled_transition_on_querying_play_state
[task 2018-05-29T18:28:07.088Z] 18:28:07     INFO -  407 INFO AddTask.js | Entering test restyling_visibility_animations_on_in_view_element
[task 2018-05-29T18:28:07.089Z] 18:28:07     INFO -  Buffered messages finished
[task 2018-05-29T18:28:07.089Z] 18:28:07     INFO -  408 INFO TEST-UNEXPECTED-FAIL | dom/animation/test/mozilla/test_restyles.html | Visibility animation running on the main-thread on in-view element should not be throttled - got 4, expected 5
[task 2018-05-29T18:28:07.090Z] 18:28:07     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2018-05-29T18:28:07.090Z] 18:28:07     INFO -      restyling_visibility_animations_on_in_view_element@dom/animation/test/mozilla/file_restyles.html:1905:5
[task 2018-05-29T18:28:07.091Z] 18:28:07     INFO -  409 INFO AddTask.js | Leaving test restyling_visibility_animations_on_in_view_element
[task 2018-05-29T18:28:07.092Z] 18:28:07     INFO -  410 INFO TEST-OK | dom/animation/test/mozilla/test_restyles.html | took 41488ms
[task 2018-05-29T18:28:07.092Z] 18:28:07     INFO -  411 INFO TEST-START | dom/animation/test/mozilla/test_restyling_xhr_doc.html
[task 2018-05-29T18:28:07.093Z] 18:28:07     INFO -  412 INFO TEST-OK | dom/animation/test/mozilla/test_restyling_xhr_doc.html | took 2253ms

:birtles Can you please take a look here?
Flags: needinfo?(bbirtles)
Whiteboard: [stockwell needswork]
Oh I am sorry, I've been leaving this since I have other higher priority bugs.  I will do this today.
Flags: needinfo?(bbirtles)
Duplicate of this bug: 1425778
Comment on attachment 8981653 [details]
Bug 1458841 - Remove test code for the old microtask behavior.

https://reviewboard.mozilla.org/r/247764/#review253836

::: dom/animation/test/mozilla/file_restyles.html:136
(Diff revision 1)
> -  // Normally we expect one restyling for each requestAnimationFrame (as
> -  // called by observeRestyling) PLUS one for the last frame because of bug
> +  // If |aAnimation| begins at the current timeline time, we will not process
> +  // restyling in the initial frame.
> -  // 1193394.  However, we won't observe that initial restyling unless BOTH of
> -  // the following two conditions hold:
> -  //
> -  // 1. We are running *before* restyling happens.
> -  // 2. The animation actually needs a restyle because it started prior to
> -  //    this frame.  Even if (1) is true, in some cases due to aligning with
> -  //    the refresh driver, the animation fame in which the ready promise is
> -  //    resolved happens to coincide perfectly with the start time of the
> -  //    animation.  In this case no restyling is needed so we won't observe
> -  //    an additional restyle.

Doesn't this part of the comment still apply?
Attachment #8981653 - Flags: review?(bbirtles) → review+
Comment on attachment 8981654 [details]
Bug 1458841 - Introduce a utility function that waits for a given animation being ready to be restyle.

https://reviewboard.mozilla.org/r/247766/#review253838

Nice.
Attachment #8981654 - Flags: review?(bbirtles) → review+
Comment on attachment 8981655 [details]
Bug 1458841 - Use waitForAnimationReadyToRestyle wherever we wait for animation.ready right after animation creation.

https://reviewboard.mozilla.org/r/247768/#review253840

::: commit-message-fde0a:1
(Diff revision 1)
> +Bug 1458841 - Use waitForAnimationReadyToRestyle whereever we wait for animation.ready right after animation creation. r?birtles

Nit: s/whereever/wherever/

::: commit-message-fde0a:3
(Diff revision 1)
> +We haven't used tweakExpectedRestyleCount where we just make sure the animation
> +restyle count is zero since those tests don't fail even if the animation
> +restyling was skipped.  But it's might be a wallpaper of real bugs, for example
> +actually we were trying to restyle but skipped it because the animation just
> +begins at the current time.  Also using waitForAnimationReadyToRestyle
> +everywhere would avoid mis-usage of 'animation.ready' when adding new tests in
> +future.

"Previously we used the tweakExpectedRestyleCount function (replaced by the waitForAnimationReadyToRestyle function in the previous patch) only in cases where we were actually expecting restyles to happen. For cases where we don't expect restyles, i.e. cases where we assert the restyle count is zero, we didn't use this method meaning we didn't bother checking if there was a restyle expected for the current frame or not.

Since we normally wait for 5 frames anyway before checking that there have been no restyles, failing to count the number of frames and waiting only 4 frames is not a problem. However, if a new test were added that just copied this code and only waited one frame, it might fail to test what it intended. So, to avoid possible future bugs and in order to be more consistent with tests that do expect restyles, this patch replaces a number of uses animation.ready with waitForAnimationReadyToRestyle."

?
Attachment #8981655 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #13)
> Comment on attachment 8981653 [details]
> Bug 1458841 - Remove test code for the old microtask behavior.
> 
> https://reviewboard.mozilla.org/r/247764/#review253836
> 
> ::: dom/animation/test/mozilla/file_restyles.html:136
> (Diff revision 1)
> > -  // Normally we expect one restyling for each requestAnimationFrame (as
> > -  // called by observeRestyling) PLUS one for the last frame because of bug
> > +  // If |aAnimation| begins at the current timeline time, we will not process
> > +  // restyling in the initial frame.
> > -  // 1193394.  However, we won't observe that initial restyling unless BOTH of
> > -  // the following two conditions hold:
> > -  //
> > -  // 1. We are running *before* restyling happens.
> > -  // 2. The animation actually needs a restyle because it started prior to
> > -  //    this frame.  Even if (1) is true, in some cases due to aligning with
> > -  //    the refresh driver, the animation fame in which the ready promise is
> > -  //    resolved happens to coincide perfectly with the start time of the
> > -  //    animation.  In this case no restyling is needed so we won't observe
> > -  //    an additional restyle.
> 
> Doesn't this part of the comment still apply?

Oh yes, right. I am going to patch the rest of comment like this;

+  // If |aAnimation| begins at the current timeline time, we will not process
+  // restyling in the initial frame because of aligning with the refresh driver,
+  // the animation frame in which the ready promise is resolved happens to
+  // coincide perfectly with the start time of the animation.  In this case no
+  // restyling is needed so we won't observe an additional restyle.
After the remaining patches in the series, it's possible you don't need the comment anymore.
(In reply to Brian Birtles (:birtles) from comment #17)
> After the remaining patches in the series, it's possible you don't need the
> comment anymore.

Indeed, but I will leave the comment for someone in future, since it's more descriptive.

And thanks for the correction in the commit message!
Oh, now I realized that I didn't set status as ASSIGNED. :/
Status: NEW → ASSIGNED
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd9b738a9ba8
Remove test code for the old microtask behavior. r=birtles
https://hg.mozilla.org/integration/autoland/rev/87d0c3102303
Introduce a utility function that waits for a given animation being ready to be restyle. r=birtles
https://hg.mozilla.org/integration/autoland/rev/3a2dd772aa0e
Use waitForAnimationReadyToRestyle wherever we wait for animation.ready right after animation creation. r=birtles
You need to log in before you can comment on or make changes to this bug.