Don't include the PID in the NS_DebugBreak crash annotation

RESOLVED FIXED in Firefox 49

Status

()

Core
XPCOM
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox48 wontfix, firefox49 fixed, firefox50 fixed, firefox-esr45 wontfix)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 months ago
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)

Updated

11 months ago
Assignee: nobody → continuation
(Assignee)

Comment 1

11 months ago
Created attachment 8769865 [details] [diff] [review]
Don't include the PID in the NS_DebugBreak crash annotation.

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)

Updated

11 months ago
Attachment #8769865 - Flags: review?(nfroyd) → review+

Comment 2

10 months ago
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
(Assignee)

Comment 3

10 months ago
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.

Comment 4

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e50eb5912d50
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox50: affected → fixed
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.)
(Assignee)

Comment 7

10 months ago
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?
(Assignee)

Updated

10 months ago
status-firefox48: --- → wontfix
status-firefox49: --- → affected
status-firefox-esr45: --- → wontfix
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+

Comment 9

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/1d80b9b0c4d9
status-firefox49: affected → fixed
You need to log in before you can comment on or make changes to this bug.