Closed Bug 773996 Opened 10 years ago Closed 1 year ago

Report plugin crashes without minidump?

Categories

(Core :: General, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: benjamin, Unassigned)

References

Details

Attachments

(2 files)

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.
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attachment #645637 - Flags: review?(ted.mielczarek)
Attachment #645637 - Flags: review?(jones.chris.g)
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
Attachment #645637 - Flags: review?(jones.chris.g) → review+
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?
Attachment #645637 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #645639 - Flags: review?(ted.mielczarek) → review+
damn, forgot about this one. I'm going to push it now and I'd like to take it for 17
No wait, not this one.
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.
Assignee: benjamin → gps
Assignee: gps → nobody
Status: ASSIGNED → NEW

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

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.