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

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(3 attachments)

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
See Also: → 303500
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.
Attachment #8927250 - Flags: review?(nfroyd) → review+
Comment on attachment 8927251 [details]
Bug 1416164 - Replace NS_NOTYETIMPLEMENTED with MOZ_ASSERT_UNREACHABLE.

https://reviewboard.mozilla.org/r/198556/#review203778
Attachment #8927251 - Flags: review?(nfroyd) → review+
Comment on attachment 8927252 [details]
Bug 1416164 - Replace NS_POSTCONDITION with MOZ_ASSERT.

https://reviewboard.mozilla.org/r/198558/#review203780
Attachment #8927252 - Flags: review?(nfroyd) → review+
(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.
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.
(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.
Pushed by cpeterson@mozilla.com:
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
See Also: → 1457813
See Also: → 1469769
You need to log in before you can comment on or make changes to this bug.