431900 - fix "debug builds and leak testing with unit tests" to be production-ready

Status: RESOLVED WONTFIX

(Release Engineering :: General, defect, P3)

Description

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Spinning out from bug#422296. 

On cvs-trunk, we currently have two different debug build setups.
1) debug builds and leak testing using tinderbox. This is production-ready. 
2) debug builds and leak testing while running unit tests using buildbot. This
is *not* production-ready yet.

If (2) was production-ready, we could use (2) on mozilla-central also. We would also be able to mothball off (1) on cvs-trunk.

What needs to be fixed to make (2) production-ready?

Comment 1

[Benjamin Smedberg]

My understanding from dbaron is that this box is doing several different things:

1) debug build
2) unit tests
3) "normal" leak test
4) leak test while doing some kind of unit test (browser chrome tests and reftests, perhaps?)

The problem is that debug builds are currently assert-ridden, which causes the unit tests to permanently orange. Fixing this requires code-level changes.

Dbaron, is my understanding correct?

In any case, we should move these debug+leak+unit tests boxes over to mozilla-central: there's little point having them on CVS at this point.

Comment 2

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

(In reply to comment #1)
> My understanding from dbaron is that this box is doing several different
> things:
> 
> 1) debug build
> 2) unit tests
> 3) "normal" leak test
> 4) leak test while doing some kind of unit test (browser chrome tests and
> reftests, perhaps?)
> 
> The problem is that debug builds are currently assert-ridden, which causes the
> unit tests to permanently orange. Fixing this requires code-level changes.
> 
> Dbaron, is my understanding correct?

I think it's a little bit off, but I'm actually not entirely sure.

I think the idea here is that we eventually want to run our unit tests in DEBUG builds with:
 * orange-on-leak
 * orange-on-assert or crash-on-assert
However, we're currently at the point where trunk fails, but I think these boxes are currently at least doing orange-on-assert.

I think what we need to do is:

 * relax crash-on-assert or orange-on-assert for now, and if possible show assertion counts in the tinderbox UI (along with the leak numbers)

 * get these boxes running with leak numbers showing (not orange-on-leak) [this may be what we do already]

so that we can get the boxes pushed somewhere with more visibility and push the leak numbers and assertion counts down.

cc:ing Waldo as well.

> In any case, we should move these debug+leak+unit tests boxes over to
> mozilla-central: there's little point having them on CVS at this point.

Mostly agreed; I think it's more important to have them on mozilla-central than on 1.9.  However, if they showed assertion counts and leak numbers (rather than orange-on-leak or orange-on-assert), they might be useful on both to avoid picking up regressions.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Add a --fatal-assertions argument to Mochitest

SeaMonkey has a box on MozillaTest that shows assertion and leak counts (sorta, it's in a tooltip, but close enough); I think it's a tinderbox-local changes, so nothing in CVS to point at.  Worth seeing what's being done there and how easily it could be adapted...


(In reply to comment #2)
> I think the idea here is that we eventually want to run our unit tests in
> DEBUG builds with:
>  * orange-on-leak
>  * orange-on-assert or crash-on-assert

Yeah, this is what we want.


> I think what we need to do is:
> 
>  * relax crash-on-assert or orange-on-assert for now, and if possible show
> assertion counts in the tinderbox UI (along with the leak numbers)

Assertion counts would be nice, but it means if you check in a test that happens to trigger one incidentally (I'm thinking of various layout assertions) it appears like a ding against the person who committed it, which I don't think is productive.

Another option that I think might be better would be to tweak the test harnesses to add an option to make assertions fatal (this attachment does it for Mochitest, would need patches elsewhere for reftest -- xpcshell already is fatal if you run in a debug build, but might not pass that way), and then run each twice -- one with fatal asserts, one without.  Tests go by default in the fatal-asserts run unless they incidentally trigger assertions, and then every so often we run through the asserting tests and pull out the ones that don't assert.  Eventually, over enough time and with some effort, we win -- and we prevent backsliding in known-good areas, which is the primary goal here.


>  * get these boxes running with leak numbers showing (not orange-on-leak)
>    [this may be what we do already]

It depends.  1.9 on OS X doesn't leak at all when I run Mochitests; a recent-ish run on Vista didn't leak either (except for the nsTArray_base temporary bustage from awhile back).  I haven't set up a Linux VM to see what the story is there, but I'd bet we're in good shape there as well.  The browser and chrome tests are a slightly different story, but I think we have a single leak-the-world going in one of the two and nothing in the other.  If we want orange-on-leak, it wouldn't take much effort to see it happen -- and in the meantime, assuming the leak count is deterministic (which admittedly is a big assumption -- the pgo box was leaking non-deterministically when it was running and reporting leak counts), you can use --leak-threshold to prevent it from growing, without requiring an absolute hardline position.

This doesn't address non-refcounted leaks; I don't have a good story for tracking those.  Given they're somewhat out of our control in platform libraries, and since much of that is owned by refcounted things, I say we tackle the easy problem first and only then address the hard one once the dust settles.


> Mostly agreed; I think it's more important to have them on mozilla-central
> than on 1.9.  However, if they showed assertion counts and leak numbers
> (rather than orange-on-leak or orange-on-assert), they might be useful on
> both to avoid picking up regressions.

Yeah, pretty much.  As I said, I don't think it would be hard for us to get leaks to a position where we orange on leak even on 1.9, and the "split" strategy would work fine on 1.9 if we're willing to accept a touch of test-only Makefile churn.

Comment 4

[Jeff Walden [:Waldo]]

Oops, forgot to CC ajschult who manages that SeaMonkey box...

Comment 5

[Andrew Schultz]

> SeaMonkey has a box on MozillaTest that shows assertion and leak counts (sorta,
> it's in a tooltip, but close enough); I think it's a tinderbox-local changes,
> so nothing in CVS to point at.  Worth seeing what's being done there and how
> easily it could be adapted...

Beyond the tooltip, I also have it process the assertions and print the # of each assertion and changes in assertions fired by each test.  It's running as part of build-seamonkey-util.pl.  I've posted the mochitest routines at

http://rheneas.eng.buffalo.edu/~andrew/tinderbox-mochi.pl

assertionstats.sh is just

grep ASSERTION $1 | sort | uniq -c | sort -n -r

assertdifflog.pl is http://rheneas.eng.buffalo.edu/~andrew/assertdifflog.pl

A build log with assertiondifflog output is
http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTest&errorparser=unix&logfile=1209637680.1209640840.5694.gz&buildtime=1209637680&buildname=Linux%20nye%20Depend%20mochi&fulltext=1

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 319182 [details] [diff] [review]
Add a --fatal-assertions argument to Mochitest

I vaguely considered adding --nonfatal-assertions as well for futureproofness, but we should probably skin that cat when we get to it.

Comment 7

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Some related work being done in bug#422296.

Comment 8

[Chris Cooper [:coop] (he/him)]

I added some assertion counting to the tinderbox/buildbot output:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=LeakTest
http://qm-unittest02.mozilla.org:2006/ (buildbot output: easier to read, functionally same as tinderbox, but requires VPN access)

The output is a simple count right now, but I think I'll change it to report total assertions *and* unique assertions in the TinderboxPrint output, because we're hitting a lot of dupes right now.

Any further thought on whether these Fx-specific machines should be building 3.0 vs. 3.next vs. mozilla-central?

Comment 9

[Chris Cooper [:coop] (he/him)]

WONTFIXing. If you want this type of coverage for a more recent branch, please file a new bug.