1185392 - [EME] Non-reentrant Mutex use reentrantly in GMP async shutdown code

Status: RESOLVED FIXED

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

Description

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

I just hit this assertion stack starting up a Netflix session:

 	KernelBase.dll!_DebugBreak@0()	Unknown
 	nss3.dll!PR_Assert(const char * s, const char * file, int ln) Line 559	C
 	nss3.dll!PR_Lock(PRLock * lock) Line 214	C
 	xul.dll!mozilla::OffTheBooksMutex::Lock() Line 382	C++
>	xul.dll!mozilla::BaseAutoLock<mozilla::Mutex>::BaseAutoLock<mozilla::Mutex>(mozilla::Mutex & aLock, mozilla::detail::GuardObjectNotifier && _notifier) Line 165	C++
 	xul.dll!mozilla::gmp::GeckoMediaPluginServiceParent::SetAsyncShutdownPluginState(mozilla::gmp::GMPParent * aGMPParent, char aId, const nsCString & aState) Line 414	C++
 	xul.dll!mozilla::gmp::GMPParent::CloseActive(bool aDieWhenUnloaded) Line 366	C++
 	xul.dll!mozilla::gmp::GeckoMediaPluginServiceParent::UnloadPlugins() Line 500	C++
 	xul.dll!nsRunnableMethodArguments<>::apply<mozilla::gmp::GeckoMediaPluginServiceParent,void (__thiscall mozilla::gmp::GeckoMediaPluginServiceParent::*)(void)>(mozilla::gmp::GeckoMediaPluginServiceParent * o, void (void) * m) Line 622	C++
 	xul.dll!nsRunnableMethodImpl<void (__thiscall mozilla::gmp::GeckoMediaPluginServiceParent::*)(void),1>::Run() Line 830	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 867	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 277	C++
 	xul.dll!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate * aDelegate) Line 355	C++
 	xul.dll!MessageLoop::RunInternal() Line 235	C++
 	xul.dll!MessageLoop::RunHandler() Line 228	C++
 	xul.dll!MessageLoop::Run() Line 202	C++
 	xul.dll!nsThread::ThreadFunc(void * aArg) Line 362	C++
 	nss3.dll!_PR_NativeRunThread(void * arg) Line 397	C
 	nss3.dll!pr_root(void * arg) Line 90	C
 	[External Code]	

We acquire the GMP service's Mutex in GeckoMediaPluginServiceParent::UnloadPlugins() and then GeckoMediaPluginServiceParent::SetAsyncShutdownPluginState() tries to also acquire it, and we hit this assertion.

Comment 1

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

Attachment: 1185392-separate-asyncshutdownpluginstates-mutex.patch

Using a separate mutex to protect mAsyncShutdownPluginStatesMutex, as mMutex could already be held when trying to update the plugin states.

The new mAsync..Mutex may be requested when mMutex is already locked (as experienced in the bug description).
however while mAsync..Mutex is held, simple self-contained operations are performed, during which mMutex is never requested.
So we cannot have a deadlock.

Comment 2

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

Attachment: 1185392-separate-asyncshutdownpluginstates-mutex.patch v2

(Missed an #ifdef in previous patch.)

Using a separate mutex to protect mAsyncShutdownPluginStatesMutex, as mMutex could already be held when trying to update the plugin states.

The new mAsync..Mutex may be requested when mMutex is already locked (as experienced in the bug description).
however while mAsync..Mutex is held, simple self-contained operations are performed, during which mMutex is never requested.
So we cannot have a deadlock.

Comment 3

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

Attachment: 1185392-separate-asyncshutdownpluginstates-mutex.patch v3

(Still missed an #ifdef in previous patch, *this* is the right one.)

Using a separate mutex to protect mAsyncShutdownPluginStatesMutex, as mMutex could already be held when trying to update the plugin states.

The new mAsync..Mutex may be requested when mMutex is already locked (as experienced in the bug description).
however while mAsync..Mutex is held, simple self-contained operations are performed, during which mMutex is never requested.
So we cannot have a deadlock.

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=829897adac79

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cdc4ff5310b

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8cdc4ff5310b

Comment 7

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

Please consider uplifting to 41 *after* bug 1183433.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82e80ffd8556

Comment 8

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

Please uplift to 41 after bug 1175765 and bug 1183433.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef66c9572b71

Comment 9

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

Please uplift the following patches together to beta/40: bug 1175783, bug 1175765, bug 1183433, bug 1185392.

BETA try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b70f2bca3b28

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Nothing is going to be uplifted if you don't request approval on the patch.

Comment 11

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

Comment on attachment 8635847 [details] [diff] [review]
1185392-separate-asyncshutdownpluginstates-mutex.patch v3

Approval Request Comment

[Feature/regressing bug #]: EME

[User impact if declined]: This patch is a necessary follow-up to bug 1183433 to remove a regression.

[Describe test coverage new/current, TreeHerder]: We have lots of EME mochitests.

[Risks and why]: Pretty low; this adds a mutex for a small section of code that does not require other mutexes.

[String/UUID change made/needed]: None.

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8635847 [details] [diff] [review]
1185392-separate-asyncshutdownpluginstates-mutex.patch v3

I asked rillian and jesup to review the series of patches for EME as well because we're late in the 40 cycle. Their suggestion is to take the changes as they put us in a better state that we were in previously and because waiting an additional ~2.5 weeks for 41 to hit Beta will be detrimental. I'm approving for Beta. We need to watch the beta8 build to ensure to the best of our ability that we have not regressed anything related to WebRTC or EME. Beta+ Aurora+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7ead0ebcc5ad

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e404ee7ef5e5