95711 - free font when no longer in use

Status: RESOLVED WORKSFORME

(Core :: Internationalization, defect)

Description

[kill this account]

Fonts that are loaded are not released even when they are no longer in use.

We need to ref count the fonts and release them when the count goes to zero.

Comment 1

[kill this account]

Attachment: patch; ref cnt the font and release when no longer used

Comment 2

[kill this account]

attachment 49313 [details] [diff] [review] frees the x/gdk font when not in use.

The node/nsFontGTK is otherwise not affected.

Katakai/Roland: could you look at this and see if it makes sense.

thanks

Comment 3

[Roland Mainz]

Two issues:
1. Is it guranteed that the font isn't freed while the GC-cache
(GTK+/Xlib/Xprint use GC caches) still has a GC cache entry which uses this font
?
2. Do you file a port to Xlib gfx or should I do that ?

Comment 4

[kill this account]

Roland: could you give me a pointer to where the GC cache accesses a font?

I'm not setup to test the Xlib code path. Would you mind creating a Xlib
patch and attaching it to this bug?

Comment 5

[Roland Mainz]

> Roland: could you give me a pointer to where the GC cache accesses a font?

See http://lxr.mozilla.org/seamonkey/source/gfx/src/xlib/nsGCCache.cpp and
http://lxr.mozilla.org/seamonkey/source/gfx/src/xlib/nsGCCache.h
The UpdateGC() method
(http://lxr.mozilla.org/seamonkey/source/gfx/src/xlib/nsRenderingContextXlib.cpp#480)
is responsible to get a matching GC entry for that font... 
(GTK+ gfx code does the same except that it plays around with the silly GDK/GTK+
wrappers).

> I'm not setup to test the Xlib code path. Would you mind creating a Xlib
> patch and attaching it to this bug?

No problem...

"Now" or can I wait until the GTK+ version has been r='ed (and you're then
r='ing the Xlib version... :-) ?

Comment 6

[kill this account]

Roland: I would recommend waiting until we think the gtk version is stable
but then proceed before the r= is done.

Comment 7

[kill this account]

We probably need to increment/decrement the counters when a font is 
put-in / removed-from a gc.

Comment 8

[Roland Mainz]

bstell wrote:
> Roland: I would recommend waiting until we think the gtk version is stable
> but then proceed before the r= is done.

OK!

----

bstell wrote:
> We probably need to increment/decrement the counters when a font is 
> put-in / removed-from a gc.

I am not sure... if I remember correctly there is one GC-cache per |Drawable|.
If all objects which keep refs to the GC-cache are destroyed (which means that
the GC-cache will finally destroy itself) before the fonts get disposed then
we're safe... otherwise we need a way to track the use of fonts by  the
GC-cache.

Comment 9

[kill this account]

We will need to add the "font" (nsXFont) to this so we can addref on 
insert and decref on remove.

http://lxr.mozilla.org/seamonkey/source/gfx/src/xlib/nsGCCache.h#92
 92 struct GCCacheEntryXlib
 93 {
 94   PRCList clist;
 95   unsigned long flags;
 96   XGCValues gcv;
 97   Region clipRegion;
 98   xGC *gc;
 99 };

Comment 10

[kill this account]

Attachment: diff on top of bug 90813 attachment 55932

Comment 11

[kill this account]

Attachment: the full diff (includes the changes from the ~4500 lines of bug 90813 attachment 55932)

Comment 12

[kill this account]

Comment on attachment 56317 [details] [diff] [review]
the full diff (includes the changes from the ~4500 lines of bug 90813 attachment 55932 [details] [diff] [review])

patch does not like attachement 56317

Comment 13

[kill this account]

Comment on attachment 56317 [details] [diff] [review]
the full diff (includes the changes from the ~4500 lines of bug 90813 attachment 55932 [details] [diff] [review])

humm. now attachment 56317 [details] [diff] [review] works

Comment 14

[kill this account]

Attachment: the full diff (includes the changes from the ~4500 lines of bug 90813 attachment 56826)

Comment 15

[kill this account]

-> 0.9.8

Comment 16

[Frank Tang]

give to shanjian. please do nothing untill bstell land his truetype patch
remove target

Comment 17

[kill this account]

I don't mind working on this with you Shanjian.

Comment 18

[Shanjian Li]

brian, what's your plan for this? We probably do want to check in this for 
machV.  

Comment 19

[kill this account]

Basically the code is in attachment 56315 [details].
It adds ref counting to the fonts.

Comment 20

[Shanjian Li]

Attachment: update patch base on current trunk tree

Comment 21

[Shanjian Li]

Attachment: removed debugging code in previous patch

Comment 22

[Shanjian Li]

bstell, would you mind take a look of the patch ?

Comment 23

[Roland Mainz]

Shanjian:
What about using |nsCOMPtr| for ref-tracking instead of using "home-grown" code
?

Comment 24

[kill this account]

Shanjian: thanks for working on this.

It has been a while since I last looked at this so I do not remember the
details.

Would it be possible for you to address these commented out lines?

+//    gdk_font_unref(mFont);

+        //mXFont->UnloadFont();

+      //mXFont->UnloadFont();

Roland: do you intend to add gfx/xlib patches?

Comment 25

[Shanjian Li]

Attachment: removed commented out code

Comment 26

[Roland Mainz]

Brian Stell wrote:
> Roland: do you intend to add gfx/xlib patches?

Sure, if you say the GTK+ patch is OK for you... :)

Comment 27

[Shanjian Li]

Roland, I didn't use nsISupport is because reference count is the only thing we
need here, and we need to special handling dereference to release font any way. 

Comment 28

[kill this account]

I talked to dbaron (and waterson). Both recommended not using nsCOMPtr since in 
debug mode it checks if the code did a queryInterface and if the uuid is non-zero.

They both did however suggest using nsRefPtr,
http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsAutoPtr.h#201.
http://lxr.mozilla.org/seamonkey/source/xpcom/tests/TestAutoPtr.cpp

David noted that he had not tried it on all systems and thus it may cause some 
issue on the ports page but he said he would resolve these. Waterson felt that 
moz developers should start using this.

Comment 29

[Shanjian Li]

I have no idea about how to use nsRefPtr. It looks like to me that nsISupport
need to be implemented before it will work. I don't see what is the point to use
that. 

Comment 30

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

No, it only requires that the refcounting methods be called AddRef and Release.
 That said, I should really add the test app for nsRefPtr to the build before
you try to use it in code.  See bug 104346.

Comment 31

[kill this account]

> it only requires that the refcounting methods be called AddRef and Release.

Specifically this means adding nsRefPtr to nsFontGTK

 -class nsFontGTK
 +class nsFontGTK : nsRefPtr

and in the patch IncXFontRefCnt() changes to AddRef:

 -inline  void         IncXFontRefCnt() { mXFontRefCnt++; };
 +inline  void         AddRef() { mXFontRefCnt++; };

and DecXFontRefCnt() changes to Release()

 -nsXFontAAScaledBitmap::DecXFontRefCnt()
 +nsXFontAAScaledBitmap::Release()

(and all the other uses of these in the patch)

Comment 32

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

> -class nsFontGTK
> +class nsFontGTK : nsRefPtr

There's no need to do that.  It will just increase the size of nsFontGTK by a
word and probably cause weird compilation errors.

Comment 33

[Frank Tang]

shanjian is no longer working on mozilla for 2 years and these bugs are still
here. Mark them won't fix. If you want to reopen it, find a good owner first. 

Comment 34

[Travis Chase]

Mass Reassign Please excuse the spam

Comment 35

[Travis Chase]

Mass Re-opening Bugs Frank Tang Closed on Wensday March 02 for no reason, all
the spam is his fault feel free to tar and feather him

Comment 36

[Travis Chase]

Reassigning Franks old bugs to Jungshik Shin for triage - Sorry for spam

Comment 37

[Mats Palmgren (inactive)]

The code changed by the patch no longer exists.  I think the font code under gfx/thebes/
now implements what this bug requested.