Closed Bug 1173634 Opened 4 years ago Closed 4 years ago

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

Categories

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

x86
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 - fixed
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 2 obsolete files)

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.
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)
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)
See Also: → 1175364
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+
Attachment #8622139 - Attachment is obsolete: true
Minor update as per review in comment 5. Carrying r+.
Attachment #8623415 - Attachment is obsolete: true
Attachment #8623980 - Flags: review+
Keywords: checkin-needed
Blocks: 1175765
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
Blocks: 1175783
https://hg.mozilla.org/mozilla-central/rev/52ee1ad644d1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Please consider uplifting to 40.
Flags: needinfo?(cpearce)
(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
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.
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+
See Also: → 1183971
See Also: → 1183972
Flags: needinfo?(cpearce)
This bug has already been fixed and is of itself not critical for the release. Tracking-
You need to log in before you can comment on or make changes to this bug.