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

RESOLVED FIXED in Firefox 40

Status

()

P2
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

(Blocks: 1 bug, {crash})

unspecified
mozilla41
x86
Windows
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40- fixed, firefox41 fixed, firefox42 fixed)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8622139 [details] [diff] [review]
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.
Attachment #8622139 - Flags: review?(cpearce)
(Assignee)

Comment 2

3 years ago
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.
Attachment #8622139 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
See Also: → bug 1175364
(Assignee)

Comment 3

3 years ago
Created attachment 8623415 [details] [diff] [review]
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).
Attachment #8623415 - Flags: review?(cpearce)
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.
Attachment #8623415 - Flags: review?(cpearce) → review+
(Assignee)

Updated

3 years ago
Attachment #8622139 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
Created attachment 8623980 [details] [diff] [review]
1173634-gmpservice-crashreport-plugins.patch

Minor update as per review in comment 5. Carrying r+.
Attachment #8623415 - Attachment is obsolete: true
Attachment #8623980 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Blocks: 1175765
(Assignee)

Updated

3 years ago
Summary: [EME] Async shutdown hang in mozilla::gmp::GeckoMediaPluginService::Observe(...) → [EME] Async shutdown hang in mozilla::gmp::GeckoMediaPluginService::Observe(...) - Add crash report annotations while waiting for plugin async shutdown
(Assignee)

Updated

3 years ago
Blocks: 1175783
https://hg.mozilla.org/mozilla-central/rev/52ee1ad644d1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Comment 9

3 years ago
Please consider uplifting to 40.
Flags: needinfo?(cpearce)
(Assignee)

Comment 10

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

Comment 11

3 years ago
I thought I made a mistake, but I was wrong.

So: Patch is in 41 (Aurora), should be uplifted to 40 (Beta).
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.
Attachment #8623980 - Flags: approval-mozilla-beta?
[Tracking Requested - why for this release]: We should get this patch uplifted, so that we can get shutdown hangs resolved ASAP.
status-firefox40: --- → affected
status-firefox42: --- → fixed
tracking-firefox40: --- → ?
Comment on attachment 8623980 [details] [diff] [review]
1173634-gmpservice-crashreport-plugins.patch

Hope this will help, taking it.
Attachment #8623980 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Updated

3 years ago
See Also: → bug 1183971
(Assignee)

Updated

3 years ago
See Also: → bug 1183972
Flags: needinfo?(cpearce)
This bug has already been fixed and is of itself not critical for the release. Tracking-
tracking-firefox40: ? → -
You need to log in before you can comment on or make changes to this bug.