1009760 - Hook up crash reporting for GMP plugins (openh264)

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Benjamin Smedberg]

We need to hook up crash reporting for the openh264 plugins and any other GMP plugins. This involves matching up a GMP plugin with a tab or tabs, showing a notification on crash and asking the user to opt in to submitting the crash. We may be able to re-use all or parts of the existing plugin-crash UI. I'll file a separate bug specifically about UX decisions/design.

Comment 1

[Lonnen :lonnen]

Is there a socorro component to this or will crash-stats "just work" once?

Comment 2

[Benjamin Smedberg]

I expect that this will be treated as a plugin crash and there will be little additional Socorro work required. The plugin name/version fields might have a slightly different meaning than they for normal NPAPI plugins.

Comment 3

[Lonnen :lonnen]

Excellent. Thanks!

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8458256 [details] [diff] [review]
Patch

Review of attachment 8458256 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't add anything to the makeNicePluginName function. I'm assuming pluginName will already come with a nice-for-the-user value. But if it doesn't it's simple to add it afterwards.

::: browser/base/content/test/plugins/browser_globalplugin_crashinfobar.js
@@ +11,5 @@
> +  submittedCrashReport: false
> +}
> +
> +// Test that plugin crash submissions still work properly after
> +// click-to-play activation.

Note: I'll remove this comment that came from the file that I copied to write this test.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

https://tbpl.mozilla.org/?tree=Try&rev=c5a4dcc70758

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: Alternative version without __noSuchMethod__

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Attachment: Refactored version

This is a refactored version that moves all the code that deals with the plugin overlay to a helper function, avoiding the need to have to null-check all calls to getPluginUI etc.

I don't know if it looks riskier than the previous version. If it does, to avoid dealing with any breakage on Aurora, we could land the simpler version now and this version on Monday/Tuesday after the uplift.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8458716 [details] [diff] [review]
Refactored version

Review of attachment 8458716 [details] [diff] [review]:
-----------------------------------------------------------------

I think this looks good and should not be a big risk, given that it mostly moves things around.
The only issue i see is present in both patches:
I'm not sure that |!(event.target instanceof Ci.nsIObjectLoadingContent)| guarantees that we got a proper GMP crash.
I fear we could end up with a bad NPAPI plugin crash or similar. Flagging johns for review for that specific question and r+ pending that as i'm out soon.

::: browser/base/content/browser-plugins.js
@@ +276,5 @@
>        return;
>      }
>  
> +    if (eventType == "PluginCrashed" &&
> +        !(event.target instanceof Ci.nsIObjectLoadingContent)) {

You can just move the |let plugin = ...| up before this.
Given the previous handling below, i wonder if we could start to hit this in other situations than the one you expect here, say a crash on a plugin that went away.

r?johns for that part.

@@ +287,5 @@
>      let plugin = event.target;
>      let doc = plugin.ownerDocument;
>  
>      if (!(plugin instanceof Ci.nsIObjectLoadingContent))
>        return;

This would now be redundant.

@@ +1311,5 @@
>        doc.defaultView.top.addEventListener("unload", function() {
>          notificationBox.removeNotification(notification);
>        }, false);
>      }
> +    

White-space.

@@ +1314,5 @@
>      }
> +    
> +    // Configure the crashed-plugin placeholder.
> +    // Returns true if the plugin overlay is visible.
> +    function setUpPluginOverlay(plugin, doPrompt, browser) {

_setUpPluginOverlay()?
|doPrompt| could be more descriptive, |doPromptSubmit|?

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8458716 [details] [diff] [review]
Refactored version

Bsmedberg said on IRC that the instanceof check looks like a reasonable way to differentiate the types of plugins we are trying to. So i'm going with that for now and if it needs adjustment later we can follow up

Comment 12

[:Felipe Gomes (needinfo for replies!)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> @@ +287,5 @@
> >      let plugin = event.target;
> >      let doc = plugin.ownerDocument;
> >  
> >      if (!(plugin instanceof Ci.nsIObjectLoadingContent))
> >        return;
> 
> This would now be redundant.

This doesn't become redundant because there are other eventTypes and the check above is only for PluginCrashed

Comment 13

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/fx-team/rev/41df67208b1b

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41df67208b1b

Comment 15

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned for verification of this bug.

Comment 16

[Benjamin Smedberg]

This bug will not be ready for QA until the dependent bug 1038961 and bug 1039575 are also landed.

Currently there isn't a good way to force the plugin to crash on linux/mac: we should start by verifying this on Windows; kill the plugin-container for the OpenH264 plugin using crashfirefox.exe from https://developer.mozilla.org/en-US/docs/How_to_Report_a_Hung_Firefox

Comment 17

[Marco Mucci [:MarcoM]]

Hi Alexandra, following up to see if this bug can be verified by the end of the iteration on Monday August 4.

Comment 18

[Ada [:adalucinet]]

Tested with latest Aurora (Build ID: 20140730004005) on Windows 7 x64, Ubuntu 13.04 64bit and Mac OS X 10.9.4. The results are inline:

1. On Windows, I crashed Firefox after the plugin is installed and a h264 call is made by using crashfirefox.exe from https://developer.mozilla.org/en-US/docs/How_to_Report_a_Hung_Firefox - Crash report e.g.: bp-3ff421a7-77ac-46a7-8975-cdb2b2140730
1.1. Intermittently, Aurora freezes after running crashfirefox.exe - the plugin-container.exe and firefox.exe are still present in Task Manager - no crash encountered. 
1.2. Intermittently, Aurora crashes before I am able to run crashfirefox.exe - Known issue: bug 1043394
Any thoughts on 1.1. and 1.2.?

2. On Ubuntu 13.04 64bit, when crashing the plugin-container during a h264 call I get this signature: bp-783dd6c0-78b2-4ade-9978-be5892140730
Note: crashed the plugin-container by using kill -n SIGABRT PID command in terminal

3. I'm not able to test this on Mac OS X because of the crash bug 1041525.

After restoring Firefox, the plugin is downloaded and installed again on both OSs (Windows and Ubuntu). I suppose this is related with bug 1045038?

Georg, since Felipe and Benjamin are on PTO, could you please give me an input here or point me to someone who could?

Comment 19

[Georg Fritzsche [:gfritzsche]]

Don't test this on Aurora yet, we're in the process of getting the patches uplifted.
Nightly should be good, at least we had all the relevant patches on mozilla-inbound yesterday evening.

Comment 20

[Georg Fritzsche [:gfritzsche]]

Bug 1043531 is good to track when Aurora should be good.

Comment 21

[Ada [:adalucinet]]

Crashed Aurora Plugin Process on MAC OS X 10.9.4 using latest Aurora (Build ID: 20140731004002) and kill -n SIGABRT PID command. Submitted the crash report via OpenH264 crash reporting notification. 
I get signature 'EMPTY: no crashing thread identified' and process type 'plugin gmpopenh264 Version:1.0 Filename:gmpopenh264' - bp-a9288a0e-d631-4ecb-8809-7677f2140731

On Ubuntu 13.04 64bit, I get 'libc-2.17.so@0xedfbd' signature and process type 'plugin gmpopenh264 Version:1.0 Filename:gmpopenh264' - bp-27ae77f5-7b90-496f-a23a-613142140801

Could you please confirm that this are the correct entries from about:crashes?

Unfortunately, on Windows I cannot test this because of bug 1047374.

Comment 22

[Maire Reavy [:mreavy]]

Alexandra -- Have you opened a bug for the crash mentioned in Comment 21?  If so, can you provide the bug number and make sure it blocks the tracking bug (Bug 948160)?   Thanks.

Comment 23

[Ada [:adalucinet]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #22)
> Alexandra -- Have you opened a bug for the crash mentioned in Comment 21? 
> If so, can you provide the bug number and make sure it blocks the tracking
> bug (Bug 948160)?   Thanks.

In order to properly verify this bug, as in comment 16 or bug 1043531 comment #39, I forced the plugin to crash by using kill -n SIGABRT PID command in terminal. This is *not* about a new crash bug.
Or did I not understand well your request?

Comment 24

[Maire Reavy [:mreavy]]

Thanks for the explanation.  My apologies.  I misread your comment to Georg.  You're doing the right thing.

Comment 25

[Georg Fritzsche [:gfritzsche]]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #21)
> Crashed Aurora Plugin Process on MAC OS X 10.9.4 using latest Aurora (Build
> ID: 20140731004002) and kill -n SIGABRT PID command. Submitted the crash
> report via OpenH264 crash reporting notification. 
> I get signature 'EMPTY: no crashing thread identified' and process type
> 'plugin gmpopenh264 Version:1.0 Filename:gmpopenh264' -
> bp-a9288a0e-d631-4ecb-8809-7677f2140731
> 
> On Ubuntu 13.04 64bit, I get 'libc-2.17.so@0xedfbd' signature and process
> type 'plugin gmpopenh264 Version:1.0 Filename:gmpopenh264' -
> bp-27ae77f5-7b90-496f-a23a-613142140801
> 
> Could you please confirm that this are the correct entries from
> about:crashes?

Those look ok.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Alexandra, can you give this another shot at verification on Windows now that 1043531 is fixed? Thanks!

Comment 27

[Bogdan Maris, Desktop QA]

Hi, Alexandra is on PTO until 20th of August, I will replace her until then. I tested again across platforms but It`s very hard to make the error message appear because of bug 1049501.

I did managed to Submit a Crash from the error message on Linux and Windows but very hard 1 out of 10 tries, on Mac I was not able to do that, all the crashes have the same signature as the one from bug 1049501. I think we should wait for the fix from that bug in order to verify this one so I`m leaving the [qa+] keyword on until then.

Comment 28

[Bogdan Maris, Desktop QA]

Now that bug 1049501 has been fixed, I managed to find a new issue that prevents this bug to be verified, bug 1054965.

Comment 29

[Ada [:adalucinet]]

Crash reporting for openH264 plugin are correctly generated with latest Aurora (Build ID: 20140828004002) on Windows 7 x64, Mac OS X 10.9.4 and Ubuntu 12.04 x32 after media.gmp.plugin.crash pref is set to true during a h264 call.

Examples of crash reports for: 
- Windows: bp-17882e75-2cc7-42db-ab72-1bf422140829
- Mac OS X: bp-27278ece-b17c-4270-88fc-3caf82140829
- Linux: bp-dfc5bcb9-9541-4412-8e54-da7362140829

Please note that bug 1056029 is still reproducible on both Aurora and Nightly branches, but *only* under Linux 14.04, not reproducible at all on 12.04.