1183972 - [GMP] Async shutdown hang in mozilla::gmp::GeckoMediaPluginService::Observe(...) in 39-44b

Status: RESOLVED FIXED

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

Description

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

This bug will track async shutdown hangs starting in 40, previously seen in bug 1183971 (and others) with a slightly different signature.

Comment 1

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

I can repro this in Nightly if I kill the Nightly.exe process using task manager. For example:
https://crash-stats.mozilla.com/report/index/abe78601-328f-48a6-a64f-70f622150719#allthreads

I had e10s disabled.

Comment 2

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

Getting reports from 41.0 and 42.0b1 as well.

Comment 3

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

Update:
Thanks to bug 1211337, recent crash reports now reveal that 'GeckoMediaPluginServiceParent::UnloadPlugins' never runs after being dispatched (from the main thread to the GMP thread).

Some reports don't show an obviously-blocked thread (to me), e.g.:
https://crash-stats.mozilla.com/report/index/d2e2bf43-b0f8-445c-bacb-2ba2b2151116

Some others, e.g.:
https://crash-stats.mozilla.com/report/index/99dff33e-1b00-4f8d-a9b4-039262151116
https://crash-stats.mozilla.com/report/index/8e220589-5071-4676-b16d-c9cbf2151117
show the GMP thread trying to do a sync-dispatch to the main thread, while the main thread is in the |while({NS_ProcessNextEvent}| loop waiting for UnloadPlugins to finish; maybe there's some unforeseen issue with this situation.
To be analyzed further...

Comment 4

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

(In reply to Gerald Squelart [:gerald] from comment #3)
> https://crash-stats.mozilla.com/report/index/99dff33e-1b00-4f8d-a9b4-039262151116
> [...]
> show the GMP thread trying to do a sync-dispatch to the main thread, while
> the main thread is in the |while({NS_ProcessNextEvent}| loop waiting for
> UnloadPlugins to finish; maybe there's some unforeseen issue with this
> situation.
> To be analyzed further...

Looking at the report linked above, the main thread has dispatched a task to run UnloadPlugins on the GMP thread, but thread 24 (which seems to be the GMP thread) is busy doing a sync-dispatch to run CreateGMPParentTask on the main thread!

NI:cpearce as requested, if you see an easy fix. I'll comment here if I can work on it in the meantime.

Comment 5

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

After discussing with Chris, we think it should be safe to remove the sync-dispatch to create GMPParent from GeckoMediaPluginServiceParent::ClonePlugin() and AddOnGMPThread(). (It used to be necessary, because GMPParent had to be created and destroyed on the main thread; this constraint was removed in bug 1121676.)

Working on a patch now.

Comment 6

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

Attachment: 1183972-no-sync-dispatch.patch

No sync-dispatch of new GMPParent.

Thanks to bug 1121676, GMPParent does not need to be created and destroyed on the main thread. Main-thread constraints have been removed.

Also, this means that GeckoMediaPluginServiceParent::ClonePlugin() and AddOnGMPThread (running on the GMP thread) do not need to sync-dispatch the creation on the main thread.
This should remove the deadlock that prevents GeckoMediaPluginServiceParent::UnloadPlugins() from running on the GMP thread.

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

Comment 7

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

Comment on attachment 8697462 [details] [diff] [review]
1183972-no-sync-dispatch.patch

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

::: dom/media/gmp/GMPServiceParent.cpp
@@ +951,3 @@
>  #if defined(XP_LINUX) && defined(MOZ_GMP_SANDBOX)
> +  if (!SandboxInfo::Get().CanSandboxMedia()) {
> +    if (!Preferences::GetBool("media.gmp.insecure.allow")) {

This asserts that it's only called on the main thread, so I think this isn't going to work... You need to cache this pref I think.

Comment 8

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

Attachment: 1183972-no-sync-dispatch.patch v2

(In reply to Chris Pearce (:cpearce) from comment #7)
> Comment on attachment 8697462 [details] [diff] [review]
> 1183972-no-sync-dispatch.patch
> 
> Review of attachment 8697462 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/gmp/GMPServiceParent.cpp
> @@ +951,3 @@
> >  #if defined(XP_LINUX) && defined(MOZ_GMP_SANDBOX)
> > +  if (!SandboxInfo::Get().CanSandboxMedia()) {
> > +    if (!Preferences::GetBool("media.gmp.insecure.allow")) {
> 
> This asserts that it's only called on the main thread, so I think this isn't
> going to work... You need to cache this pref I think.

Good catch, thank you. Fixed in this patch.

Comment 9

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

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

Comment 10

[Carsten Book [:Tomcat]]

this failed to apply:

Hunk #2 FAILED at 939
Hunk #3 FAILED at 1002
3 out of 3 hunks FAILED -- saving rejects to file dom/media/gmp/GMPServiceParent.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1183972.patch

Comment 11

[Carsten Book [:Tomcat]]

ah never mind my bad :)

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d563e290ca5

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d563e290ca5

Comment 14

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

Added slightly-different crash signature for 44b.