1057484 - Determine whether gPluginHandler needs to handle the gmp-plugin-crash notification

Status: RESOLVED INVALID

(Firefox :: General, defect)

Description

[Drew Willcoxon :adw]

+++ This bug was initially created as a clone of Bug #1053473 +++

For non-GMP plugin crashes, there's a plugin-crashed observer service notification and a PluginCrashed DOM event fired in the page.  gPluginHandler in browser-plugins.js ends up handling both [1,2], in different ways.

For GMP plugin crashes, there's the same PluginCrashed DOM event [3], but there's also a gmp-plugin-crash notification [4].  gPluginHandler already handles the PluginCrashed event, and due to that and bug 1024672, GMP crashes that are reported by this event now correctly trigger crash reporting UI and crash submission paths.  (Well, they should anyway.)

The question is whether gPluginHandler needs to observe the gmp-plugin-crash notification in the same way it observes plugin-crashed.  I don't know why it observes plugin-crashed; are there cases where plugin-crashed is broadcasted but PluginCrashed isn't fired?  Also, for GMP crashes, it looks like PluginCrashed is fired when gmp-plugin-crash is observed [5].  So it looks to me like the answer is No, but I don't know for sure.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=82d1a1b7e098#1343
[2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=82d1a1b7e098#799
[3] http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp?rev=50e64b3d2d88#1748
[4] http://mxr.mozilla.org/mozilla-central/source/content/media/gmp/GMPParent.cpp?rev=358058008e40#541
[5] http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js?rev=50e64b3d2d88#153

Comment 1

[Georg Fritzsche [:gfritzsche]]

(In reply to Drew Willcoxon :adw from comment #0)
> The question is whether gPluginHandler needs to observe the gmp-plugin-crash
> notification in the same way it observes plugin-crashed. 

No, we need it to handle PluginCrashed for GMP only (they always go through peerconnection etc., which was the workable path that was found to actually add the crash details).

Comment 2

[Georg Fritzsche [:gfritzsche]]

(In reply to Drew Willcoxon :adw from comment #0)
> I don't know why
> it observes plugin-crashed; are there cases where plugin-crashed is
> broadcasted but PluginCrashed isn't fired?  Also, for GMP crashes, it looks
> like PluginCrashed is fired when gmp-plugin-crash is observed [5].  So it
> looks to me like the answer is No, but I don't know for sure.

They are different (and trigger different code paths):
http://hg.mozilla.org/mozilla-central/annotate/0b9dd32d1e16/browser/base/content/browser-plugins.js#l1131
http://hg.mozilla.org/mozilla-central/annotate/0b9dd32d1e16/browser/base/content/browser-plugins.js#l1154

Comment 3

[Drew Willcoxon :adw]

(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> They are different (and trigger different code paths):
> http://hg.mozilla.org/mozilla-central/annotate/0b9dd32d1e16/browser/base/
> content/browser-plugins.js#l1131
> http://hg.mozilla.org/mozilla-central/annotate/0b9dd32d1e16/browser/base/
> content/browser-plugins.js#l1154

Right, but I don't know why they trigger different paths, why doing everything in the PluginCrashed listener wouldn't work.

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> No, we need it to handle PluginCrashed for GMP only (they always go through
> peerconnection etc., which was the workable path that was found to actually
> add the crash details).

So we can close this bug then?

Comment 4

[Drew Willcoxon :adw]

Georg, can we close this bug?

Comment 5

[Georg Fritzsche [:gfritzsche]]

Sorry for the earlier quick-and-not-so-informative drive-by.

Yes, we do need both handlers:
* plugin-crashed is "always" fired: http://hg.mozilla.org/mozilla-central/annotate/dc352a7bf234/dom/plugins/base/nsPluginHost.cpp#l3506
* "PluginCrashed" is per-alive-plugin-element and not always fired: http://hg.mozilla.org/mozilla-central/annotate/dc352a7bf234/dom/plugins/base/nsPluginHost.cpp#l3523