682609 - (NS_ASSERTION_SUX) [meta] Tracking bug for NS_ASSERTIONs that should be actual assertions

Status: NEW

(Core :: General, defect, P3)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Our codebase is plagued by the NS_ASSERTION macro whose name wrongly suggests that it's an assertion, which it currently isn't by default (only is with XPCOM_DEBUG_BREAK set to a suitable value). As a result, we have lots of bugs that are harder than necessary to track down, because people have been relying on NS_ASSERTION for stuff that should have been fatal.

Lately there's been discussion on dev-platform about that, and I don't know what the conclusions were; the present bug will serve as a tracking bug for all the places where NS_ASSERTION is used where a real assert is needed.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

David: bug 678327 comment 5 says that you are working on resolving this general issue. Is there a bug filed for that? The present bug is only a tracking bug for places where NS_ASSERTION is used and its current default behavior is unsuitable.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

NS_PRECONDITION has the same problem as NS_ASSERTION.

Comment 3

[Chris Peterson [:cpeterson]]

Bug 1457813 will replace 1436 non-asserting NS_PRECONDITIONs with MOZ_ASSERT and two asserting NS_PRECONDITIONs with NS_ASSERTIONS: bug 478135 and bug 749326.

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

This was raised again in https://phabricator.services.mozilla.com/D69858#inline-414015. Am I understanding right that an NS_ASSERTION should be replaced by MOZ_ASSERT unless doing so causes a crash?

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Kagami :saschanaz from comment #4)

This was raised again in https://phabricator.services.mozilla.com/D69858#inline-414015. Am I understanding right that an NS_ASSERTION should be replaced by MOZ_ASSERT unless doing so causes a crash?

New code should prefer MOZ_ASSERT over NS_ASSERTION.

MOZ_ASSERT will abort in debug builds so we can find and fix assertion failures hit during automated tests. NS_ASSERTION will just print a warning which is easy to miss. Some of our test runners will detect NS_ASSERTION warnings and treat them as test failures, but I don't know if all the test runners do.