Closed Bug 1168759 Opened 9 years ago Closed 8 years ago

Intermittent test_deferred_start.html | Starting an animation with a delay starts from the correct point - Starting an animation with a delay starts from the correct point: assert_equals: Got a valid transform matrix on the compositor (got: "") expected 6

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- fixed

People

(Reporter: cbook, Assigned: birtles)

References

(
URL
)

Details

(Keywords: intermittent-failure)

Attachments

(3 files)

Disable test_deferred_start.html on B2G emulator
942 bytes, patch
RyanVM
: review+
Details | Diff | Splinter Review
Bug 1168759 - Part 1: Rewrite async_test with promise_test.
58 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1168759 - Part 2: Use waitForAllPaints() instead of animation.ready.
58 bytes, text/x-review-board-request
boris
: review+
Details 
b2g_emulator_vm mozilla-inbound debug test mochitest-debug-1

https://treeherder.mozilla.org/logviewer.html#?job_id=10185603&repo=mozilla-inbound

01:35:59 INFO - 208 INFO TEST-UNEXPECTED-FAIL | dom/animation/test/mozilla/test_deferred_start.html | Starting an animation with a delay starts from the correct point - Starting an animation with a delay starts from the correct point: assert_equals: Got a valid transform matrix on the compositor (got: "") expected 6 but got 0
Brian, can you please take a look at this? :)
Component: DOM → Layout
Flags: needinfo?(bbirtles)
See Also: → 1119981, 1113425, 1148949, 1150351
It turns out this is just the continuation of bug 1148949. According to bug 1148949 comment 65, the first failures coincided roughly with changeset https://hg.mozilla.org/mozilla-central/rev/962e15b81684. Then, when that got backed out, the failure rate dropped significantly for a while.

We've tweaked a number of places in order to get OMTA shipping. I'm guessing we are now tripping up on some of these new optimizations and not hitting the OMTA path causing the test to fail. This is probably going to take days to diagnose--if we're falling off the OMTA path when we shouldn't, though, I guess it's necessary.
I'm just going to disable this on B2G for now since it will be a few days before I can look into it
Attachment #8634592 - Flags: review?(ryanvm)
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Flags: needinfo?(bbirtles)
Comment on attachment 8634592 [details] [diff] [review]
Disable test_deferred_start.html on B2G emulator

Review of attachment 8634592 [details] [diff] [review]:
-----------------------------------------------------------------

That'll work, but it'll also skip it on B2G desktop (and possibly Mulet). Assuming you only want it skipped on the emulator builds, toolkit == 'gonk' is more directed. And being that I think it's debug-only, you could further do (toolkit == 'gonk' && debug) to skip only on debug emulator :)
Attachment #8634592 - Flags: review?(ryanvm) → review+
Keywords: leave-open
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
I did not notice this failure. I think bug 1223658 caused the recent failures.
Flags: needinfo?(hiikezoe)
I can imagine the failure reason on B2G.  The reason is that waiting for an animation.ready is not sufficient, in particular in the case that compositor runs on a different process.  I think we should have wait for a MozAfterPaint before proceeding the test.
I think we do still need it, but before doing it, I need to know why 1223658 triggers the recent failures.  I've tried to reproduce the failure locally, but not yet.  Though I did try with --repeat=1000!
Flags: needinfo?(hiikezoe)
OK, I realized a reason of the failure triggered by bug 1223658, especially https://hg.mozilla.org/mozilla-central/rev/00799db70975 (passed fill mode to the compositor).  We had been using fill:both on the compositor before bug 1223658, that means even if the initial time stamp on the compositor is less than the animation start time, the initial animation style was rendered.  Yes, this is also related to bug 1208411.  The time stamp on the compositor delays more frequent than before.

A solution I can think of is to wait for MozAfterPaint instead of animation.ready.
Just replacing animation.ready with waitForAllPaints() was not sufficient.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61343cb9a040

That's because there is another async_test, so sometimes a different MozAfterPaint event was received.
Rewriting async_test with promise_test seems good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ced219334763&duplicate_jobs=visible
Keywords: leave-open
Comment on attachment 8804587 [details]
Bug 1168759 - Part 1: Rewrite async_test with promise_test.

https://reviewboard.mozilla.org/r/88496/#review87880
Attachment #8804587 - Flags: review?(boris.chiou) → review+
Comment on attachment 8804587 [details]
Bug 1168759 - Part 1: Rewrite async_test with promise_test.

https://reviewboard.mozilla.org/r/88496/#review87898

::: dom/animation/test/mozilla/file_deferred_start.html:55
(Diff revision 1)
> -    animation.ready.then(function() {
> +    return animation.ready.then(function() {
>        promiseCallbackDone = true;
>      }).catch(function() {
>        assert_unreached('ready promise was rejected');
>      });
>  
>    // We need to wait for up to three frames. This is because in some
>    // cases it can take up to two frames for the initial layout
>    // to take place. Even after that happens we don't actually resolve the
>    // ready promise until the following tick.
>    })
>    .then(waitForFrame)
>    .then(waitForFrame)
>    .then(waitForFrame)

Just wonder do we still need to wait for three frames if we change to reteurn a promise? Thanks.
(In reply to Boris Chiou [:boris] from comment #131)
> Comment on attachment 8804587 [details]
> Bug 1168759 - Part 1: Rewrite async_test with promise_test.
> 
> https://reviewboard.mozilla.org/r/88496/#review87898
> 
> ::: dom/animation/test/mozilla/file_deferred_start.html:55
> (Diff revision 1)
> > -    animation.ready.then(function() {
> > +    return animation.ready.then(function() {
> >        promiseCallbackDone = true;
> >      }).catch(function() {
> >        assert_unreached('ready promise was rejected');
> >      });
> >  
> >    // We need to wait for up to three frames. This is because in some
> >    // cases it can take up to two frames for the initial layout
> >    // to take place. Even after that happens we don't actually resolve the
> >    // ready promise until the following tick.
> >    })
> >    .then(waitForFrame)
> >    .then(waitForFrame)
> >    .then(waitForFrame)
> 
> Just wonder do we still need to wait for three frames if we change to
> reteurn a promise? Thanks.

Indeed! Good catch! We should return a promise there.  I will revise it. Thanks!  Though I don't know why the try did not fail without waiting for the three frames.
Comment on attachment 8804588 [details]
Bug 1168759 - Part 2: Use waitForAllPaints() instead of animation.ready.

https://reviewboard.mozilla.org/r/88498/#review87900
Attachment #8804588 - Flags: review?(boris.chiou) → review+
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/autoland/rev/bc6a7847f200
Part 1: Rewrite async_test with promise_test. r=boris
https://hg.mozilla.org/integration/autoland/rev/986b18fdfdba
Part 2: Use waitForAllPaints() instead of animation.ready. r=boris
https://hg.mozilla.org/mozilla-central/rev/bc6a7847f200
https://hg.mozilla.org/mozilla-central/rev/986b18fdfdba
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
OS: Gonk (Firefox OS) → Unspecified
No longer blocks: 1426017
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: