crash in mozilla::WidevineVideoDecoder::InitDecode

RESOLVED FIXED in Firefox 47

Status

()

Core
Audio/Video: Playback
P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: gerald)

Tracking

(Blocks: 1 bug, {crash})

Trunk
mozilla49
Unspecified
All
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 unaffected, firefox47 fixed, firefox48 fixed, firefox49 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
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.
Blocks: 1222845, 1265270
status-firefox46: --- → unaffected
status-firefox47: --- → ?
OS: Mac OS X → All
We should fix this crash for 47. cpearce says the fix should be easy.
status-firefox49: --- → affected
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.
(Assignee)

Comment 4

2 years ago
I'd still like to fix a few potential issues on the Widevine side, which should at least prevent these crashes.
See Also: → bug 1268984
(Assignee)

Comment 5

2 years ago
Created attachment 8749013 [details]
MozReview Request: Bug 1266336 - Check sCDMWrapper before creating video decoder - r?cpearce

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)
(Assignee)

Comment 6

2 years ago
Created attachment 8749014 [details]
MozReview Request: Bug 1266336 - Check actual CDM creation - r?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/
(Assignee)

Comment 7

2 years ago
Created attachment 8749015 [details]
MozReview Request: Bug 1266336 - Clarify expected usage of CDM wrapper - r?cpearce

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+
(Assignee)

Comment 11

2 years ago
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/
(Assignee)

Comment 12

2 years ago
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/
(Assignee)

Comment 13

2 years ago
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/

Comment 14

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bc06c379df7
https://hg.mozilla.org/integration/mozilla-inbound/rev/22cfbb1cc6c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/567a13dc5e51
(Assignee)

Comment 15

2 years ago
Reminder to uplift to 47b.
Flags: needinfo?(gsquelart)

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5bc06c379df7
https://hg.mozilla.org/mozilla-central/rev/22cfbb1cc6c4
https://hg.mozilla.org/mozilla-central/rev/567a13dc5e51
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 17

2 years ago
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?

Updated

2 years ago
status-firefox47: ? → affected
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 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/636655e95079
https://hg.mozilla.org/releases/mozilla-aurora/rev/74518dbeb5d5
https://hg.mozilla.org/releases/mozilla-aurora/rev/403037ee85e4
status-firefox48: affected → fixed
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+

Comment 21

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/44a9281f2a77
https://hg.mozilla.org/releases/mozilla-beta/rev/5495417dc2fc
https://hg.mozilla.org/releases/mozilla-beta/rev/5f0df9e62467
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.