Closed Bug 1266336 Opened 4 years ago Closed 4 years ago

crash in mozilla::WidevineVideoDecoder::InitDecode

Categories

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

Unspecified
All
defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- unaffected
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: njn, Assigned: gerald)

References

Details

(Keywords: crash)

Crash Data

Attachments

(3 files)

This bug was filed from the Socorro interface and is 
report bp-ca052fb1-714f-4042-984a-705e12160420.
=============================================================

This crash first appeared in Nightly 20160417030601. We've seen four occurrences on Mac and one on Windows.

> 0 	XUL 	mozilla::WidevineVideoDecoder::InitDecode(GMPVideoCodec const&, unsigned char const*, unsigned int, GMPVideoDecoderCallback*, int) 	dom/media/gmp/widevine-adapter/WidevineUtils.h
> 1 	XUL 	mozilla::gmp::GMPVideoDecoderChild::RecvInitDecode(GMPVideoCodec const&, nsTArray<unsigned char>&&, int const&) 	dom/media/gmp/GMPVideoDecoderChild.cpp
> 2 	XUL 	mozilla::gmp::PGMPVideoDecoderChild::OnMessageReceived(IPC::Message const&) 	obj-firefox/x86_64/ipc/ipdl/PGMPVideoDecoderChild.cpp
> 3 	XUL 	mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) 	ipc/glue/MessageChannel.cpp

All of them are trying to access address 0x8, so it looks like a straightforward null deref.
OS: Mac OS X → All
We should fix this crash for 47. cpearce says the fix should be easy.
Priority: -- → P1
Anthony says Gerald is looking at Widevine crashes.
Assignee: nobody → gsquelart
Note: this is one of the Widevine-trying-to-decode-WebRTC-H264 crashes:
https://crash-stats.mozilla.com/report/index/d9223957-8778-4253-bf10-507312160503
which look identical, so I'm betting these are the same thing.
I'd still like to fix a few potential issues on the Widevine side, which should at least prevent these crashes.
See Also: → 1268984
Ensure that there is a CDM before creating a video decoder that relies on that
CDM. This is mainly to prevent using the Widevine video decoder alone, without
decryption.

Review commit: https://reviewboard.mozilla.org/r/50685/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50685/
Attachment #8749013 - Flags: review?(cpearce)
Attachment #8749014 - Flags: review?(cpearce)
Attachment #8749015 - Flags: review?(cpearce)
Check the return result from Widevine's CDM creation function, and handle
failure.

Review commit: https://reviewboard.mozilla.org/r/50687/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50687/
Assert that the CDM wrapper is given a non-null CDM pointer.
(so GetCDM() doesn't need to be null-checked.)
Renamed WidevineVideoDecoder mCDM to mCDMWrapper, to avoid (my) confusion.
Assert that WidevineVideoDecoder is given a non-null CDM-wrapper pointer.
Assert that WidevineVideoDecoder only accesses the CDM before DecodingComplete.

Small optimization: Move aCDM into mCDM (to save an AddRef/Release pair).

Review commit: https://reviewboard.mozilla.org/r/50689/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50689/
Comment on attachment 8749013 [details]
MozReview Request: Bug 1266336 - Check sCDMWrapper before creating video decoder - r?cpearce

https://reviewboard.mozilla.org/r/50685/#review47665
Attachment #8749013 - Flags: review?(cpearce) → review+
Attachment #8749014 - Flags: review?(cpearce) → review+
Comment on attachment 8749014 [details]
MozReview Request: Bug 1266336 - Check actual CDM creation - r?cpearce

https://reviewboard.mozilla.org/r/50687/#review47667
Comment on attachment 8749015 [details]
MozReview Request: Bug 1266336 - Clarify expected usage of CDM wrapper - r?cpearce

https://reviewboard.mozilla.org/r/50689/#review47669

::: dom/media/gmp/widevine-adapter/WidevineVideoDecoder.cpp:243
(Diff revision 1)
>    Log("WidevineVideoDecoder::DecodingComplete()");
> -  if (mCDM) {
> +  if (mCDMWrapper) {
>      CDM()->DeinitializeDecoder(kStreamTypeVideo);
> -    mCDM = nullptr;
> +    mCDMWrapper = nullptr;
>    }
>    Release();

Could add a comment here to note where the addref happens.
Attachment #8749015 - Flags: review?(cpearce) → review+
Comment on attachment 8749013 [details]
MozReview Request: Bug 1266336 - Check sCDMWrapper before creating video decoder - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50685/diff/1-2/
Comment on attachment 8749014 [details]
MozReview Request: Bug 1266336 - Check actual CDM creation - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50687/diff/1-2/
Comment on attachment 8749015 [details]
MozReview Request: Bug 1266336 - Clarify expected usage of CDM wrapper - r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50689/diff/1-2/
Reminder to uplift to 47b.
Flags: needinfo?(gsquelart)
Comment on attachment 8749013 [details]
MozReview Request: Bug 1266336 - Check sCDMWrapper before creating video decoder - r?cpearce

For all 3 patches please:

Approval Request Comment
[Feature/regressing bug #]: Regressing bug 1245789 and its uplift bug 1265270

[User impact if declined]: Some crashes when using Widevine

[Describe test coverage new/current, TreeHerder]: Media tests in aurora try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81f07038d681e03e28ce3242a6bbb5fbe997f191 and beta try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=405eeb713476607716e4e8b6d2fce0e98804b1ba

[Risks and why]: Low risk, as it's adding some checks to bail before a nullptr may be stored and later used.
Plus some new assert's, but if they trigger, a null dereference would otherwise have followed anyway. I.e., there shouldn't be *more* crashes than before; in the worst case there will be as many, but hopefully we would get a better idea of this issue thanks to these new assertions.

[String/UUID change made/needed]: None
Flags: needinfo?(gsquelart)
Attachment #8749013 - Flags: approval-mozilla-beta?
Attachment #8749013 - Flags: approval-mozilla-aurora?
Comment on attachment 8749013 [details]
MozReview Request: Bug 1266336 - Check sCDMWrapper before creating video decoder - r?cpearce

Let's uplift to Aurora48 and bake over the weekend. Hopefully we will see a downward trend and can uplift to Beta47 before I gtb 47.0b4
Attachment #8749013 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8749013 [details]
MozReview Request: Bug 1266336 - Check sCDMWrapper before creating video decoder - r?cpearce

Crash data on Nightly49 and Aurora48 looks promising. We haven't seen any instances of this crash since it landed on those channels. Beta47+
Attachment #8749013 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.