Closed Bug 1183433 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Audio/Video, defect)

41 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 --- fixed
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

(Blocks 1 open bug)

Details

Crash Data

Attachments

(4 files, 3 obsolete files)

898 bytes, patch
cpearce
: review+
Details | Diff | Splinter Review
2.18 KB, patch
mozbugz
: review+
Details | Diff | Splinter Review
5.93 KB, patch
mozbugz
: review+
Details | Diff | Splinter Review
8.99 KB, patch
mozbugz
: review+
Details | Diff | Splinter Review
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}"
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.
Removing GeckoMediaPServiceParent::AbortAsyncShutdown() declaration, as it's not defined nor used anywhere.
Attachment #8634391 - Flags: review?(cpearce)
Making GMPParent's AbortWaitingForGMPAsyncShutdown() class-static (instead of a file-almost-static function), to simplify upcoming work.
Attachment #8634394 - Flags: review?(cpearce)
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
Attachment #8634400 - Flags: review?(cpearce)
Replacing GMPParent's crash report annotations (using multiple keys) with the new GeckoMediaPluginServiceParent's centralized reporting.
Attachment #8634402 - Flags: review?(cpearce)
Attachment #8634391 - Flags: review?(cpearce) → review+
Attachment #8634394 - Flags: review?(cpearce) → review+
Attachment #8634402 - Flags: review?(cpearce) → review+
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.
Attachment #8634400 - Flags: review?(cpearce) → review+
Nit addressed; carrying r+.
Attachment #8634394 - Attachment is obsolete: true
Attachment #8634566 - Flags: review+
Nit addressed; carrying r+.
Attachment #8634400 - Attachment is obsolete: true
Attachment #8634567 - Flags: review+
Nit addressed; carrying r+.
Attachment #8634402 - Attachment is obsolete: true
Attachment #8634568 - Flags: review+
Uplifting to 41 please? (To improve previously-uplifted bug 1175783).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc36c544287b
Flags: needinfo?(cpearce)
Cancelling uplift request, there's probably a bug in there, see bug 1185392.
Flags: needinfo?(cpearce)
See Also: → 1185392
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
Depends on: 1185392
Flags: needinfo?(cpearce)
Blocks: 1185392
No longer depends on: 1185392
(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.
(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 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.
Attachment #8634568 - Flags: approval-mozilla-beta?
Attachment #8634568 - Flags: approval-mozilla-aurora?
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+
Attachment #8634568 - Flags: approval-mozilla-beta?
Attachment #8634568 - Flags: approval-mozilla-beta+
Attachment #8634568 - Flags: approval-mozilla-aurora?
Attachment #8634568 - Flags: approval-mozilla-aurora+
Attachment #8634567 - Flags: approval-mozilla-beta+
Attachment #8634567 - Flags: approval-mozilla-aurora+
Attachment #8634566 - Flags: approval-mozilla-beta+
Attachment #8634566 - Flags: approval-mozilla-aurora+
Attachment #8634391 - Flags: approval-mozilla-beta+
Attachment #8634391 - Flags: approval-mozilla-aurora+
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.