278165 - Pages with many animated GIFs leak RAM very quickly

Status: RESOLVED FIXED

(Core Graveyard :: GFX: OS/2, defect)

Description

[Peter Weilbacher]

The ZIP file at the URL above contains a HTML page from a collection contained
in some forum software. After this page has fully loaded and it can be seen that
every minute RAM usage of Mozilla increases by 2MB or more. Theseus' leak
detection tool attributes about 99% of the new allocations to FT2LIB. I verified
the Innotek Font Engine as the "cause" by deactivating it in the registry for
mozilla.exe. No more leaks or other memory allocations were observed after every
GIF had rotated once, although I don't immediate understand why the _Font_
Engine should have anything to do with the ani-GIFs...

So it could be either a flaw in the Gfx component where the FT2* function are
used or in the Font Engine itself.

(This was reported to me by Michael Massenberg and I can confirm so I start it
as NEW.)

Comment 1

[Peter Weilbacher]

I went through gfx/src/os2/nsFontMetricsOS2.cpp which seems to be the only file
that really uses the FT2 APIs and looked for RAM allocation with missing
deletes. Although I didn't find anything specific to FT2 I found three cases
(below) where I am not sure if the memory is released correctly or at all.
Nothing of this could cause this bug, though...

1. in nsFontMetricsOS2::InitializeGlobalFonts()
line 772
   HPS ps = ::WinGetScreenPS(HWND_DESKTOP);
does not seem to get released in cases of futher failures, e.g. for "if
(!globalEntry)"?
It could get released immediately after
   lRemFonts = GFX (::GpiQueryFonts(ps, QF_PUBLIC, NULL, &lNumFonts,
                                    sizeof (FONTMETRICS), pFontMetrics),
                    GPI_ALTERROR);
instead of the end of the method, as "ps" is not referenced further down.

2. in nsFontMetricsOS2::InitializeGlobalFonts()
line 852
   nsMiniMetrics* metrics = new nsMiniMetrics;
This metrics variable does not seem to get deleted anywhere?

3. in nsFontMetricsOS2::FindWesternFont()
line 1691
   nsFontOS2* font = new nsFontOS2();
The contents of the font variable seem to get copied to mWesternFont but it is
not deleted...

Comment 2

[Mike Kaply [:mkaply]]

Javier, you glance at these potential leaks pretty please?

Comment 3

[jhp (no longer active)]

(In reply to comment #1)
> 1. in nsFontMetricsOS2::InitializeGlobalFonts()
> line 772
>    HPS ps = ::WinGetScreenPS(HWND_DESKTOP);

Yes, you can move the ::WinReleasePS(ps) call up to line 780, but I don't think
this is a leak.  The only time ::WinReleasePS(ps) won't get called is if the
function returns due to an OUT_OF_MEMORY error.

> 2. in nsFontMetricsOS2::InitializeGlobalFonts()
> line 852
>    nsMiniMetrics* metrics = new nsMiniMetrics;
> This metrics variable does not seem to get deleted anywhere?

True, this should be deleted during the call to FreeGlobals(), which is called
during shutdown.  Again, this probably isn't the leak, since these stuctures
need to be around for the entire lifetime of the browser.

> 3. in nsFontMetricsOS2::FindWesternFont()
> line 1691
>    nsFontOS2* font = new nsFontOS2();

No, this font is added to mLoadedFonts, and that is cleared in the destructor.

But if you only see the memory leaks with FT2 enabled, then you need to look in
the nsFontMetricsOS2FT code; that's the code that is only loaded when FT2 is
enabled.  Of course, the leak could be in FT2 itself.  Something else to think
about!

Comment 4

[knut st. osmundsen]

I asked Alex to look at this. With the latest (unreleased) FT2LIB he didn't see
any big leaks. He also tells me that some leaks have been fixed since the last
release. We'll probably put out a FT2LIB beta before long, test again with that
and see if it's fixed or not.

Kind Regards,
 knut

Comment 5

[Peter Weilbacher]

Javier, thanks for the comments. Yes, I went through nsFontMetricsOS2FT as well
but didn't spot any obvious leaks there. All that deals just with font handling
anyway which is unlikely to cause a leak triggered by lots of ani-GIFs.

I will make a small patch for the two cases that you agreed with (just in case),
and then we can wait for the new FT2LIB release.

Comment 6

[Peter Weilbacher]

OK, the new FT2LIB is out (2.60 beta 1) and I have tried it. I don't see the
huge memory leak any more, I see RAM usage increasing only a few tens of bytes
every few minutes, and that seems to be cross-platform (see bug 110048 and
perhaps bug 123050), so we shouldn't need to bother here.

(In reply to comment #3)
> > 2. in nsFontMetricsOS2::InitializeGlobalFonts()
> > line 852
> >    nsMiniMetrics* metrics = new nsMiniMetrics;
> > This metrics variable does not seem to get deleted anywhere?
> 
> True, this should be deleted during the call to FreeGlobals(), which is called
> during shutdown.

Hmm, for that I have to admit that I don't know how to delete it in
FreeGlobals(). Maybe I missed the relation of mMetrics and globalEntry which in
turn is part of gGlobalFonts, right? If that is true, then I suspect it does get
deleted by nsFontMetricsOS2::gGlobalFonts->Clear(); although I didn't find out
what that method exactly does.

Comment 7

[jhp (no longer active)]

Looks like I was wrong about the nsMiniMetrics structure.  It gets properly
deleted in the destructor for GlobalFontEntry.

Comment 8

[Peter Weilbacher]

Attachment: move WinReleasePS

OK, thanks for the confirmation, Javier. Then it is just this micropatch to
move WinReleasePS. It's probably a waste of time because it cannot really cause
any problems, but here it is...

Comment 9

[Peter Weilbacher]

Can one of you please check this small patch in?

Comment 10

[Mike Kaply [:mkaply]]

Sorry about that.

Fix checked in.