Closed
Bug 1412048
Opened 6 years ago
Closed 5 years ago
Replace NS_RUNTIMEABORT with MOZ_CRASH and MOZ_CRASH_UNSAFE_OOL
Categories
(Core :: XPCOM, enhancement, P3)
Core
XPCOM
Tracking
()
People
(Reporter: cpeterson, Assigned: cpeterson)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
Replace NS_RUNTIMEABORT with MOZ_CRASH, MOZ_CRASH_UNSAFE_OOL, and MOZ_CRASH_UNSAFE_PRINTF then remove NS_RUNTIMEABORT. NS_RUNTIMEABORT's app notes just add "###!!! ABORT" with file and line numbers and "AbortMessage(nonPIDBuf.buffer)" annotations to the crash report, which is no more information than MOZ_CRASH adds with gMozCrashReason. Green Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f016bc359fab8b52566541d8dc888be301ef8cf&selectedJob=139801515
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
Assignee | |
Comment 5•6 years ago
|
||
Comment on attachment 8922430 [details] Bug 1412048 - Replace NS_RUNTIMEABORT(var) with MOZ_CRASH_UNSAFE_OOL(var). data-review?francois Francois, MOZ_CRASH_UNSAFE_OOL causes data collection because crash strings are annotated to crash-stats and are publicly visible. Firefox data stewards must do data review on usages of this macro. However, all the crash strings this patch collects with MOZ_CRASH_UNSAFE_OOL are already collected with NS_RUNTIMEABORT.
Attachment #8922430 -
Flags: review?(francois)
|
|
Assignee | |
Comment 6•6 years ago
|
||
Comment on attachment 8922431 [details] Bug 1412048 - Replace some NS_RUNTIMEABORT(var) calls with MOZ_CRASH_UNSAFE_PRINTF. data-review?francois Francois, MOZ_CRASH_UNSAFE_PRINTF causes data collection because crash strings are annotated to crash-stats and are publicly visible. Firefox data stewards must do data review on usages of this macro. However, all the crash strings this patch collects with MOZ_CRASH_UNSAFE_PRINTF are already collected with NS_RUNTIMEABORT.
Attachment #8922431 -
Flags: review?(francois)
|
||
Comment 7•6 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #5) > all the crash strings this patch collects with MOZ_CRASH_UNSAFE_OOL are > already collected with NS_RUNTIMEABORT. (In reply to Chris Peterson [:cpeterson] from comment #5) > all the crash strings this patch collects with MOZ_CRASH_UNSAFE_OOL are > already collected with NS_RUNTIMEABORT. So if I understand you correctly, there's no additional data collection as a result of this patch?
Flags: needinfo?(cpeterson)
|
|
Assignee | |
Comment 8•6 years ago
|
||
(In reply to François Marier [:francois] from comment #7) > (In reply to Chris Peterson [:cpeterson] from comment #5) > > all the crash strings this patch collects with MOZ_CRASH_UNSAFE_OOL are > > already collected with NS_RUNTIMEABORT. > > So if I understand you correctly, there's no additional data collection as a > result of this patch? Correct. The same data that was added to crash reports with NS_RUNTIMEABORT will now be added with MOZ_CRASH_UNSAFE_OOL. I requested data-review because the MOZ_CRASH_UNSAFE_OOL comments in Assertions.h ask for it: * @note This macro causes data collection because crash strings are annotated * to crash-stats and are publicly visible. Firefox data stewards must do data * review on usages of this macro. https://searchfox.org/mozilla-central/source/mfbt/Assertions.h#283-285
Flags: needinfo?(cpeterson)
|
|
||
Comment 9•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8922430 [details] Bug 1412048 - Replace NS_RUNTIMEABORT(var) with MOZ_CRASH_UNSAFE_OOL(var). data-review?francois https://reviewboard.mozilla.org/r/193464/#review198774 datareview+ based on comment 8
Attachment #8922430 -
Flags: review?(francois) → review+
|
|
||
Comment 10•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8922431 [details] Bug 1412048 - Replace some NS_RUNTIMEABORT(var) calls with MOZ_CRASH_UNSAFE_PRINTF. data-review?francois https://reviewboard.mozilla.org/r/193466/#review198776 datareview+ based on comment 8
Attachment #8922431 -
Flags: review?(francois) → review+
|
||
Comment 11•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8922429 [details] Bug 1412048 - Replace NS_RUNTIMEABORT("...") with MOZ_CRASH("..."). https://reviewboard.mozilla.org/r/193462/#review199148
Attachment #8922429 -
Flags: review?(nfroyd) → review+
|
|
||
Comment 12•6 years ago
|
||
| mozreview-review | ||
Comment on attachment 8922430 [details] Bug 1412048 - Replace NS_RUNTIMEABORT(var) with MOZ_CRASH_UNSAFE_OOL(var). data-review?francois https://reviewboard.mozilla.org/r/193464/#review199150
Attachment #8922430 -
Flags: review?(nfroyd) → review+
|
||
Comment 13•6 years ago
|
||
Pushed by cpeterson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb3d74bb529 Replace NS_RUNTIMEABORT("...") with MOZ_CRASH("..."). r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/856d69fd9827 Replace NS_RUNTIMEABORT(var) with MOZ_CRASH_UNSAFE_OOL(var). r=froydnj data-review=francois
|
|
Assignee | |
Comment 14•6 years ago
|
||
leaving open because I only landed patches 1 and 2 of 4 so far.
Keywords: leave-open
|
||
Comment 15•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/7fb3d74bb529 https://hg.mozilla.org/mozilla-central/rev/856d69fd9827
|
|
||
Comment 16•5 years ago
|
||
| mozreview-review | ||
Comment on attachment 8922431 [details] Bug 1412048 - Replace some NS_RUNTIMEABORT(var) calls with MOZ_CRASH_UNSAFE_PRINTF. data-review?francois https://reviewboard.mozilla.org/r/193466/#review201430
Attachment #8922431 -
Flags: review?(nfroyd) → review+
|
|
||
Comment 17•5 years ago
|
||
| mozreview-review | ||
Comment on attachment 8922432 [details] Bug 1412048 - Remove NS_RUNTIMEABORT. https://reviewboard.mozilla.org/r/193468/#review201432 ::: xpcom/base/nsDebug.h:166 (Diff revision 1) > * Trigger an debug-only abort. > * > - * @see NS_RUNTIMEABORT for release-mode asserts. > + * @see MOZ_RELEASE_ASSERT or MOZ_CRASH for release-mode asserts. It looks like this macro is not used in many places; should we file a followup for removing it?
Attachment #8922432 -
Flags: review?(nfroyd) → review+
|
|
Assignee | |
Comment 18•5 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #17) > ::: xpcom/base/nsDebug.h:166 > (Diff revision 1) > > * Trigger an debug-only abort. > > * > > - * @see NS_RUNTIMEABORT for release-mode asserts. > > + * @see MOZ_RELEASE_ASSERT or MOZ_CRASH for release-mode asserts. > > It looks like this macro is not used in many places; should we file a > followup for removing it? Yeah. I have a follow-up patch to replace NS_ABORT with MOZ_ASSERT_UNREACHABLE. I just didn't want to inundate you with too many big cleanup patches. :) My follow-up patch also replaces NS_NOTREACHED and NS_NOTYETIMPLEMENTED with MOZ_ASSERT_UNREACHABLE. That change isn't as straightforward because NS_NOTREACHED and NS_NOTYETIMPLEMENTED just log warnings instead of asserting, whereas MOZ_ASSERT_UNREACHABLE will assert. Unfortunately, a few NS_NOTREACHED assertions fail on a Try test run. I need to figure out if those NS_NOTREACHED failures are real bugs (or just comment out those assertions and file new bugs for each).
|
|
||
Comment 19•5 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #18) > (In reply to Nathan Froyd [:froydnj] from comment #17) > > ::: xpcom/base/nsDebug.h:166 > > (Diff revision 1) > > > * Trigger an debug-only abort. > > > * > > > - * @see NS_RUNTIMEABORT for release-mode asserts. > > > + * @see MOZ_RELEASE_ASSERT or MOZ_CRASH for release-mode asserts. > > > > It looks like this macro is not used in many places; should we file a > > followup for removing it? > > Yeah. I have a follow-up patch to replace NS_ABORT with > MOZ_ASSERT_UNREACHABLE. I just didn't want to inundate you with too many big > cleanup patches. :) My life consists of being inundated by big cleanup patches; have you seen the code I'm supposed to be responsible for? =D
|
|
||
Comment 20•5 years ago
|
||
Pushed by cpeterson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e85f59ea455d Replace some NS_RUNTIMEABORT(var) calls with MOZ_CRASH_UNSAFE_PRINTF. r=froydnj data-review=francois https://hg.mozilla.org/integration/mozilla-inbound/rev/17decbfb0720 Remove NS_RUNTIMEABORT. r=froydnj
|
|
||
Comment 21•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/e85f59ea455d https://hg.mozilla.org/mozilla-central/rev/17decbfb0720
|
|
||
Comment 22•5 years ago
|
||
I had a very old entry down the bottom of my todo.txt file:
> - assertions
> - NS_RUNTIMEABORT --> MOZ_CRASH
I just removed it! Thank you for the clean-up.
|
||
Comment 23•5 years ago
|
||
I noticed some visible perf improvements thanks to this bug: == Change summary for alert #10347 (as of Sat, 04 Nov 2017 02:46:32 GMT) == Improvements: 8% TestStandardURL Perf osx-10-10 opt 55,994.12 -> 51,309.83 5% Stylo Servo_DeclarationBlock_GetPropertyById_Bench osx-10-10 opt 213,421.29 -> 202,141.08 5% TestStandardURL NormalizePerf osx-10-10 opt 52,870.12 -> 50,174.83 4% Stylo Gecko_nsCSSParser_ParseSheet_Bench osx-10-10 opt 69,573.08 -> 66,727.00 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10347
|
|
Assignee | |
Comment 24•5 years ago
|
||
Resolving as FIXED and removing the leave-open keyword because all the patches have landed.
You need to log in
before you can comment on or make changes to this bug.
Description
•