Closed Bug 1146955 Opened 9 years ago Closed 9 years ago

Make GMP plugin crash reporting UI work in e10s

Categories

(Core Graveyard :: Plug-ins, defect)

x86
macOS
defect
Not set
normal

Tracking

(e10sm6+, firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
e10s m6+ ---
firefox40 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

STR:

0) Ensure bug 1057908 is resolved, or apply the patches from that bug for Open H.264 support in e10s.
1) Ensure you are on a machine with a video capture device for streaming
2) In an e10s window, browse to https://mozilla.github.io/webrtc-landing/pc_test.html
3) Ensure that Require H.264 video is checked
4) Click "Start"
5) Approve Firefox's usage of your video capture device
6) Find the plugin-container that just started, and either use crashfirefox[1] on that process, or kill -SIGABRT it.

ER:

A notification bar should appear alerting the user to the crash and giving them the ability to submit a crash report.

AR:

No notification bar. This works properly in non-e10s mode.

The other use of GMP (EME) will also suffer from this problem in e10s even after bug 1135541 is fixed. This is because the code that bubbles up the PluginCrashed event in content is assuming that it will hear the gmp-plugin-crash observer notification, which is not true in the e10s case (the parent process will hear it, not the content process).

We should probably use a solution similar to the one in bug 1110887, where the parent process messages all content processes when it detects the crash, passing along a unique run ID. The PluginCrashed events that bubble up should probably also include that run ID for GMP plugins. The PluginContent.jsm's in the content processes will then receive the message from the parent and hear the event (it's non-deterministic which will arrive first, so we have to account for both cases), and once both arrive with matching run IDs, the content should message the parent saying "yes, I'm running this recently crashed GMP", which will cause the notification bar to display in the parent.

Something like that, anyhow.

[1]: https://github.com/bsmedberg/crashfirefox-intentionally/
See Also: → 1110887
Assignee: nobody → mconley
Assuming we go the run ID route, we might want to have GMP / NPAPI use the same static counter to get run IDs, otherwise we'll need two separate collections for crash data, since run IDs might collide.
Status: NEW → ASSIGNED
Flags: needinfo?(wmccloskey)
Just to reiterate what we talked about during the triage meeting - the hard part (in my mind) is making the PluginCrashed event fire in the content process, and having it include some kind of runID in its property bag. If we have that, then the rest of this I can take care of pretty easily.

Note that the PluginCrashed event does not need to pass up the pluginDumpID. We just need a run ID, and the gmpPlugin argument set.
Mike, I think this is what you were looking for from Bill, let me know otherwise.
Flags: needinfo?(wmccloskey) → needinfo?(mconley)
So I think this'll work, but PeerConnectionImpl is still using the plugin ID to match instances with the crash notification. billm advised that we switch to something like run IDs for NPAPI plugins to avoid the following situation:

1) NPAPI Flash plugin A is running in the browser
2) plugin A crashes, just as NPAPI Flash plugin B starts up in the same browser in a new process
3) Parent sends down a message about crash of the Flash plugin
4) Both A and B are shown in the crashed state even though B is still running.

So, assuming there are similar concerns about GMP's, we'd need to add a run ID to distinguish between different "runnings" of the GMP, and pass that through the gmp-plugin-crash observer notification, and down to the content process for the PluginCrashed event.

billm, can I assume the same issues we ran into with NPAPI would apply for GMPs, requiring us to use run IDs to avoid mixups?
Flags: needinfo?(mconley) → needinfo?(wmccloskey)
If mapping on plugin IDs is considered low risk for collisions though, I can work with this patch to fix GMP crash reporting for e10s.

Just let me know if we want to go the run ID route, or take the plugin ID route and fix the run ID case in a follow-up, etc.
I'm not familiar with PeerConnection.js at all. But if this is the same plugin ID as appears in the GMP code, then it is already a run ID. That is, we create a new one for each GMPParent instance. So I think it should be fine to use.
Flags: needinfo?(wmccloskey)
Yeah, ok - I see that: the mPluginId on the GMPParent is casted from the memory address of the GMPParent. I suppose we can use that as the "run ID" in this case.

Alright great - I'm unblocked here. Thanks all!
Hrm. So the patch attached to this bug doesn't actually seem to work... the PeerConnection.js script is not guaranteed to be running in the parent process.

I'm looking for alternative mechanisms to send this message down now.
/r/7685 - Bug 1146955 - Unify pluginID for GMP and runID for NPAPI plugins to use the same internal incrementor. r=?
/r/7689 - Bug 1146955 - Make the GMP pluginID a uint32_t, and dispatch it in the PluginCrashed event. r=?
/r/7687 - Bug 1146955 - Dispatch PluginCrashed event in content process on GMP crash for PeerConnection. r=?
/r/7691 - Bug 1146955 - Have PluginContent and PluginCrashReporter work with GMPs and pluginIDs. r=?

Pull down these commits:

hg pull -r de57d6ae9f4fd423268ace894e2ec9ba4368e3ac https://reviewboard-hg.mozilla.org/gecko/
Blocks: 1046052
Comment on attachment 8598190 [details]
MozReview Request: bz://1146955/mconley

/r/7685 - Bug 1146955 - Unify pluginID for GMP and runID for NPAPI plugins to use the same internal incrementor. r=?
/r/7689 - Bug 1146955 - Make the GMP pluginID a uint32_t, and dispatch it in the PluginCrashed event. r=?
/r/7687 - Bug 1146955 - Dispatch PluginCrashed event in content process on GMP crash for PeerConnection. r=?
/r/7691 - Bug 1146955 - Have PluginContent and PluginCrashReporter work with GMPs and pluginIDs. r=?

Pull down these commits:

hg pull -r 9bd70c58c5e677f2e3ba55de00b9af02420a2b76 https://reviewboard-hg.mozilla.org/gecko/
So this works, but I have a few comments:

1) I'm not sure if PluginCrashReporter should be charged with informing the content process of GMP crashes. That seems wrong because PluginCrashReporter is in a browser module...

I suspect what we should do is have the GMP backend fire the PluginCrashed event itself, if that's even possible.

2) There's the notion of a "runID" that I added in bug 1110887, which is a unique identifier per plugin-container for NPAPI plugins. I wanted to use something similar for GMP's, and it looked like a "pluginID" already existed that had the same uniqueness requirements.

So I've shifted the GMP pluginID to use the same counter as NPAPI plugins' runIDs, so that the front-end can be unified without risk of ID collisions.

This means I've got some places that refer to "run IDs", and some places that refer to "plugin IDs", which isn't amazing. Unfortunately, NPAPI plugins already have something called pluginIDs, but they do not follow the same semantics as the run ID or GMP pluginID. :/

Somewhat related, we seem to use both pluginId and pluginID in various places, and that was kinda frustrating.

3) I changed the signature of the gmp-plugin-crash notification to pass a property bag, like NPAPI plugins, and changed the plugin ID to be a uint32_t instead of a string.

This had me modify chunks of dom/media and media that I'm entirely unfamiliar with, so if I did wrong things down there, that's why.

So I think I want some feedback from somebody who's more familiar with this stuff... gfritzsche, do I have the right idea here?
Flags: needinfo?(gfritzsche)
gfritzsche gave me what amounts to a feedback+ over IRC, but suggested I run the GMP backend changes by rjesup.
Flags: needinfo?(gfritzsche)
Attachment #8598190 - Flags: review?(rjesup)
Attachment #8598190 - Flags: review?(felipc)
Comment on attachment 8598190 [details]
MozReview Request: bz://1146955/mconley

/r/7685 - Bug 1146955 - Unify pluginID for GMP and runID for NPAPI plugins to use the same internal incrementor. r=?
/r/7689 - Bug 1146955 - Make the GMP pluginID a uint32_t, and dispatch it in the PluginCrashed event. r=?
/r/7687 - Bug 1146955 - Dispatch PluginCrashed event in content process on GMP crash for PeerConnection. r=?
/r/7691 - Bug 1146955 - Have PluginContent and PluginCrashReporter work with GMPs and pluginIDs. r=?

Pull down these commits:

hg pull -r 9bd70c58c5e677f2e3ba55de00b9af02420a2b76 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8598190 [details]
MozReview Request: bz://1146955/mconley

https://reviewboard.mozilla.org/r/7683/#review6753

Looks good; perhaps a comment that bits are MainThread-only, but that's probably superfluous
Attachment #8598190 - Flags: review?(rjesup) → review+
Comment on attachment 8598190 [details]
MozReview Request: bz://1146955/mconley

/r/7685 - Bug 1146955 - Unify pluginID for GMP and runID for NPAPI plugins to use the same internal incrementor. r=jesup.
/r/7689 - Bug 1146955 - Make the GMP pluginID a uint32_t, and dispatch it in the PluginCrashed event. r=jesup.
/r/7687 - Bug 1146955 - Dispatch PluginCrashed event in content process on GMP crash for PeerConnection. r=jesup.
/r/7691 - Bug 1146955 - Have PluginContent and PluginCrashReporter work with GMPs and pluginIDs. r=felipe.

Pull down these commits:

hg pull -r f82371eba570a6977a5125d6406cae5ebd5e3d15 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8598190 - Flags: review?(felipc)
Attachment #8598190 - Flags: review+
So while this works for the e10s case, for the non-e10s case, I seem to be crashing the whole browser.

Investigating now...
Attachment #8598190 - Flags: review?(mrbkap)
Comment on attachment 8598190 [details]
MozReview Request: bz://1146955/mconley

/r/7685 - Bug 1146955 - Unify pluginID for GMP and runID for NPAPI plugins to use the same internal incrementor. r=jesup.
/r/7689 - Bug 1146955 - Make the GMP pluginID a uint32_t, and dispatch it in the PluginCrashed event. r=jesup.
/r/7687 - Bug 1146955 - Dispatch PluginCrashed event in content process on GMP crash for PeerConnection. r=jesup.
/r/7691 - Bug 1146955 - Have PluginContent and PluginCrashReporter work with GMPs and pluginIDs. r=felipe.

Pull down these commits:

hg pull -r f82371eba570a6977a5125d6406cae5ebd5e3d15 https://reviewboard-hg.mozilla.org/gecko/
Looks like I can reproduce that crash even without my patches, so I'll file separately.

It turns out I need a DOM peer to sign off on my webidl changes, so r?ing mrbkap.
Comment on attachment 8598190 [details]
MozReview Request: bz://1146955/mconley

https://reviewboard.mozilla.org/r/7683/#review6873

Ship It!
Attachment #8598190 - Flags: review?(mrbkap) → review+
Comment on attachment 8598190 [details]
MozReview Request: bz://1146955/mconley

/r/7685 - Bug 1146955 - Unify pluginID for GMP and runID for NPAPI plugins to use the same internal incrementor. r=jesup.
/r/7689 - Bug 1146955 - Make the GMP pluginID a uint32_t, and dispatch it in the PluginCrashed event. r=jesup.
/r/7687 - Bug 1146955 - Dispatch PluginCrashed event in content process on GMP crash for PeerConnection. r=jesup.
/r/7691 - Bug 1146955 - Have PluginContent and PluginCrashReporter work with GMPs and pluginIDs. r=felipe.

Pull down these commits:

hg pull -r f82371eba570a6977a5125d6406cae5ebd5e3d15 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8598190 - Flags: review+
Thanks!
Depends on: 1161587
Attachment #8598190 - Attachment is obsolete: true
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.