Seek promise might be rejected in test_seekToNextFrame.html

RESOLVED FIXED in Firefox 55

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
Bug 1318226 fixes the order of regular tasks and reveals a bug in test_seekToNextFrame.html which makes a wrong assumption about the event order.

For the last seek, the seek promise is resolved before the 'ended' event is fired. The test seeks again seeing v.seenEnded is false and the seek promise will be rejected because the 'ended' event handler calls |removeNodeAndSource(v)| which will shut down the MediaDecoder and reject the pending seek promise.
(Assignee)

Updated

9 months ago
Assignee: nobody → jwwang
Blocks: 1345711
Priority: -- → P3
Comment hidden (mozreview-request)

Comment 2

9 months ago
mozreview-review
Comment on attachment 8845268 [details]
Bug 1345713 - the seek promise might be rejected because the 'ended' event handler shut down the decoder.

https://reviewboard.mozilla.org/r/118442/#review120444

::: dom/media/test/test_seekToNextFrame.html:42
(Diff revision 1)
>          if (!v.seenEnded)
>            callSeekToNextFrame();
>        },
>        () => {
> -        ok(false, "seekToNextFrame() failed.");
> +        // removeNodeAndSource(v) will shutdown the MediaDecoder and reject
> +        // the seek promise. This should the only reason seek might fail.

This shoule "be" the only...

or

This "is" the only ...

::: dom/media/test/test_seekToNextFrame.html:43
(Diff revision 1)
>            callSeekToNextFrame();
>        },
>        () => {
> -        ok(false, "seekToNextFrame() failed.");
> +        // removeNodeAndSource(v) will shutdown the MediaDecoder and reject
> +        // the seek promise. This should the only reason seek might fail.
> +        ok(v.finished, "seekToNextFrame() failed.");

How about let's explicitly write down the logic?

```
if (v.finished) {
  // While seeking to the end, the last seekToNextFrame() operation is resolved before the ended event is received. 
  // The resolving-callback calls seekToNextFrame() again and then the onEnded() calls removeNodeAndSource() which destories the media decoder. 
  // By this way, the newly invoked seekToNextFrame() will be rejected, so we get here and this is should not be treated as an error.
  ok(true);
} else {
  ok(false, "seekToNextFrame() failed.");
}
```
Attachment #8845268 - Flags: review?(kaku) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 4

9 months ago
Thanks!

Comment 5

9 months ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2d92403096a
the seek promise might be rejected because the 'ended' event handler shut down the decoder. r=kaku

Comment 6

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c2d92403096a
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.