Closed Bug 1470374 Opened 2 years ago Closed 2 years ago

Replace NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE in C-C

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 62.0

People

(Reporter: cpeterson, Assigned: jorgk-bmo)

References

Details

Attachments

(1 file)

Jorg, similar to s/NS_PRECONDITION/MOZ_ASSERT/ bug 1459470, I will be removing Gecko's NS_NOTREACHED assertion macro in Firefox Nightly 63 next week in bug 1469769. NS_NOTREACHED just logs an error message instead of aborting like MOZ_ASSERT_UNREACHABLE.

* For NS_NOTREACHED calls that do not assert during Firefox's Try tests, I will replace them with fatal MOZ_ASSERT_UNREACHABLE.

* For NS_NOTREACHED calls that do assert, I will replace them with non-fatal NS_ERROR, which just logs an error message like NS_NOTREACHED does.

It looks like there are 24 NS_NOTREACHED calls in comm-central's ldap, mailnews, and rdp directories:

https://searchfox.org/comm-central/search?q=NS_NOTREACHED
Flags: needinfo?(jorgk)
Thanks. It escaped you that I already filed bug 1469773 for it. But we can go with this bug since it has the better description.
Flags: needinfo?(jorgk)
Summary: Replace NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE → Replace NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE in C-C
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Comment on attachment 8987008 [details] [diff] [review]
1470374-replace-NS_NOTREACHED.patch

Green try. Chris, could you take this simple review or would you prefer a mailnews peer to do it?
Attachment #8987008 - Flags: review?(cpeterson)
Attachment #8987008 - Flags: review?(acelists)
Comment on attachment 8987008 [details] [diff] [review]
1470374-replace-NS_NOTREACHED.patch

Review of attachment 8987008 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with converting them all to the fatal crashing assertion.
I just wonder how it is implemented, that the try was green and no compiler didn't complain about the unreachable return after the statement.

Chris, is there a style for these cases in m-c? Should there be a return after the macro?
Attachment #8987008 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #5)

> I'm fine with converting them all to the fatal crashing assertion.
> I just wonder how it is implemented, that the try was green and no compiler
> didn't complain about the unreachable return after the statement.
> 
> Chris, is there a style for these cases in m-c? Should there be a return
> after the macro?

MOZ_ASSERT_UNREACHABLE is an alias for MOZ_ASSERT(false). In debug builds, it will abort. In release builds, it is a no-op.

https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/mfbt/Assertions.h#556-557

Since MOZ_ASSERT_UNREACHABLE is a debug-only macro, so we still need a return or some other error handling after MOZ_ASSERT_UNREACHABLE to handle the "unreachable" code path in release builds. :)

If reaching some code path would be a critical bug, then you could use MOZ_CRASH to abort in both debug and release builds. But that is pretty serious and would affect users.
Comment on attachment 8987008 [details] [diff] [review]
1470374-replace-NS_NOTREACHED.patch

Review of attachment 8987008 [details] [diff] [review]:
-----------------------------------------------------------------

The code changes LGTM and comment 4 says Try tests are green.
Attachment #8987008 - Flags: review?(cpeterson) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9fc916302e1a
Port bug 1469769: Replace NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE in C-C. r=aceman,cpeterson
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 62.0
You need to log in before you can comment on or make changes to this bug.