1276530 - Intermittent test_texttrackevents_video.html | TrackElement should fire cue change 6 times. - got 2, expected 6

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P5)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

13:23:42     INFO -  1836 INFO TEST-START | dom/media/test/test_texttrackevents_video.html
13:23:42     INFO -  [376] WARNING: no triggering principal available via loadInfo, assuming load is cross-origin: file c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/netwerk/protocol/http/HttpBaseChannel.cpp, line 1410
13:23:42     INFO -  [376] WARNING: no triggering principal available via loadInfo, assuming load is cross-origin: file c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/netwerk/protocol/http/HttpBaseChannel.cpp, line 1410
13:23:42     INFO -  ++DOMWINDOW == 28 (175B8400) [pid = 376] [serial = 694] [outer = 1CB82400]
13:23:45     INFO -  TEST-INFO | started process screenshot
13:23:45     INFO -  TEST-INFO | screenshot: exit 0
13:23:45     INFO -  1837 INFO TEST-PASS | dom/media/test/test_texttrackevents_video.html | Track::ReadyState should be set to LOADED.
13:23:45     INFO -  1838 INFO TEST-PASS | dom/media/test/test_texttrackevents_video.html | Track::OnCueChange should exist.
13:23:45     INFO -  1839 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_texttrackevents_video.html | TrackElement should fire cue change 6 times. - got 2, expected 6
13:23:45     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:268:5
13:23:45     INFO -      @dom/media/test/test_texttrackevents_video.html:56:7
13:23:45     INFO -      EventListener.handleEvent*@dom/media/test/test_texttrackevents_video.html:53:5
13:23:45     INFO -      Async*@dom/media/test/test_texttrackevents_video.html:17:1
13:23:45     INFO -  Not taking screenshot here: see the one that was previously logged
13:23:45     INFO -  1840 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_texttrackevents_video.html | TrackElement.track should fire cue change 6 times. - got 2, expected 6
13:23:45     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:268:5
13:23:45     INFO -      @dom/media/test/test_texttrackevents_video.html:57:7
13:23:45     INFO -      EventListener.handleEvent*@dom/media/test/test_texttrackevents_video.html:53:5
13:23:45     INFO -      Async*@dom/media/test/test_texttrackevents_video.html:17:1

Comment 1

[Intermittent Failures Robot]

19 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 10
* try: 4
* fx-team: 3
* mozilla-central: 2

Platform breakdown:
* windows7-32: 19

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1276530&startday=2016-05-30&endday=2016-06-05&tree=all

Comment 2

[Intermittent Failures Robot]

19 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 11
* try: 2
* mozilla-central: 2
* fx-team: 2
* mozilla-aurora: 1
* ash: 1

Platform breakdown:
* windows7-32: 19

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1276530&startday=2016-06-06&endday=2016-06-12&tree=all

Comment 3

[Benjamin Chen [:bechen]]

See https://html.spec.whatwg.org/multipage/embedded-content.html#time-marches-on
If the playback position is not smooth, step 15 will remove the duplicates if there are multiple oncuechange events on the same TextTrack object.
I will modify the testcase by adding onenter/onexit events.

Comment 4

[Benjamin Chen [:bechen]]

Attachment: Bug 1276530 - Fix test_texttrackevents_video.html.

Review commit: https://reviewboard.mozilla.org/r/59138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59138/

Comment 5

[Benjamin Chen [:bechen]]

INFO -  1934 INFO TEST-PASS | dom/media/test/test_texttrackevents_video.html | Track::ReadyState should be set to LOADED.
INFO -  1935 INFO TEST-PASS | dom/media/test/test_texttrackevents_video.html | Track::OnCueChange should exist.
INFO -  1936 INFO TEST-PASS | dom/media/test/test_texttrackevents_video.html | textTrack.cues.length should 3.
INFO -  1937 INFO TEST-PASS | dom/media/test/test_texttrackevents_video.html | TrackElement should fire cue change at least one times.
INFO -  1938 INFO TEST-PASS | dom/media/test/test_texttrackevents_video.html | trackElementCueChangeCount should <= 6
INFO -  1939 INFO TEST-PASS | dom/media/test/test_texttrackevents_video.html | TrackElement.track should fire cue change at least one times.
INFO -  1940 INFO TEST-PASS | dom/media/test/test_texttrackevents_video.html | trackCueChangeCount should <= 6
INFO -  1941 INFO TEST-PASS | dom/media/test/test_texttrackevents_video.html | cueEnterCount should fire three times.
INFO -  1942 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_texttrackevents_video.html | cueExitCount should fire three times. - got 1, expected 3

There are 3 onenter events but only 1 onexit events, maybe there is a bug in TimeMarchesOn().

Comment 6

[Benjamin Chen [:bechen]]

(In reply to Benjamin Chen [:bechen] from comment #5)

> 
> There are 3 onenter events but only 1 onexit events, maybe there is a bug in
> TimeMarchesOn().

https://dxr.mozilla.org/mozilla-central/source/dom/html/TextTrackManager.cpp#663

The cue might not has id.

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8762462 [details]
Bug 1276530 - Fix test_texttrackevents_video.html.

https://reviewboard.mozilla.org/r/59138/#review56178

Valgrind failure on try looks unrelated.

::: dom/media/test/test_texttrackevents_video.html:80
(Diff revision 1)
>  
>      video.addEventListener('ended', function() {
>        // Should be fired 6 times, as there are 3 cues, with a change event
>        // for when it is activated/deactivated (6 events total)
> -      is(trackElementCueChangeCount, 6, "TrackElement should fire cue change 6 times.");
> -      is(trackCueChangeCount, 6, "TrackElement.track should fire cue change 6 times.");
> +      isnot(trackElementCueChangeCount, 0, "TrackElement should fire cue change at least one times.");
> +      ok(trackElementCueChangeCount <= 6, 'trackElementCueChangeCount should <= 6');

Should be "at least one time." Singular 'time' to agree with 'one'.

Comment 8

[Benjamin Chen [:bechen]]

Attachment: Bug 1276530 - part2: The id of cue might be empty, so don't use it for checking.

Review commit: https://reviewboard.mozilla.org/r/59200/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59200/

Comment 9

[Benjamin Chen [:bechen]]

Comment on attachment 8762462 [details]
Bug 1276530 - Fix test_texttrackevents_video.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59138/diff/1-2/

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8762621 [details]
Bug 1276530 - part2: The id of cue might be empty, so don't use it for checking.

https://reviewboard.mozilla.org/r/59200/#review56384

Comment 11

[Benjamin Chen [:bechen]]

Comment on attachment 8762462 [details]
Bug 1276530 - Fix test_texttrackevents_video.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59138/diff/2-3/

Comment 12

[Benjamin Chen [:bechen]]

Comment on attachment 8762621 [details]
Bug 1276530 - part2: The id of cue might be empty, so don't use it for checking.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59200/diff/1-2/

Comment 13

[Carsten Book [:Tomcat]]

failed to apply:

applying fa5b89817264
patching file dom/media/TextTrackCue.h
Hunk #1 FAILED at 297
1 out of 1 hunks FAILED -- saving rejects to file dom/media/TextTrackCue.h.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

could you take a look, thanks!

Comment 14

[Benjamin Chen [:bechen]]

Comment on attachment 8762462 [details]
Bug 1276530 - Fix test_texttrackevents_video.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59138/diff/3-4/

Comment 15

[Benjamin Chen [:bechen]]

Comment on attachment 8762621 [details]
Bug 1276530 - part2: The id of cue might be empty, so don't use it for checking.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59200/diff/2-3/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b7466d93f7
Fix test_texttrackevents_video.html. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/21e3d0701b3d
part2: The id of cue might be empty, so don't use it for checking. r=rillian

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c9b7466d93f7
https://hg.mozilla.org/mozilla-central/rev/21e3d0701b3d

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Can you please nominate this for Aurora uplift when you get a chance? Also, Part 2 will need rebasing if/when it's approved.

Comment 19

[Benjamin Chen [:bechen]]

Attachment: bug1276530part1fixtestcase.aurora.patch

Approval Request Comment
[Feature/regressing bug #]: none
[User impact if declined]: none
[Describe test coverage new/current, TreeHerder]: test_texttrackevents_video.html
[Risks and why]: low risk because testcase is wrong
[String/UUID change made/needed]: none

Comment 20

[Benjamin Chen [:bechen]]

Attachment: bug1276530part2cueid.aurora.patch

Approval Request Comment
[Feature/regressing bug #]:882718
[User impact if declined]: onenter/onexit events might lost.
[Describe test coverage new/current, TreeHerder]:test_texttrackevents_video.html
[Risks and why]: low.
[String/UUID change made/needed]: none

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8765353 [details] [diff] [review]
bug1276530part1fixtestcase.aurora.patch

Test fixes, ok to uplift

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8765356 [details] [diff] [review]
bug1276530part2cueid.aurora.patch

Fix for video events, ok to uplift to aurora along with the changed tests.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/87af6e5c29e8
https://hg.mozilla.org/releases/mozilla-aurora/rev/b9faa10c9918