1173634 - [EME] Async shutdown hang in mozilla::gmp::GeckoMediaPluginService::Observe(...) - Add crash report annotations while waiting for plugin async shutdown

Status: RESOLVED FIXED

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

Description

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

Spawned from bug 1164264 comment 16:
"""
Now that [the patch in bug 1164264] has been applied, and we are still getting reports from the same location (waiting for GMPs to shutdown), at least we know the problem is not that we are wrongly waiting for nothing!

Unfortunately I cannot see an obvious reason for the hangs, as it may be happening in the plugin itself, whose information is not included in these reports.

The strange thing is that there are already timeouts around GMP handling, which should prevent these long hangs. As per my bug 1164264 comment 4, my next recommended step would be to add a timeout in GMPService itself and make it crash the plugin so that hopefully we can get more information that way.
"""

Another idea is to add recognizable patterns in reports' stack traces to determine which plugin(s) are running when the hang happens.

Comment 1

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

Attachment: 1173634-gmpservice-crashreport-plugins.patch

Starting with better reporting: While shutting down async-shutdown plugins, a new crash report annotation 'AsyncPluginShutdown' contains the names of all plugins we are waiting for.

Comment 2

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

Comment on attachment 8622139 [details] [diff] [review]
1173634-gmpservice-crashreport-plugins.patch

Getting in similar troubles as with bug 1173631 comment 5 (shutdown crash after using both clearkey and h264 webrtc).
I need to investigate what this seemingly-harmless patch triggers.

Comment 3

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

Attachment: 1173634-gmpservice-crashreport-plugins.patch

This patch was not the cause of the issue seen above (see bug 1175364), so it's back on the review board.
Better version now anyway, with some debug logging, and more efficient updating of the crash report annotation (only when needed, not after every single event).

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f0233f22650

Comment 5

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

Comment on attachment 8623415 [details] [diff] [review]
1173634-gmpservice-crashreport-plugins.patch

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

::: dom/media/gmp/GMPServiceParent.cpp
@@ +231,4 @@
>      // an event to do this, as we must ensure the main thread processes an
>      // event to run its loop. This will unblock the main thread, and shutdown
>      // of other components will proceed.
>      //

Please add a note here that indicates that there is a timeout on async shutdown.

Comment 6

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

Attachment: 1173634-gmpservice-crashreport-plugins.patch

Minor update as per review in comment 5. Carrying r+.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52ee1ad644d1

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/52ee1ad644d1

Comment 9

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

Please consider uplifting to 40.

Comment 10

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

(In reply to Gerald Squelart [:gerald] from comment #9)
> Please consider uplifting to 40.

Wrong number, this patch is already in 41 (now Aurora).
Please uplift to 42 (Beta).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=164bf7c61327

Comment 11

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

I thought I made a mistake, but I was wrong.

So: Patch is in 41 (Aurora), should be uplifted to 40 (Beta).

Comment 12

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

Comment on attachment 8623980 [details] [diff] [review]
1173634-gmpservice-crashreport-plugins.patch

Approval Request Comment
[Feature/regressing bug #]: EME

[User impact if declined]: We're seeing some shutdown hangs waiting for EME or OpenH264 plugins to shutdown. This patch adds "annotations" to the crash report generated to help diagnose these failures. So user impact if declined is that it will take us longer to debug the shutdown hangs.

[Describe test coverage new/current, TreeHerder]: Lots of EME mochitests.

[Risks and why]: Fairly low. This has been on Nightly/Aurora for a while now, so seems safe.

[String/UUID change made/needed]: None.

Comment 13

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

[Tracking Requested - why for this release]: We should get this patch uplifted, so that we can get shutdown hangs resolved ASAP.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8623980 [details] [diff] [review]
1173634-gmpservice-crashreport-plugins.patch

Hope this will help, taking it.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/93c229409478

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

This bug has already been fixed and is of itself not critical for the release. Tracking-