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

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

That sounds like a good plan

Comment 2

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

Attachment: 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.

Comment 3

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

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?

Comment 4

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

Attachment: 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.

Comment 5

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0096fbb08b2e

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a44d73430aa

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a44d73430aa