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)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(1 file)
(deleted),
patch
|
froydnj
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•8 years ago
|
Assignee: nobody → continuation
Assignee | ||
Comment 1•8 years ago
|
||
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•8 years ago
|
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
Assignee | ||
Comment 3•8 years 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 5•8 years ago
|
||
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...
Comment 6•8 years ago
|
||
(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•8 years 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•8 years 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•8 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•