Closed Bug 928636 Opened 6 years ago Closed 6 years ago

Add MOZ_RELEASE_ASSERT to mfbt

Categories

(Core :: MFBT, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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?
MOZ_RUNTIME_ASSERT?  Or, ahem, assert? ;-)
Attached patch Implement MOZ_RUNTIME_ASSERT. v1 (obsolete) — Splinter Review
We piggyback on the plugin crash testing code to make sure that
MOZ_RUNTIME_ASSERT really does crash.
Attachment #831916 - Flags: review?(jwalden+bmo)
This is just a macro reindentation.
Attachment #831917 - Flags: review?(jwalden+bmo)
Summary: Add MOZ_RELEASEMODE_ASSERT to mfbt → Add MOZ_RUNTIME_ASSERT to mfbt
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.
Attachment #831916 - Attachment is obsolete: true
Attachment #831916 - Flags: review?(jwalden+bmo)
Attachment #832299 - Flags: review?(jwalden+bmo)
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?
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.
(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.
I agree test coverage is important. People will just have to be confused, until TBPL figures out how to filter it out.
(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?
Flags: needinfo?(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? :-)
Flags: needinfo?(emorley)
s/as a follow-up/in a follow-up/
Blocks: 941402
(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 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.
Attachment #832299 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #831917 - Flags: review?(jwalden+bmo) → review+
Summary: Add MOZ_RUNTIME_ASSERT to mfbt → Add MOZ_RELEASE_ASSERT to mfbt
Assignee: nobody → bobbyholley+bmo
https://hg.mozilla.org/mozilla-central/rev/7d723a160ca2
https://hg.mozilla.org/mozilla-central/rev/5c27d8c686c0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.