1469769 - Replace NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE or NS_ERROR

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Chris Peterson [:cpeterson]]

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 1

[Chris Peterson [:cpeterson]]

Attachment: Bug 1469769 - Part 1: a11y: Replace failing NS_NOTREACHED with NS_ERROR.

I'm replacing non-failing calls to NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE, but this SelectionManager assertion fails when running the testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py TestBackForwardNavigation.test_non_remote_about_pages marionette test. This assertion failure is bug 1126649.

This patch DOES NOT fix the cause of the assertion failure (a missing HyperTextAccessible). It just replaces this failing NS_NOTREACHED with NS_ERROR because I can't replace with a fatal MOZ_ASSERT_UNREACHABLE.

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

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Bug 1469769 - Part 2: animation: Replace failing NS_NOTREACHED with NS_ERROR.

I'm replacing non-failing calls to NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE, but this NS_NOTREACHED fails when running the devtools/client/animationinspector/test/browser_animation_refresh_on_removed_animation.js test. This assertion failure is bug 1189015.

This patch DOES NOT fix the cause of the assertion failure (a missing TextNodeCorrespondenceProperty). It just replaces this failing NS_NOTREACHED with NS_ERROR because I can't replace with a fatal MOZ_ASSERT_UNREACHABLE.

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

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1469769 - Part 3: css: Replace failing NS_NOTREACHED with NS_ERROR.

I'm replacing non-failing calls to NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE, but this SelectionManager assertion fails when running the Linux debug Web platform tests with e10s test-linux32/debug-web-platform-tests-reftests-e10s-6 W-e10s(Wr6). This assertion failure is bug 1221888.

Marionette  INFO    Testing http://web-platform.test:8000/css/CSS2/ui/outline-applies-to-005.xht == http://web-platform.test:8000/css/CSS2/reference/no-red-on-blank-page-ref.xht
###!!! ASSERTION: we should have saved a frame property: 'Error', file /builds/worker/workspace/build/src/layout/painting/nsCSSRendering.cpp, line 1038

This patch DOES NOT fix the cause of the assertion failure (a missing HyperTextAccessible). It just replaces this failing NS_NOTREACHED with NS_ERROR because I can't replace with a fatal MOZ_ASSERT_UNREACHABLE.

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

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: Bug 1469769 - Part 4: svg: Replace failing NS_NOTREACHED with NS_ERROR.

I'm replacing non-failing calls to NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE, but this NS_NOTREACHED fails when running the dom/svg/crashtests/412104-1.svg test. This assertion failure is bug 903785.

This patch DOES NOT fix the cause of the assertion failure (a missing TextNodeCorrespondenceProperty). It just replaces this failing NS_NOTREACHED with NS_ERROR because I can't replace with a fatal MOZ_ASSERT_UNREACHABLE.

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

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: Bug 1469769 - Part 5: widget/gtk: Replace failing NS_NOTREACHED with NS_ERROR.

I'm replacing non-failing calls to NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE, but this NS_NOTREACHED fails when running the browser/base/content/test/general/browser_tab_dragdrop2.js test because mContainer is unexpectedly *not* null. This is bug 1469183.

This patch DOES NOT fix the cause of the assertion failure. It just replaces this failing NS_NOTREACHED with NS_ERROR because I can't replace with a fatal MOZ_ASSERT_UNREACHABLE.

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

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: Bug 1469769 - Part 6a: Replace non-failing NS_NOTREACHED with MOZ_ASSERT_UNREACHABLE.

This patch is an automatic replacement of s/NS_NOTREACHED/MOZ_ASSERT_UNREACHABLE/. Reindenting long lines and whitespace fixups follow in patch 6b.

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

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: Bug 1469769 - Part 6b: Word-wrap and reindent long lines with MOZ_ASSERT_UNREACHABLE.

This patch is mostly whitespace changes to fix lines longer than 80 characters after replacing s/NS_NOTREACHED/MOZ_ASSERT_UNREACHABLE/. There are a couple other formatting cleanups, too.

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

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: Bug 1469769 - Part 7: Remove NS_NOTREACHED definition.

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

Comment 9

[Karl Tomlinson (:karlt)]

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

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8986378 [details]
Bug 1469769 - Part 2: animation: Replace failing NS_NOTREACHED with NS_ERROR.

https://reviewboard.mozilla.org/r/251750/#review258388

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8986379 [details]
Bug 1469769 - Part 3: css: Replace failing NS_NOTREACHED with NS_ERROR.

https://reviewboard.mozilla.org/r/251752/#review258390

Comment 12

[Nathan Froyd [:froydnj]]

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

Comment 13

[Nathan Froyd [:froydnj]]

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

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8986384 [details]
Bug 1469769 - Part 7: Remove NS_NOTREACHED definition.

https://reviewboard.mozilla.org/r/251762/#review258586

Comment 15

[Chris Peterson [:cpeterson]]

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. :)

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8986380 [details]
Bug 1469769 - Part 4: svg: Replace failing NS_NOTREACHED with NS_ERROR.

https://reviewboard.mozilla.org/r/251754/#review258990

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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 18

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/9b109476dbfb
https://hg.mozilla.org/mozilla-central/rev/6ebcc2af51f4
https://hg.mozilla.org/mozilla-central/rev/10ad8a173782
https://hg.mozilla.org/mozilla-central/rev/220da56d2141
https://hg.mozilla.org/mozilla-central/rev/33346f699996

Comment 19

[Chris Peterson [:cpeterson]]

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. :)

Comment 20

[alexander :surkov (:asurkov)]

Comment on attachment 8986377 [details]
Bug 1469769 - Part 1: a11y: Replace failing NS_NOTREACHED with NS_ERROR.

https://reviewboard.mozilla.org/r/251748/#review264382

Comment 21

[alexander :surkov (:asurkov)]

(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.

Comment 22

[Chris Peterson [:cpeterson]]

(In reply to alexander :surkov from comment #20)
>  Attachment #8986377 [details] - Flags: review?([e-mail]) → 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?

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/9503f9df661a
https://hg.mozilla.org/mozilla-central/rev/4bb7be981ded