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

Status: VERIFIED FIXED

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

Description

[Chris Peterson [:cpeterson]]

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

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: MozReview Request: Bug 1270689 - Null-check WidevineDecryptor::mCallback before use. r=gerald

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/

Comment 3

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

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'

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

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/

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

I must remember to uplift.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d509c083a9

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b2d509c083a9

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1bb8e2207e8e

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/47f753b5632b

Comment 12

[Simona Badau, Desktop QA]

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.

Comment 13

[Chris Peterson [:cpeterson]]

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.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Clear WidevineDecryptor::mCallback in WidevineDecryptor::DecryptingComplete()

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

Comment 15

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

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'.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30f9e7dfcd3d3de1d949f523d353d9956f2b976a
Bug 1270689 - Clear WidevineDecryptor::mCallback in WidevineDecryptor::DecryptingComplete(). r=gerald

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/30f9e7dfcd3d

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8752640 [details] [diff] [review]
Clear WidevineDecryptor::mCallback in WidevineDecryptor::DecryptingComplete()

Crash fix, Aurora48+, Beta47+

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-beta/rev/63933fe5bb3f

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

ni? me; I need to remember to land the aurora uplift.

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ba18aae69e6

Comment 24

[Simona Badau, Desktop QA]

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

Comment 25

[Simona Badau, Desktop QA]

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.