1175765 - [EME] Async shutdown hang in mozilla::gmp::GeckoMediaPluginService::Observe(...) - Improve timer in GMPParent

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

Follow-up to bug 1173634, where the patch adds the list of pending GMPs to crash reports.

In this bug, a timer will be added in GMPService itself to force the hung plugin(s) to crash -- or do something else if more appropriate.

Comment 1

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

After further study, there used to be a timer in GMPService but it was moved to GMPParent to catch timeouts in non-xpcom-shutdown cases, see bug 1085168. So re-adding a timer in GMPService may help with this particular shutdown hang, but would not actually cover all possible hangs.

The current GMPParent timer is only started when async-shutdown proper starts, which is after CloseActive completes (when receiving an expected number of RecvPGMPContentChildDestroyed messages); So if this first asynchronous process stalls, there is no timeout to bail from it.

The timer should therefore be started earlier, to cover the CloseActive-RecvPGMPContentChildDestroyed part of the shutdown process.

Comment 2

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

Attachment: 1175765-gmpparent-closeactive-timer.patch

The main goal of this patch is to start the async-shutdown watchdog timer from CloseActive(), in case we don't receive the expected RecvPGMPContentChildDestroyed's in time.

In addition:
- Some (!a||b) tests have been split to reduce (my) confusion, but more importantly to get more precise crash report annotations.
- A (rare) case is handled, where CloseActive() is called but all content children are already destroyed, in which case CloseIfUnused() is called again to give it a chance to run async-or-not shutdown processes if not already done.

Comment 3

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

Comment on attachment 8629166 [details] [diff] [review]
1175765-gmpparent-closeactive-timer.patch

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

::: dom/media/gmp/GMPParent.cpp
@@ +364,5 @@
> +#endif
> +      AbortAsyncShutdown();
> +    } else if (IsUsed()) {
> +      // We're expecting RecvPGMPContentChildDestroyed's -> Start async-shutdown timer now.
> +      if (NS_FAILED(EnsureAsyncShutdownTimeoutSet())) {

We don't actually need the timer if the plugin doesn't require async shutdown, right? i.e. mAsyncShutdownRequired==false? 

So please make this:

  if (mAsyncShutdownRequired && NS_FAILED(EnsureAsyncShutdownTimeoutSet()))

Please also add an MOZ_ASSERT(mAsyncShutdownRequired) in EnsureAsyncShutdownTimeoutSet().

Comment 4

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

Attachment: 1175765-gmpparent-closeactive-timer.patch

Updated as per comment 3; carrying r+.

Comment 5

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

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

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68602685db0f

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/68602685db0f

Comment 8

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

Please uplift to 41, followed by bug 1183433 then bug 1185392.

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

Comment 9

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

Please uplift the following patches together to beta/40: bug 1175783, bug 1175765, bug 1183433, bug 1185392.

BETA try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b70f2bca3b28

Comment 10

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

Comment on attachment 8629542 [details] [diff] [review]
1175765-gmpparent-closeactive-timer.patch

Approval Request Comment

[Feature/regressing bug #]: EME

[User impact if declined]: We're seeing crash reports which are shutdown hangs on Beta40 waiting for either EME plugins or OpenH264 to shutdown.
This patch starts the timer as soon as the shutdown is requested (instead of later in the process, which may miss a cause of hangs); It also adds further and more precise annotations, so that we can debug and fix these shutdown hangs.
Denying uplift on this patch will make it take longer for us to fix the shutdown hangs.
Also uplift candidates bug 1183433 and bug 1185392 work on top of this patch.

[Describe test coverage new/current, TreeHerder]: We have lots of EME mochitests.

[Risks and why]: Pretty low; this just extends an existing timer to include more of the shutdown process, and adds crash reporter annotations.

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

Comment 11

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

Comment on attachment 8629542 [details] [diff] [review]
1175765-gmpparent-closeactive-timer.patch

I asked rillian and jesup to review the series of patches for EME as well because we're late in the 40 cycle. Their suggestion is to take the changes as they put us in a better state that we were in previously and because waiting an additional ~2.5 weeks for 41 to hit Beta will be detrimental. I'm approving for Beta. We need to watch the beta8 build to ensure to the best of our ability that we have not regressed anything related to WebRTC or EME. Beta+ Aurora+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c9613dd57ab0

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b54849220095