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

RESOLVED FIXED in Firefox 40

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cpearce, Assigned: gerald)

Tracking

(Blocks 1 bug)

unspecified
mozilla42
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 wontfix, firefox40 fixed, firefox41 fixed, firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
See Also: → 1183433
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.
Attachment #8635842 - Flags: review?(cpearce)
Assignee: nobody → gsquelart
(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.
Attachment #8635842 - Attachment is obsolete: true
Attachment #8635842 - Flags: review?(cpearce)
Attachment #8635846 - Flags: review?(cpearce)
(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.
Attachment #8635846 - Attachment is obsolete: true
Attachment #8635846 - Flags: review?(cpearce)
Attachment #8635847 - Flags: review?(cpearce)
Attachment #8635847 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/8cdc4ff5310b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Blocks: 1183433
No longer blocks: 1183433
Depends on: 1183433
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
Flags: needinfo?(cpearce)
Nothing is going to be uplifted if you don't request approval on the patch.
Flags: needinfo?(gsquelart)
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.
Flags: needinfo?(gsquelart)
Attachment #8635847 - Flags: approval-mozilla-beta?
Attachment #8635847 - Flags: approval-mozilla-aurora?
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+
Attachment #8635847 - Flags: approval-mozilla-beta?
Attachment #8635847 - Flags: approval-mozilla-beta+
Attachment #8635847 - Flags: approval-mozilla-aurora?
Attachment #8635847 - Flags: approval-mozilla-aurora+
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.