Closed Bug 1425009 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

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

References

Details

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

Attachments

(1 file)

Flags: needinfo?(hikezoe)
I suspected this failure is also caused by clamping pending ready time.  Given that this failure is not so high frequency, I'd suggest to fix bug 1417354 instead of tweaking test cases.

A initial try seemed to work;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccb2aaf93dee01257c95888a4bfd424710705a59
Depends on: 1417354
Flags: needinfo?(hikezoe)
Whiteboard: [stockwell unknown]
There are 32 failures in the past 7 days, most of the occurrences can be seen on android-api-16-gradle platform (opt) and the rest of them on Linux x64 (debug) and windows10-64 (debug).
A recent log example:
https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-central&job_id=152632305&lineNumber=12676

And a relevant part of it:
15:03:43     INFO -  Buffered messages logged at 15:03:43
12667
15:03:43     INFO -  720 INFO TEST-PASS | dom/animation/test/mozilla/test_restyles.html | Applying animation style with different duration should restyle twice
12668
15:03:43     INFO -  721 INFO SpawnTask.js | Leaving test necessary_update_should_be_invoked
12669
15:03:43     INFO -  722 INFO SpawnTask.js | Entering test changing_cascading_result_for_main_thread_animation
12670
15:03:43     INFO -  723 INFO TEST-PASS | dom/animation/test/mozilla/test_restyles.html | The opacity animation is running on the compositor
12671
15:03:43     INFO -  724 INFO TEST-FAIL | dom/animation/test/mozilla/test_restyles.html | Changing cascading result for the property running on the main thread does not cause synchronization layer of opacity animation running on the compositor - got 6, expected +0
12672
15:03:43     INFO -  725 INFO SpawnTask.js | Leaving test changing_cascading_result_for_main_thread_animation
12673
15:03:43     INFO -  726 INFO SpawnTask.js | Entering test restyling_for_animation_on_orphaned_element
12674
15:03:43     INFO -  727 INFO TEST-PASS | dom/animation/test/mozilla/test_restyles.html | Animation on orphaned element should not cause restyles
12675
15:03:43     INFO -  Buffered messages finished
12676
15:03:43    ERROR -  728 INFO 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 1, expected 2
12677
15:03:43     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:312:5
12678
15:03:43     INFO -      restyling_for_animation_on_orphaned_element@dom/animation/test/mozilla/file_restyles.html:967:7
12679
15:03:43     INFO -      async*add_task/</<@SimpleTest/SpawnTask.js:298:21
12680
15:03:43     INFO -      onFulfilled@SimpleTest/SpawnTask.js:69:15
12681
15:03:43     INFO -      promise callback*next@SimpleTest/SpawnTask.js:104:45
12682
15:03:43     INFO -      onFulfilled@SimpleTest/SpawnTask.js:73:7
12683
15:03:43     INFO -      promise callback*next@SimpleTest/SpawnTask.js:104:45
12684
15:03:43     INFO -      onFulfilled@SimpleTest/SpawnTask.js:73:7


:birtles Can you please take a look at this?
Flags: needinfo?(bbirtles)
Whiteboard: [stockwell unknown] → [stockwell needswork]
I take this.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(bbirtles)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8e11b24142806282be6c98f9fe578fc6bc6062e

As I commented in comment 1 we should fix bug 1417354, but I am not a big fan of the fix in the try in comment 1.  That's because, to fix bug 1417354 we need to drop an element (and pseudos) from mElementsToRestyle *before* calling ClearInDocument(), whereas for CSS animations/transitions we need to call  DeleteProperty() for the elements (and pseudos) *after* calling ClearInDocument().  I think we should unify them somehow.
Attachment #8938844 - Flags: review?(bbirtles)
Comment on attachment 8938844 [details]
Bug 1425009 - Take into account the possibility that the animation begins at the moment when the animation was detached from the document.

https://reviewboard.mozilla.org/r/209336/#review215590

::: commit-message-c2922:1
(Diff revision 1)
> +Bug 1425009 - Take into account the possibility that the animation begins at the moment when the animation was dettached from the document. r?

s/dettached/detached/

::: dom/animation/test/mozilla/file_restyles.html:104
(Diff revision 1)
>           aAnimation.currentTime === 0;
>  }
>  
>  function tweakExpectedRestyleCount(aAnimation, aExpectedRestyleCount) {
>    // Normally we expect one restyling for each requestAnimationFrame (as
>    // called by observeRestyling) PLUS one for the last frame becasue of bug

(Nit: Mind fixing this typo at the same time? s/becasue/because/)

::: dom/animation/test/mozilla/file_restyles.html:944
(Diff revision 1)
>  
>      await animation.ready;
>  
>      div.remove();
>  
> +    // It's possible that the animation begins at this momenet.  If this is the

s/momenet/moment/

::: dom/animation/test/mozilla/file_restyles.html:945
(Diff revision 1)
>      await animation.ready;
>  
>      div.remove();
>  
> +    // It's possible that the animation begins at this momenet.  If this is the
> +    // case, an additional superfluos restyle request that we will check later

s/superluos/superfluous/
Attachment #8938844 - Flags: review?(bbirtles) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ede11fe526e
Take into account the possibility that the animation begins at the moment when the animation was detached from the document. r=birtles
https://hg.mozilla.org/mozilla-central/rev/3ede11fe526e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.