Closed Bug 1237842 Opened 5 years ago Closed 5 years ago

[GMP] Mutex&task locking between UnloadPlugins/GMP and G-M-P-ServiceParent::Observe/main

Categories

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

43 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: gerald, Assigned: gerald)

Details

Crash Data

Attachments

(1 file, 1 obsolete file)

As experienced in this shutdown-hang crash report:
https://crash-stats.mozilla.com/report/index/caa21100-c76f-4ec7-91f1-d18232160107#allthreads

The following scenario seems to be happening:
1. GMP thread: G-M-P-ServiceParent::UnloadPlugins locks mMutex
2. Main: G-M-P-ServiceParent::Observe waits for the same mMutex
3. GMP: UnloadPlugins calls GMPParent::CloseActive() on all mPlugins,
   which calls GMPParent::EnsureAsyncShutdownTimeoutSet(),
   which calls G-M-P-ServiceParent::GetSingleton(),
   which sync-dispatches GMPServiceCreateHelper to the main thread.
But the main thread is stuck on step 2, so the task will never run and the GMP thread gets stuck too.

Proposed solution:
The lock is used to safely access mPlugins.
So UnloadPlugins could Move the contents of the mPlugins array to a local on-stack storage, unlock mMutex, and then call CloseActive on all plugins.
That sounds like a good plan
Rank: 15
Priority: -- → P1
Unlock mMutex before calling CloseActive.

UnloadPlugins runs in the GMP thread. While it locks mMutex, any locking
attempt on the main thread would effectively block the main thread while we are
calling CloseActive on each plugin.
The problem is that CloseActive calls GeckoMediaServiceParent::GetSingleton(),
which dispatches a task to the main thread, which may be blocked because of
mMutex.

To solve this inter-locking issue, the list of plugins is first copied to a
local array, after which mMutex may be released, and we may now call
CloseActive without risking locking.
Attachment #8707247 - Flags: review?(cpearce)
Comment on attachment 8707247 [details] [diff] [review]
1237842-unlock-mMutex-before-CloseActive.patch

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

::: dom/media/gmp/GMPServiceParent.cpp
@@ +673,4 @@
>  #endif
> +  // Note: CloseActive may be async; it could actually finish
> +  // shutting down when all the plugins have unloaded.
> +  for (size_t i = 0; i < plugins.Length(); i++) {

Nit: you could use a range based for loop here for semantic niceness?
Attachment #8707247 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #3)
> Comment on attachment 8707247 [details] [diff] [review]
> 1237842-unlock-mMutex-before-CloseActive.patch
> 
> Review of attachment 8707247 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/gmp/GMPServiceParent.cpp
> @@ +676,1 @@
> > +  for (size_t i = 0; i < plugins.Length(); i++) {
> 
> Nit: you could use a range based for loop here for semantic niceness?

You should tell that to whoever wrote this first! ;-)
But good idea, we might as well clean up some code while we're nearby.
Attachment #8707247 - Attachment is obsolete: true
Attachment #8707292 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6a44d73430aa
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.