763000 - Remove MOZ_Assert

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

Now that MOZ_ASSERT expands to inline code to crash, vastly better than crashing in some method other than the one performing the assertion, MOZ_Assert's just a bad idea, and confusing to boot.

There was some trickiness to this.  Some calls I just replaced with crashes.  Breakpad will easily point out the relevant location and significance for those.  Others I preserved the output message with MOZ_ReportAssertionFailure, an ugly thing if ever there was one.  Still, WebKit's WTF has a similarly-named, similarly-functioning function, so that's one independent concurrence in the idea.  (There's been some thought to making MOZ_CRASH take a string as argument, then it'd dump it before crashing.  Maybe we should do that, not sure.)  The MOZ_ASSERT_NR bit I replaced with a different function that also has noreturn semantics -- I don't see why we can't use abort() for that purpose just as readily as we were using MOZ_Assert.

As for MOZ_CRASH...at some point I realized every expansion of it was near-identical.  From what I understand, raise(SIGABRT) *is* what abort() does.  So the case using raise() can change to be like the non-Windows cases.  And the Windows case differs *only* in using exit(3) rather than abort().  Bug 345118 has a little history which actually seems wrong, or at least my experience now doesn't accord with that bug's experience, but after initially folding it in I'm setting it aside for the moment.  Followup territory.

As far as review splitup: ted for the Assertions.h changes and the other Makefile.in junk bits, terrence for the JS changes (note that dmandelin said he accidentally readded the CrashInJS bits when landing a totally unrelated patch several months ago), Ms2ger for whatever you feel like, mfbt bits especially.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 631475 [details] [diff] [review]
Patch

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

The non-js/, non-Makefile stuff lgtm.

::: js/public/Utility.h
@@ +66,5 @@
>  #define JS_STATIC_ASSERT(cond)           MOZ_STATIC_ASSERT(cond, "JS_STATIC_ASSERT")
>  #define JS_STATIC_ASSERT_IF(cond, expr)  MOZ_STATIC_ASSERT_IF(cond, expr, "JS_STATIC_ASSERT_IF")
>  
> +extern MOZ_NORETURN JS_PUBLIC_API(void)
> +JS_Assert(const char *s, const char *file, int ln);

And here it is again :)

::: mfbt/Assertions.h
@@ +106,5 @@
>  #endif
>  
> +/*
> + * MOZ_CRASH crashes the program, plain and simple, in a Breakpad-compatible
> + * way, in both debug and release builds.

Followup for the message argument?

@@ +265,5 @@
>  #  define MOZ_ASSERT_IF(cond, expr)  do { } while (0)
>  #endif
>  
> +/*
> + * MOZ_NOT_REACHED_MARKER() expands (in compilers which support it) to an

It expands to something everywhere now, right?

@@ +318,5 @@
>   */
> +#if defined(DEBUG)
> +#  define MOZ_NOT_REACHED(reason) \
> +     do { \
> +       MOZ_ReportAssertionFailure(reason, __FILE__, __LINE__); \

Not MOZ_ASSERT?

@@ +319,5 @@
> +#if defined(DEBUG)
> +#  define MOZ_NOT_REACHED(reason) \
> +     do { \
> +       MOZ_ReportAssertionFailure(reason, __FILE__, __LINE__); \
> +       MOZ_NOT_REACHED_MARKER();        \

Fix up these backslashes, please. Either one space before it, or put them all on column 80.

Comment 2

[Jeff Walden [:Waldo]]

(In reply to :Ms2ger from comment #1)
> > +extern MOZ_NORETURN JS_PUBLIC_API(void)
> > +JS_Assert(const char *s, const char *file, int ln);
> 
> And here it is again :)

Yeah.  :-\  It's JSAPI, easier to leave it than to change it right now, particularly without finding out if any JSAPI users are using it.

> Followup for the message argument?

Sure.

> It expands to something everywhere now, right?

Hmm, right.

> > +#if defined(DEBUG)
> > +#  define MOZ_NOT_REACHED(reason) \
> > +     do { \
> > +       MOZ_ReportAssertionFailure(reason, __FILE__, __LINE__); \
> 
> Not MOZ_ASSERT?

Maybe.  I had some reason to not do this here, but I no longer remember it.

> Fix up these backslashes, please. Either one space before it, or put them
> all on column 80.

Did the former.  The latter in my experience seems far more an attractive nuisance than anything.

Comment 3

[Mike Hommey [:glandium]]

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #2)
> (In reply to :Ms2ger from comment #1)
> > > +extern MOZ_NORETURN JS_PUBLIC_API(void)
> > > +JS_Assert(const char *s, const char *file, int ln);
> > 
> > And here it is again :)
> 
> Yeah.  :-\  It's JSAPI, easier to leave it than to change it right now,
> particularly without finding out if any JSAPI users are using it.

After all the API and ABI incompatible changes that happened in JSAPI, do you really have to worry about that?

Comment 4

[Jeff Walden [:Waldo]]

We generally have tried not to make changes for changes' sake.  This isn't quite that, but it's closer to that than to cleanup motivated by some pressing engine goal.  And people do push back when we make API changes just as cleanup measures -- see .js-engine discussion of removing JS_ConstructObject*, for example:

https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tech.js-engine/5xqxa2IAOoQ

It's also true that JS_Assert is easily reimplemented.  But so was JS_ConstructObject, and see the discussion that triggered.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 631475 [details] [diff] [review]
Patch

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

The JS bits look fine to me.

::: js/src/jsinfer.cpp
@@ -324,5 @@
>  
> -    /* Always active, even in release builds */
> -    MOZ_Assert(msgbuf, __FILE__, __LINE__);
> -
> -    *((volatile int *)NULL) = 0;  /* Should never be reached */

I didn't realize we had so many ever-so-slightly-different customized methods of crashing.

::: js/src/jsutil.cpp
@@ -62,5 @@
> -#else
> -    raise(SIGABRT);  /* To continue from here in GDB: "signal 0". */
> -#endif
> -}
> -

I loved this function, sad to see it go. :-)

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 631475 [details] [diff] [review]
Patch

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

Overall it seems a bit unfortunate to replace lines that say "assert" with lines that say "crash", since they read differently.

Otherwise your changes seem fine.

Comment 7

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01844720b147

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/01844720b147