Closed Bug 1469769 Opened 2 years ago Closed 2 years ago

Replace NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE or NS_ERROR

Categories

(Core :: XPCOM, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 5 open bugs)

Details

Attachments

(8 files)

NS_NOTREACHED and NS_ERROR log non-fatal warning messages. MOZ_ASSERT_UNREACHABLE aborts in debug builds. These patches remove the NS_NOTREACHED macro definition and replace NS_NOTREACHED calls with:

* MOZ_ASSERT_UNREACHABLE if the call is not hit during Try test runs. (558 calls)
* NS_ERROR if the call is hit during Try test runs. (6 calls)

There were bugs already on file for 5 of the 6 failing NS_NOTREACHED calls.
Comment on attachment 8986381 [details]
Bug 1469769 - Part 5: widget/gtk: Replace failing NS_NOTREACHED with NS_ERROR.

https://reviewboard.mozilla.org/r/251756/#review258338
Attachment #8986381 - Flags: review?(karlt) → review+
Comment on attachment 8986378 [details]
Bug 1469769 - Part 2: animation: Replace failing NS_NOTREACHED with NS_ERROR.

https://reviewboard.mozilla.org/r/251750/#review258388
Attachment #8986378 - Flags: review?(cam) → review+
Comment on attachment 8986379 [details]
Bug 1469769 - Part 3: css: Replace failing NS_NOTREACHED with NS_ERROR.

https://reviewboard.mozilla.org/r/251752/#review258390
Attachment #8986379 - Flags: review?(cam) → review+
Comment on attachment 8986382 [details]
Bug 1469769 - Part 6a: Replace non-failing NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE.

https://reviewboard.mozilla.org/r/251758/#review258582
Attachment #8986382 - Flags: review?(nfroyd) → review+
Comment on attachment 8986383 [details]
Bug 1469769 - Part 6b: Word-wrap and reindent long lines with MOZ_ASSERT_UNREACHABLE.

https://reviewboard.mozilla.org/r/251760/#review258584

rs=me
Attachment #8986383 - Flags: review?(nfroyd) → review+
Comment on attachment 8986384 [details]
Bug 1469769 - Part 7: Remove NS_NOTREACHED definition.

https://reviewboard.mozilla.org/r/251762/#review258586
Attachment #8986384 - Flags: review?(nfroyd) → review+
Blocks: 1470374
Tyson, just a heads up: similar to s/NS_PRECONDITION/MOZ_ASSERT/ bug 1459470, I will be replacing the NS_NOTREACHED assertion macro with MOZ_ASSERT_UNREACHABLE in Firefox Nightly 63, probably next week. Your fuzzers may hit some new MOZ_ASSERT_UNREACHABLE crashes. :)
Summary: Replace NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE → Replace NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE or NS_ERROR
Comment on attachment 8986380 [details]
Bug 1469769 - Part 4: svg: Replace failing NS_NOTREACHED with NS_ERROR.

https://reviewboard.mozilla.org/r/251754/#review258990
Attachment #8986380 - Flags: review?(cam) → review+
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b109476dbfb
Part 2: animation: Replace failing NS_NOTREACHED with NS_ERROR. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ebcc2af51f4
Part 3: css: Replace failing NS_NOTREACHED with NS_ERROR. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/10ad8a173782
Part 4: svg: Replace failing NS_NOTREACHED with NS_ERROR. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/220da56d2141
Part 5: widget/gtk: Replace failing NS_NOTREACHED with NS_ERROR. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/33346f699996
Part 6: Replace non-failing NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE. r=froydnj
Comment on attachment 8986377 [details]
Bug 1469769 - Part 1: a11y: Replace failing NS_NOTREACHED with NS_ERROR.

Alexander, I tried to send you a review request through MozReview, but I think MozReview got confused because my commit message used the wrong Bugzilla nickname ("r?asurkov") instead "r?surkov". I'll just ask for your r? here in the bug. :)
Attachment #8986377 - Flags: review?(surkov.alexander)
Comment on attachment 8986377 [details]
Bug 1469769 - Part 1: a11y: Replace failing NS_NOTREACHED with NS_ERROR.

https://reviewboard.mozilla.org/r/251748/#review264382
Attachment #8986377 - Flags: review?(surkov.alexander) → review+
(In reply to Chris Peterson [:cpeterson] from comment #19)
> Comment on attachment 8986377 [details]
> Bug 1469769 - Part 1: a11y: Replace failing NS_NOTREACHED with NS_ERROR.
> 
> Alexander, I tried to send you a review request through MozReview, but I
> think MozReview got confused because my commit message used the wrong
> Bugzilla nickname ("r?asurkov") instead "r?surkov". I'll just ask for your
> r? here in the bug. :)

yeah, I probably should rename myself to :asurkov here to make it closer to moz email and github account.
(In reply to alexander :surkov from comment #20)
>  Attachment #8986377 [details] - Flags: review?(surkov.alexander@gmail.com) → review+

Thanks!

(In reply to alexander :surkov from comment #21)
> yeah, I probably should rename myself to :asurkov here to make it closer to
> moz email and github account.

If you add both :asurkov and :surkov to your Bugzilla name, perhaps MozReview can match either of them?
Keywords: leave-open
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9503f9df661a
Part 1: a11y: Replace failing NS_NOTREACHED with NS_ERROR. r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb7be981ded
Part 7: Remove NS_NOTREACHED definition. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/9503f9df661a
https://hg.mozilla.org/mozilla-central/rev/4bb7be981ded
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.