1183433 - GMPParent crash report annotations shouldn't use too many key names

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

Spawned from bug 1175783 comment 14: "I'm worried that Socorro won't be happy with the explosion in keys (in particular I'm thinking of things like the "non-searchable keys" list on the admin page)"

The pointer value was used to differentiate potentially-concurrent connections to the same plugin (e.g. multiple encrypted videos), and see where each one ends up when a shutdown-hang happens.

I'll have to find a way to gather these per plugin, to use fewer keys.
E.g., could be something like: "AsyncPluginShutdown-clearkey":"{0x1234:stalled,0x5678:ended}"

Comment 1

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

After further discussion, we should avoid using GMP names in keys, so we don't have to worry about future plugins (if any) clogging up key lists.

So the one entry could be something like: "AsyncPluginShutdownStates":"{clearkey:{0x1:stalled}}". Exact scheme TBD.

Comment 2

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

Attachment: 1183433-p1-remove-extinct-method.patch

Removing GeckoMediaPServiceParent::AbortAsyncShutdown() declaration, as it's not defined nor used anywhere.

Comment 3

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

Attachment: 1183433-p2-make-timeout-function-class-static.patch

Making GMPParent's AbortWaitingForGMPAsyncShutdown() class-static (instead of a file-almost-static function), to simplify upcoming work.

Comment 4

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

Attachment: 1183433-p3-centralized-asyncshutdown-reporting.patch

Implementing centralized crash report annotations, so that all async-shutdown hangs reports use only one key, and also keep track of all shutdown-related states each plugin has been through.

Note: This is using a now-deprecated method nsClassHashtable::EnumerateRead(), to simplify uplift to < 42. If this review passes and the patch lands, I will immediately open a new bug (that won't need uplifting) to replace EnumerateRead with the new ConstIter iterator that appeared in 42. The code has already been written (assuming no major changes from this review), see: https://hg.mozilla.org/try/rev/31c76716e094

Comment 5

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

Attachment: 1183433-p4-gmpparent-use-new-asyncshutdown-reporting.patch

Replacing GMPParent's crash report annotations (using multiple keys) with the new GeckoMediaPluginServiceParent's centralized reporting.

Comment 6

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

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

Comment 7

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

Comment on attachment 8634400 [details] [diff] [review]
1183433-p3-centralized-asyncshutdown-reporting.patch

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

::: dom/media/gmp/GMPServiceParent.cpp
@@ +431,5 @@
> +    NS_LITERAL_CSTRING("AsyncPluginShutdownStates"),
> +    note);
> +}
> +
> +//static

Nit:

"// static"

That is, space between "//" and comment text. Ditto for earlier patches.

Comment 8

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

Attachment: 1183433-p2-make-timeout-function-class-static.patch

Nit addressed; carrying r+.

Comment 9

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

Attachment: 1183433-p3-centralized-asyncshutdown-reporting.patch

Nit addressed; carrying r+.

Comment 10

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

Attachment: 1183433-p4-gmpparent-use-new-asyncshutdown-reporting.patch

Nit addressed; carrying r+.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b843fd8927e
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a240e476877
https://hg.mozilla.org/integration/mozilla-inbound/rev/c08d8f212727
https://hg.mozilla.org/integration/mozilla-inbound/rev/48f3e3e08f5b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b843fd8927e
https://hg.mozilla.org/mozilla-central/rev/3a240e476877
https://hg.mozilla.org/mozilla-central/rev/c08d8f212727
https://hg.mozilla.org/mozilla-central/rev/48f3e3e08f5b

Comment 13

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

Uplifting to 41 please? (To improve previously-uplifted bug 1175783).

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

Comment 14

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

Cancelling uplift request, there's probably a bug in there, see bug 1185392.

Comment 15

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

Please consider uplifting to 41, followed by bug 1185392.

Rebased patches can be found in the try below (or let me know if I should attach them here); The main difference is in p4, because bug 1175765 was not uplifted -- Should it be too?

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

Comment 16

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

(In reply to Gerald Squelart [:gerald] from comment #15)
> Please consider uplifting to 41, followed by bug 1185392.
> 
> Rebased patches can be found in the try below (or let me know if I should
> attach them here); The main difference is in p4, because bug 1175765 was not
> uplifted -- Should it be too?

Yes, we should uplift that, it fixes a shutdown hang.

Comment 17

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

(In reply to Chris Pearce (:cpearce) from comment #16)
> (In reply to Gerald Squelart [:gerald] from comment #15)
> > Please consider uplifting to 41, followed by bug 1185392.
> > Rebased patches can be found in the try below (or let me know if I should
> > attach them here); The main difference is in p4, because bug 1175765 was not
> > uplifted -- Should it be too?
> Yes, we should uplift that, it fixes a shutdown hang.

If you uplift bug 1175765 first, the patches here should now just apply cleanly.

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

Comment 18

[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 19

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

Comment on attachment 8634568 [details] [diff] [review]
1183433-p4-gmpparent-use-new-asyncshutdown-reporting.patch

Approval Request Comment - for all patches in this bug

[Feature/regressing bug #]: EME

[User impact if declined]: These patches (and bug 1185392) are a necessary follow-up to bug 1175783 and bug 1175765, to ensure that annotations do not overload the Socorro server with many key names (see bug 1175783 comment 14).

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

[Risks and why]: Pretty low; this just moves crash reporter annotations to a centralized locations.

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

Comment 20

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

Comment on attachment 8634568 [details] [diff] [review]
1183433-p4-gmpparent-use-new-asyncshutdown-reporting.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 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ea05b312769
https://hg.mozilla.org/releases/mozilla-aurora/rev/89906a39a473
https://hg.mozilla.org/releases/mozilla-aurora/rev/d24f6bdde69b
https://hg.mozilla.org/releases/mozilla-aurora/rev/04ac446eef4c

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/407e4f2de1a3
https://hg.mozilla.org/releases/mozilla-beta/rev/9e5bafec2c68
https://hg.mozilla.org/releases/mozilla-beta/rev/603c57935f3d
https://hg.mozilla.org/releases/mozilla-beta/rev/dcabd9b629d6