177258 - [ps] Memory leak of nsFontMetricsPS and nsFontPS object in ps module

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Pete Zha]

nsFontPS should not hold nsCOMPtr for nsIFontMetrics. Otherwise nsFontMetricsPS
will never be destoried. And I should delete mFontPS of nsFontMetricsPS when it
deleted. It's related with check in for bug 144665
I will upload a patch soon.

Comment 1

[Pete Zha]

Attachment: patch

patch to delete mFontPS and not hold nsCOMPtr for nsFontMetricsPS in nsFontPS.
request r=/sr=...

Comment 2

[kill this account]

Is nsFontMetricsPS always guarenteed to be destroyed before the object that
mFontMetrics points to is destroyed?

Comment 3

[Felix Miata]

I don't know how to read all that cpp-speak. What is nsFontMetricsPS and
nsFontPS? I opened 2002102808 trunk for OS/2 late yesterday. By shutting it down
a few minutes ago, I recovered 160 MB of RAM. I had had many tabs open, and some
were displays full of various fonts. Many others were png screenshots. During
that time, I made many trips to prefs to change font sizes and faces.

Comment 4

[Pete Zha]

>I don't know how to read all that cpp-speak. What is nsFontMetricsPS and
>nsFontPS?
nsFontMetricsPS is used to handle font info during printing.
nsFontPS is used to render font during printing.
So, they only leak when you are printing.

>Is nsFontMetricsPS always guarenteed to be destroyed before the object that
>mFontMetrics points to is destroyed?
I think yes. Because nsFontMetricsPS is managed by nsFontCache. nsFontCache will
hold nsIFontMetrics till nobody need it. nsFontPS is managered by
nsFontMetricsPS. So, it should be destoried when nsFontMetricsPS is dead.

Comment 5

[Roland Mainz]

I am not sure whether this patch is correct.
1. What happens if the font cache gets flushed ?
2. nsFontCache _may_ cache the objects, but if I recall correctly there are some
stunts which bypass the font cache for some purposes.

Instead of killing the refcounting we should look who else may hold a reference
to the object.

Comment 6

[Pete Zha]

Nobody hold this object except nsFontMetricsPS...
So, if nsFontMetrics is dead, nsFontPS should be delete.

Comment 7

[Roland Mainz]

Pete Zha wrote:
> Nobody hold this object except nsFontMetricsPS...
> So, if nsFontMetrics is dead, nsFontPS should be delete.

That would happen with the current nsCOMPtr<>-version, too - if there would be
really noone else who holds a reference to the object.

Since |nsFontPS| doesn't get destroyed there seems to be someone else still
using it.

Comment 8

[Pete Zha]

>That would happen with the current nsCOMPtr<>-version, too - if there would be
>really noone else who holds a reference to the object.
As I tested, it will not happen. Because nsFontPS hold a COMPtr os
nsFontMetricsPS and nsFontPS is managed by nsFontMetricsPS. So, we can't hold a
nsCOMPtr od nsFontMetricsPS.

>Since |nsFontPS| doesn't get destroyed there seems to be someone else still
>using it.
Really nobody need nsFontPS except nsFontMetricsPS. Please read into
code(nsFontMetricsPS.cpp and nsRenderContextPS.cpp). You will understand that I
implement nsFontPS just for nsFontMetricsPS.

Comment 9

[Pete Zha]

Comment on attachment 104473 [details] [diff] [review]
patch

who can review this code?

Comment 10

[Pete Zha]

Comment on attachment 104473 [details] [diff] [review]
patch

Roland,
Can you review this patch? Since I have explained that why mFontPS should be
deleted when fontMetrics deleted.
Thanks!

Comment 11

[Roland Mainz]

Pete Zha wrote:
> Can you review this patch?

Sure, but I have to wait for my new builds to finish (>= 19h from now... ;-(( )
...

Comment 12

[Pete Zha]

depend on bug 144668 which will fix this bug.

Comment 13

[Pete Zha]

fixed because of bug 144668