Closed Bug 1457813 Opened 6 years ago Closed 6 years ago

Replace NS_PRECONDITION with MOZ_ASSERT

Categories

(Core :: XPCOM, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(4 files)

NS_PRECONDITION is an alias for NS_ASSERTION, which just logs an "###!!! ASSERTION" warning on an assertion failure and continues running. The "###!!! ASSERTION" warnings are easy to miss. Replacing NS_PRECONDITION with MOZ_ASSERT will turn assertion failures into crashes so bugs will be found sooner, remove an extra layer of debug macros, and reduce line lengths with a shorter name.

Of the 1438 calls to NS_PRECONDITION, only two fail when running Try tests and these already have bugs filed: bug 478135 and bug 749326. I changed these NS_PRECONDITIONs to NS_ASSERTIONs.

In bug 1416164, I similarly replaced NS_ABORT, NS_NOTYETIMPLEMENTED, and NS_POSTCONDITION with MOZ_ASSERT.

I considered replacing the NS_ASSERTIONs testing mRefCnt values with MOZ_DIAGNOSTIC_ASSERT instead of MOZ_ASSERT because those could be serious bugs, but I don't know whether that overhead would regress performance benchmarks of Nightly builds.
See Also: → NS_ASSERTION_SUX
I think a wholesale s/NS_PRECONDITION/NS_ASSERTION/ is reasonable.  I am less sure about s/NS_ASSERTION/MOZ_ASSERT/; ISTR some dev-platform threads where people objected to this idea:

https://groups.google.com/d/msg/mozilla.dev.platform/gqSIOc5b-BI/VESzJwiYjikJ

https://groups.google.com/d/msg/mozilla.dev.platform/Jw_4xdUTNnY/5vuQpPoz_G8J (more warnings-focused)\

https://groups.google.com/d/msg/mozilla.dev.platform/UZ7c2EnBzdI/Y5g2pqnkFjMJ

https://blog.mozilla.org/nnethercote/2011/06/07/what-is-the-point-of-non-fatal-assertions/ (which inspired the above thread)

I think various people have come down on the side of nonfatal assertions being OK (and we do have test coverage for these).  My recollection is that dbaron was one of the strongest proponents of keeping nonfatal assertions; ni? to him to see if my recollection is correct.

OTOH, perhaps you have been running these patches through Try, and fatal assertions haen't made much of a difference, so...?
Flags: needinfo?(dbaron)
(In reply to Nathan Froyd [:froydnj] from comment #5)
> I think various people have come down on the side of nonfatal assertions
> being OK (and we do have test coverage for these).  My recollection is that
> dbaron was one of the strongest proponents of keeping nonfatal assertions;
> ni? to him to see if my recollection is correct.

Since we have test coverage for expected NS_ASSERTION failure counts, replacing NS_PRECONDITION with fatal MOZ_ASSERT wouldn't help automated tests find regressions sooner but it could help developers find regressions locally. Developers would hit fatal MOZ_ASSERTs but wouldn't notice a change in NS_ASSERTION failure counts.

> OTOH, perhaps you have been running these patches through Try, and fatal
> assertions haven't made much of a difference, so...?

I have been running these fatal s/NS_PRECONDITION/MOZ_ASSERT/ patches on Try and only hit the known NS_PRECONDITION failures noted in patch #1.
So I have mixed feelings here.

One of the goals of adding support to the test harnesses to cause failures on NS_ASSERTION failures was so that we would stop introducing *new* NS_ASSERTION test failures, with an eye towards eventually replacing them with MOZ_ASSERT.  However, given that many reftests and mochitests are still annotated as having known assertions, we're clearly not ready to do that mass substitution.

We may, on the other hand, be able to do that mass substitution for *most* NS_ASSERTIONs.  And I suppose it's possible that all NS_PRECONDITIONs are in that "most" subset that could be replaced with MOZ_ASSERT, though if they're not, I think it probably makes more sense to replace them with NS_ASSERTION for now, since that's semantically equivalent to what was there before.  (I agree there's no good reason for NS_PRECONDITION to be separate from NS_ASSERTION.)

But does that mean we should remove NS_ASSERTION entirely?  I tend to think not.  At a minimum, I think NS_ASSERTION is useful as a transitional state -- when we're working to try to ensure that we can assert a condition, but we're not quite there yet.  It's useful to have that state, where adding new cases would trigger a test failure, but where we haven't yet fixed all the existing cases yet.  It's a case where our tooling can clearly help us make progress towards an end goal, and I don't see a reason to remove that tooling and make it harder for us to reach those goals.

Then there's the question of whether NS_ASSERTION makes sense as a permanent state.  I think there's a decent argument that it does, in cases of "if you hit this state, something has very likely gone wrong, to the level that we're pretty sure you want to drop into the debugger and look at it... but it's possible that this state might happen in some error cases as well".  Maybe we wouldn't have that state in code that we fully understand, but in code that isn't well understood right now, or code with known architectural problems that we'd like to fix but don't currently have the resources, that might be a long-term state.

I think roc had other arguments as well for NS_ASSERTION as a permanent state.
Flags: needinfo?(dbaron)
I think another argument for non-fatal assertions that roc had is that, when debugging a problem, the best point to start debugging the problem that happens in a release build may, in a debug build, be *after* the first assertion.  If your assertions are fatal, you can't do that, but if they're not fatal, such debugging strategies are easy.
(A common case of what I mentioned in the previous comment would be a case where an underlying problem triggers two different assertions, and the second of those assertions has more direct links to the data with the bug.  In that case, with a debugger from rr, it's easy to debug backwards from the second assertion to the problem, but harder from the first assertion.)
While NS_PRECONDITION is an alias for NS_ASSERTION, the convention is to use NS_PRECONDITION to validate function arguments or an object's internal state. If a function argument or an object's internal state is bad, then the fault is probably in the caller so the callee asserting hard and early (with MOZ_ASSERT) might be best. Otherwise the bad argument or state could further corrupt internal state by the time the second soft assertion fails.

Regardless, I can change this patch from MOZ_ASSERT to NS_ASSERTION, if you prefer we take just one step forward now.
OK, I hadn't actually looked at the patches here and was led astray by the commit message for part 2; I assumed that we were wholesale running `s/NS_ASSERTION/MOZ_ASSERT/` and was very confused, especially by the commit message in part 3, coming after part 2.  The bug title is correct, but I wasn't really processing that for some reason.

Thinking a little harder, I take back what I said in comment 5: My sense is that people use(d) NS_PRECONDITION in a Eiffel-sense, as something that should definitely hold before this function executes, rather than in the NS_ASSERTION "this should hold: we can deal with it not holding, but you might want to check out what's going wrong" sense.  And I think if we did `s/NS_PRECONDITION/NS_ASSERTION/`, we'd lose information about which NS_ASSERTIONs were used in which sense.  Preserving that information seems valuable.

So I think we should move forward with this patch series; I'm going to r+ this, but dbaron has the final say.  ni? to him to see what he thinks of the above reasoning (which echoes what Chris said in comment 10).

We are not yet getting rid of NS_ASSERTION, to be clear; that is a discussion for another day.  Perhaps we should collect the rationales on non-fatal NS_ASSERTION and stick them in a comment on NS_ASSERTION, so people tempted to remove it can be informed without searching all over the Mozilla firehose spew.

Chris, would you please file a bug for Thunderbird depending on this bug, so the comm-central folks can be aware of upcoming compile and/or runtime bustage?
Flags: needinfo?(dbaron)
Flags: needinfo?(cpeterson)
Comment on attachment 8972774 [details]
Bug 1457813 - Part 2: Replace non-asserting NS_ASSERTIONs with MOZ_ASSERTs.

https://reviewboard.mozilla.org/r/240674/#review247626

rs=me; I skimmed through the first page.

::: commit-message-d35d2:1
(Diff revision 1)
> +Bug 1457813 - Part 2: Replace non-asserting NS_ASSERTIONs with MOZ_ASSERTs. r?froydnj

I get that `NS_PRECONDITION` is equivalent in some sense to `NS_ASSERTION`, but the commit message here should say `NS_PRECONDITION`, because that's really what the patch is doing.
Attachment #8972774 - Flags: review?(nfroyd) → review+
Comment on attachment 8972775 [details]
Bug 1457813 - Part 3: Reindent replaced NS_PRECONDITIONS.

https://reviewboard.mozilla.org/r/240676/#review247682

rs=me, having skimmed through the first page in mozreview.
Attachment #8972775 - Flags: review?(nfroyd) → review+
Comment on attachment 8972773 [details]
Bug 1457813 - Part 1: Replace asserting NS_PRECONDITIONs with NS_ASSERTIONs.

https://reviewboard.mozilla.org/r/240672/#review247684
Attachment #8972773 - Flags: review?(nfroyd) → review+
Comment on attachment 8972776 [details]
Bug 1457813 - Part 4: Remove NS_PRECONDITION definition.

https://reviewboard.mozilla.org/r/240678/#review247686
Attachment #8972776 - Flags: review?(nfroyd) → review+
Chris, please also post to dev-platform when this patch set has landed.  Thank you!
Yeah, I'm fine with this.  (Also not sure why I have "final say".)  That said, I'm a little confused by comment 11 since I think going ahead with the patch series goes ahead with removing the information that you said you didn't want to remove.

One note is that I think the premise here is that the assertions being converted to MOZ_ASSERT are the ones that aren't currently firing.  I think if that turns out to not be the case for some of them, we should be quick to revert to NS_ASSERTION as a form of "partial backout".

(Also, in part 1, I think maybe there's a convention of putting the bug number right after the "FIXME" rather than at the end of the line.)
Flags: needinfo?(dbaron)
(In reply to Nathan Froyd [:froydnj] from comment #11)
> OK, I hadn't actually looked at the patches here and was led astray by the
> commit message for part 2; I assumed that we were wholesale running
> `s/NS_ASSERTION/MOZ_ASSERT/` and was very confused

Sorry! The commit message was a wrong.

> Chris, would you please file a bug for Thunderbird depending on this bug, so
> the comm-central folks can be aware of upcoming compile and/or runtime
> bustage?

Sure. I'll file a Thunderbird bug.

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #17)
> One note is that I think the premise here is that the assertions being
> converted to MOZ_ASSERT are the ones that aren't currently firing.  I think
> if that turns out to not be the case for some of them, we should be quick to
> revert to NS_ASSERTION as a form of "partial backout".

I'll alert the sheriffs before I land the patches. To avoid any beta merge problems for the sheriffs, I'll wait to land these patches next week after Nightly 62 starts.

> (Also, in part 1, I think maybe there's a convention of putting the bug
> number right after the "FIXME" rather than at the end of the line.)

OK. I'll fix the FIXME comments.
Flags: needinfo?(cpeterson)
Blocks: 1459470
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/173cff4225fb
Part 1: Replace asserting NS_PRECONDITIONs with NS_ASSERTIONs. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/a31c1b8a41f8
Part 2: Replace non-asserting NS_PRECONDITIONs with MOZ_ASSERTs. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/b57df5aa1534
Part 3: Remove NS_PRECONDITION definition. r=froydnj
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b74b76204d2
Backed out changeset b57df5aa1534 for conflict after bug 833098 got backed out. CLOSED TREE
Backed out changeset b57df5aa1534 (bug 1457813) for conflict after bug 833098 got backed out. CLOSED TREE 

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b74b76204d2e1a0b23bc816b57373a6df79f407
Flags: needinfo?(cpeterson)
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41c8fafb84fb
Part 3: Remove NS_PRECONDITION definition. r=froydnj
Trying to land part 3 again
Flags: needinfo?(cpeterson)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/41c8fafb84fb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1461979
Depends on: 1464165
See Also: → 1469769
You need to log in before you can comment on or make changes to this bug.