Closed
Bug 1266336
Opened 8 years ago
Closed 8 years ago
crash in mozilla::WidevineVideoDecoder::InitDecode
Categories
(Core :: Audio/Video: Playback, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox46 | --- | unaffected |
firefox47 | --- | fixed |
firefox48 | --- | fixed |
firefox49 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: mozbugz)
References
Details
(Keywords: crash)
Crash Data
Attachments
(3 files)
58 bytes,
text/x-review-board-request
|
cpearce
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
58 bytes,
text/x-review-board-request
|
cpearce
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
cpearce
:
review+
|
Details |
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.
Updated•8 years ago
|
Blocks: widevine-uplift
status-firefox46:
--- → unaffected
status-firefox47:
--- → ?
OS: Mac OS X → All
Comment 1•8 years ago
|
||
We should fix this crash for 47. cpearce says the fix should be easy.
status-firefox49:
--- → affected
Priority: -- → P1
Comment 2•8 years ago
|
||
Anthony says Gerald is looking at Widevine crashes.
Assignee: nobody → gsquelart
Comment 3•8 years ago
|
||
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•8 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: → 1268984
Assignee | ||
Comment 5•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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 8•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8749014 -
Flags: review?(cpearce) → review+
Comment 9•8 years ago
|
||
Comment on attachment 8749014 [details] MozReview Request: Bug 1266336 - Check actual CDM creation - r?cpearce https://reviewboard.mozilla.org/r/50687/#review47667
Comment 10•8 years ago
|
||
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•8 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•8 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•8 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•8 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
Comment 16•8 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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 17•8 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?
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•8 years ago
|
||
bugherder uplift |
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
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•8 years ago
|
||
bugherder uplift |
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
You need to log in
before you can comment on or make changes to this bug.
Description
•