Closed Bug 1412048 Opened 2 years ago Closed 2 years ago

Replace NS_RUNTIMEABORT with MOZ_CRASH and MOZ_CRASH_UNSAFE_OOL

Categories

(Core :: XPCOM, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr52 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

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 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)
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)
(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)
(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 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 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 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 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+
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
leaving open because I only landed patches 1 and 2 of 4 so far.
Keywords: leave-open
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 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+
(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).
(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
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
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.
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
Resolving as FIXED and removing the leave-open keyword because all the patches have landed.
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.