Closed Bug 1270689 Opened 4 years ago Closed 4 years ago

Widevine crash in non-virtual thunk to mozilla::WidevineDecryptor::OnSessionClosed when using DASH.js player

Categories

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

Unspecified
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox46 --- unaffected
firefox47 --- verified
firefox48 --- verified
firefox49 --- verified

People

(Reporter: cpeterson, Assigned: cpearce)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-dcc4aefa-c837-490a-b026-aadbc2160505.
=============================================================

STR from bug 1268694:
1. Load http://dashif.org/reference/players/javascript/v2.1.1/samples/dash-if-reference-player/index.html
2. Load manifest http://html5.cablelabs.com:8100/cenc/wv/dash.mpd (also accessible from the DASH player's menu: Sample Streams > CableLabs Test Content > CableLabs Cenc Widevine) on DASH.js player menu.
3. Quickly hit the "Load" button 2–3 more times. If the crash happens, it always seems to happen within the first 1–2 button clicks when you first play the video after the browser starts.

0 	XUL 	non-virtual thunk to mozilla::WidevineDecryptor::OnSessionClosed(char const*, unsigned int) 	dom/media/gmp/widevine-adapter/WidevineDecryptor.cpp:396
Ø 1 	libwidevinecdm.dylib 	libwidevinecdm.dylib@0x4f88 	
Ø 2 	libwidevinecdm.dylib 	libwidevinecdm.dylib@0x1bddf 	
Ø 3 	libwidevinecdm.dylib 	libwidevinecdm.dylib@0x2615e 	
Ø 4 	libwidevinecdm.dylib 	libwidevinecdm.dylib@0x18677 	
Ø 5 	libwidevinecdm.dylib 	libwidevinecdm.dylib@0x1bf50 	
6 	XUL 	mozilla::gmp::GMPRunnable::Run() 	dom/media/gmp/GMPPlatform.cpp:40
7 	XUL 	RunnableMethod<mozilla::gmp::GMPRunnable, void (mozilla::gmp::GMPRunnable::*)(), mozilla::Tuple<> >::Run 	ipc/chromium/src/base/task.h:28
8 	XUL 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc:335
9 	XUL 	base::MessagePumpDefault::Run(base::MessagePump::Delegate*) 	ipc/chromium/src/base/message_pump_default.cc:34
10 	XUL 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:233
11 	XUL 	XRE_InitChildProcess 	toolkit/xre/nsEmbedFunctions.cpp:637
12 	plugin-container 	content_process_main(int, char**) 	ipc/contentproc/plugin-container.cpp:237
13 	plugin-container 	start
I can't repro this, but I suspect it could happen if we try to shutdown the CDM before we've finished starting it up. We can probably just add a null check here.
We've observed some crashes derefing the callback pointer, which may be
occuring due to shutdown happening before init has setup the callback pointer.

Review commit: https://reviewboard.mozilla.org/r/51303/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51303/
Attachment #8750139 - Flags: review?(gsquelart)
Comment on attachment 8750139 [details]
MozReview Request: Bug 1270689 - Null-check WidevineDecryptor::mCallback before use. r=gerald

https://reviewboard.mozilla.org/r/51303/#review47969

r+ with this fixed:

::: dom/media/gmp/widevine-adapter/WidevineDecryptor.cpp:318
(Diff revision 1)
> +    Log("Decryptor::OnRejectPromise(aPromiseId=%d, err=%d, sysCode=%d, msg=%s) FAIL; !mCallback",
> +        aPromiseId, (int)aError, aSystemCode, aErrorMessage);

'syscode=%u'
Attachment #8750139 - Flags: review?(gsquelart) → review+
Assignee: nobody → cpearce
Comment on attachment 8750139 [details]
MozReview Request: Bug 1270689 - Null-check WidevineDecryptor::mCallback before use. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51303/diff/1-2/
Attachment #8750139 - Attachment description: MozReview Request: Bug 1270689 - Null-check WidevineDecryptor::mCallback before use. r?gerald → MozReview Request: Bug 1270689 - Null-check WidevineDecryptor::mCallback before use. r=gerald
I must remember to uplift.
Flags: needinfo?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/b2d509c083a9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8750139 [details]
MozReview Request: Bug 1270689 - Null-check WidevineDecryptor::mCallback before use. r=gerald

Approval Request Comment
[Feature/regressing bug #]: Widevine EME
[User impact if declined]: Widevine EME video may fail to play. Under some unknown circumstances, the Widevine CDM is crashing. I think this happens when we try to shutdown before we've completed startup (so failing to play may actually not be noticed). We only have a handful of crash reports, so it should be rare.
[Describe test coverage new/current, TreeHerder]: We have lots of EME mochitests.
[Risks and why]: Low; we're just adding null checks.
[String/UUID change made/needed]: None.
Attachment #8750139 - Flags: approval-mozilla-beta?
Attachment #8750139 - Flags: approval-mozilla-aurora?
Comment on attachment 8750139 [details]
MozReview Request: Bug 1270689 - Null-check WidevineDecryptor::mCallback before use. r=gerald

I only saw one instance of this crash in the past 7 days and that was on Nightly49, however I am still taking this patch as it puts some safeguards in place, Aurora48+, Beta47+
Attachment #8750139 - Flags: approval-mozilla-beta?
Attachment #8750139 - Flags: approval-mozilla-beta+
Attachment #8750139 - Flags: approval-mozilla-aurora?
Attachment #8750139 - Flags: approval-mozilla-aurora+
On Mac OS X 10.11 - Nightly 49 and Firefox 47 beta 5 are still crashing for me.

Here are the crash reports:
https://crash-stats.mozilla.com/report/index/9d207049-6012-4dc0-b5eb-6e7fc2160513
https://crash-stats.mozilla.com/report/index/4d41abad-acad-4a39-8f30-60f732160513

Here are the STR:
1. Open Firefox
2. Start playing several amazon videos (I played 7 videos)
3. Close Firefox by clicking on the X button from the upper left side of the window
4. In the Dock bar click on the Firefox icon to open a new window
5. Navigate to about:crashes

Actual results:
A new crash report is added to the Submitted crash reports list.

Chris, should I Reopen this bug or file a new one due to the fact the scenario is different.
Let's just reopen this bug. When I filed this bug, we knew we had crash reports for both DASH.js and Amazon. So these crashes are all the same bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The OnSessionClosed() callback is happening on a timer, after
WidevineDecryptor::DecryptingComplete() has been called. So mCallback could be
non-null, be actually because DecryptingComplete() has been called, the object
pointed to be mCallback has been deallocated, and mCallback is a dangling
pointer.

MozReview-Commit-ID: 4xdHYRn7EAS
Attachment #8752640 - Flags: review?(gsquelart)
Comment on attachment 8752640 [details] [diff] [review]
Clear WidevineDecryptor::mCallback in WidevineDecryptor::DecryptingComplete()

Review of attachment 8752640 [details] [diff] [review]:
-----------------------------------------------------------------

r+, but please revisit your commit description:
'could be non-null, be actually because' -> 'could actually be non-null because' (assuming that's what you meant),
'pointed to be mCallback' -> 'pointed to by mCallback'.
Attachment #8752640 - Flags: review?(gsquelart) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/30f9e7dfcd3d3de1d949f523d353d9956f2b976a
Bug 1270689 - Clear WidevineDecryptor::mCallback in WidevineDecryptor::DecryptingComplete(). r=gerald
(In reply to Simona B [:simonab] from comment #12)
> On Mac OS X 10.11 - Nightly 49 and Firefox 47 beta 5 are still crashing for
> me.
> 
> Here are the crash reports:
> https://crash-stats.mozilla.com/report/index/9d207049-6012-4dc0-b5eb-
> 6e7fc2160513
> https://crash-stats.mozilla.com/report/index/4d41abad-acad-4a39-8f30-
> 60f732160513
> 
> Here are the STR:
> 1. Open Firefox
> 2. Start playing several amazon videos (I played 7 videos)
> 3. Close Firefox by clicking on the X button from the upper left side of the
> window
> 4. In the Dock bar click on the Firefox icon to open a new window
> 5. Navigate to about:crashes
> 
> Actual results:
> A new crash report is added to the Submitted crash reports list.
> 
> Chris, should I Reopen this bug or file a new one due to the fact the
> scenario is different.

Thanks Simona, I was able to repro and debug this using these STR. :)
Comment on attachment 8752640 [details] [diff] [review]
Clear WidevineDecryptor::mCallback in WidevineDecryptor::DecryptingComplete()

Approval Request Comment
[Feature/regressing bug #]: Widevine EME
[User impact if declined]: Crashes during shutdown if Widevine EME video is playing; the impact on users is very low, since typically the crash isn't observable, but we will receive crash reports for these crashes, so we should eliminate the crash, otherwise crashes with the same signature would be missed.
[Describe test coverage new/current, TreeHerder]: We have lots of EME mochitests
[Risks and why]: Low; annulling a pointer on shutdown, and we always null-check that pointer before using it (added in the previous patch in this bug).
[String/UUID change made/needed]: None.
Attachment #8752640 - Flags: approval-mozilla-beta?
Attachment #8752640 - Flags: approval-mozilla-aurora?
Flags: needinfo?(cpearce)
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/30f9e7dfcd3d
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Comment on attachment 8752640 [details] [diff] [review]
Clear WidevineDecryptor::mCallback in WidevineDecryptor::DecryptingComplete()

Crash fix, Aurora48+, Beta47+
Attachment #8752640 - Flags: approval-mozilla-beta?
Attachment #8752640 - Flags: approval-mozilla-beta+
Attachment #8752640 - Flags: approval-mozilla-aurora?
Attachment #8752640 - Flags: approval-mozilla-aurora+
ni? me; I need to remember to land the aurora uplift.
Flags: needinfo?(cpearce)
Flags: needinfo?(cpearce)
I can confirm the issue is fixed on the latest Nightly 49.0a1 (Build ID: 20160519030232)and Firefox 47 beta 7 (Build ID: 20160518173344) - neither of this Firefox versions are crashing when following the STR from Comment 12 on Max OS X 10.11
Verified as fixed also Firefox Developer Edition 48.0a2  (Build:20160522004024) - there is no crash with the signature [@ non-virtual thunk to mozilla::WidevineDecryptor::OnSessionClosed ]- when following the scenarios from the Description and from Comment 12.

However, I did encountered a new crash when playing on the Dashif webpage, filed Bug 1274963 to cover that.
Status: RESOLVED → VERIFIED
Depends on: 1279025
Blocks: 1279026
Duplicate of this bug: 1279025
Duplicate of this bug: 1279026
You need to log in before you can comment on or make changes to this bug.