305987 - NS_WARN_IF_FALSE asserts

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Blake Kaplan (:mrbkap) (inactive)]

See http://lxr.mozilla.org/mozilla/source/xpcom/glue/nsDebug.h#121 . I think
NS_WARN_IF_FALSE should call nsDebug::Warning (or another function that prints
out the expression as well as the message, which Warning does not do). I noticed
this the other day when I broke into my debugger because of a NS_WARN_IF_FALSE.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think NS_ABORT_IF_FALSE and NS_WARN_IF_FALSE were an attempt to end the war
over whether assertions should be fatal by means of a compromise acceptable to
neither side, but I could be wrong.  They probably both ought to be as much like
assertions as possible.

Comment 2

[Alex Vincent [:WeirdAl]]

NS_WARN_IF_FALSE is used a lot, but not too many times.  LXR says 64 files, for
a total of 116 uses, have this macro.

NS_ABORT_IF_FALSE, by comparison, shows up in 29 files, for a total of 155 uses.

So someone could take a day to go through all the NS_WARN_IF_FALSE uses and
check them.  It's grunt work, but it's doable.

In any case, there is a bug with NS_WARN_IF_FALSE or with its javadoc commenting:
* Program execution continues past the usage of this macro.

Comment 3

[Darin Fisher]

It seems to me that [NS_WARN]_IF_FALSE should correspond to [NS_WARN]ING.  Anything else is just confusing.

-> default owner

Comment 4

[Darin Fisher]

Attachment: v1 patch

OK, fix NS_WARN_IF_FALSE to do what is advertized and the only that makes any sense.  I going to assume naively that most of the consumers of NS_WARN_IF_FALSE expect the documented behavior rather than the behavior of NS_ASSERTION.

Comment 5

[Darin Fisher]

fixed-on-trunk

Comment 6

[Darin Fisher]

Comment on attachment 213401 [details] [diff] [review]
v1 patch

dbaron: if you object to this patch, i'd be happy to discuss that objection when you get back.  for now, i'm going to proceed with this patch since it helps us get unblocked.

Comment 7

[Brendan Eich [:brendan]]

Darin: who is blocked by lack of this patch?  For what warn-if-false uses?

/be

Comment 8

[Darin Fisher]

> Darin: who is blocked by lack of this patch?  For what warn-if-false uses?

balsa asserts on shutdown about the main thread leaking because some timer objects leaked.  here's my mail to the bonecho mailing list:

> The assertion occurs when we leak a timer object.  We don't leak any places
> objects.  The assertion only happens sometimes.  Turning on full tracerefcnt
> logging causes the assertion not to appear on balsa.  The assertion also does
> not appear on developer systems.  Therefore we feel that the assertion is
> triggered by some race condition.  The assertion is also completely benign.
> Me and bryner concluded that the assertion is bogus and that we probably
> have had the shutdown leak anyways independent of places.  So however you
> look at it, there just isn't any reason to hold up places for this, and as you > said NS_WARN_IF_FALSE clearly should emit a warning as advertized.

Also, note that this thread code is being removed on the trunk (for bug 326273).

Comment 9

[Darin Fisher]

fixed1.8.1

Comment 10

[Darin Fisher]

Attachment: 1.8 branch version

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

NS_WARN_IF_FALSE and NS_ABORT_IF_FALSE were an attempt to solve the question of whether assertions should be fatal by deprecating all other assertion macros and making the author of each assertion choose whether assertions should be fatal.  Since, at the time, NS_ASSERTION was non-fatal, I don't think NS_WARN_IF_FALSE should be any different from NS_ASSERTION.  But we should really get rid of both and replace them with NS_ASSERTION.

Is the fact that places causes this leak at all related to the other 47K of nsTraceRefcnt-logged objects that leak?  It seems reasonably likely to be.

Comment 12

[Darin Fisher]

> But we should
> really get rid of both and replace them with NS_ASSERTION.

See bug 328925.


> Is the fact that places causes this leak at all related to the other 47K of
> nsTraceRefcnt-logged objects that leak?  It seems reasonably likely to be.

I'm actually not convinced that places causes the leak.  Based on the investigation that bryner and I did, it seems likely that the leak always existed and that places merely perturbed the system enough to make it show up on balsa sometimes when previously it did not.  Note: the leak does not show up on other systems, and it goes away on balsa when full refcnt logging is enabled.  We know that someone is holding on references to some timer objects, and that's causing the problem, but it definitely isn't places that is holding onto those timer objects.  Maybe places is indirectly responsible for setting up that reference cycle, but we don't have much information to help us uncover how that might be happening :-/