1416164 - Replace NS_ABORT, NS_NOTYETIMPLEMENTED, and NS_POSTCONDITION with MOZ_ASSERT(_UNREACHABLE)

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement, P3)

Description

[Chris Peterson [:cpeterson]]

Replace NS_ABORT, NS_NOTYETIMPLEMENTED, and NS_POSTCONDITION with MOZ_ASSERT(_UNREACHABLE).

NS_ABORT is a fatal debug assertion like MOZ_ASSERT, but NS_NOTYETIMPLEMENTED and NS_POSTCONDITION (and NS_PRECONDITION, NS_ASSERTION, and NS_NOTREACHED) just log an "###!!! ASSERTION" warning and continue running. We can safely upgrade NS_NOTYETIMPLEMENTED and NS_POSTCONDITION to fatal MOZ_ASSERT because they aren't hit during a debug Try run or when browsing some popular top sites.

We can't replace NS_PRECONDITION, NS_ASSERTION, or NS_NOTREACHED with fatal MOZ_ASSERT yet because we do hit instances of each of these assertion types in a debug Try run. Perhaps I can comment out the failing assertions and file follow-up bugs for each in their relevant Bugzilla component.

Green Try run (with some unrelated intermittent oranges);

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab0b237d98ed2cc9b8310ed73a9c96eb0bf9f90e

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: Bug 1416164 - Replace NS_ABORT with MOZ_ASSERT_UNREACHABLE.

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

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Bug 1416164 - Replace NS_NOTYETIMPLEMENTED with MOZ_ASSERT_UNREACHABLE.

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

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1416164 - Replace NS_POSTCONDITION with MOZ_ASSERT.

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

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8927250 [details]
Bug 1416164 - Replace NS_ABORT with MOZ_ASSERT_UNREACHABLE.

https://reviewboard.mozilla.org/r/198554/#review203776

I almost feel like turning these into release asserts, just so we can find out if we're accidentally hitting these cases in the wild...but this change is fine as-is.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8927251 [details]
Bug 1416164 - Replace NS_NOTYETIMPLEMENTED with MOZ_ASSERT_UNREACHABLE.

https://reviewboard.mozilla.org/r/198556/#review203778

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8927252 [details]
Bug 1416164 - Replace NS_POSTCONDITION with MOZ_ASSERT.

https://reviewboard.mozilla.org/r/198558/#review203780

Comment 7

[Chris Peterson [:cpeterson]]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> I almost feel like turning these into release asserts, just so we can find
> out if we're accidentally hitting these cases in the wild...but this change
> is fine as-is.

NS_ABORT, NS_NOTYETIMPLEMENTED, and NS_POSTCONDITION? Or just NS_ABORT?

I could change them to MOZ_DIAGNOSTIC_ASSERT, which is enabled in release builds of Nightly and Dev Edition. I would be hesitant to change them to MOZ_RELEASE_ASSERT.

Comment 8

[Nathan Froyd [:froydnj]]

NS_ABORT and NS_NOTYETIMPLEMENTED.  Those generally look like the intent is not to hit them, in all cases, and hitting them in the wild would probably indicate we were doing something incorrect?

NS_POSTCONDITION might be a little much, since it's more like MOZ_ASSERT in intent, I think.

MOZ_DIAGNOSTIC_ASSERT would be fine.  If you did want to follow through with doing that, I would recommend getting reviews from the individual component owners.

Comment 9

[Chris Peterson [:cpeterson]]

(In reply to Nathan Froyd [:froydnj] from comment #8)
> NS_ABORT and NS_NOTYETIMPLEMENTED.  Those generally look like the intent is
> not to hit them, in all cases, and hitting them in the wild would probably
> indicate we were doing something incorrect?
...
> MOZ_DIAGNOSTIC_ASSERT would be fine.  If you did want to follow through with
> doing that, I would recommend getting reviews from the individual component
> owners.

I reviewed all the NS_ABORT and NS_NOTYETIMPLEMENTED calls again. I don't think changing them to MOZ_DIAGNOSTIC_ASSERT instead of MOZ_ASSERT_UNREACHABLE is worth the effort and extra reviews. Many of the calls are already in #ifdef DEBUG blocks, so MOZ_DIAGNOSTIC_ASSERT would make no difference.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b970d24b438
Replace NS_ABORT with MOZ_ASSERT_UNREACHABLE. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/c555efc93048
Replace NS_NOTYETIMPLEMENTED with MOZ_ASSERT_UNREACHABLE. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/4d7c0110f849
Replace NS_POSTCONDITION with MOZ_ASSERT. r=froydnj

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7b970d24b438
https://hg.mozilla.org/mozilla-central/rev/c555efc93048
https://hg.mozilla.org/mozilla-central/rev/4d7c0110f849