Global s/NS_ABORT_IF_FALSE/MOZ_ASSERT

NEW
Unassigned

Status

()

6 years ago
6 years ago

People

(Reporter: justin.lebar+bug, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

All the cool kids are using MOZ_ASSERT these days.

In all seriousness, it's confusing to have two debug-only fatal assert macros.  bz tells me that they are actually slightly different (NS_ABORT_IF_FALSE triggers the same machinery as NS_ASSERTION, but at a different level), but I seriously doubt that matters to anyone.

Can we get rid of NS_ABORT_IF_FALSE?

I suppose, like all tree-wide changes, the trick is understanding the implications for SM and TB.  We could have a #defne NS_ABORT_IF_FALSE MOZ_ASSERT, if necessary.
It's often useful to actually have a textual description of what the problem is, as NS_ABORT_IF_FALSE does.

Why did we introduce MOZ_ASSERT, anyway?
I also think it's useful to have some similarity between the fatal and non-fatal assertions so that things can be switched between them.  Since MOZ_ASSERT has become fashionable, it's become impossible for DEBUG builds to run for more than a day or two without crashing due to one.  In comparison, I had a build from early July last for 7 weeks without crashing.

(In reply to David Baron [:dbaron] from comment #1)
> Why did we introduce MOZ_ASSERT, anyway?

To be clear:  this is a serious question.  One of the things people complain about in our codebase is that we have too many ways to do the same thing.  Adding new-and-different things to mfbt (where things do have the advantage of being able to be shared between JS and non-JS) rather than moving existing things to it doesn't help this problem.
(In reply to David Baron [:dbaron] from comment #1)
> It's often useful to actually have a textual description of what the problem
> is, as NS_ABORT_IF_FALSE does.

MOZ_ASSERT does accept an optional description string, so this is not a problem.

(In reply to David Baron [:dbaron] from comment #2)
> To be clear:  this is a serious question.  One of the things people complain
> about in our codebase is that we have too many ways to do the same thing. 
> Adding new-and-different things to mfbt (where things do have the advantage
> of being able to be shared between JS and non-JS) rather than moving
> existing things to it doesn't help this problem.

I agree. Adding stuff to mfbt comes with a requirement to remove the stuff it replaces, sooner or later ... hopefully not too much later.

> I also think it's useful to have some similarity between the fatal and
> non-fatal assertions so that things can be switched between them.  Since
> MOZ_ASSERT has become fashionable, it's become impossible for DEBUG builds
> to run for more than a day or two without crashing due to one.  In
> comparison, I had a build from early July last for 7 weeks without crashing.

Yes, I'm not a fan of fatal assertions myself, although that's a somewhat orthogonal issue to this bug.

We had a dev.platform thread in January ("Performing assertions in Mozilla code ...") where I think everyone agreed we could add "MOZ_NONFATAL_ASSERT" next to MOZ_ASSERT. Of course we'd have to plug it into the reftest assertion counting machinery somehow. But that also belongs to another bug.
(Reporter)

Updated

6 years ago
Whiteboard: [mentor=jlebar][lang=c++]
Hi,

I would like to work on this bug, could you please guide me on getting started with it ? 

Thanks
There are basically three things you need to do (perhaps not in the order you should do them).

1) Replace all instances of "NS_ABORT_IF_FALSE" with "MOZ_ASSERT".
2) Remove the definition of NS_ABORT_IF_FALSE.
3) Figure out if the NS_ABORT_IF_FALSE integration with the NS_ASSERTION machinery is important.  (See comment 0.)
4) Coordinate this change with the other people who use mozilla-central (Thunderbird and SeaMonkey).

(1) and (2) should be straightforward.  I think the answer to (3) is "no", but maybe bz can speak up if he thinks otherwise?

That leaves (4), which is the real trick to this bug.

The path of least-resistance here would be to change only mozilla-central, and leave a #define NS_ABORT_IF_FALSE MOZ_ASSERT somewhere in our code, so we don't break anyone else.  Then we can worry about TB and SM separately.

Would the module owners be OK with this approach?
I think ending up with MOZ_ASSERT, NS_ASSERTION, and NS_WARN_IF_FALSE having three *different* naming styles is a net loss; I think if we want to replace these with new things we should actually get all of the new pieces set up first.

I'd also note that item (1) in comment 5 requires care be given to indentation.
> I think ending up with MOZ_ASSERT, NS_ASSERTION, and NS_WARN_IF_FALSE having three *different* 
> naming styles is a net loss; I think if we want to replace these with new things we should actually 
> get all of the new pieces set up first.

Do you mean you'd want to change NS_ASSERTION over to MOZ_NONFATAL_ASSERT at the same time as s/NS_ABORT_IF_FALSE/MOZ_ASSERT/?
(Would still appreciate clarification from dbaron in comment 7.)

Turns out, this is a horrible mentored bug; it's quite a bit more involved than I'd thought.  Sorry, Abhishek.

I should have thought harder about this one before annotating it.

Let me know (via e-mail, or on IRC) if you'd like some help finding a new bug.  Sorry again about this one.
(In reply to Justin Lebar [:jlebar] from comment #7)
> > I think ending up with MOZ_ASSERT, NS_ASSERTION, and NS_WARN_IF_FALSE having three *different* 
> > naming styles is a net loss; I think if we want to replace these with new things we should actually 
> > get all of the new pieces set up first.
> 
> Do you mean you'd want to change NS_ASSERTION over to MOZ_NONFATAL_ASSERT at
> the same time as s/NS_ABORT_IF_FALSE/MOZ_ASSERT/?

Probably, yes.

I have to say, though, that there's more cost to changing things here than you might think.  For example, I and a lot of other people know that I can break on any of the current assertion types by breaking on NS_DebugBreak_P or by setting the XPCOM_DEBUG_BREAK environment variable to a bunch of different values.  If we change all that to something else, I'm going to have to look it up the first five times.  I'm fine with paying that cost if there's a benefit, but I'm having trouble seeing what the benefit is here to rewriting things.
MOZ_ASSERT will break in your debugger, too.  That's all it will do if you're in a debugger, so there's nothing else to look up or remember.  If you're not in a debugger, it will segfault, end of story; again, nothing to remember...

> I'm fine with paying that cost if there's a benefit, but I'm having trouble seeing what the benefit 
> is here to rewriting things.

I think you articulated the reasons pretty well in comment 2, actually!

> One of the things people complain about in our codebase is that we have too many ways to do the same 
> thing.  Adding new-and-different things to mfbt (where things do have the advantage of being able to 
> be shared between JS and non-JS) rather than moving existing things to it doesn't help this problem.
(In reply to Justin Lebar [:jlebar] from comment #10)
> MOZ_ASSERT will break in your debugger, too.  That's all it will do if
> you're in a debugger, so there's nothing else to look up or remember.  If
> you're not in a debugger, it will segfault, end of story; again, nothing to
> remember...

Yes, but will the MOZ_NONFATAL_ASSERT that isn't even written yet?


> > I'm fine with paying that cost if there's a benefit, but I'm having trouble seeing what the benefit 
> > is here to rewriting things.
> 
> I think you articulated the reasons pretty well in comment 2, actually!

No, those are reasons for going all the way if we do rewrite things and not leaving them in a halfway state.  I don't see any reasons there for rewriting anything in the first place.
> No, those are reasons for going all the way if we do rewrite things and not leaving them in a 
> halfway state.  I don't see any reasons there for rewriting anything in the first place.

Okay, but we both agree that the current state of affairs is sub-optimal.  That is, we should either go all the way, or revert MOZ_ASSERT.  The debate is between

 1) Remove MOZ_ASSERT and replace it with NS_ABORT_IF_FALSE and whatever the likely no-longer-existent JS equivalent is (*),
 2) Remove NS_ABORT_IF_FALSE and replace it with MOZ_ASSERT, and
 3) Do (2) and also s/NS_ASSERTION/MOZ_NONFATAL_ASSERT/.

(*) Note that we'd have to modify NS_ABORT_IF_FALSE so it's OK with being called without a message.  For consistency, we'd probably want to do the same to all the other macros in nsDebug.h.

I agree that (3) is better than (2), and it sounds like you agree that (3) is at least as good as (1).  So the only remaining question is whether (2) is better than (1), right?

If your criterion is "it's better to have fewer ways of doing the same thing", then surely (2) is better than (1).  Is there some other criterion you're concerned about?

I don't think it's right to say that we shouldn't do (2) merely because an additional change (3) would be better still -- that just feels like stalling to me.  :)  Surely there are lots of related changes which would be an improvement on (3) -- we could get rid of all the NS_DebugBreak macros in nsDebug.h, but you're not suggesting we'd have to do that in order to s/NS_ABORT_IF_FALSE/MOZ_ASSERT/.  Why not?

> Yes, but will the MOZ_NONFATAL_ASSERT that isn't even written yet?

Ah, okay.  I think we can do two things to mitigate your concern here.

* Have MOZ_NONFATAL_ASSERT understand the old-style env vars, so you don't have to learn anything new.  I don't know if Waldo would be OK with this, but I would be, in principal.  I agree that ergonomics are important.

* Document this in-source, in an easy-to-remember location.  I have no idea where I'd go to look for the XPCOM assertion env vars (honestly, I have them written down in a book somewhere, because I felt bad asking on IRC so often), but I know that this MOZ_NONFATAL_ASSERT would be documented entirely in mfbt/Assertions.h.  If I ever forgot, grepping/MXR'ing for "#define MOZ_NONFATAL_ASSERT" would take me right to the documentation.
(In reply to Justin Lebar [:jlebar] from comment #12)
>  1) Remove MOZ_ASSERT and replace it with NS_ABORT_IF_FALSE and whatever the
> likely no-longer-existent JS equivalent is (*),

We also use MOZ_ASSERT in gfx/2d (which intentionally does not depend on XPCOM).

The goal of MFBT is to have a small library of shared primitives which can be used by Gecko components that do not want to depend on XPCOM. It's also taking the chance to improve those primitives a bit over the XPCOM versions (e.g. using C++ namespaces instead of the "ns" prefix).

Whether this goal is worth the cost of pursuing it (including the cost of replacing usage of XPCOM APIs with their MFBT equivalents) is certainly open for debate, and is probably difficult to determine scientifically. But I guess MFBT authors are betting it is worthwhile. So I think we should go ahead and follow path #3.
Another complaint about MOZ_ASSERT I heard today:  "does that give stack traces yet?"

NS_ABORT_IF_FALSE's code (in NS_DebugBreak) has:

#ifdef DEBUG
     nsTraceRefcntImpl::WalkTheStack(stderr);
#endif
     Abort(buf.buffer);

so that it prints a stack trace before aborting.  So switching would regress that.
(In reply to Justin Lebar [:jlebar] from comment #12)
> Okay, but we both agree that the current state of affairs is sub-optimal. 
> That is, we should either go all the way, or revert MOZ_ASSERT.  The debate
> is between
> 
>  1) Remove MOZ_ASSERT and replace it with NS_ABORT_IF_FALSE and whatever the
> likely no-longer-existent JS equivalent is (*),
>  2) Remove NS_ABORT_IF_FALSE and replace it with MOZ_ASSERT, and
>  3) Do (2) and also s/NS_ASSERTION/MOZ_NONFATAL_ASSERT/.

I'll toss in a fourth option, which is to migrate the nsDebug code to live in mfbt and have better names, but to maintain the various features it has that people depend on (rather than starting over from scratch).
I did the replace step in comment 5.  test_bug455311.js fails in xpcshell-tests and some 30 35 tests fail under Mochitest.

If I remove the definition of NS_ABORT_IF_FALSE, some files under ~/mozilla-central/obj-x86_64-unknown-linux-gnu/ fail to compile.  They were not under hg.

How should I handle these?
> Turns out, this is a horrible mentored bug; it's quite a bit more involved than I'd thought.

I meant to remove the [mentor] flag when I wrote this, but apparently I didn't do that.

> test_bug455311.js fails in xpcshell-tests and some 30 35 tests fail under Mochitest.

Hard to say what's going wrong without the error messages.

> Some files under ~/mozilla-central/obj-x86_64-unknown-linux-gnu/ fail to compile.  They were not 
> under hg.

That whole directory is not under hg -- it looks like that's your object directory.  I'd have to see the error message to say what's going wrong.

That said, like I said in comment 8, this is probably a bad mentored bug, because doing option (3) from comment 12, which is what we decided on, is pretty non-trivial.  If you want to keep trying, I'll guide you as best I can.  Otherwise, maybe I can help you find a different bug to work on?
Whiteboard: [mentor=jlebar][lang=c++]
> > test_bug455311.js fails in xpcshell-tests and some 30 35 tests fail under Mochitest.
> 
> Hard to say what's going wrong without the error messages.

I don't know where to find the test logs or results, sorry, this is my first bug.

> 
> > Some files under ~/mozilla-central/obj-x86_64-unknown-linux-gnu/ fail to compile.  They were not 
> > under hg.
> 
> That whole directory is not under hg -- it looks like that's your object
> directory.  I'd have to see the error message to say what's going wrong.

Pasting the error below:

This error happens when I remove the definition of NS_ABORT_IF_FALSE in nsDebug.h.

I was trying to find out how this InputStreamParams.h was generated.  But couldn't find.

---BEGIN PASTE----

c++ -o nsStringStream.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /home/venk/mozilla-central/config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DEXCLUDE_SKIA_DEPENDENCIES  -DOS_POSIX=1 -DOS_LINUX=1  -D_IMPL_NS_COM -I/home/venk/mozilla-central/ipc/chromium/src -I/home/venk/mozilla-central/ipc/glue -I../../ipc/ipdl/_ipdlheaders  -I.. -I/home/venk/mozilla-central/xpcom/io -I. -I../../dist/include  -I/home/venk/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/include/nspr -I/home/venk/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/include/nss      -fPIC  -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -pipe -DNDEBUG -DTRIMMED -g -Os -freorder-blocks  -fno-strict-aliasing -fomit-frame-pointer   -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/nsStringStream.o.pp /home/venk/mozilla-central/xpcom/io/nsStringStream.cpp
In file included from ../../dist/include/mozilla/TimeStamp.h:13:0,
                 from ../../dist/include/ipc/IPCMessageUtils.h:12,
                 from ../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h:14,
                 from ../../dist/include/mozilla/ipc/InputStreamUtils.h:8,
                 from /home/venk/mozilla-central/xpcom/io/nsMultiplexInputStream.cpp:21:
../../dist/include/nsDebug.h:43:1: warning: multi-line comment [-Wcomment]
In file included from ../../dist/include/mozilla/TimeStamp.h:13:0,
                 from ../../dist/include/ipc/IPCMessageUtils.h:12,
                 from /home/venk/mozilla-central/xpcom/io/nsStringStream.cpp:11:
../../dist/include/nsDebug.h:43:1: warning: multi-line comment [-Wcomment]
In file included from ../../dist/include/mozilla/ipc/InputStreamUtils.h:8:0,
                 from /home/venk/mozilla-central/xpcom/io/nsMultiplexInputStream.cpp:21:
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h: In member function ‘void mozilla::ipc::InputStreamParams::AssertSanity() const’:
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h:515:67: error: ‘NS_ABORT_IF_FALSE’ was not declared in this scope
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h: In member function ‘void mozilla::ipc::InputStreamParams::AssertSanity(mozilla::ipc::InputStreamParams::Type) const’:
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h:522:68: error: ‘NS_ABORT_IF_FALSE’ was not declared in this scope
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h: In member function ‘void mozilla::ipc::OptionalInputStreamParams::AssertSanity() const’:
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h:787:67: error: ‘NS_ABORT_IF_FALSE’ was not declared in this scope
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h: In member function ‘void mozilla::ipc::OptionalInputStreamParams::AssertSanity(mozilla::ipc::OptionalInputStreamParams::Type) const’:
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h:794:68: error: ‘NS_ABORT_IF_FALSE’ was not declared in this scope
make[6]: *** [nsMultiplexInputStream.o] Error 1
make[6]: *** Waiting for unfinished jobs....
In file included from ../../dist/include/mozilla/ipc/InputStreamUtils.h:8:0,
                 from /home/venk/mozilla-central/xpcom/io/nsStringStream.cpp:23:
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h: In member function ‘void mozilla::ipc::InputStreamParams::AssertSanity() const’:
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h:515:67: error: ‘NS_ABORT_IF_FALSE’ was not declared in this scope
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h: In member function ‘void mozilla::ipc::InputStreamParams::AssertSanity(mozilla::ipc::InputStreamParams::Type) const’:
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h:522:68: error: ‘NS_ABORT_IF_FALSE’ was not declared in this scope
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h: In member function ‘void mozilla::ipc::OptionalInputStreamParams::AssertSanity() const’:
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h:787:67: error: ‘NS_ABORT_IF_FALSE’ was not declared in this scope
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h: In member function ‘void mozilla::ipc::OptionalInputStreamParams::AssertSanity(mozilla::ipc::OptionalInputStreamParams::Type) const’:
../../ipc/ipdl/_ipdlheaders/mozilla/ipc/InputStreamParams.h:794:68: error: ‘NS_ABORT_IF_FALSE’ was not declared in this scope
make[6]: *** [nsStringStream.o] Error 1
make[6]: Leaving directory `/home/venk/mozilla-central/obj-x86_64-unknown-linux-gnu/xpcom/io'
make[5]: *** [libs] Error 2
make[5]: Leaving directory `/home/venk/mozilla-central/obj-x86_64-unknown-linux-gnu/xpcom'
make[4]: *** [libs_tier_platform] Error 2
make[4]: Leaving directory `/home/venk/mozilla-central/obj-x86_64-unknown-linux-gnu'
make[3]: *** [tier_platform] Error 2
make[3]: Leaving directory `/home/venk/mozilla-central/obj-x86_64-unknown-linux-gnu'
make[2]: *** [default] Error 2
make[2]: Leaving directory `/home/venk/mozilla-central/obj-x86_64-unknown-linux-gnu'
make[1]: *** [realbuild] Error 2
make[1]: Leaving directory `/home/venk/mozilla-central'
make: *** [build] Error 2
---- END PASTE ----

> 
> That said, like I said in comment 8, this is probably a bad mentored bug,
> because doing option (3) from comment 12, which is what we decided on, is
> pretty non-trivial.  If you want to keep trying, I'll guide you as best I
> can.  Otherwise, maybe I can help you find a different bug to work on?

Well, I will find a simpler bug.
You need to log in before you can comment on or make changes to this bug.