This is a breakdown of GMP crash reporting in general. In the case of webrtc using a GMP plugin, when the plugin crashes, the crash needs to be communicated back to every DOM window that was using the plugin. ekr in email says: We create the GMP structures in: vcmEnsureExternalCodec(): http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp#68 Which is called from: http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp#1556 and http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp#2394 And in both cases these have a pointer to the PC (via PCWrapper). And the PC has a pointer to the DOMWindow: http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h#292 So you should be able to trace all this, modulo lifetime issues The signal of the actual crash is when the ActorDestroy method is called on a protocol, any of GMPParent/GMPVideoDecoderParent/GMPVideoEncoderParent. From that we need to end up notifying the DOM on the main thread. The actual DOM event should be an event "PluginCrashed" with the following properties: pluginName is the user-visible plugin name: pluginDumpID is the crash dump ID obtained from PCrashReporter submittedCrashReport is a boolean false (the UI may change it to true later) http://hg.mozilla.org/mozilla-central/annotate/835e22069c1a/content/base/src/nsObjectLoadingContent.cpp#l335 has equivalent code for NPAPI plugins. I'll attach a patch in a second which properly gets the crash report ID from ActorDestroy.

I attached the patch(es) I mentioned in bug 1039575, bug 1039577, bug 1039579. It was a little more complicated than I thought ;-)

These are WIP; compiles but untested

Comment on attachment 8459372 [details] [diff] [review] Patch 1 - Send GMP plugin crashes to observer, and implement PluginID system Review of attachment 8459372 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/gmp/GMPParent.cpp @@ +329,5 @@ > nsString dumpID; > #ifdef MOZ_CRASHREPORTER > GetCrashID(dumpID); > #endif > + nsString id; nsString id; id.AppendInt(reinterpret_cast<uint64_t>(this)); #ifdef MOZ_CRASHREPORTER nsString dumpID; GetCrashID(dumpID); id.Append(NS_LITERAL_STRING(" ")); id.Append(dumpID); #endif ?? Then you don't append a phantom " " in the non-crash reporter case.

Comment on attachment 8459403 [details] [diff] [review] Patch 2 - Associate GMP plugin crash with a window and notify it r=me on the webidl bit if the return value is documented.

Comment on attachment 8459403 [details] [diff] [review] Patch 2 - Associate GMP plugin crash with a window and notify it Review of attachment 8459403 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits. ::: dom/media/PeerConnection.js @@ +116,5 @@ > + if (pc._pc.pluginCrash(pluginID)) { > + // Notify DOM window of the crash > + let event = new CustomEvent("PluginCrashed", > + { bubbles: false, cancelable: false, detail: { > + pluginName: name, pluginDumpId: crashReport, submittedCrashReport: false }}); not the prettiest indent ever. @@ +154,5 @@ > } > + else if (topic == "gmp-plugin-crash") { > + // a plugin crashed; if it's associated with any of our PCs, fire an > + // event to the DOM window > + let sep = data.indexOf(' '); I guess ' ' is guaranteed here so no need to test sep != -1. @@ +160,5 @@ > + let crashId = data.slice(sep+1); > + // This presumes no spaces in the name! > + sep = crashId.indexOf(' '); > + let name = crashId.slice(0, sep); > + let crashId = crashId.slice(sep+1); I would rename the second crashId here to id to keep things clear (or rename the temp one tmp). @@ +163,5 @@ > + let name = crashId.slice(0, sep); > + let crashId = crashId.slice(sep+1); > + for (let winId in this._list) { > + hasPluginId(this._list, winId, pluginId, name, crashId); > + } whitespace ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +1659,5 @@ > +bool > +PeerConnectionImpl::PluginCrash(uint64_t aPluginID) > +{ > + // fire an event to the DOM window if this is "ours" > + bool result = mMedia->AnyCodecHasPluginID(aPluginID); check mMedia? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ +655,5 @@ > +bool > +LocalSourceStreamInfo::AnyCodecHasPluginID(uint64_t aPluginID) > +{ > + // Scan the videoConduits for this plugin ID > + for (std::map<int, mozilla::RefPtr<mozilla::MediaPipeline> >::iterator it = auto? @@ +669,5 @@ > +bool > +RemoteSourceStreamInfo::AnyCodecHasPluginID(uint64_t aPluginID) > +{ > + // Scan the videoConduits for this plugin ID > + for (std::map<int, mozilla::RefPtr<mozilla::MediaPipeline> >::iterator it = auto?

With some nits discussed/r+'d over IRC https://hg.mozilla.org/integration/mozilla-inbound/rev/354f9f82b857 https://hg.mozilla.org/integration/mozilla-inbound/rev/0a6154bfb261 Note: bug 1041232 gets in the way here, as well as bug 1041226

Build error in --disable-webrtc builds: > content/media/gmp/GMPParent.cpp:18:39: fatal error: mtransport/runnable_utils.h: No such file or directory Appears to be caused by "part 1" here, which added that #include in GMPParent.cpp. jesup, should we spin off a helper-bug for this, or do you want to push a followup (or backout & reland w/ #ifdeffing added)?

I'll push a followup ASAP

while you're at it, I also get the following build error in a --disable-crashreporter--enable-warnings-as-errors build, because the only usage of this function is in a #ifdef MOZ_CRASHREPORTER block: { content/media/gmp/GMPParent.cpp:327:1: error: 'void mozilla::gmp::GMPNotifyObservers(nsAString_internal&)' defined but not used [-Werror=unused-function] } (That function-definition should be #ifdeffed as well.)

also quell a warning

Comment on attachment 8459740 [details] [diff] [review] Fix --disable_webrtc breakage due to mtransport/runnable_utils Thanks! Looks good to me, and fixes my build. (with the build options mentioned in comment 9 & comment 11)

Comment on attachment 8459403 [details] [diff] [review] Patch 2 - Associate GMP plugin crash with a window and notify it This is for the set of patches on this bug Approval Request Comment [Feature/regressing bug #]:NA [User impact if declined]: We need Plugin crashreporting for GMP plugins, and this is a critical part of that [Describe test coverage new/current, TBPL]: The entire crashreporting feature will have test coverage (there are some other bits that need to uplift as well). [Risks and why]: low risk, early in cycle. Missed uplift by hours due to working on more critical issues. [String/UUID change made/needed]: none

https://hg.mozilla.org/mozilla-central/rev/354f9f82b857 https://hg.mozilla.org/mozilla-central/rev/0a6154bfb261

Comment on attachment 8459740 [details] [diff] [review] Fix --disable_webrtc breakage due to mtransport/runnable_utils Review of attachment 8459740 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/gmp/GMPParent.cpp @@ +332,5 @@ > nsString temp(aData); > obs->NotifyObservers(nullptr, "gmp-plugin-crash", temp.get()); > } > } > +#endif This doesn't seem actively harmful in the non-crashreporter case, does it really need to be ifdefed out?

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #17) > This doesn't seem actively harmful in the non-crashreporter case, does it > really need to be ifdefed out? Yes; otherwise, it triggers a "defined but not used" build warning. (see comment 11)

https://hg.mozilla.org/releases/mozilla-aurora/rev/cd2fe0d4f4a9 Comment 16 landed ahead of the merge, so only the follow-up needed uplift.