Closed Bug 5547 Opened 25 years ago Closed 24 years ago

Unix: MLK: Fonts leakn - noise to the leak reports

Categories

(Core Graveyard :: GFX, defect, P4)

Product:
Core Graveyard
Component:
GFX
Platform:
x86
Linux
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: bruce, Assigned: bstell)

References

Details

(Keywords: helpwanted, memory-leak, Whiteboard: [nsbeta3-][tind-mlk]) 

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.
Severity: critical → major
Status: NEW → ASSIGNED
Target Milestone: M6
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.
Summary: MLK: Fonts leak → Unix: MLK: Fonts leak
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.
Target Milestone: M6 → M10
M9 is feature freeze. Will try to fix this bug during bug fixing phase.
Marking M10.
Component: other → Compositor
QA Contact: leger → bruce
bruce, internal QA doesn't test Solaris, so I am gonna make you the QA Contact
to mark this Verified once fixed. :-)
QA Contact: bruce → leger
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.)
QA Contact: leger → beppe
beppe, please update QA Contact to correct Linux tester for this area.  Thanks!
Target Milestone: M10 → M11
Target Milestone: M11 → M12
Target Milestone: M12 → M15
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?
OS: Solaris → Linux
Hardware: Sun → PC
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...
Summary: Unix: MLK: Fonts leak → Unix: MLK: Fonts leakn - noise to the leak reports
Blocks: 14516
QA Contact: beppe → paulmac
Whiteboard: HELP WANTED
If Erik is deluged, we can take off HELP WANTED, but somehow I doubt there will
be a flood.

/be
Keywords: mlk
Moving all of my M15s to M16. Please add comments if you disagree.
Target Milestone: M15 → M16
Keywords: helpwanted
Whiteboard: HELP WANTED
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.
Target Milestone: M16 → 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
QA Contact: paulmac → jrgm
*** Bug 35778 has been marked as a duplicate of this bug. ***
Keywords: nsbeta3
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!
Whiteboard: reassign?
nsbeta3+ per jaimejr
Whiteboard: reassign? → [nsbeta3+]
P1 per bug meeting (ekrock). This prevent people find more leaks
Priority: P3 → P1
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]
Priority: P1 → P4
nsbeta3- this for now. If we have the fix , then we will reconsider it.
Whiteboard: [nsbeta3+] → [nsbeta3-]
Assignee: erik → bstell
Status: ASSIGNED → NEW
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.
Status: NEW → ASSIGNED
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?
Whiteboard: [nsbeta3-] → [nsbeta3-][tind-mlk]
*** 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.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.