Closed Bug 1450845 Opened 6 years ago Closed 6 years ago

Assertion failure: mSeekPromise.IsEmpty() (No sample requests allowed while seeking), at /builds/worker/workspace/build/src/dom/media/MediaFormatReader.cpp:1806

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
59 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: jkratzer, Assigned: bryce)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 2 obsolete files)

trigger.html
967 bytes, text/html
Details
Bug 1450845 - MediaDecoderStateMachine now ignores SeekToNextFrame if already seeking.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1450845 - Add crashtest for seekToNextFrame when a seek is already in progress.
59 bytes, text/x-review-board-request
jya
: review+
Details
Attached file trigger.html — 
Testcase found while fuzzing mozilla-central rev c44f60c43432.

rax = 0x0000000000000000   rdx = 0x0000000000000000
rcx = 0x00007f1d3f2042dd   rbx = 0x00007f1d20cd3000
rsi = 0x00007f1d3f4d3770   rdi = 0x00007f1d3f4d2540
rbp = 0x00007f1d1f75f5b0   rsp = 0x00007f1d1f75f570
r8 = 0x00007f1d3f4d3770    r9 = 0x00007f1d1f760700
r10 = 0x0000000000000012   r11 = 0x0000000000000000
r12 = 0x00007f1d1f75f5c0   r13 = 0x00007f1d1f502ab0
r14 = 0x00007f1d1f502ee0   r15 = 0x00007f1d1f502fd0
rip = 0x00007f1d2f25177b
OS|Linux|0.0.0 Linux 4.4.0-103-generic #126-Ubuntu SMP Mon Dec 4 16:23:28 UTC 2017 x86_64
CPU|amd64|family 6 model 60 stepping 3|1
GPU|||
Crash|SIGSEGV|0x0|23
23|0|libxul.so|mozilla::MediaFormatReader::RequestVideoData|hg:hg.mozilla.org/mozilla-central:dom/media/MediaFormatReader.cpp:c44f60c43432d468639b5fe078420e60c13fd3de|1806|0x18
23|1|libxul.so|mozilla::detail::ProxyRunnable<mozilla::MozPromise<RefPtr<mozilla::VideoData>, mozilla::MediaResult, true>, RefPtr<mozilla::MozPromise<RefPtr<mozilla::VideoData>, mozilla::MediaResult, true> > (mozilla::MediaFormatReader::*)(const mozilla::media::TimeUnit&), mozilla::MediaFormatReader, StoreCopyPassByRRef<mozilla::media::TimeUnit> >::Run|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.h:c44f60c43432d468639b5fe078420e60c13fd3de|1164|0x8
23|2|libxul.so|mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run|hg:hg.mozilla.org/mozilla-central:xpcom/threads/TaskDispatcher.h:c44f60c43432d468639b5fe078420e60c13fd3de|214|0x11
23|3|libxul.so|mozilla::TaskQueue::Runner::Run|hg:hg.mozilla.org/mozilla-central:xpcom/threads/TaskQueue.cpp:c44f60c43432d468639b5fe078420e60c13fd3de|243|0x6
23|4|libxul.so|nsThreadPool::Run|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadPool.cpp:c44f60c43432d468639b5fe078420e60c13fd3de|228|0x12
23|5|libxul.so|nsThread::ProcessNextEvent|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:c44f60c43432d468639b5fe078420e60c13fd3de|1096|0x15
23|6|libxul.so|NS_ProcessNextEvent|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:c44f60c43432d468639b5fe078420e60c13fd3de|519|0x11
23|7|libxul.so|mozilla::ipc::MessagePumpForNonMainThreads::Run|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:c44f60c43432d468639b5fe078420e60c13fd3de|334|0xa
23|8|libxul.so|MessageLoop::RunInternal|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:c44f60c43432d468639b5fe078420e60c13fd3de|326|0x17
23|9|libxul.so|MessageLoop::Run|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:c44f60c43432d468639b5fe078420e60c13fd3de|319|0x8
23|10|libxul.so|nsThread::ThreadFunc|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:c44f60c43432d468639b5fe078420e60c13fd3de|425|0x8
23|11|libnspr4.so|_pt_root|hg:hg.mozilla.org/mozilla-central:nsprpub/pr/src/pthreads/ptthread.c:c44f60c43432d468639b5fe078420e60c13fd3de|201|0x7
23|12|libpthread-2.23.so||||0x76ba
23|13|libc-2.23.so||||0x10741d
Flags: in-testsuite?
Component: Audio/Video → Audio/Video: Playback
Bryce do you have time to take a look at this?
Rank: 20
Flags: needinfo?(bvandyk)
Priority: -- → P2
Can do.
Assignee: nobody → bvandyk
Flags: needinfo?(bvandyk)
Observations as I go:

The test case can be further reduced to:

```
<html>
  <head>
    <script>
      function start() {
        document.getElementById('video').play();
        document.getElementById('video').seekToNextFrame();
      }
      window.addEventListener('load', start)
    </script>
  </head>
  <body>
    <video id='video' src='data:video/webm;base64,GkXfowEAAAAAAAAfQoaBAUL3gQFC8oEEQvOBQoWBAhhTgGcBAAAAAAAB6BFNm3RALE27i1OrhBVJqWZTrIHfTbuMU6uEFlSua1OsggEwTbuMU6uEHFO7a1OsggHL7AEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAVSalmAQAAAAAAAEUqTYCNTGF2ZjU3LjI5LjEwMVdBjUxhdmY1Ny4yOS4xMDFzpJBAb17Yv2oNAF1ZEESuco33RImIQFCAAAAAAAAWVK5rAQAAAAAAADyuAQAAAAAAADPXgQFzxYEBnIEAIrWcg3VuZIaFVl9WUDmDgQEj44OEAfygVeABAAAAAAAAB7CCAUC6gfAfQ7Z1AQAAAAAAAEfngQCjqYEAAICCSYNCABPwDvYAOCQcGFQAAFBh9jAAABML7AAATEnjdRwIJ+gAo5eBACEAhgBAkpwATEAABCasAABekcXgAB'>
  </body>
</html>
```

- The order of the play and seekToNextFrame don't matter, the seekToNextFrame can precede the play and this still breaks.
- Repros with other sources.
- Other seek functions don't seem to cause issue.
- If we wait for the play promise to resolve then we're okay.
- The problem appears specific to when the SeekTarget::NextFrame arg used to seek[0].

Looks like this worked for a little bit and broke between 2017-01-17 (central changeset 3e275d37a06236981bff399b7d7aa0646be3fee7) and 2017-01-18 (central changeset 80eac484366ad881c6a10bf81e8d9b8f7a676c75)[1]. And appears to stem from the changes to dormant state here[2].

So seems there's something unhappy going on between the dormant state and the seek to next frame state. Will continue to dig about.

[0]: https://searchfox.org/mozilla-central/rev/bf4def01bf8f6ff0d18f02f2d7e9efc73e12c63f/dom/media/MediaDecoder.cpp#524
[1]: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3e275d37a06236981bff399b7d7aa0646be3fee7&tochange=80eac484366ad881c6a10bf81e8d9b8f7a676c75
[2]: https://hg.mozilla.org/mozilla-central/rev/09fb98312335
Looks like the problem is that Dormant state sets up an accurate seek task[0]. There is a special state to move from Dormant to SeekToNextFrame[1], but if we play first we don't end up passing through this state. Instead the accurate seek the Dormant state sets up ends up being processed as we transition to play, and queues a seek with the MediaFormatReader which is still in flight when the NextFrameSeek takes place. Since the NextFrameSeek requests video data while the accurate seek is still pending we get the failure seen for this bug.

Normally HTMLMediaElement gates SeekToNextFrame if we have another seek pending[2], but since a seek hasn't actively been called on the element we pass this check for this case. We could also prevent SeekToNext frame if we have pending play promises to fix this bug. However, I think there will remain issues where if we SeekToNext frame while a video is playing we can end up with flakiness. See the following example for another crash:

```
<html>
  <head>
    <script>
      async function start() {
        await document.getElementById('video').play();
        for (let i = 0; i < 1000; i++) {
          await document.getElementById('video').seekToNextFrame();
        }
      }
      window.addEventListener('load', start);
    </script>
  </head>
  <body>
    <video id='video' src='https://hg.mozilla.org/mozilla-central/raw-file/tip/dom/media/test/bipbop_480wp_663kbps.mp4'>
  </body>
</html>
```

I see two approaches to dealing with these problems:
- Further restrict when SeekToNextFrame can be used to when we are paused.
- Rework the MediaDecoderStateMachine to more robustly handle SeekToNextFrame.

I'm not sure it makes sense to SeekToNextFrame while playing a video (what's the use case?), so favour the first approach above.

[0]: https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/dom/media/MediaDecoderStateMachine.cpp#401
[1]: https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/dom/media/MediaDecoderStateMachine.cpp#1748
[2]: https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/dom/html/HTMLMediaElement.cpp#2715
Comment on attachment 8983045 [details]
Bug 1450845 - HTMLMediaElement.seekToNextFrame now fails if not called on paused element.

https://reviewboard.mozilla.org/r/248910/#review255404

I can't see any definition that seekToNextFrame can only be performed once the element is paused(), it would be rather counter-intuitive to do that especially when normal seeking doesn't. 
I agree that there's almost no use case to using seekToNextFrame while the media is playing.

However, that approach seems to be a bit hackish and going around the problem without fixing the core issue. The proper fix is to fix the state handling in MDSM. And I would prefer that you did that.

If coming from dormant, seektonextframe and normal seek should have exactly the same behaviour I think

::: dom/html/HTMLMediaElement.cpp:2715
(Diff revision 1)
>  }
>  
>  already_AddRefed<Promise>
>  HTMLMediaElement::SeekToNextFrame(ErrorResult& aRv)
>  {
> +  if (!mPaused) {

if the current element is seeking, it would be paused.

as such, this test needs to be done after the test of if (mSeekDOMPromise)

::: dom/html/HTMLMediaElement.cpp:2716
(Diff revision 1)
>  
>  already_AddRefed<Promise>
>  HTMLMediaElement::SeekToNextFrame(ErrorResult& aRv)
>  {
> +  if (!mPaused) {
> +    RefPtr<Promise> promise = CreateDOMPromise(aRv);

you must check that the creation of the promise succeeded.
Comment on attachment 8983045 [details]
Bug 1450845 - HTMLMediaElement.seekToNextFrame now fails if not called on paused element.

https://reviewboard.mozilla.org/r/248910/#review255404

There is indeed nothing inherant to seekToNext frame at the moment that means it shouldn't be used if playing. I would update our docs on the matter should we pursue this route.

I will take another look at reworking the MDSM to handle this internally. I will revisit this changeset should that not be bearing fruit.
Bug 1410225 contains some changes to prevent a prior issue in the same class as this. However, it doesn't catch all cases. I'm current exploring having the MDSM handles these cases with the hope that it may also handle a wider range of cases.
See Also: → 1410225
Comment on attachment 8983045 [details]
Bug 1450845 - HTMLMediaElement.seekToNextFrame now fails if not called on paused element.

https://reviewboard.mozilla.org/r/248910/#review256164

as discussed earlier.
Let's do that via better state handing in the MDSM...
Attachment #8983045 - Flags: review?(jyavenard) → review-
Comment on attachment 8983046 [details]
Bug 1450845 - Add test to ensure seekToNextFrame rejects if a media element is playing.

https://reviewboard.mozilla.org/r/248912/#review256166
Attachment #8983046 - Flags: review?(jyavenard) → review-
Attachment #8983045 - Attachment is obsolete: true
Attachment #8983046 - Attachment is obsolete: true
:jya, could you let me know what you think of the revised patches when you have a moment?
Flags: needinfo?(jyavenard)
Comment on attachment 8984191 [details]
Bug 1450845 - MediaDecoderStateMachine now ignores SeekToNextFrame if already seeking.

https://reviewboard.mozilla.org/r/249990/#review258682

::: dom/media/MediaDecoderStateMachine.cpp:884
(Diff revision 1)
> +  {
> +    if (aTarget.IsNextFrame()) {
> +      // We ignore next frame seeks if we already have a seek pending
> +      SLOG("Already SEEKING, ignoring seekToNextFrame");
> +      MOZ_ASSERT(!mSeekJob.mPromise.IsEmpty(), "Seek shouldn't be finished");
> +      return MediaDecoder::SeekPromise::CreateAndReject(/* aIgnored = */ true,

Rather than returning a rejected promise, can't we chain a promise to the existing pending one instead?
so that both will be resolved at the same time.

may have to change the seek promise to not be exclusive.

Having said that, how can we get here if HTMLMediaElement::SeekToNextFrame already returns the existing pending seek promise?
Attachment #8984191 - Flags: review?(jyavenard) → review+
Comment on attachment 8984192 [details]
Bug 1450845 - Add crashtest for seekToNextFrame when a seek is already in progress.

https://reviewboard.mozilla.org/r/249992/#review258684

::: dom/media/test/crashtests/1450845.html:6
(Diff revision 1)
> +<!DOCTYPE html>
> +<html class="reftest-wait">
> +<head>
> +  <title>Bug 1450845: Avoid seek to next frame when already seeking</title>
> +  <script>
> +    async function boom() {

crashboomboom?

::: dom/media/test/crashtests/1450845.html:14
(Diff revision 1)
> +      // Internally play causes a seek, make sure we don't crash during this
> +      video.play();
> +      try {
> +        await document.getElementById('video').seekToNextFrame();
> +      } catch (e) {
> +        // We don't mind if the promise was rejected so long as we don't crash

I think it would be nicer if we called seektonextframe while a seek is pending to return the same promise again
Attachment #8984192 - Flags: review?(jyavenard) → review+
(In reply to Bryce Van Dyk (:bryce) from comment #15)
> :jya, could you let me know what you think of the revised patches when you
> have a moment?

sorry for this delay, I had left it at my last r- , didn't see that it had been updated.
Flags: needinfo?(jyavenard)
Comment on attachment 8984191 [details]
Bug 1450845 - MediaDecoderStateMachine now ignores SeekToNextFrame if already seeking.

https://reviewboard.mozilla.org/r/249990/#review258682

> Rather than returning a rejected promise, can't we chain a promise to the existing pending one instead?
> so that both will be resolved at the same time.
> 
> may have to change the seek promise to not be exclusive.
> 
> Having said that, how can we get here if HTMLMediaElement::SeekToNextFrame already returns the existing pending seek promise?

Internally the MDSM sets up a seek when `play()` is called. Since this isn't surfaced to HTMLMediaElement, it doesn't know it shouldn't run seekToNextFrame while that seek is pending and causes the bug here. Here we essentially do the same thing the element was doing, but in the state machine (and we reject), which does know when the play seek is happening.

I'll investigate chaining the promises. The reason I did this is that the seekToNextFrame will not happen if we have another seek, and want to indicate to the user that their seek is not going to happen. It's a pretty niche case that they'd need the seek to nextFrame to happen before the other seek though (or after, don't know what that behaviour should look like), and this is experimental, so not particuarly fussed.
After more discussion bug, 1470482 has been created as a follow up to discuss nailing down behaviour of seekToNextFrame. Going to land this as is to get the issue resolved in tree.
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be60095b4e99
MediaDecoderStateMachine now ignores SeekToNextFrame if already seeking. r=jya
https://hg.mozilla.org/integration/autoland/rev/b369f6cf661f
Add crashtest for seekToNextFrame when a seek is already in progress. r=jya
https://hg.mozilla.org/mozilla-central/rev/be60095b4e99
https://hg.mozilla.org/mozilla-central/rev/b369f6cf661f
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
status-firefox60: --- → wontfix
status-firefox61: --- → wontfix
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → wontfix
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: