1285248 - Crash in mozilla::MediaDecoderReaderWrapper::RequestAudioData

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-f3ce2f76-5ac8-40ba-9291-40a7b2160707.
=============================================================

Seen while looking at nightly crash stats. Low volume Mac and Windows crash seen on both Nightly and Beta 5 - http://bit.ly/29t6RhM

ni on jwwang since he worked in this code area.

Comment 1

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

Attachment: Bug 1285248. Part 1 - the value of aSeekJob.mTarget.IsVideoOnly() is wrong because the members are reset in the move constructor.

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

Comment 2

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

Hi Gerald,
Taipei office is closed due to typhoon today. Can you review this patch or find someone else?
Thanks!

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Good luck with the weather! I'll have a look...

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8768995 [details]
Bug 1285248. Part 1 - the value of aSeekJob.mTarget.IsVideoOnly() is wrong because the members are reset in the move constructor.

https://reviewboard.mozilla.org/r/62972/#review59916

Seems reasonable to me as a band-aid.
Please make sure you open a new bug to come back to this issue.

If it's not too late in the cycle, please consider this:

::: dom/media/AccurateSeekTask.cpp:110
(Diff revision 1)
> -  MOZ_ASSERT(!mDoneAudioSeeking);
> -  MOZ_ASSERT(!mReader->IsRequestingAudioData());
> -  MOZ_ASSERT(!mReader->IsWaitingAudioData());
> +  //MOZ_ASSERT(!mDoneAudioSeeking);
> +  //MOZ_ASSERT(!mReader->IsRequestingAudioData());
> +  //MOZ_ASSERT(!mReader->IsWaitingAudioData());

Wouldn't it be worth keeping these for a few weeks, and actually changing them to MOZ_DIAGNOSTIC_ASSERT's, so that we can get more information about what's actually wrong, in Nightly&Aurora?

Comment 5

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

Good idea! I will add more assertions to help debugging.

Comment 6

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

Attachment: Bug 1285248. Part 2 - fix logic for video-only seek.

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

Comment 7

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

Comment on attachment 8768995 [details]
Bug 1285248. Part 1 - the value of aSeekJob.mTarget.IsVideoOnly() is wrong because the members are reset in the move constructor.

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

Comment 8

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

Comment on attachment 8768995 [details]
Bug 1285248. Part 1 - the value of aSeekJob.mTarget.IsVideoOnly() is wrong because the members are reset in the move constructor.

https://reviewboard.mozilla.org/r/62972/#review59968

Comment 9

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

https://reviewboard.mozilla.org/r/62972/#review59970

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8768995 [details]
Bug 1285248. Part 1 - the value of aSeekJob.mTarget.IsVideoOnly() is wrong because the members are reset in the move constructor.

https://reviewboard.mozilla.org/r/62972/#review59972

Confirming r+ for this fix.

::: dom/media/AccurateSeekTask.cpp:48
(Diff revision 2)
>                                     const media::TimeUnit& aEnd,
>                                     int64_t aCurrentMediaTime)
>    : SeekTask(aDecoderID, aThread, aReader, Move(aSeekJob))
>    , mCurrentTimeBeforeSeek(media::TimeUnit::FromMicroseconds(aCurrentMediaTime))
>    , mAudioRate(aInfo.mAudio.mRate)
> -  , mDoneAudioSeeking(!aInfo.HasAudio() || aSeekJob.mTarget.IsVideoOnly())
> +  , mDoneAudioSeeking(!aInfo.HasAudio() || mSeekJob.mTarget.IsVideoOnly())

Good find!
We really need static analysis to find use-after-move issues.

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8769074 [details]
Bug 1285248. Part 2 - fix logic for video-only seek.

https://reviewboard.mozilla.org/r/63044/#review59974

::: dom/media/AccurateSeekTask.cpp:312
(Diff revision 1)
>    // resolved.
>  
>    SAMPLE_LOG("OnAudioDecoded [%lld,%lld] disc=%d",
>      audio->mTime, audio->GetEndTime(), audio->mDiscontinuity);
>  
> +  // video-only seek doesn't reset audio decoder. There might be pending audio

Nit: 'video' -> 'Video' with capital 'V'.

Comment 12

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

Thanks for the review!

Comment 13

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

Comment on attachment 8769074 [details]
Bug 1285248. Part 2 - fix logic for video-only seek.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32de23e9cca6
Part 1 - the value of aSeekJob.mTarget.IsVideoOnly() is wrong because the members are reset in the move constructor. r=gerald
https://hg.mozilla.org/integration/autoland/rev/0aa6ee3eb1d8
Part 2 - fix logic for video-only seek. r=gerald

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/32de23e9cca6
https://hg.mozilla.org/mozilla-central/rev/0aa6ee3eb1d8

Comment 16

[Gerry Chang [:gchang]]

Because the volume of crash is small, we don't need to uplift the patch in beta. Let's let it ride on 50.