1111114 - [EME] Crash shutting down EMEDecryptor

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

While trying to catch bug 1101308 in a debugger, I found another crash:

xul!mozilla::EMEDecryptor::DeliverDecrypted::Decrypted+0x80 [c:\users\bob\src\mozilla\purple\dom\media\fmp4\eme\emedecodermodule.cpp @ 71]	C/C++/ASM
xul!mozilla::CDMProxy::gmp_Decrypt+0x130 [c:\users\bob\src\mozilla\purple\dom\media\eme\cdmproxy.cpp @ 526]	C/C++/ASM
xul!nsRunnableMethodImpl<void (__thiscall mozilla::CDMProxy::*)(nsAutoPtr<mozilla::CDMProxy::DecryptJob>),nsAutoPtr<mozilla::CDMProxy::DecryptJob>,1>::Run+0x2b [c:\users\bob\src\mozilla\purple\objdir\dist\include\nsthreadutils.h @ 365]	C/C++/ASM
xul!nsThread::ProcessNextEvent+0x592 [c:\users\bob\src\mozilla\purple\xpcom\threads\nsthread.cpp @ 830]	C/C++/ASM
xul!NS_ProcessNextEvent+0x62 [c:\users\bob\src\mozilla\purple\xpcom\glue\nsthreadutils.cpp @ 265]	C/C++/ASM

What's happening is we're in DeliverDecrypted::Decrypted(), dereferenceing mDecryptor->mCallback, and mCallback is invalid. mDecryptor is a strong ref, but mCallback is not, and we've run EMEDecryptor::Shutdown(), which doesn't clear mCallback.

We should clear mCallback in EMEDecryptor::Shutdown(), and null check it here.

Comment 1

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

Attachment: Patch

Null mCallback on shutdown, and null-check it before derefing.

Comment 2

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/87aaa1d526e5

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8535938 [details] [diff] [review]
Patch

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

::: dom/media/fmp4/eme/EMEDecoderModule.cpp
@@ +151,5 @@
>      mTaskQueue->BeginShutdown();
>      mTaskQueue->AwaitShutdownAndIdle();
>      mTaskQueue = nullptr;
>      mProxy = nullptr;
> +    mCallback = nullptr;

Do we need a memory barrier so that the change to mCallback is visible to DeliverDecrypted::Decrypted?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87aaa1d526e5

Comment 5

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

Mass update firefox-status to track EME uplift.

Comment 6

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

(In reply to JW Wang [:jwwang] from comment #3)
> Comment on attachment 8535938 [details] [diff] [review]
> Patch
> 
> Review of attachment 8535938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/fmp4/eme/EMEDecoderModule.cpp
> @@ +151,5 @@
> >      mTaskQueue->BeginShutdown();
> >      mTaskQueue->AwaitShutdownAndIdle();
> >      mTaskQueue = nullptr;
> >      mProxy = nullptr;
> > +    mCallback = nullptr;
> 
> Do we need a memory barrier so that the change to mCallback is visible to
> DeliverDecrypted::Decrypted?

We now use MozPromises here, mCallbackk is only ever used on the one task queue now.