Fonts leak. The nsFontCache on the nsDeviceContextImpl is getting destroyed, so something, somewhere else is holding on to them. Solaris 2.6, gcc 2.7.2.3, pull, build from today, but has been happening for sometime, but wasn't sure until today that it wasn't part of the WebShell leak. Might be XP, no way for me to know. I don't know what component this would be filed under.

Thanks, Bruce. Actually, I know about this leak. We are holding on to the fonts from a global variable. Eventually, I will try to find some way to free them, or to manage their memory in an appropriate way.

Bruce, if this bug report is based on a Purify report, could you please extract the relevant info from Purify, and paste it into this bug report? Thanks!

Look at the current report linked from http://www.puremagic.com/~bruce/pure-moz/ for apprunner. It bleeds fonts.

Yes, I just checked in a fix for the XListFonts leak. I still need to fix the others, so I'm going to leave this bug report open.

M9 is feature freeze. Will try to fix this bug during bug fixing phase. Marking M10.

bruce, internal QA doesn't test Solaris, so I am gonna make you the QA Contact to mark this Verified once fixed. :-)

Um, the font memory leakage that Bruce describes is _very_ unlikely to be specific to Solaris. If you can verify the fix on Linux, feel free to mark it VERIFIED. (QA Contact reverted.)

beppe, please update QA Contact to correct Linux tester for this area. Thanks!

What is holding this up? What makes this hard? What would make it easy? Does this bug depend on other things being done first? Why M15? Is this not going to be fixed for beta? I'm sure I have a lot of other questions but you can anticipate them I'm sure. With some information, maybe others could step in and help?

We have some font info stored in tree-like structures hanging from a global variable. So we probably need to register a shutdown proc that gets called by apprunner or whatever when it shuts down. I'm marking it M15 since I have a number of features to work on first, before I address less important bugs like this. You're not saying that the memory occupied by this font info is terribly large, are you?

I do not recall the size of the data, but it contributed a lot of noise to the leak reports in Purify until I suppressed all of it. Where is this global variable? This sounds like a good 'help wanted' bug perhaps?

The global variable is gFamilies in mozilla/gfx/src/gtk/nsFontMetricsGTK.cpp. We can make it a HELP WANTED, as long as that doesn't trigger a flood of questions to me about how to fix this...

If Erik is deluged, we can take off HELP WANTED, but somehow I doubt there will be a flood. /be

Moving all of my M15s to M16. Please add comments if you disagree.

I don't know how many people are currently running Purify and similar tools, but it doesn't seem like very many, since I've heard no complaints about this bug recently. I have lots of other work on my plate, so if this bug isn't that important, I'm going to have to target M18.

Subject: Re: Module Unloading From: Suresh Duddi <dp@netscape.com> To: mozilla-xpcom@mozilla.org Dll unloading is ifdeffed out at the lowest level, as you have found due to bugs like dlls saying they can be unloaded while there is some code that reference it. Plus our leak detector needed it. We hope to turn it on once modules implement it well. Now to address modules of leaks of global objects in modules, the switch over to using nsIModule was done for this. Each dll has an object that implements the nsIModule interface. The object is created via a call to NSGetModule() after loading the dll and before the dll is ready to be shutdown, the module object is released. How this shows up in code is if you use NS_IMPL_NSMODULE_WITH_DTOR() macro and give it a function that gets called before the dll *will* be unloaded. That function is where you should release global memory. Look at comments at the end of : http://lxr.mozilla.org/seamonkey/source/xpcom/sample/nsSampleModule.cpp

changing qa contact to jrgm@netscape.com on some random bugs

*** Bug 35778 has been marked as a duplicate of this bug. ***

Bruce, do you want to take this? You seem to have looked at it already and Erik won't be back until late August, which is rather late in the nsbeta3 cycle if we want this to get in.

Ok, Bruce can't take this until he gets his Solaris/Purify setup running. John: Could you find someone who would take this from Erik's plate? He's doomed. He has 15 nsbeta3 bugs and will only have 2 weeks to fix them all. Cheers!

nsbeta3+ per jaimejr

P1 per bug meeting (ekrock). This prevent people find more leaks

We cannot fix all leaks. We need to know if this is a big leak. Moving to P4 unless a large leak can be determined. We cannot take time to clean up all noise now. Moving to P4. Adding [PDTP4]

nsbeta3- this for now. If we have the fix , then we will reconsider it.

bstell is working on this bug now

I found a font leak: the strech fonts were not being free'd. Here is a proposed patch: Index: nsFontMetricsGTK.cpp =================================================================== RCS file: /cvsroot/mozilla/gfx/src/gtk/nsFontMetricsGTK.cpp,v retrieving revision 1.120.2.1 diff -u -r1.120.2.1 nsFontMetricsGTK.cpp --- nsFontMetricsGTK.cpp 2000/10/05 23:04:30 1.120.2.1 +++ nsFontMetricsGTK.cpp 2000/10/20 21:26:39 @@ -485,7 +485,16 @@ FreeStretch(nsFontStretch* aStretch) { PR_smprintf_free(aStretch->mScalable); - // XXX nsVoidArray mScaledFonts; + + PRInt32 count; + while ((count = aStretch->mScaledFonts.Count())) { + // go backwards to keep nsVoidArray from memmoving everything each time + count--; // nsVoidArray is zero based + nsFontGTK *font = (nsFontGTK*)aStretch->mScaledFonts.ElementAt(count); + aStretch->mScaledFonts.RemoveElementAt(count); + if (font) delete font; + } + for (int i = 0; i < aStretch->mSizesCount; i++) { delete aStretch->mSizes[i]; }

r=erik for the patch above.

I'm still waiting to be allowed to checkin to mozilla

Actually, you need super-review (sr= or a=) before the patch can be checked in, but even without direct CVS checkin permission, you can get someone to check the patch in for you. Lots of people do this. Blizzard should probably super-review this patch: you should mail him about it and cc: reviewers@mozilla.org.

First, this is a small leak. I put the note in as it had been a while. I've had a fix waiting for a while. My manager, ftang, has designated me as the X font persion so wouldn't it be best if cvsblame showed my name? I have been waiting 3 weeks for cvs checkin privledge at mozilla.org as they have changed the rules. Currently mozilla.org does not actually have a working set of rules to grant cvs write permission. Are we requiring super review for trunk checkin? I read the current rules http://www.mozilla.org/: New Check-in Rules To improve code quality, mozilla.org now requires all changes to be approved by a designated Mozilla code reviewer. This extra level of review applies to everyone, including Netscape engineers. From http://www.mozilla.org/hacking/reviewers.html exceptions blizzard@mozilla.org appears in many platform-specific areas, but does not claim expertise across all platforms. He has decided that all porting changes to widget, gfx, and similar platform-specific code need not get his personal review; module owner review is enough. So do I need more that pavlov's (and erik's) review?

*** Bug 48881 has been marked as a duplicate of this bug. ***

I think the "porting changes" in the quote above refers to changes to get ports not currently at the level of the major platforms (e.g., BeOS, OS/2) to work. You should have a super-review, so you should email blizzard and cc: reviewers if you want. However, the patch above seems a bit strange to me since it attempts to remove the font at index -1 in the array. Perhaps the following would be better: Index: nsFontMetricsGTK.cpp =================================================================== RCS file: /cvsroot/mozilla/gfx/src/gtk/nsFontMetricsGTK.cpp,v retrieving revision 1.126 diff -u -d -r1.126 nsFontMetricsGTK.cpp --- nsFontMetricsGTK.cpp 2000/10/28 22:13:47 1.126 +++ nsFontMetricsGTK.cpp 2000/11/03 01:58:44 @@ -485,7 +485,15 @@ FreeStretch(nsFontStretch* aStretch) { PR_smprintf_free(aStretch->mScalable); - // XXX nsVoidArray mScaledFonts; + + PRInt32 i; + for (i = aStretch->mScaledFonts.Count() - 1; i >= 0; i--) { + nsFontGTK *font = (nsFontGTK*)aStretch->mScaledFonts.ElementAt(i); + delete font; + // There's no need to remove the elements since we're about to + // delete the whole array with |delete aStretch| below. + } + for (int i = 0; i < aStretch->mSizesCount; i++) { delete aStretch->mSizes[i];

Sorry this has taken so long. I had wanted to do the checkin so cvsblame would have my name on the lines. I have been waiting over 4 weeks to get cvs checking priviledge as mozilla.org has just been changing the rules for getting cvs checkin priviledge. I never thought it would take so long (still not there). If I had known it would take weeks I would have had someone else check it in. David, would you do this when it is reviewed? The patch I submitted was styled after other bits of code. /editor/base/TypeInState.cpp, line 84 -- count--; // nsVoidArray is zero based /editor/base/TypeInState.cpp, line 92 -- count--; // nsVoidArray is zero based /editor/base/TypeInState.cpp, line 182 -- count--; // nsVoidArray is zero based /editor/base/TypeInState.cpp, line 200 -- count--; // nsVoidArray is zero based /editor/base/TypeInState.cpp, line 273 -- index--; // nsVoidArray is zero based /editor/base/nsTableEditor.cpp, line 1317 -- count--; // nsVoidArray is zero based /editor/base/nsTableEditor.cpp, line 2165 -- count--; // nsVoidArray is zero based see http://lxr.mozilla.org/seamonkey/search?string=nsVoidArray+is+zero+based David's patch also looks okay.

Looks good to me. sr=blizzard

Chris, just to confirm, which patch are you approving? Brian's or David's? (See comments above.)

The second one. The first one is broken.

OK, so I looked at david baron's comment about it deleting the -1'th element in the array, looked at the code and for some reason thought it was true. I was wrong. Given that I actually prefer bstell's patch since the loop seems clearer.

OK, I'll check in bstell's patch. (However, it does seem strange to call it clearer when we both misread it the first time. It also seems unnecessary to shrink the array one element at a time when it's just going to be deleted at the end -- it's a bit of extra work. But I'll shut up now...)

At this point any remaining discussion is about efficiency and style. The first patch caused confusion which is clearly undesireable. The second patch is clearly more efficient. (I had been away from Netscape for a couple years. Not knowing the current stylistic conventions when I did the 1st patch I found and followed a pre-existing style.) In the future I'll work on better efficiency and style. Something like this: PRInt32 i; PRInt32 count = aStretch->mScaledFonts.Count(); for (i=0; i < count; i++) { nsFontGTK *font = (nsFontGTK*)aStretch->mScaledFonts.ElementAt(i); delete font; // There's no need to remove the elements since we're about to // delete the whole array with |delete aStretch| below. } Thank you all for your help with this. PS: David, would you let me know when you check in? Thanks

bstell's patch checked in, 2000-11-07 18:56 PDT.