Closed Bug 1235723 Opened 7 years ago Closed 7 years ago

Intermittent browser_animation_playerState.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/server/tests/browser/browser_animation_playerState.js:93 - TypeError: player is undefined

Categories

(DevTools :: General, defect)

Product:
DevTools
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: philor, Unassigned)

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

MozReview Request: Bug 1235723 - Part 1: Use longer duration to avoid finishing animations at some point. r?pbrosset
58 bytes, text/x-review-board-request
pbro
: review+
Details
MozReview Request: Bug 1235723 - Part 2: Check that the short animation is surely finished before proceeding test. r?pbrosset
58 bytes, text/x-review-board-request
pbro
: review+
Details
Flags: needinfo?(hiikezoe)
Patrick, this orange gets more frequent in these days because Telemetry or some other components spew lots of exceptions.  An instance is https://treeherder.mozilla.org/logviewer.html#?job_id=17012443&repo=try

We should use longer duration for animations in devtools tests.
Flags: needinfo?(hiikezoe)
Comment on attachment 8721886 [details]
MozReview Request: Bug 1235723 - Part 1: Use longer duration to avoid finishing animations at some point. r?pbrosset

https://reviewboard.mozilla.org/r/35795/#review32445

Thanks for working on this.
I think all changes are good except for one that will cause browser_animation_keepFinished.js not to cover a specific case anymore.

::: devtools/server/tests/browser/animation.html:67
(Diff revision 1)
> -    animation: move 1s;
> +    animation: move 100s;

The problem with making this particular animation longer is that there's one test that waits for it to be finished:

\devtools\server\tests\browser\browser_animation_keepFinished.js

This test adds the animation to a node, and then waits for 2 seconds to make sure it is finished, and then checks that we have received the right number of mutations.

With this change, the test will still pass, but for the wrong reason.

Indeed, the animation-inspector tool does not send the "removed" mutation events for animations that can still be used in the tool, so even when they are finished, we just capture the event on the server-side, and don't send it to the toolbox. That's why we must make sure that this animation remains short, so that we can add it, get the "added" mutation, then wait for some time, and verify that we didn't get the "removed" mutation.
Attachment #8721886 - Flags: review?(pbrosset)
Comment on attachment 8721886 [details]
MozReview Request: Bug 1235723 - Part 1: Use longer duration to avoid finishing animations at some point. r?pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35795/diff/1-2/
Attachment #8721886 - Attachment description: MozReview Request: Bug 1235723 - Use longer duration to avoid finishing animations at some point. r?pbrosset → MozReview Request: Bug 1235723 - Part 1: Use longer duration to avoid finishing animations at some point. r?pbrosset
Attachment #8721886 - Flags: review?(pbrosset)
https://reviewboard.mozilla.org/r/35795/#review32445

> The problem with making this particular animation longer is that there's one test that waits for it to be finished:
> 
> \devtools\server\tests\browser\browser_animation_keepFinished.js
> 
> This test adds the animation to a node, and then waits for 2 seconds to make sure it is finished, and then checks that we have received the right number of mutations.
> 
> With this change, the test will still pass, but for the wrong reason.
> 
> Indeed, the animation-inspector tool does not send the "removed" mutation events for animations that can still be used in the tool, so even when they are finished, we just capture the event on the server-side, and don't send it to the toolbox. That's why we must make sure that this animation remains short, so that we can add it, get the "added" mutation, then wait for some time, and verify that we didn't get the "removed" mutation.

Thanks.  I did not notice the eole of short animation at all!  This patch dropped the change for the short animation.  And a subsequent patch does check that the short animation is surely finished to avoid the mistake just like I did. :-)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> https://reviewboard.mozilla.org/r/35795/#review32445
> 
> > The problem with making this particular animation longer is that there's one test that waits for it to be finished:
> > 
> > \devtools\server\tests\browser\browser_animation_keepFinished.js
> > 
> > This test adds the animation to a node, and then waits for 2 seconds to make sure it is finished, and then checks that we have received the right number of mutations.
> > 
> > With this change, the test will still pass, but for the wrong reason.
> > 
> > Indeed, the animation-inspector tool does not send the "removed" mutation events for animations that can still be used in the tool, so even when they are finished, we just capture the event on the server-side, and don't send it to the toolbox. That's why we must make sure that this animation remains short, so that we can add it, get the "added" mutation, then wait for some time, and verify that we didn't get the "removed" mutation.
> 
> Thanks.  I did not notice the eole of short animation at all!  This patch

'the role'.
Comment on attachment 8721886 [details]
MozReview Request: Bug 1235723 - Part 1: Use longer duration to avoid finishing animations at some point. r?pbrosset

https://reviewboard.mozilla.org/r/35795/#review32647
Attachment #8721886 - Flags: review?(pbrosset) → review+
Comment on attachment 8722305 [details]
MozReview Request: Bug 1235723 - Part 2: Check that the short animation is surely finished before proceeding test. r?pbrosset

https://reviewboard.mozilla.org/r/35975/#review32649
Attachment #8722305 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/883d9427c0ab
https://hg.mozilla.org/mozilla-central/rev/f3c559370097
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
All of oranges in the last week happened before the patches has been landed in mozilla-central.

The last failure revision is:
https://hg.mozilla.org/mozilla-central/rev/4f64ec5486f4
 push date	2016-02-25 09:42 +0000

The push date of the land is:
 push date	2016-02-25 10:58 +0000
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.