Bug 36527 – Need an ASSERT that's enabled in release builds

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jeremy Moskovich]

The current ASSERT() macro is disabled in release builds, call sites that would otherwise call it make direct use of the CRASH() macro.

It would be nice to have a version of ASSERT() that does work in release builds to use instead of making calls to CRASH() - FATAL_ASSERT() ?

In Chromium we have CHECK & DCHECK macros for this purpose.

[Jeremy Orlow]

I definitely agree.  For example, I'm reviewing audio code that has ASSERTs for sanity checks on buffer lengths.  If there is some bad assumption that would cause us to exceed our buffer, we'd much rather crash rather than allow a possible exploit.  But it doesn't make sense to add complex error code paths for stuff that should never happen.  So instead I've been suggesting adding |if (...) CRASH();|s.  I think having a proper macro would be beneficial though.

Another example of where this is useful is https://bugs.webkit.org/show_bug.cgi?id=36426 where Jeremy (the other one) added some debug code to the Chromium WebKit API to try and expose a bug.

Any thoughts on the best name for this?

[Darin Adler]

The major reason the expression goes inside the assert invocation is so that the entire expression can be compiled out.

For the rare cases where we want assertions even in production code, I think it's good to have all the code outside any macro.

We can give CRASH a better name to make it clearer how it should be used and maybe even add a message, but I'd prefer not to also have an ASSERT-style macro where the contents are always evaluated. Such things can be written as if statements. But since the Chrome project already has this, maybe you guys don't agree.

[Jeremy Orlow]

(In reply to comment #2)
> The major reason the expression goes inside the assert invocation is so that
> the entire expression can be compiled out.
> 
> For the rare cases where we want assertions even in production code, I think
> it's good to have all the code outside any macro.
> 
> We can give CRASH a better name to make it clearer how it should be used and
> maybe even add a message, but I'd prefer not to also have an ASSERT-style macro
> where the contents are always evaluated. Such things can be written as if
> statements. But since the Chrome project already has this, maybe you guys don't
> agree.

Normally I'd agree with you.  I guess the main reason why having a macro seems natural here is that I see it as a variant of ASSERT and ASSERT is a macro.  I suppose that's the only reason though.  I don't feel super strongly, so if others feel the same way, we can close this as WONTFIX.

[Darin Fisher (:fishd, Google)]

Can we see some examples of code that would be cleaner with FATAL_ASSERT?  Maybe that would be helpful in evaluating the need.  I tend to agree that explicitly calling CRASH is fine, and I don't see the need for a CHECK equivalent in WebKit.

For reference, the Chromium codebase inherits DCHECK and CHECK from internal Google code.  Those are part of a standard logging package that everyone at Gogole uses, and we wanted to make it easy for anyone at Google to hack Chromium.

[Jeremy Orlow]

https://bugs.webkit.org/show_bug.cgi?id=36426 (that I posted earlier) is a good example.

Also https://bugs.webkit.org/attachment.cgi?id=51461&action=review (search for "CRASH").

[Darin Adler]

If we think the word FATAL expresses this well, then I think we probably a name like FATAL_ERROR for something that invokes CRASH. I personally think we can live with only FATAL_ERROR, but I would not be strongly opposed to FATAL_ASSERT.

I think we need a brief note on the website explaining the proper use of these. I worry that they will be used too often.

[Jeremy Orlow]

(In reply to comment #6)
> If we think the word FATAL expresses this well, then I think we probably a name
> like FATAL_ERROR for something that invokes CRASH. I personally think we can
> live with only FATAL_ERROR, but I would not be strongly opposed to
> FATAL_ASSERT.
> 
> I think we need a brief note on the website explaining the proper use of these.
> I worry that they will be used too often.

Since both of you (Darins) are against this, I'm perfectly fine with us dropping this.

But, if we do go ahead with this, I'd say that |FATAL_ERROR(some_expression);| is less clearly an assertion than |FATAL_ASSERT(some_expression);|.

And where on the web page would be good for a note?

Anyone else have thoughts?  If no one else is in favor, maybe we should just forget about this and continue with |if () CRASH;|

[Darin Adler]

FATAL_ERROR is a new name for CRASH, not a name for an assertion. The macro would not take an argument.

[Darin Adler]

I think the note could be like the RefPtr one I wrote a while back. I'm not sure where the link to that is, but the key is the content; linking to it is easy.

[Jeremy Orlow]

(In reply to comment #8)
> FATAL_ERROR is a new name for CRASH, not a name for an assertion. The macro
> would not take an argument.

Ohhh.  I see.  Yeah, I guess FATAL_ERROR is a bit more clear than CRASH, but honestly I'm not sure it's enough of an improvement to warrant a change.  But I assume you do?


(In reply to comment #9)
> I think the note could be like the RefPtr one I wrote a while back. I'm not
> sure where the link to that is, but the key is the content; linking to it is
> easy.

I agree linking is easy, but if no one can find the link, what's the use?  :-)

To be honest, I wonder if a code comment above CRASH is the right thing to do here.  I think a separate web page for PassRefPtr/RefPtr makes sense since it's indexed by search engines and because it's pretty big for putting inside the code (plus it's not clear whether it'd go in PassRefPtr or RefPtr.h) but I don't think CRASH/FATAL_ERROR suffer from this problem.

[Darin Adler]

(In reply to comment #10)
> To be honest, I wonder if a code comment above CRASH is the right thing to do
> here.

We can start with that.

> I don't think CRASH/FATAL_ERROR suffer from this problem.

I think we need a guide for when to use assertions and when to use CRASH. Not just a comment in the code, but a background about rationale for the project.

[Jeremy Orlow]

(In reply to comment #11)
> (In reply to comment #10)
> > To be honest, I wonder if a code comment above CRASH is the right thing to do
> > here.
> 
> We can start with that.

Sounds good.  Jeremy are you willing to do this?

> > I don't think CRASH/FATAL_ERROR suffer from this problem.
> 
> I think we need a guide for when to use assertions and when to use CRASH. Not
> just a comment in the code, but a background about rationale for the project.

I can try taking a stab at this if you'd like, but I can't think of much more than a paragraph or so to say on the subject.

I guess the gist is that you should ASSERT on stuff that should never happen that will cause incorrect results but shouldn't result in a crash or memory corruption.  CRASH should be used when we're reasonably sure something should never happen but it happening would be _really_ bad.  It can also be used when there's no way to recover (like OOM).  The ideal is to handle errors properly, possibly after ASSERTing on them if they should never happen, but this is sometimes not practical.

Is there more to say on the subject (beyond maybe a few examples and more clear language)?

[Darin Adler]

A few thoughts.

ASSERT has no effect on production code; it is used during development to help us find programming mistakes.

CRASH has a direct effect on end users, and is used to gather information from the field and protect end users.

The benefits of a CRASH (not worded quite right):

    - Browser vendors will get crash reports from the field from our crash tracing technologies.
    - Code after the CRASH is guaranteed unreachable, which can help prevent some bugs from being security liabilities.

The cost of a CRASH:

    - Takes an internal programming error and turns it into a crash. Blows away at least a tab, and in some cases the entire web browser, in cases that otherwise might be harmless.

Then we need examples where an ASSERT is right, and examples where a CRASH is right.

And we need to explain the largest hazard for ASSERT: expressions inside the assertion are evaluated in debug builds but not in production code.

We also would want to explain ASSERT_UNUSED.

[Alexey Proskuryakov]

> We also would want to explain ASSERT_UNUSED.

Possibly also COMPILE_ASSERT, ASSERT_NOT_REACHED and ASSERT_ARG.

[Jeremy Moskovich]

Created attachment 52156 [details]
Step 1 - Beef up documentation in Assertions.h

[Jeremy Orlow]

Comment on attachment 52156 [details]
Step 1 - Beef up documentation in Assertions.h

Looks good to me.  Will r+ tomorrow if no one has any feedback/objections in the mean time.

[Geoffrey Garen]

> CRASH should be used when we're reasonably sure something should
> never happen but it happening would be _really_ bad.

ASSERT is compiled out of release builds for good reason -- if it were compiled in, WebKit would be slow. WebKit + Windows debug builds come with some many ASSERTs that a debug build is almost unusably slow.

So far, we've used CRASH extremely sparingly, avoiding this performance problem. Specifically, we've used ASSERT for problems that we don't expect to encounter, and CRASH for problems we know to be possible, like out-of-memory conditions.

I'm very worried about the notion expressed here that we should have compile-time checks for all potentially bad errors. We can only afford these checks if the error would be bad enough, its likelihood is high enough, and the cost of checking is low enough.

+   For use when an unrecoverable error condition has occured.
+   Macro is enabled in both debug and release mode.

This comment would be better if it mentioned the performance cost of CRASH(), and suggested it be used sparingly. Something like, "Use CRASH() for known, unrecoverable errors like out-of-memory. CRASH() is compiled into release builds, so it has a performance cost."

+/* ASSERT, ASSERT_NOT_REACHED, ASSERT_UNUSED
+
+  By default these macros are disabled in release mode, expressions inside the assertions are
+  similarly evaluated in debug builds but not in production code.

This is a run-on sentence. "By default" is confusing because it's not clear when the default is overridden. I would say, "These macros are compiled out of release builds. Expressions inside them are evaluated in debug builds only."

[Darin Adler]

(In reply to comment #17)
> This comment would be better if it mentioned the performance cost of CRASH(),
> and suggested it be used sparingly. Something like, "Use CRASH() for known,
> unrecoverable errors like out-of-memory. CRASH() is compiled into release
> builds, so it has a performance cost."

While I agree with the sentiment, it's not the performance cost of CRASH that I think you're concerned about. It's the cost of the code that decides whether to invoke CRASH.

Geoff, maybe you can revise your suggested wording to be more accurate.

[Geoffrey Garen]

(In reply to comment #18)
> (In reply to comment #17)
> > This comment would be better if it mentioned the performance cost of CRASH(),
> > and suggested it be used sparingly. Something like, "Use CRASH() for known,
> > unrecoverable errors like out-of-memory. CRASH() is compiled into release
> > builds, so it has a performance cost."
> 

> While I agree with the sentiment, it's not the performance cost of CRASH that I
> think you're concerned about. It's the cost of the code that decides whether to
> invoke CRASH.
> 
> Geoff, maybe you can revise your suggested wording to be more accurate.

Good point. How about: "Use CRASH() in response to known, unrecoverable errors like out-of-memory. CRASH() is compiled into debug and release builds. To test for unknown errors and verify assumptions, use ASSERT instead, to avoid impacting performance in release builds."

[Jeremy Moskovich]

Created attachment 52276 [details]
Step 1 - Beef up documentation in Assertions.h

Revised wording per Darin & Geoffrey' comments

[Geoffrey Garen]

Comment on attachment 52276 [details]
Step 1 - Beef up documentation in Assertions.h

r=me

[WebKit Commit Bot]

Comment on attachment 52276 [details]
Step 1 - Beef up documentation in Assertions.h

Clearing flags on attachment: 52276

Committed r57016: <http://trac.webkit.org/changeset/57016>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jeremy Moskovich]

Some more technical documentation still needed...

[Jeremy Orlow]

Please open a new bug for the subsequent patches.  You can link between them, but this should stay 'fixed'.