545712 - "A crash report was submitted" even when it wasn't

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

If you have MOZ_CRASHREPORTER_NO_REPORT set in the environment, the plugin-crashed UI will say that a crash report was submitted when it actually wasn't.

I don't know if there are other cases where this might be true: as filed this is pretty inconsequential, but if it shows up in Linux distros which build --enable-crashreporter but disable crash report submission that might not be so good.

Comment 1

[(not currently active) Ted Mielczarek]

I believe distros generally build --disable-crashreporter currently, FWIW.

Comment 2

[Benjamin Smedberg]

This also shows up in normal release builds when you kill a mozilla-runtime process.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Like so? Hmm, bah, guess not, I just read and tested comment 2.

Sounds like I'm also missing a check somewhere to see if the child's "crash" was really a crash or not.

Comment 4

[Benjamin Smedberg]

the crashreportID coming with the global "plugin-crashed" notification is empty, but the Firefox code is unprepared for that case. Patch in a sec.

Comment 5

[Benjamin Smedberg]

Attachment: Deal with empty crashreportID, rev. 1

This patch has two parts. First we detect an empty crashreportID and also check the return value of CrashReport.submit() in order to set the crashreportsubmitted boolean value correctly.

Secondly it suppresses the message in the crashed-plugin UI in the case where the plugin crashed but you don't have crash submission disabled. Without this change, when you kill mozilla-runtime.exe it says 'You have disabled crash report submission.' which is incorrect.

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 427766 [details] [diff] [review]
Deal with empty crashreportID, rev. 1

>-    let submitReports = gCrashReporter.submitReports;
>+    let submitted = gCrashReporter.submitReports && minidumpID.length > 0;

Nit: Omit "> 0"?

>+    if (submittedReport) {
>+      helpClass = "submitLink";
>+      showClass = "msg msgSubmitted";
>+    }
>+    else if (!gCrashReporter.submitReports) {

Nit: Brace and else on same line?

>+    if (helpClass !== undefined) {

Nit: Just "if (helpClass)"?

I think it's worth a brief comment here to note why it is that neither "msgSubmitted" not "msgNotSubmitted" is being used, since that would normally seem like an obvious binary choice.

Or maybe we should just add a 3rd message here for "No crash report was generated". But that doesn't seem all that useful, unless there are other unexpected and likely ways to trigger this path (other than killing a process manually).

Comment 7

[Justin Dolske [:Dolske]]

Pushed in http://hg.mozilla.org/mozilla-central/rev/8b56adc3241c

Comment 8

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/68f91ff6e1ad

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 10

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430