1337265 - Intermittent dom/media/test/test_playback_errors.html | application crashed [@ mozilla::MediaFormatReader::DecoderFactory::RunStage] after Assertion failure: !data.mDecoder

Status: RESOLVED DUPLICATE of bug 1340969

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

Description

[Treeherder Bug Filer]

Filed by: philringnalda [at] gmail.com

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

https://queue.taskcluster.net/v1/task/bkEvvh3KRpCsFxvAqjeVvQ/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Fallout of bug 1319987?

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1337265: Properly reset state when shutting down decoder.

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

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834436 [details]
Bug 1337265: Properly reset state when shutting down decoder.

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

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8834436 [details]
Bug 1337265: Properly reset state when shutting down decoder.

https://reviewboard.mozilla.org/r/110386/#review111856

::: dom/media/MediaFormatReader.cpp:259
(Diff revision 2)
>  {
>    MOZ_ASSERT(aTrack == TrackInfo::kAudioTrack
>               || aTrack == TrackInfo::kVideoTrack);
> +
> +  auto& data = aTrack == TrackInfo::kAudioTrack ? mAudio : mVideo;
> +  if (data.mStage == Stage::CreateDecoder

https://hg.mozilla.org/integration/mozilla-inbound/file/f0453084d86e87070b2894eab61a2b58f1964768/dom/media/MediaFormatReader.cpp#l339

I don't think this is a right fix.

1. The check for |data.mStage == Stage::WaitForInit| doesn't help because we failed the assertion at #339 where data.mStage is Stage::CreateDecoder.
2. The check for |data.mStage == Stage::CreateDecoder| doesn't help either because the current call flow [a] ensures data.mStage is never Stage::CreateDecoder inside MediaFormatReader::DecoderFactory::CreateDecoder().

[a] data.mStage is changed to Stage::CreateDecoder at #320 and the following RunStage() changes it to Stage::WaitForInit immediately.

Comment 5

[Jean-Yves Avenard [:jya]]

well, you can take it then

Comment 6

[Jean-Yves Avenard [:jya]]

oh doh !

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834436 [details]
Bug 1337265: Properly reset state when shutting down decoder.

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

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8834436 [details]
Bug 1337265: Properly reset state when shutting down decoder.

https://reviewboard.mozilla.org/r/110386/#review112218

::: dom/media/MediaFormatReader.cpp:219
(Diff revision 3)
>        // return the existing promise.
>        data.mShutdownRequest.Disconnect();
>        RefPtr<ShutdownPromise> p = data.mShutdownPromise.forget();
>        return p;
>      }
> +    data.mStage = Stage::None;

I can't see how this will help.

Assertion failure: !data.mDecoder, at /home/worker/workspace/build/src/dom/media/MediaFormatReader.cpp:339

#220 clear data.mDecoder so it is impossible to fail the assertion if this function is ever called.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d0134/dom/media/MediaFormatReader.cpp#202

Can we just ignore the return value of MediaDataDecoder::Shutdown() in this case? It appears to me that MFR only cares about when to call Shutdown() on a MediaDataDecoder instead of when Shutdown() is done.

http://searchfox.org/mozilla-central/rev/672c83ed65da286b68be1d02799c35fdd14d0134/dom/media/MediaFormatReader.cpp#2625

Likewise, ReleaseResources() also ignores the shutdown promise returned by MediaDataDecoder::Shutdown().

We will be able to make the shutdown sequence much easier if we can ignore the return value of MediaDataDecoder::Shutdown() in most cases.

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834436 [details]
Bug 1337265: Properly reset state when shutting down decoder.

https://reviewboard.mozilla.org/r/110386/#review112218

> I can't see how this will help.
> 
> Assertion failure: !data.mDecoder, at /home/worker/workspace/build/src/dom/media/MediaFormatReader.cpp:339
> 
> #220 clear data.mDecoder so it is impossible to fail the assertion if this function is ever called.

Exactly. It asserts that mDecoder exists when it doesn't as it's been shutdown before init completed. Yet the state would still be WaitingForInit

The MFR calls shutdown while an init is in progress and then attempt to recreate one. This go straight to WaitingForInit which will assert.

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8834436 [details]
Bug 1337265: Properly reset state when shutting down decoder.

https://reviewboard.mozilla.org/r/110386/#review114072

It looks like the issue has been fixed by bug 1340969.