Closed Bug 1088938 Opened 6 years ago Closed 6 years ago

Allow annotations set using CrashReporter::AnnotateCrashReport() to be removed subsequently

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: smichaud, Assigned: smichaud)

Details

Attachments

(1 file, 1 obsolete file)

Right now crash log annotations (added using AnnotateCrashReport or AppendCrashNotesToCrashReport) can't be removed.  So, unless you *cause* a crash just after adding an annotation, they're limited to things that are always relevant, no matter how we crash (since we don't know whether or not we'll crash where we might expect to).

It'd be very convenient to also be able to make "speculative" annotations -- ones that are relevant if we crash where we think we might, but aren't at all relevant if we don't crash there (if we crash later, somewhere else, for different reasons).  For example we have a case, in bug 1086977, where we crash whenever a particular plugin is loaded, but we're not entirely sure what it's filename is.  If we log all plugin filenames, most of our annotations will be irrelevant.

But the situation changes if we're able to remove an annotation after it's become irrelevant -- for example if we can log every plugin's filename, then remove that annotation if we don't crash where we expect to.

Right now can do something like this with AnnotateCrashReport().  We can add a "speculative" note under a key just before we might crash, and then change it to something like "please ignore" afterwards if we don't crash.  But it'd be even better if calling AnnotateCrashReport() with the same key and "" for data caused the previous annotation to be removed entirely.
Attached patch Fix (obsolete) — Splinter Review
This works in my tests.
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #8511363 - Flags: review?(ted)
Attached patch Fix rev1Splinter Review
Talking on IRC, Ted, you suggested I add a call to explicitly remove crash report annotations -- to make it clearer how this can be done.
Attachment #8511363 - Attachment is obsolete: true
Attachment #8511363 - Flags: review?(ted)
Attachment #8512229 - Flags: review?(ted)
Comment on attachment 8512229 [details] [diff] [review]
Fix rev1

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

This is fine. I still think you could just remove the entry from the hashtable, but it's not that big of a deal.
Attachment #8512229 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/712679cd32c7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.