1295146 - Crash in <T>::operator() in MediaFormatReader::EnsureDecoderInitialized()

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-c77c1309-3ff8-4f80-901a-59fb92160814.
=============================================================

This is a crash inside MediaFormatReader::EnsureDecoderInitialized(). It is only happening in great quantity in Firefox 51. I'm not sure when it regressed.

I'll get this added to the skip list so it is easier to separate out.

Comment 1

[Andrew McCreight [:mccr8]]

Could you take a look, Jean-Yves? Thanks.

Comment 2

[Andrew McCreight [:mccr8]]

This is the number 8 crash on Windows Nightly on 8-10.

Comment 3

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

https://crash-stats.mozilla.com/report/index/c77c1309-3ff8-4f80-901a-59fb92160814
https://hg.mozilla.org/mozilla-unified/annotate/6cf0089510fa/dom/media/MediaFormatReader.cpp#l483

Looks like a null deref (decoder.mDecoder). Let's add some assertions to confirm it.

Comment 4

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

Attachment: Bug 1295146 - add assertions to debug null-deref.

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

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8781327 [details]
Bug 1295146 - add assertions to debug null-deref.

https://reviewboard.mozilla.org/r/71744/#review69288

::: dom/media/MediaFormatReader.cpp:483
(Diff revision 1)
>    decoder.mInitPromise.Begin(decoder.mDecoder->Init()
>         ->Then(OwnerThread(), __func__,
>                [self] (TrackType aTrack) {
> -                MOZ_ASSERT(!self->IsSuspended());
> +                MOZ_DIAGNOSTIC_ASSERT(!self->IsSuspended());
>                  auto& decoder = self->GetDecoderData(aTrack);
> +                MOZ_DIAGNOSTIC_ASSERT(decoder.mDecoder);

I don't see how decoder.mDecoder could be null here, however, if it is, it means that somehow the decoder was destroyed between the time it's been created and initialised. In which case I believe we should simply exit early rather than asserting.

Comment 6

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

I would like to debug the root cause of null |decoder.mDecoder| instead of wallpapering it with a null-check which somewhat adds complexity with a new call path.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b63dfece689a
add assertions to debug null-deref. r=jya

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/b63dfece689a

Comment 11

[Andrew McCreight [:mccr8]]

The last build this signature appears in is 08-17, so I think bug 1295541 fixed it.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature '<T>::operator() | mozilla::MozPromise<T>::FunctionThenValue<T>::DoResolveOrRejectInternal':
 - nightly (version 51): 49 crashes from 2016-08-01.
 - aurora  (version 50): 0 crashes from 2016-08-01.
 - beta    (version 49): 0 crashes from 2016-08-02.
 - release (version 48): 2 crashes from 2016-07-25.
 - esr     (version 45): 0 crashes from 2016-05-02.

Crash volume on the last weeks (Week N is from 08-22 to 08-28):
            W. N-1  W. N-2  W. N-3
 - nightly      39       0       0
 - aurora        0       0       0
 - beta          0       0       0
 - release       0       0       0
 - esr           0       0       0

Affected platform: Windows

Crash rank on the last 7 days:
           Browser   Content     Plugin
 - nightly #253      #25
 - aurora
 - beta
 - release #5969
 - esr

Comment 13

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

Per comment 11.