Closed Bug 1185392 Opened 9 years ago Closed 9 years ago

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

Categories

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

x86_64
Windows 8.1
defect

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 --- fixed
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: cpearce, Assigned: mozbugz)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
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: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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.