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

RESOLVED FIXED in Firefox 46

Status

()

Core
Audio/Video: GMP
P1
normal
Rank:
15
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

43 Branch
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 2

2 years ago
Created attachment 8707247 [details] [diff] [review]
1237842-unlock-mMutex-before-CloseActive.patch

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+
(Assignee)

Comment 4

2 years ago
Created attachment 8707292 [details] [diff] [review]
1237842-unlock-mMutex-before-CloseActive.patch v2

(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+

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6a44d73430aa
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.