Closed Bug 1316050 Opened 3 years ago Closed 3 years ago

Intermittent /web-animations/interfaces/AnimationEffectTiming/endDelay.html | onfinish event is fired currentTime is after endTime - assert_equals: length of array should be one expected 1 but got 0

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- unaffected
firefox52 --- fixed
firefox53 --- fixed

People

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

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Hi Olli,  I am guessing this failure is affected by bug 1315570.  There is no change in dom/animation code when the first failure happened. <https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=73a97f8ffebac71fb4b5d505e2aa0da35a013d8c>

Is there any possibility that the change delays animation's finish event?  If so, is there anything we can do to avoid the delay?
Flags: needinfo?(bugs)
Affected by bug 1315570? That hasn't landed. You probably mean bug 1306591.
That may have revealed a bug in animations code or in the test. Vsync (refreshDriver/animationFrameCallbacks) may be run now at different time. But it could have been run at that time also before, but just not as often. It is after all somewhat random when vsync runs.
Flags: needinfo?(bugs)
finish event is dispatched async here http://searchfox.org/mozilla-central/rev/846adaea6ccd1428780ed47d0d94da18478acdbb/dom/animation/Animation.cpp#1393
and any random code may run between that call and and when the DOM event actually is dispatched.
As far as I see, the test is buggy at least.
Nothing guarantees assert_equals(receivedEvents.length, 1, 'length of array should be one'); should be actually true (per HTML spec's processing model).
If browser does something heavy between animation frames (waitForAnimationFrames(2);), the finish event may be dispatched later.
Thank you, Olli for your deep insight!

IIUC, we will have to

a) In case that test case expects an event:
 just wait for the event.

b) In case that test case expects that no event receives:
 wait for an requestIdleCallback, and then check the event.
Blocks: 1244635
Priority: -- → P1
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63ef431f19747b5a26b35276a989ae1075affef2

If it's not a good idea to use requestIdleCallback in web animation's tests (neither Edge nor Safari supports it yet), I will land only the part 2.  Brian?
Flags: needinfo?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=63ef431f19747b5a26b35276a989ae1075affef2
> 
> If it's not a good idea to use requestIdleCallback in web animation's tests
> (neither Edge nor Safari supports it yet), I will land only the part 2. 
> Brian?

Oops, part 1 does a) what I commented in comment 5, part 2 does b).
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=63ef431f19747b5a26b35276a989ae1075affef2
> > 
> > If it's not a good idea to use requestIdleCallback in web animation's tests
> > (neither Edge nor Safari supports it yet), I will land only the part 2. 
> > Brian?
> 
> Oops, part 1 does a) what I commented in comment 5, part 2 does b).

Opposite...  1 does b), 2 does a).
I agree we shouldn't use requestIdleCallback yet.

https://hg.mozilla.org/try/rev/a3225012cbf624d591cd8ca17a18d20a7fdc5899 looks good to me.

I wonder if we need to check the event time though--that seems orthogonal to fixing this orange? Perhaps we could just have:

  anim.ready.then(function() {
    anim.onfinish = t.step_func(function(event) {
      assert_unreached('onfinish event should not be fired during endDelay');
    });
    anim.currentTime = 110000; // during endDelay
    return waitForAnimationFrames(2);
  }).then(t.step_func(function() {
    anim.onfinish = t.step_func(function(event) {
      t.done();
    });
    anim.currentTime = 130000; // after endTime
  }));

What do you think?
Flags: needinfo?(bbirtles)
Ah, exactly.  The check is totally out of scope of this test case.
Comment on attachment 8812654 [details]
Bug 1316050 - Wait for a single finish event itself instead of waiting two requestAnimationFrame and checking the event.

https://reviewboard.mozilla.org/r/94318/#review94526

Thanks for taking care of this!
Attachment #8812654 - Flags: review?(bbirtles) → review+
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/autoland/rev/0ac505253b26
Wait for a single finish event itself instead of waiting two requestAnimationFrame and checking the event. r=birtles
https://hg.mozilla.org/mozilla-central/rev/0ac505253b26
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.