928636 - Add MOZ_RELEASE_ASSERT to mfbt

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Bobby Holley (:bholley)]

Currently, release-mode assertions need to manually check a condition and then MOZ_CRASH if it's the case. These slightly-convenient ergonomics provide unhelpful inertia against using release-mode assertions when they're warranted.

Adding such an API to MFBT should be straightforward. Any bikeshedding on the name before doing so?

Comment 1

[(no longer active)]

MOZ_RUNTIME_ASSERT?  Or, ahem, assert? ;-)

Comment 2

[Masatoshi Kimura [:emk]]

We disables plain assert() in optimized builds.
https://mxr.mozilla.org/mozilla-central/search?string=NDEBUG&case=1&find=&findi=&filter=\bNDEBUG&hitlimit=&tree=mozilla-central

Comment 3

[Bobby Holley (:bholley)]

Attachment: Implement MOZ_RUNTIME_ASSERT. v1

We piggyback on the plugin crash testing code to make sure that
MOZ_RUNTIME_ASSERT really does crash.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Whitespace changes. v1

This is just a macro reindentation.

Comment 5

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=5dc219cf73fd

Comment 6

[Bobby Holley (:bholley)]

Attachment: Implement MOZ_RUNTIME_ASSERT. v2

Looks like changing the plugin crashing to use MOZ_RUNTIME_ASSERT causes the
intentional crashes to appear on tinderbox, which is suboptimal. Let's just
kill that part.

Comment 7

[Benjamin Smedberg]

Could you please add a test to ensure that this triggers the crash reporter, here:
* http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/nsTestCrasher.cpp?force=1#49
* http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/CrashTestUtils.jsm#20
* http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/unit/test_crash_moz_crash.js

Unless this just forwards to MOZ_CRASH, in which case it's already tested.

Comment 8

[Bobby Holley (:bholley)]

Well, it all funnels into MOZ_REALLY_CRASH, which is the same thing that MOZ_CRASH does. I'm concerned about adding tests for it because, per comment 6, it confuses TBPL. Do you think the patch as it stands is sufficient?

Comment 9

[Benjamin Smedberg]

I don't think those "assertion failure" lines actually caused the orange in that try run. I do think it's important to have test coverage that MOZ_RUNTIME_ASSERT triggers the crash reporter, but if the existing MOZ_CRASH check is sufficient, then this is fine.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> I don't think those "assertion failure" lines actually caused the orange in
> that try run.

They didn't. But it's still going to show up on every log summary ever, and confuse people, until TBPL finds a way to special-case it.

> I do think it's important to have test coverage that
> MOZ_RUNTIME_ASSERT triggers the crash reporter, but if the existing
> MOZ_CRASH check is sufficient, then this is fine.

I think it probably is.

Comment 11

[Jason Orendorff [:jorendorff]]

I agree test coverage is important. People will just have to be confused, until TBPL figures out how to filter it out.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #11)
> I agree test coverage is important. People will just have to be confused,
> until TBPL figures out how to filter it out.

I dunno, it kind of seems similarly dangerous to tell TBPL to ignore the string "Assertion failure", which I'd imagine it unconditionally logs at the moment. Or maybe it would be easy to just do that for the plugin crashing tests?

Ed - I'm adding a runtime assertion, and want to add test coverage for it in our test suite. But when I do, it shows up on TBPL (see above). Should I just go ahead with it, and will TBPL be able to deal in a timely fashion?

Comment 13

[Ed Morley [:emorley]]

There is no easy way for TBPL to differentiate the strings at present - and it also seems like something we should be handling at a harness level (similar to how we handle the crash reporter tests).

I'd prefer that the tests for this land as a follow-up that resolves this in the harness etc - if that's ok? :-)

Comment 14

[Ed Morley [:emorley]]

s/as a follow-up/in a follow-up/

Comment 15

[Bobby Holley (:bholley)]

(In reply to Ed Morley [:edmorley UTC+0] from comment #14)
> s/as a follow-up/in a follow-up/

Filed bug 941402.

So this bug is ready to go, pending Waldo's review.

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 832299 [details] [diff] [review]
Implement MOZ_RUNTIME_ASSERT. v2

Review of attachment 832299 [details] [diff] [review]:
-----------------------------------------------------------------

I am not particularly a fan of this macro, because I think it makes it easier for people to just paper over bad code with safe behavior, rather than fixing the underlying flaws in it, that will show up in debug builds regardless whether this or MOZ_ASSERT is used.  But the point that people doing stupid things in extensions aren't likely to test in debug builds is unfortunately fair.  Feh.

::: mfbt/Assertions.h
@@ +275,5 @@
>   *              "we already set [[PrimitiveThis]] for this String object");
>   *
>   * MOZ_ASSERT has no effect in non-debug builds.  It is designed to catch bugs
> + * *only* during debugging, not "in the field". If you want the latter, use
> + * MOZ_RUNTIME_ASSERT, which is applies to non-debug builds as well.

"runtime" is more than a bit vague.  MOZ_RELEASE_ASSERT seems preferable to me, or even MOZ_ASSERT_EVEN_IN_RELEASE as jorendorff possibly-seriously suggested on IRC.  (The benefit to longer being that people will use it less -- which I think is the right move, the point of assertions is to do whatever laborious checking you want without regard to the cost, because you know you won't pay that cost when it matters, i.e. in release builds [because who cares about debug perf, really].  If this macro were ever commonly used, it would indicate some fundamental problem with the relevant code in terms of its quality, methods of operation, construction, etc.)  That said, MOZ_ASSERT_EVEN_IN_RELEASE is length-wise a big enough roadbump it might even be over-obnoxiously so, so I'm kind of slightly inclined to shy away from it.

Also, "is applies".

@@ +319,4 @@
>       MOZ_ASSERT_GLUE(MOZ_ASSERT_CHOOSE_HELPER(MOZ_COUNT_ASSERT_ARGS(__VA_ARGS__)), \
>                       (__VA_ARGS__))
> +#ifdef DEBUG
> +#  define MOZ_ASSERT(...) MOZ_RUNTIME_ASSERT(__VA_ARGS__)

Given the MSVC varargs comments above, you want to be careful with this, with that compiler -- could hit MSVC-specific compilation errors.  Just to note.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 831917 [details] [diff] [review]
Whitespace changes. v1

Review of attachment 831917 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Assertions.h
@@ +313,5 @@
> + /* Pick the right helper macro to invoke. */
> +#define MOZ_ASSERT_CHOOSE_HELPER2(count) MOZ_ASSERT_HELPER##count
> +#define MOZ_ASSERT_CHOOSE_HELPER1(count) MOZ_ASSERT_CHOOSE_HELPER2(count)
> +#define MOZ_ASSERT_CHOOSE_HELPER(count) MOZ_ASSERT_CHOOSE_HELPER1(count)
> + /* The actual macro. */

May as well make this "macros" while you're here.

Comment 18

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=61db4d7a25b5

Comment 19

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7d723a160ca2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5c27d8c686c0

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d723a160ca2
https://hg.mozilla.org/mozilla-central/rev/5c27d8c686c0