773996 - Report plugin crashes without minidump?

Status: RESOLVED INVALID

(Core :: General, defect)

Description

[Benjamin Smedberg]

In bug 767775 we're introduction plugin crash behavior which may be new: when the plugin sends us a message on a deleted object, we explicitly kill off the plugin. The current user experience for this crash will be "no report available" because we don't collect a minidump from the plugin-container process.

In addition, while investigating Flash I've found several different ways that code could maliciously or accidentally disable crash reporting (most often by inserting an "unsafe" exception handler into the exception handling chain, which causes Windows to bypass all normal crash protection and go straight to WER reporting).

So I'm thinking that in these cases I would like to submit a plugin crash report with whatever details we have: the plugin name and version, details about the abort from bug 767775 if appropriate, etc. This will allow us to get crash statistics on these cases.

Comment 1

[Benjamin Smedberg]

Attachment: Part 1 - Hook up crash annotations/notes for FatalError events, rev. 1

Comment 2

[Benjamin Smedberg]

Attachment: Part 2 - Send a 0-byte minidump for no-minidump plugin events, rev. 1

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 645637 [details] [diff] [review]
Part 1 - Hook up crash annotations/notes for FatalError events, rev. 1

>diff --git a/dom/ipc/CrashReporterParent.h b/dom/ipc/CrashReporterParent.h

>+  virtual bool
>+    RecvAnnotateCrashReport(const nsCString& key, const nsCString& data);
>+  virtual bool
>+    RecvAppendAppNotes(const nsCString& data);
>+

Please refactor thes into public helpers called by the protected
Recv*() methods.  Calling those directly is confusing.

I'm not enthused about the distributed FatalError() logic, but it
would be slightly tricky to rework that to use existing mechanisms yet
preserve the string error message.  I'm ok with doing that here or in
a followup.

r=me with change above

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 645637 [details] [diff] [review]
Part 1 - Hook up crash annotations/notes for FatalError events, rev. 1

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

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +360,5 @@
> +    if (!c)
> +        return;
> +
> +    c->RecvAnnotateCrashReport(NS_LITERAL_CSTRING("PluginFatalExitMessage"),
> +                               nsDependentCString(msg));

Are these free-form text? If you're putting it in a separate field it might be nice to invent an enumerated list of values so that managing the data is simpler.

::: ipc/ipdl/ipdl/lower.py
@@ +3177,1 @@
>          ## FatalError()       

Can you strip the trailing whitespace while you're here?

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 645639 [details] [diff] [review]
Part 2 - Send a 0-byte minidump for no-minidump plugin events, rev. 1

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

r+ assuming you've verified that CrashSubmit.jsm doesn't choke on zero-byte minidump files.

::: toolkit/crashreporter/CrashSubmit.jsm
@@ +241,5 @@
>    },
>  
>    notifyStatus: function Submitter_notify(status, ret)
>    {
> +    Components.utils.reportError("Submitter_notify: " + status + " stack: " + (new Error()).stack);

Remove the debugging code here before you land this.

Comment 6

[Benjamin Smedberg]

damn, forgot about this one. I'm going to push it now and I'd like to take it for 17

Comment 7

[Benjamin Smedberg]

No wait, not this one.

Comment 8

[Benjamin Smedberg]

So oddly now that we have bug 875562, I don't think we should do this. We should monitor this rate primarily though FHR data, perhaps with a piece of metadata saying that this failure was not caught by breakpad.  So I'm going to assign this to gps so that he can fix this after the FHR log is done.

Comment 9

[Mats Palmgren (inactive)]

We're in the process of removing support for plugins (bug 1677160), so I think this bug is irrelevant now.