Closed Bug 1038961 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: benjamin, Assigned: jesup)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 2 obsolete files)

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.
Assignee: nobody → rjesup
I attached the patch(es) I mentioned in bug 1039575, bug 1039577, bug 1039579. It was a little more complicated than I thought ;-)
Attachment #8459372 - Flags: review?(cpearce)
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.
Attachment #8459372 - Flags: review?(cpearce) → review+
Attachment #8459373 - Attachment is obsolete: true
Attachment #8459403 - Flags: review?(jib)
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.
Attachment #8459403 - Flags: review+
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?
Attachment #8459403 - Flags: review?(jib) → review+
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
Depends on: 1041232, 1041226
Target Milestone: --- → mozilla33
Depends on: 1041525
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)?
Flags: needinfo?(rjesup)
I'll push a followup ASAP
Flags: needinfo?(rjesup)
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.)
Attachment #8459732 - Flags: review?(ted)
Attachment #8459732 - Attachment is obsolete: true
Attachment #8459732 - Flags: review?(ted)
Attachment #8459740 - Flags: review?(ted)
Attachment #8459740 - Flags: review?(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)
Attachment #8459740 - Flags: review?(dholbert) → review+
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
Attachment #8459403 - Flags: approval-mozilla-aurora?
Attachment #8459403 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Attachment #8459740 - Flags: review?(ted) → review+
(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)
Whiteboard: [openh264-uplift]
Duplicate of this bug: 1042277
https://hg.mozilla.org/releases/mozilla-aurora/rev/cd2fe0d4f4a9

Comment 16 landed ahead of the merge, so only the follow-up needed uplift.
Target Milestone: mozilla33 → mozilla34
Whiteboard: [openh264-uplift]
You need to log in before you can comment on or make changes to this bug.