Closed Bug 1286005 Opened 8 years ago Closed 8 years ago

Don't include the PID in the NS_DebugBreak crash annotation

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- wontfix
firefox49 --- fixed
firefox-esr45 --- wontfix
firefox50 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(1 file)

NS_DebugBreak includes the PID at the start of the abort message. This is fine for printing to the console in a debug build, but it prevents anybody from using the abort message as a facet in crash-stats searches, as seen in bug 1273770. One solution would be to generate a separate string for printing that includes the pid (or only include the PID when doing the final printout). Another solution would be to just never include the PID in a non-debug build.
Assignee: nobody → continuation
I tested locally and it seems to produce the same output for buf, and the appropriate non-pid output for nonPIDBuf. Including the PID makes it impossible to aggregate crash reports on crash-stats. This also reduces buffer size to ensure that having two buffers does not increase total stack size, though that is unlikely to matter. 1000 characters is likely excessive in any event. I changed the print macro to ALL_CAPS because I think it is confusing to have a macro with an unbound variable in it that looks like a regular function call.
Attachment #8769865 - Flags: review?(nfroyd)
Attachment #8769865 - Flags: review?(nfroyd) → review+
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e50eb5912d50 Don't include the PID in the NS_DebugBreak crash annotation. r=froydnj
Nick, since you've been looking at MOZ_CRASH() messages, you might find this interesting, in case you start looking at NS_DebugBreak crash messages.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Oops, I had more extensive patches in bug 1277448 that I hadn't gotten to addressing review comments on that did this and some other things...
(I think the right thing to do will be to just back this out when I land those, but I'll look more closely first.)
Comment on attachment 8769865 [details] [diff] [review] Don't include the PID in the NS_DebugBreak crash annotation. Approval Request Comment [Feature/regressing bug #]: old [User impact if declined]: This bug makes it easier to understand why certain crashes are happening. [Describe test coverage new/current, TreeHerder]: We have some testing of crashing, but not for this particular detail of behavior. [Risks and why]: Low. It has been on trunk for a while, and it only affects crash behavior. [String/UUID change made/needed]: Low.
Attachment #8769865 - Flags: approval-mozilla-beta?
Comment on attachment 8769865 [details] [diff] [review] Don't include the PID in the NS_DebugBreak crash annotation. Diagnostic patch for crashes, please uplift to beta.
Attachment #8769865 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: