1412048 - Replace NS_RUNTIMEABORT with MOZ_CRASH and MOZ_CRASH_UNSAFE_OOL

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P3)

Description

[Chris Peterson [:cpeterson]]

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 1

[Chris Peterson [:cpeterson]]

Attachment: Bug 1412048 - Replace NS_RUNTIMEABORT("...") with MOZ_CRASH("...").

And remove unreachable code after MOZ_CRASH().

Review commit: https://reviewboard.mozilla.org/r/193462/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193462/

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Bug 1412048 - Replace NS_RUNTIMEABORT(var) with MOZ_CRASH_UNSAFE_OOL(var). data-review?francois

And remove unreachable code after MOZ_CRASH_UNSAFE_OOL().

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.

Review commit: https://reviewboard.mozilla.org/r/193464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193464/

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1412048 - Replace some NS_RUNTIMEABORT(var) calls with MOZ_CRASH_UNSAFE_PRINTF. data-review?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.

Review commit: https://reviewboard.mozilla.org/r/193466/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193466/

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: Bug 1412048 - Remove NS_RUNTIMEABORT.

Use MOZ_CRASH, MOZ_CRASH_UNSAFE_OOL, or MOZ_CRASH_UNSAFE_PRINTF instead.

Review commit: https://reviewboard.mozilla.org/r/193468/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193468/

Comment 5

[Chris Peterson [:cpeterson]]

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.

Comment 6

[Chris Peterson [:cpeterson]]

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.

Comment 7

[François Marier [: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?

Comment 8

[Chris Peterson [: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

Comment 9

[François Marier [:francois]]

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

Comment 10

[François Marier [:francois]]

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

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8922429 [details]
Bug 1412048 - Replace NS_RUNTIMEABORT("...") with MOZ_CRASH("...").

https://reviewboard.mozilla.org/r/193462/#review199148

Comment 12

[Nathan Froyd [:froydnj]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Chris Peterson [:cpeterson]]

leaving open because I only landed patches 1 and 2 of 4 so far.

Comment 15

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/7fb3d74bb529
https://hg.mozilla.org/mozilla-central/rev/856d69fd9827

Comment 16

[Nathan Froyd [:froydnj]]

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

Comment 17

[Nathan Froyd [:froydnj]]

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?

Comment 18

[Chris Peterson [:cpeterson]]

(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

[Nathan Froyd [:froydnj]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e85f59ea455d
https://hg.mozilla.org/mozilla-central/rev/17decbfb0720

Comment 22

[Nicholas Nethercote [inactive]]

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

[Ionuț Goldan [:igoldan]]

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

Comment 24

[Chris Peterson [:cpeterson]]

Resolving as FIXED and removing the leave-open keyword because all the patches have landed.