429930 - NS_ABORT_IF_FALSE lies

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Do as I say, not as I do

Comment 1

[Mike Shaver (:shaver emeritus)]

What motivates this change, exactly?  Are we confident that the existing callers expect an ABORT?

Comment 2

[Benjamin Smedberg]

I do not think we want to make this change without a complete audit.

Comment 3

[Jesse Ruderman]

The caller in bug 328558 sure expects to abort ;)

I want this change so it will be easier for me to make specific assertions fatal, at least locally.  My tree is littered with "if (!(cond)) NS_ABORT();" after assertions that I know are associated with random memory corruption.

NS_ABORT_IF_FALSE used to abort, right?  How many of the callers were added after it changed to not abort?

Comment 4

[Mike Shaver (:shaver emeritus)]

I'm mostly concerned with the ones in layout.  I'd be OK with landing this on the post-1.9 development trunk (not CVS HEAD leading to 1.9.0.x) if we got someone to scan the layout/dom/content/parser cases and see if any need to be non-fatal.

We could also change all current callers to use NS_WHINE_INEFFECTUALLY_IF_FALSE, and then make the switch, so that at least future uses get what they want.

Mind you, fatal assertions are coming (right?) so making this subset fatal a little earlier doesn't seem like all that perilous...

Thanks for the patch and idea, Jeff; should definitely make this switch once we get a bit of confidence that we're not going to abort the world on startup in RTL locales or something.

Comment 5

[Benjamin Smedberg]

Well... if there was going to be a difference between NS_ABORT_IF_FALSE and NS_ASSERTION, I would want it to be that:

NS_ABORT_IF_FALSE is always checked, even in release code, and aborts into the crash reporter in production. Therefore you have to be careful not to use it in places that would be performance sensitive.

NS_ASSERTION is debug-only and you can sprinkle it pretty much anywhere.

And yes, with a basic audit I'm all for doing this in mozilla-central.

Comment 6

[Jesse Ruderman]

FWIW, none of the callers in content/, layout/, or dom/ are among the 360 assertions listed in my known_assertions.txt file.  That basically means I've never hit them in my automated testing (or if I did, the corresponding bugs were fixed).

Comment 7

[Jeff Walden [:Waldo]]

One, there's no macro for a real, fatal assertion that prints an error message without rolling your own.  Two, this macro is pointless right now since it exactly duplicates NS_ASSERTION.  Three, if they didn't expect to die (and really, is that reasonable given the name?), they should have at least used NS_ASSERTION.  Four, this macros isn't used very much yet anyway:

http://mxr.mozilla.org/mozilla/search?string=NS_ABORT_IF_FALSE

I don't recognize any of the assertions except possibly the ones in nsHTMLDocument.cpp, and we don't hit any of them in any of the various flavors of Mochitest, nor do we hit them in reftests or crashtests.  This shouldn't even matter anyway, tho -- we don't run any of those tests in debug builds yet.  If we somehow do start hitting a particular abort and can't fix the problem, we can just change the assert for the moment and move on with life.

Comment 8

[Mike Shaver (:shaver emeritus)]

Yeah, I think we should take this when we get the post-1.9 trunk opened; if Jesse hasn't hit the assertions, they're unlikely to cripple us, and as Waldo points out there's a really easy path to recovery if we find one that's unexpectedly severe.

I don't want to take it now on the CVS trunk, because a lot of us are living in debug builds to try and find+fix bugs that hit add-ons, and adding another peril to that process at this stage in the schedule feels like bad economics.

Comment 9

[Mike Shaver (:shaver emeritus)]

Comment on attachment 316697 [details] [diff] [review]
Do as I say, not as I do

r+sr=shaver

Comment 10

[Jesse Ruderman]

This was checked in 2008-06-04.