Intermittent layout/style/test/test_transitions_replacement_on_busy_frame.html | From value of transition is updated since the moment when it was generated - didn't expect "none", but got it

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

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

Tracking

({intermittent-failure})

unspecified
mozilla57
intermittent-failure
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

15 failures in 833 pushes (0.018 failures/push) were associated with this bug in the last 7 days.  
Repository breakdown:
* mozilla-inbound: 9
* try: 2
* mozilla-central: 2
* autoland: 2

Platform breakdown:
* android-4-3-armv7-api15: 14
* windows8-64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1339648&startday=2017-02-13&endday=2017-02-19&tree=all
1 failures in 718 pushes (0.001 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 1

Platform breakdown:
* linux64-stylo: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1339648&startday=2017-06-26&endday=2017-07-02&tree=all
1 failures in 822 pushes (0.001 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 1

Platform breakdown:
* linux64-stylo-sequential: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1339648&startday=2017-07-17&endday=2017-07-23&tree=all
17 failures in 1008 pushes (0.017 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 8
* mozilla-inbound: 7
* try: 1
* mozilla-central: 1

Platform breakdown:
* linux64-stylo: 10
* linux64-stylo-sequential: 7

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1339648&startday=2017-07-24&endday=2017-07-30&tree=all
8 failures in 888 pushes (0.009 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 4
* mozilla-inbound: 2
* mozilla-beta: 2

Platform breakdown:
* macosx64-stylo: 3
* linux64-stylo-sequential: 2
* osx-10-10: 1
* linux64-stylo: 1
* linux64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1339648&startday=2017-07-31&endday=2017-08-06&tree=all
3 failures in 901 pushes (0.003 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* try: 2
* autoland: 1

Platform breakdown:
* linux64-stylo-sequential: 2
* linux64-stylo: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1339648&startday=2017-08-07&endday=2017-08-13&tree=all
(Assignee)

Comment 7

a year ago
Ok, I think there are two bugs here:

(a) We don't actually trigger the transition when we mean to because at [1] we have:

      getComputedStyle(div);

    which doesn't actually flush style. We want:

      getComputedStyle(div).transform;

(b) Probably don't actually wait for the transition to get past the first frame so it's possible when we trigger the second transition that the computed value is still at `matrix(1, 0, 0, 1, 0, 0)` and hence the second transition won't be created.

I haven't seen (b) happen on automation but I see it often locally (presumably because the automation machines are slower and hence we're less likely to try to trigger the second transition whilst still on the first frame).

[1] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/style/test/file_transitions_replacement_on_busy_frame.html#46
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Component: Layout → CSS Parsing and Computation
(Assignee)

Updated

a year ago
Blocks: 1382841
(Assignee)

Comment 8

a year ago
I messed something up when rebasing bug 1382841 on top of my fix for this. I'm going to look into it tomorrow instead.
Comment hidden (mozreview-request)
(Assignee)

Comment 10

a year ago
mozreview-review
Comment on attachment 8900560 [details]
Bug 1339648 - Fix flaky file_transitions_replacement_on_busy_frame.html test;

https://reviewboard.mozilla.org/r/171960/#review177172

::: commit-message-f0abd:19
(Diff revision 1)
> +    generated (although the first transition will be canceled) since the
> +    there is no change.

nit: superfluous 'the' at the end of the line.

::: layout/style/test/file_transitions_replacement_on_busy_frame.html:45
(Diff revision 1)
>      finish();
>      return;
>    }
>  
>    var div = document.getElementById("target");
> +

This extra line wasn't actually intentional but having added it, I think it probably helps readability.

Comment 11

a year ago
mozreview-review
Comment on attachment 8900560 [details]
Bug 1339648 - Fix flaky file_transitions_replacement_on_busy_frame.html test;

https://reviewboard.mozilla.org/r/171960/#review177174

I am convinced that this flakiness is related what I commented in bug 1366603 comment 18. The MozAfterPaint that waitForAllPaints waits is not what we expected (i.e. transition start).
Anyway, this change looks reasonable to me. Thanks for fixing this!

::: layout/style/test/file_transitions_replacement_on_busy_frame.html:59
(Diff revision 1)
>      var previousKeyframeValue;
>      var anim;
>      requestAnimationFrame(function() {
>        // Start second transition
> -      div.style.transform = "translateX(0px)";
> +      div.style.transform = "translateX(600px)";
>        getComputedStyle(div).transform;

Nit: should we remove this getComputedStyle() for consistency?
Attachment #8900560 - Flags: review?(hikezoe) → review+
(Assignee)

Comment 12

a year ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> Comment on attachment 8900560 [details]
> Bug 1339648 - Fix flaky file_transitions_replacement_on_busy_frame.html test;
> 
> https://reviewboard.mozilla.org/r/171960/#review177174
> 
> I am convinced that this flakiness is related what I commented in bug
> 1366603 comment 18. The MozAfterPaint that waitForAllPaints waits is not
> what we expected (i.e. transition start).
> Anyway, this change looks reasonable to me. Thanks for fixing this!
> 
> ::: layout/style/test/file_transitions_replacement_on_busy_frame.html:59
> (Diff revision 1)
> >      var previousKeyframeValue;
> >      var anim;
> >      requestAnimationFrame(function() {
> >        // Start second transition
> > -      div.style.transform = "translateX(0px)";
> > +      div.style.transform = "translateX(600px)";
> >        getComputedStyle(div).transform;
> 
> Nit: should we remove this getComputedStyle() for consistency?

Yes, good idea.

Actually, I would also like to rename |anim| to |secondTransition| for consistency, but I was trying to keep the patch small. Maybe I should just do that, however.
Comment hidden (mozreview-request)

Comment 14

a year ago
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ac13a91244f
Fix flaky file_transitions_replacement_on_busy_frame.html test; r=hiro
https://hg.mozilla.org/mozilla-central/rev/2ac13a91244f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 16

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/c0bd0885603f
status-firefox56: --- → fixed
Flags: in-testsuite+
1 failures in 908 pushes (0.001 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-beta: 1

Platform breakdown:
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1339648&startday=2017-08-21&endday=2017-08-27&tree=all
You need to log in before you can comment on or make changes to this bug.