1620152 - Implement MOZ_ALWAYS_TRUE()/etc using MOZ_DIAGNOSTIC_ASSERT() instead of MOZ_ASSERT()

Status: RESOLVED FIXED

(Core :: MFBT, task, P3)

Description

[Chris Peterson [:cpeterson]]

If a developer confidently opts for MOZ_ALWAYS_TRUE() instead of explicit error handling, I think we should more thoroughly test that no error actually happens in the wild. I propose here to reimplement MOZ_ALWAYS_TRUE()/etc to use MOZ_DIAGNOSTIC_ASSERT() in Nightly release builds instead of MOZ_ASSERT().

This extra assertion should have minimal performance impact in Nightly release builds because it just adds a MOZ_LIKELY-optimized if check for an expression that is supposedly always true. The MOZ_ALWAYS_TRUE() expression is already evaluated in release builds and is probably more expensive that the if check.

I have a couple followup patches for some uses of MOZ_ALWAYS_TRUE(), but I wanted to get your feedback for this proposal before asking others to review those patches.

Here is a Try test run that is mostly green. AFAICT, none of the test failures are related to failing MOZ_ALWAYS_TRUE() assertions.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74ce7d8404dd572e34db61b6b6535f8c9cf28e57

Some stats on the use of MOZ_ALWAYS_TRUE()/etc:

• 578 references to MOZ_ALWAYS_TRUE
• 43 references to MOZ_ALWAYS_FALSE
• 614 references to MOZ_ALWAYS_SUCCEEDS
• 10 uses of MOZ_ALWAYS_TRUE(NS_SUCCEEDED(x)) that could use the MOZ_ALWAYS_SUCCEEDS shorthand
• 0 uses of MOZ_ALWAYS_OK (the 2 references are the macro definition itself)
• 0 uses of MOZ_ALWAYS_ERR (the 2 references are the macro definition itself)

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: Bug 1620152 - Part 2: Implement MOZ_ALWAYS_TRUE()/etc using MOZ_DIAGNOSTIC_ASSERT() instead of MOZ_ASSERT(). r?froydnj

Consolidate the various definitions of MOZ_ALWAYS_{TRUE|FALSE|SUCCEEDS|OK|ERR} and implement them using MOZ_DIAGNOSTIC_ASSERT() so they assert in both debug builds and Nightly (and early Beta) release builds.

Comment 2

[Simon Giesecke [:sg] [he/him]]

The MOZ_ALWAYS_TRUE() expression is already evaluated in release builds and is probably more expensive that the if check.

I think there might be effects beyond that, since the size of the generated code in release builds will increase by the assertion branch directly, code locality might be impacted, and inlining (or other optimizations) might be prevented by that. It may still be beneficial overall to change this, but it should be considered.

Apart from that, I am just wondering if these macros should be renamed to MOZ_DIAGNOSTIC_ALWAYS_TRUE etc. to make it clearer when they are active? This might better be done in an extra step/bug, though.

Comment 3

[(Away)]

Apart from that, I am just wondering if these macros should be renamed to MOZ_DIAGNOSTIC_ALWAYS_TRUE etc. to make it clearer when they are active? This might better be done in an extra step/bug, though.

Such macros already exist... immediately below the affected code. :/ This seems like too much duplication, they should be consolidated before landing IMO.

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to :dmajor from comment #3)

Apart from that, I am just wondering if these macros should be renamed to MOZ_DIAGNOSTIC_ALWAYS_TRUE etc. to make it clearer when they are active? This might better be done in an extra step/bug, though.

Such macros already exist... immediately below the affected code. :/ This seems like too much duplication, they should be consolidated before landing IMO.

I have a patch to consolidate MOZ_DIAGNOSTIC_ALWAYS_TRUE (from bug 1542154). I didn't submit the patch to this bug yet because I wanted to get feedback on this proposal first.

Comment 5

[Chris Peterson [:cpeterson]]

Correction of my comment 0 and patch comments: MOZ_DIAGNOSTIC_ALWAYS is enabled for release builds of NIGHTLY_BUILD and MOZ_DEV_EDITION, but not EARLY_BETA:

https://searchfox.org/mozilla-central/rev/2fd8ffcf087bc59a8e5c962965bbb7bf230bcd28/mfbt/Assertions.h#499-500

Comment 6

[Chris Peterson [:cpeterson]]

I think this consolidation is good. I think the question of whether it should be implemented with MOZ_DIAGNOSTIC_ASSERT under the hood (to which I would probably answer "yes") is something to be addressed in a separate patch that can also remove MOZ_DIAGNOSTIC_ALWAYS_TRUE etc. Can you please change this to just do the consolidation, and then we can look at flipping things to diagnostic asserts in a separate bug? (Easier to see performance changes, back patches out, etc. as a result.)

OK, but I will instead spin out a new bug for the MOZ_ALWAYS_TRUE consolidation instead of morphing this bug and then filing a new bug repeating this bug's MOZ_DIAGNOSTIC_ASSERT proposal.

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: Bug 1620152 - Part 1: MOZ_ASSERT(stackMap) in debug builds instead of MOZ_ALWAYS_TRUE(stackMap) in release builds. r=lth

MOZ_ALWAYS_TRUE() evaluates its expression in both debug and release builds. MOZ_ALWAYS_TRUE(stackMap) would previously be evaluated as a no-op stackMap; in release builds, but this bug will change MOZ_ALWAYS_TRUE() to use MOZ_DIAGNOSTIC_ASSERT() instead of MOZ_ASSERT(). MOZ_ALWAYS_TRUE(stackMap) will then fail in Nightly release builds because stackMap may be null if there are no refs to track.

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: Bug 1620152 - Part 3: Implement MOZ_ALWAYS_TRUE()/etc using MOZ_DIAGNOSTIC_ASSERT() instead of MOZ_ASSERT(). r=froydnj

MOZ_ALWAYS_TRUE() evaluates its expression in both debug and release builds. This bug will change MOZ_ALWAYS_TRUE() to use MOZ_DIAGNOSTIC_ASSERT() instead of MOZ_ASSERT() so it will also assert in Nightly and DevEdition release builds.

Depends on D66920

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: Bug 1620152 - Part 4: Replace MOZ_DIAGNOSTIC_ALWAYS_TRUE() with MOZ_ALWAYS_TRUE(). r=janv

MOZ_ALWAYS_TRUE() evaluates its expression in both debug and release builds. This bug will change MOZ_ALWAYS_TRUE() to use MOZ_DIAGNOSTIC_ASSERT() instead of MOZ_ASSERT(), so MOZ_DIAGNOSTIC_ALWAYS_TRUE() will be redundant.

Depends on D66921

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: Bug 1620152 - Part 2: MOZ_ASSERT() GetSharedPrefValue() result instead of MOZ_ALWAYS_TRUE(). r=njn

MOZ_ALWAYS_TRUE() evaluates its expression in both debug and release builds. This bug will change MOZ_ALWAYS_TRUE() to use MOZ_DIAGNOSTIC_ASSERT() instead of MOZ_ASSERT(). MOZ_ALWAYS_TRUE(NS_SUCCEEDED(rv)) will then fail in Nightly release builds, reintroducing InitStaticPrefsFromShared crash bug 1573731.

Depends on D66920

Comment 11

[Chris Peterson [:cpeterson]]

Pascal, heads up as release owner for Fx77: I plan to land these patches early in the 77 Nightly cycle (this week). The patches change about 1200 uses of MOZ_ALWAYS_TRUE()/etc macros from debug-only MOZ_ASSERT() macro to MOZ_DIAGNOSTIC_ASSERT(), which will force-crash on assertion failure in release builds of Nightly and DevEdition. This might trigger some new crash signatures or spikes in 77 Nightly. These macro changes will not crash in release builds of Beta or Release.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c80be97934f
Part 1: MOZ_ASSERT(stackMap) in debug builds instead of MOZ_ALWAYS_TRUE(stackMap) in release builds. r=lth
https://hg.mozilla.org/integration/autoland/rev/005a31ffa4bf
Part 2: MOZ_ASSERT() GetSharedPrefValue() result instead of MOZ_ALWAYS_TRUE(). r=njn
https://hg.mozilla.org/integration/autoland/rev/a13507db74f7
Part 3: Implement MOZ_ALWAYS_TRUE()/etc using MOZ_DIAGNOSTIC_ASSERT() instead of MOZ_ASSERT(). r=froydnj
https://hg.mozilla.org/integration/autoland/rev/c6fe172dd237
Part 4: Replace MOZ_DIAGNOSTIC_ALWAYS_TRUE() with MOZ_ALWAYS_TRUE(). r=janv

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out because it blocks the backout of Bug 1626967:
https://hg.mozilla.org/integration/autoland/rev/f2ca5ea3c2633fc1f8d1bf8e012fe0f3bcabfa50

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1322f2d1b1aa
Part 1: MOZ_ASSERT(stackMap) in debug builds instead of MOZ_ALWAYS_TRUE(stackMap) in release builds. r=lth
https://hg.mozilla.org/integration/autoland/rev/bc5f2f1649f3
Part 2: MOZ_ASSERT() GetSharedPrefValue() result instead of MOZ_ALWAYS_TRUE(). r=njn
https://hg.mozilla.org/integration/autoland/rev/4c816c835b55
Part 3: Implement MOZ_ALWAYS_TRUE()/etc using MOZ_DIAGNOSTIC_ASSERT() instead of MOZ_ASSERT(). r=froydnj
https://hg.mozilla.org/integration/autoland/rev/9b00ab9d1a8d
Part 4: Replace MOZ_DIAGNOSTIC_ALWAYS_TRUE() with MOZ_ALWAYS_TRUE(). r=janv

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1322f2d1b1aa
https://hg.mozilla.org/mozilla-central/rev/bc5f2f1649f3
https://hg.mozilla.org/mozilla-central/rev/4c816c835b55
https://hg.mozilla.org/mozilla-central/rev/9b00ab9d1a8d

Comment 16

[Chris Peterson [:cpeterson]]

Clearing needinfo because I relanded the patches. The conflicting patches from bug 1626967 have also relanded.

Comment 17

[Karl Tomlinson (:karlt)]

Announced in https://groups.google.com/g/mozilla.dev.platform/c/MInOJeAt-nc/m/mDc-FHH4AAAJ