1400537 - Crash in mozilla::MediaFormatReader::DecoderFactory::Wrapper::Init

Status: RESOLVED FIXED

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

Description

[Julian Seward [:jseward]]

This bug was filed from the Socorro interface and is 
report bp-e18c63ab-abc9-4654-b54c-71de60170915.
=============================================================

This is topcrash #12 in the Windows nightly of 20170914220209.

Comment 1

[Blake Wu [:bwu][:blakewu]]

[Tracking Requested - why for this release]:
Per comment 0, topcrash#12 on 57.

Comment 2

[Blake Wu [:bwu][:blakewu]]

It looks like bug 1400371 may have the same root cause as this one

Comment 3

[Jean-Yves Avenard [:jya]]

If it wasn't for the regression range provided earlier, I would have thought bug 1397307 was the culprit.

Comment 6

[Jean-Yves Avenard [:jya]]

At a guess bug 1397131

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1400537 - P1. Don't assume that the PDM will properly report an error.

Only the Windows H264 decoder supports CreateDecoderParam::mError, all the other PDM leave the value untouched.

As such, it can't be assumed that in case of failure, the mError attribute will be set.

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

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1400537 - P2. Pass video framerate to decoder.

Continuation on bug 1397307 which was incomplete.

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

Comment 9

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

Comment on attachment 8909130 [details]
Bug 1400537 - P2. Pass video framerate to decoder.

https://reviewboard.mozilla.org/r/180706/#review185776

Comment 10

[Alastor Wu [:alwu]]

Comment on attachment 8909129 [details]
Bug 1400537 - P1. Don't assume that the PDM will properly report an error.

https://reviewboard.mozilla.org/r/180704/#review185814

Thanks!

Comment 11

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

(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #10)
> Comment on attachment 8909129 [details]
> Bug 1400537 - P1. Don't assume that the PDM will properly report an error.
> 
> https://reviewboard.mozilla.org/r/180704/#review185814
> 
> Thanks!

It would be nice to describe how this change fixes the crash.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f59c34720c80
https://hg.mozilla.org/mozilla-central/rev/14eff5fba2f1

Comment 13

[Jean-Yves Avenard [:jya]]

No crash since this change went in, so this is promising, though it may be too early to tell.

Comment 14

[Jean-Yves Avenard [:jya]]

Crashes even with the change :(

Comment 15

[Jean-Yves Avenard [:jya]]

JW, youre the most familiar with this area of the code.

From when this bug started, the most likely problem was bug 1397141.

Comment 16

[Jean-Yves Avenard [:jya]]

https://hg.mozilla.org/mozilla-central/rev/57edd7246a04#l1.21
Seems wrong to me, doesn't handle the case was NS_ERROR_NOT_INITIALIZED

Comment 17

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

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #11)
> (In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #10)
> > Comment on attachment 8909129 [details]
> > Bug 1400537 - P1. Don't assume that the PDM will properly report an error.
> > 
> > https://reviewboard.mozilla.org/r/180704/#review185814
> > 
> > Thanks!
> 
> It would be nice to describe how this change fixes the crash.

I still don't understand how this patch fixes the crash which seems to me a null-deref.

Comment 18

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #17)
> (In reply to JW Wang [:jwwang] [:jw_wang] from comment #11)
> > (In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #10)
> > > Comment on attachment 8909129 [details]
> > > Bug 1400537 - P1. Don't assume that the PDM will properly report an error.
> > > 
> > > https://reviewboard.mozilla.org/r/180704/#review185814
> > > 
> > > Thanks!
> > 
> > It would be nice to describe how this change fixes the crash.
> 
> I still don't understand how this patch fixes the crash which seems to me a
> null-deref.

Sorry, I meant how this change likely caused the crash.it  (it's the only change that occurred in the given regression range that could make sense)

Comment 19

[Julien Cristau [:jcristau]]

set 57 back to affected per comment 14.

Comment 20

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

https://hg.mozilla.org/mozilla-central/annotate/a20de99fa3c1/dom/media/MediaFormatReader.cpp#l703

It looks like we pass null to new Wrapper(). Since aData.mDecoder comes from DoCreateDecoder(), the only possibility is:

1. aData.mTrack is bad so DoCreateDecoder() exits with aData.mDecoder==nullptr and result==NS_OK.
2. mOwner->mPlatform->CreateDecoder is bad which doesn't set the error when returning null.

Comment 21

[Alastor Wu [:alwu]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #20)
> 2. mOwner->mPlatform->CreateDecoder is bad which doesn't set the error when
> returning null.

Maybe here[1]? Should set |ownerData.mError = result|.

[1] http://searchfox.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#774

Comment 22

[Jean-Yves Avenard [:jya]]

I believe that's taking care of in http://searchfox.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#699

result is returned, and then passed as error.

I think we just have to ensure that me mDecoder is null, we never use the return value and instead pass an error if result is NS_OK (which it can be)

Comment 23

[Alastor Wu [:alwu]]

Here we should also set the error when returning nullptr.

http://searchfox.org/mozilla-central/source/dom/media/platforms/PDMFactory.cpp#242

Comment 24

[Alastor Wu [:alwu]]

And here.
http://searchfox.org/mozilla-central/source/dom/media/platforms/PDMFactory.cpp#277

Comment 25

[Jean-Yves Avenard [:jya]]

The crash inidcates that the PDM factory didn't create a decoder, yet returned ns_ok This is something the patch in this bug fixed. But I guess I miss another possible case.

Comment 26

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

Might be worth considering to adopt Result<V, E> to fix such issues once and for all. http://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/mfbt/Result.h#272

Comment 27

[Jean-Yves Avenard [:jya]]

I don't see how Result would have provided a different outcome than using MediaResult, for our purpose they are better suited. The issue here is the silliness of having an optional output parameters.

Comment 28

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

Result<RefPtr<MediaDataDecoder>, MediaResult> CreateDecoder(...)

The result is either a success (so we get a RefPtr<MediaDataDecoder>), or a failure (so we check MediaResult for the error code and description).

In the worse case, we get a MediaResult with error code == NS_OK.