Robert Kaiser Assignee
	Description • 16 years ago

Filing this as a new bug to use what bug 448416 implemented on the SeaMonkey tester buildbots, as this doesn't actually fix bug 448125 - it only hides the leaks but doesn't actually fix them.

	Robert Kaiser Assignee
	Comment 1 • 16 years ago

This might seem over-engineered but it makes the task easy to later reduce the thresholds until we come down to zero.
It also codifies the currently seems numbers of leaks as thresholds for the moment. We surely want to bring those down, that's what bug 448125 is still here for.

	Justin Wood (:Callek)
	Comment 2 • 16 years ago

Comment on attachment 334205 [details] [diff] [review]
make thresholds configurable in config.py
[Checkin: Comment 4]

My only nit is I think it would be easier to visualize and edit if you group the platform leak Values together in the config.py rather than by test type.

	Justin Wood (:Callek)
	Comment 3 • 16 years ago

My review did not include actually verifying the leak values being entered. I will double check soon anyway, and I do feel that tweaking these by you can have an rs+ from me for initial green; and rs+ for tuning them down to 0.

	Robert Kaiser Assignee
	Comment 4 • 16 years ago

OK, checked in the current state, and did pull + reconfig on the master. The next cycle should pick this up, let's see how green it turns.

	Robert Kaiser Assignee
	Comment 5 • 16 years ago

I'll at least reopen this as the values don't get through to the actual mochitest runs. It may be that it's actually bug 448416 that wasn't fixed correctly though.

	Justin Wood (:Callek)
	Comment 6 • 16 years ago

From some investigation Bug 448416 was fixed correctly, but sadly it does not really fix us, as we currently need to set command manually for all these ourselves, (to pass in correct --appname)

The easy fix is to manually append each command in our buildbot config with a --leakThreshold option for now.

The slightly harder and probably more correct fix is to extend the buildbotcustom code to allow specifying an appname as well.  I'll eventually get this done but I suspect at least a week before I can develop the patch, not counting review.

my suggestion is to manually set/hack in the leakThreshold option to these and once I get the appname patch done/approved/etc we can revert that hack and do it the right way.

	(not currently active) Ted Mielczarek
	Comment 7 • 16 years ago

I don't think passing in appName should be necessary at all, as automation.py already knows this information. I think its presence in the unittest configs is an accident.
http://mxr.mozilla.org/mozilla-central/source/build/pgo/automation.py.in#80

	Robert Kaiser Assignee
	Comment 8 • 16 years ago

I actually think we should change the implementation of all this over to the new make targets and pass the leakThreshold as a make variable.

	Justin Wood (:Callek)
	Comment 9 • 16 years ago

(In reply to comment #7)
> I don't think passing in appName should be necessary at all, as automation.py
> already knows this information. I think its presence in the unittest configs is
> an accident.
> http://mxr.mozilla.org/mozilla-central/source/build/pgo/automation.py.in#80
> 

We don't run pgo and I don't see how those are easily available to other portions of the tree, perhaps they should be?

(In reply to comment #8)
> I actually think we should change the implementation of all this over to the
> new make targets and pass the leakThreshold as a make variable.
> 

I could be wrong as I haven't studied those build targets heavily yet, but I don't remember leakThreshold being an option using that method.

Either way, I still feel that my hackier way is best for the moment (we really need to catch for increases in leaks sooner than later)

	(not currently active) Ted Mielczarek
	Comment 10 • 16 years ago

(In reply to comment #9)
> (In reply to comment #7)
> > I don't think passing in appName should be necessary at all, as automation.py
> > already knows this information. I think its presence in the unittest configs is
> > an accident.
> > http://mxr.mozilla.org/mozilla-central/source/build/pgo/automation.py.in#80
> > 
> 
> We don't run pgo and I don't see how those are easily available to other
> portions of the tree, perhaps they should be?

automation.py is used by runtests.py, it's just a library. It happens to live in build/pgo because it's also used by the pgo profiler script.

	Justin Wood (:Callek)
	Comment 11 • 16 years ago

After Bug 451812 is fixed, this should turn our buildbots green.

	Serge Gautherie (:sgautherie)
	Comment 12 • 16 years ago

(In reply to comment #11)
> Created an attachment (id=335147) [details]
> After Bug 451812 is fixed, this should turn our buildbots green.

(Dumb question: exact same patch file as in that other bug !?)

(I'm wondering further as it looks like a Hg patch to a cvs repository:
 http://mxr.mozilla.org/mozilla/source/tools/buildbotcustom/unittest/steps.py)

	Justin Wood (:Callek)
	Comment 13 • 16 years ago

...this should be the right patch, thanks Gerv.

	Justin Wood (:Callek)
	Comment 14 • 16 years ago

> ...this should be the right patch, thanks Gerv.

err, Serg

	Robert Kaiser Assignee
	Comment 15 • 16 years ago

Comment on attachment 335169 [details] [diff] [review]
real patch for followup
[Checkin: See comment 42]

looks good to me - if bug 451812 gets r+

Have you verified that automation.py looks for the right out of "seamonkey-bin", "seamonkey.exe" and "seamonkey" (Mac/Win/unix)? I'm mostly concerned about the Mac case as FF may actually ship and possibly use a "firefox" script there even if it's unneeded.

	Justin Wood (:Callek)
	Comment 16 • 16 years ago

Comment on attachment 335169 [details] [diff] [review]
real patch for followup
[Checkin: See comment 42]

Hmm, I actually have not tested mac case...

stefanh can you please test that
|python runtests.py --autorun --console-level=INFO --close-when-done| works from |build/objdir/mozilla/_tests/testing/mochitest| on Mac for me and if so, grant review here.

	Serge Gautherie (:sgautherie)
	Comment 17 • 16 years ago

For the record, before this bug is fixed, there are already some leaks reduced to warning only:

Linux comm-central dep unit test on 2008/08/23 23:31:34
mochitest
TEST-PASS | runtests-leaks | WARNING leaked 143568 bytes during test execution

Linux / MacOSX / Windows
browser
TEST-PASS | runtests-leaks | WARNING leaked 8... bytes during test execution

NB: Though I don't know why. I guess there already some settings somewhere !?

	Justin Wood (:Callek)
	Comment 18 • 16 years ago

Comment on attachment 335169 [details] [diff] [review]
real patch for followup
[Checkin: See comment 42]

rbgray over IRC verified that what I asked stefanh to do works, great!

	Justin Wood (:Callek)
	Comment 19 • 16 years ago

Comment on attachment 335169 [details] [diff] [review]
real patch for followup
[Checkin: See comment 42]

$ hg push
pushing to ssh://hg.mozilla.org/build/buildbot-configs
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files

$ hg tip -v
changeset:   246:7682755245d4
tag:         tip
user:        Justin Wood <Callek@gmail.com>
date:        Mon Aug 25 18:53:31 2008 -0400
files:       seamonkey-unittest/master.cfg
description:
Bug 450983, followup -- do not pass command to buildsteps for mochikit steps
r=KaiRo

	Justin Wood (:Callek)
	Comment 20 • 16 years ago

(leaving bug open until KaiRo pushes these changes to buildbots themselves)

	Robert Kaiser Assignee
	Comment 21 • 16 years ago

the change did not work, we ended up with strange commandlines like in http://cn-sea-qm-centos5-01.nl.mozilla.org:8010/builders/MacOSX%2010.4%20comm-central%20dep%20unit%20test/builds/590/steps/mochitest/logs/stdio so I went back to the changeset before this was checked in to get the buildbots running again. Could you back out your patch and try to figure out the problem?

	Justin Wood (:Callek)
	Comment 22 • 16 years ago

Comment on attachment 335169 [details] [diff] [review]
real patch for followup
[Checkin: See comment 42]

backed out per buildbots failing oddly, KaiRo has had them running without this changeset for a bit now.

	Robert Kaiser Assignee
	Comment 23 • 16 years ago

I checked in http://hg.mozilla.org/build/buildbot-configs/rev/e2205fdd23c7 for now, setting leakThreshold directly in the commands we're calling. I hope it works, but it's only a temporary fix until we get a real one.

Maybe the buildbotcustom instance should always set --leaThreshold and just default the actual value to 0 unless something else is handed to the buildstep?

	Robert Kaiser Assignee
	Comment 24 • 16 years ago

We gained 240 bytes on mac and 232 bytes on Windows in mochitest leaks since comment #1 (Linux has some much higher value indicating a larger additional problem there).
On mochichrome we gained 140 bytes on Linux, 132 on Windows, and also 140 on Mac.
mochibrowser has a real test failure, so leak reports might not be accurate.

I'm upping the thresholds for the mochitest/mochichrome test, but the difference numbers reported here might give a pointer to what actually got added, so I wanted to note them here.

	Serge Gautherie (:sgautherie)
	Comment 25 • 16 years ago

(In reply to comment #24)

NB: "gain" meaning "more" leaks newly happening (or simply detected).

> a pointer to what actually got added

I think unless we would have a graphic (or even text) report [hint !] like nye has, chasing in (tinderbox waterfall, 3 weeks) history would be too painful.

As these leaks are not even reported on the waterfall itself [hint !],
I think this will help for now on (only).

Yet, that's just what I've been hoping for all along ;-)

	Serge Gautherie (:sgautherie)
	Comment 26 • 16 years ago

Just as an uncompleted example of what happened.

	Serge Gautherie (:sgautherie)
	Comment 27 • 16 years ago

Apart from the 2 usual MacOSX failing tests,

we have 1 new failing test (bug 454513) and 1 new leak (bug 454524),
which keep the boxes Orange even after comment 23 checkin;
I would suggest to not workaround them immediately, until we see if we can get them fixed.

	Robert Kaiser Assignee
	Comment 28 • 16 years ago

I added the current amount of mochibrowser leaks to the config.py and will reconfig soon to make it live, as it's better to actually see boxes going from green to orange when something new is introduced as to stay in indefinite orange.

	Serge Gautherie (:sgautherie)
	Comment 29 • 16 years ago

(In reply to comment #25)
> I think unless we would have a graphic (or even text) report [hint !]

As Callek asked me, I filed Tb bug 456274, Tb bug 456275 and SM bug 456279.

	Serge Gautherie (:sgautherie)
	Comment 30 • 16 years ago

Thanks to fixing bug 433132, we can reduce the threshold on the Linux test box:
{
mochitest

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1222418644.1222423001.26272.gz&fulltext=1
Linux comm-central dep unit test on 2008/09/26 01:44:04
TEST-PASS | runtests-leaks | WARNING leaked 143116 bytes during test execution

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1222426721.1222431311.32673.gz&fulltext=1
Linux comm-central dep unit test on 2008/09/26 03:58:41
TEST-PASS | runtests-leaks | WARNING leaked 21416 bytes during test execution
}

Now, we have (what seems to be) the same leak reports on all 3 platforms :-)

***

The MacOSX and Windows boxes didn't report bug 433122 :-/
(Which I could reproduce on my local Windows.)
Do they have a (dumb) printer set up ? ...

	Robert Kaiser Assignee
	Comment 31 • 16 years ago

They probably have no printer set up - none of our boxes is actually connected to one. The reason why Linux catches the bug might be that Linux always has the "print to PDF" stuff that GNOME gives us. :)

I'll do the threshold change when I'm back on my normal machine, probably Monday.

	Serge Gautherie (:sgautherie)
	Comment 32 • 16 years ago

(In reply to comment #30)
> The MacOSX and Windows boxes didn't report bug 433122 :-/

Bug 433132 !

(In reply to comment #31)
> The reason why Linux catches the bug might be that Linux always has the
> "print to PDF" stuff that GNOME gives us. :)

Actually, that's the reason why I couldn't work on that bug until recently, when I installed the (Windows) doPDF utility ;-)

	Serge Gautherie (:sgautherie)
	Comment 33 • 16 years ago

(In reply to comment #31)
> I'll do the threshold change when I'm back on my normal machine, probably
> Monday.

Done by bug 457440 comment 1.

	Serge Gautherie (:sgautherie)
	Comment 34 • 16 years ago

I noticed that we have a small but consistent improvement on mochi+browser+chrome mochitests:
Linux -80, Mac and Win -76.

	Serge Gautherie (:sgautherie)
	Comment 35 • 16 years ago

(In reply to comment #34)
> I noticed that we have a small but consistent improvement on
> mochi+browser+chrome mochitests:
> Linux -80, Mac and Win -76.

Robert, Justin,
could you update these chrome and browser thresholds ?

mochi has regressed in the meantime: see bug 463355.

	Robert Kaiser Assignee
	Comment 36 • 16 years ago

I adjusted our thresholds for this now, see http://hg.mozilla.org/build/buildbot-configs/rev/58af505b0e9e

	Serge Gautherie (:sgautherie)
	Comment 37 • 16 years ago

(In reply to comment #21)
> http://cn-sea-qm-centos5-01.nl.mozilla.org:8010/builders/MacOSX%2010.4%20comm-central%20dep%20unit%20test/builds/590/steps/mochitest/logs/stdio

This link is HTTP 404 now :-(

> try to figure out the problem?

Guessing: maybe related to bug 456281 comment 10 ??

	Serge Gautherie (:sgautherie)
	Comment 38 • 16 years ago

See bug 391318 comment 45 for a brief explanation of what got fixed.

KaiRo, can you update our thresholds per "Current state" ? Thanks.

	Robert Kaiser Assignee
	Comment 39 • 16 years ago

http://hg.mozilla.org/build/buildbot-configs/rev/88ee32f431b0 adjusts thresholds to new levels after fixes for both bug 465556 and bug 391318. OS X leaks 1112+8 on mochitest and 1112+0 on mochichrome, and even still leaks slightly more than the others on browser chrome tests - the 1112 seems to be some magtc number across the machines ;-)

	Serge Gautherie (:sgautherie)
	Comment 40 • 16 years ago

(In reply to comment #39)

I filed bug 470709 about the remaining BrowserChrome leak.

> OS X leaks [...] more [...]

I filed bug 470710 about the MacOSX case.

	Robert Kaiser Assignee
	Comment 41 • 16 years ago

We're using leak thresholds correctly with current configs.

	Serge Gautherie (:sgautherie)
	Comment 42 • 16 years ago

Comment on attachment 335169 [details] [diff] [review]
real patch for followup
[Checkin: See comment 42]


Ftr,
checkin: comment 19, backout: comment 22;
eventually done by
http://hg.mozilla.org/build/buildbot-configs/rev/86561671f4cc
and
http://hg.mozilla.org/build/buildbotcustom/rev/4a337a624db2

	Serge Gautherie (:sgautherie)
	Comment 43 • 16 years ago

V.Fixed