1038961 - Associate a GMP crash with a DOM window and fire an event at that window

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Benjamin Smedberg]

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.

Comment 1

[Benjamin Smedberg]

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

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 1 - Send GMP plugin crashes to observer, and implement PluginID system

These are WIP; compiles but untested

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 2 - Associate GMP plugin crash with a window and notify it

Comment 4

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

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 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch 2 - Associate GMP plugin crash with a window and notify it

Comment 6

[Boris Zbarsky [:bzbarsky]]

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 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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?

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 9

[Daniel Holbert [:dholbert]]

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)?

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

I'll push a followup ASAP

Comment 11

[Daniel Holbert [:dholbert]]

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.)

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Fix --disable_webrtc breakage due to mtransport/runnable_utils

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Fix --disable_webrtc breakage due to mtransport/runnable_utils

also quell a warning

Comment 14

[Daniel Holbert [:dholbert]]

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 15

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 16

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

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

Comment 17

[(not currently active) Ted Mielczarek]

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?

Comment 18

[Daniel Holbert [:dholbert]]

(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)

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c375c6d129d

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c375c6d129d

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16 landed ahead of the merge, so only the follow-up needed uplift.