166833 - nsFT2FontCatalog leaks memory

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Andrew Schultz]

from Valgrind:
12 bytes in 1 blocks are definitely lost in loss record 23 of 211
 at calloc (vg_clientfuncs.c:239)
 by nsFT2FontCatalog::NewDirCatalog(void) (nsFT2FontCatalog.cpp:1539)
 by nsFT2FontCatalog::doInitGlobals(FT_LibraryRec_ *) (nsFT2FontCatalog.cpp:1347)
(leaks nsDirCatalog)

32 bytes in 1 blocks are definitely lost in loss record 62 of 211
 at __builtin_new (vg_clientfuncs.c:125)
 by nsFT2FontCatalog::InitGlobals(FT_LibraryRec_ *) (nsFT2FontCatalog.cpp:1268)
(leaks gFT2FontCatalog)

2112 bytes in 3 blocks are definitely lost in loss record 175 of 211
 at realloc (vg_clientfuncs.c:270)
 by nsFT2FontCatalog::AddFont(nsFontCatalog *, nsFontCatalogEntry *)
(nsFT2FontCatalog.cpp:314)
(leaks fc-fonts)

it looks like the dirCatalog can be freed immeadiately, while the other stuff is
just not completely cleaning up on shutdown.

Comment 1

[Andrew Schultz]

Attachment: patch

this patch makes Valgrind happy, except that it now notices this:

44 bytes in 1 blocks are definitely lost in loss record 69 of 188
 at __builtin_new (vg_clientfuncs.c:125)
 by nsFT2FontCatalog::doInitGlobals(FT_LibraryRec_ *)
(nsFT2FontCatalog.cpp:1349)

it looks like mRange*CharSetNames are both leaked.  Can these just be deleted
in doFreeGlobals (or do they need to be Reset)?

Comment 2

[kill this account]

Thanks for working on this!

Right now I am strugging to upgrade my Linux system but I will look at this
soon.

Comment 3

[Rui Xu]

Is this bug affect on browser performance?

Comment 4

[Andrew Schultz]

not really.  most of the leak is on shutdown (memory is not freed when you quit)
so this would be completely unnoticable.

the dirCatalog can be freed after startup, but the leak is relatively small
(overhead + a string for each TTF directory).

Comment 5

[kill this account]

Louie: could you port the patch and FreeDirCatalog() to the new code?

Comment 6

[Andrew Schultz]

FWIW, the patch still applies to current trunk

Comment 7

[Andrew Schultz]

Attachment: patch v2

Most of the original patch still applies.

Comment 8

[Andrew Schultz]

it looks like things are still getting shaken up in bug 180473...
anyway for the record, even with the above patch, there are still a lot of leaks:

(1) 168 bytes in 3 blocks are possibly lost
 malloc (vg_clientfuncs.c:100)
 __builtin_new (nsAppRunner.cpp:178)
 operator new() (vg_clientfuncs.c:139)
 nsSupportsArray::Create() (nsSupportsArray.cpp:232)
 NS_NewISupportsArray() (nsSupportsArray.cpp:700)
 nsFT2FontCatalog::GetFontCatalogEntries() (nsFT2FontCatalog.cpp:186)

186  NS_NewISupportsArray(getter_AddRefs(entries));

(2) 1024 bytes in 1 blocks are definitely lost
 realloc (vg_clientfuncs.c:270)
 nsFT2FontCatalog::AddFont() (nsFT2FontCatalog.cpp:282)
 nsFT2FontCatalog::GetFontNames() (nsFT2FontCatalog.cpp:839)

282    fc->fonts = (nsFontCatalogEntry **)realloc(fc->fonts,
283                           fc->numSlots*sizeof(nsFontCatalogEntry *));

(3) 110092 bytes in 227 blocks are possibly lost in loss record 1188 of 1188
 malloc (vg_clientfuncs.c:100)
 PR_Malloc (prmem.c:434)
 nsCompressedCharMap::NewCCMap() (nsCompressedCharMap.cpp:188)
 nsFT2FontCatalog::NewFceFromSummary() (nsFT2FontCatalog.cpp:1859)
 nsFT2FontCatalog::ReadFontSummaries() (nsFT2FontCatalog.cpp:2154)

1859  fce->mCCMap = ccmapObj.NewCCMap();

most of the leaking (at least 2 and 3) are due to the new destructor
(nsFT2FontCatalog::~nsFT2FontCatalog()) never being called.  nsFT2FontNode used
to call nsFreeTypeFreeGlobals, which eventually did
nsFT2FontCatalog::FreeGlobals.  The new sFcs service in nsFT2FontNode is never
released

Comment 9

[kill this account]

Comment on attachment 107229 [details] [diff] [review]
patch v2

bug 180473 is unrelated to this issue. It provides a xpcom service for
FreeType2 and generates postscript.

Louie: would you mind looking at this?

Comment 10

[Andrew Schultz]

this worksforme with current trunk.  the only FT2-related leak I get is:

36 bytes in 1 blocks are still reachable in loss record 1347 of 2117
 malloc (vg_clientfuncs.c:100)
 __builtin_new (nsAppRunner.cpp:177)
 operator new() (vg_clientfuncs.c:139)
 nsFT2FontCatalogConstructor() (nsGfxFactoryGTK.cpp:94)
 nsGenericFactory::CreateInstance() (nsGenericFactory.cpp:86)
 nsComponentManagerImpl::CreateInstanceByContractID() (nsComponentManager.cpp:1923)
 nsComponentManagerImpl::GetServiceByContractID() (nsComponentManager.cpp:2322)
 nsServiceManager::GetService() (nsServiceManagerObsolete.cpp:120)
 nsFT2FontNode::InitGlobals() (nsFT2FontNode.cpp:87)
 InitGlobals() (nsFontMetricsGTK.cpp:1157)

but I'm not sure what that means or that it's FreeType's fault (valgrind
reported quite a few leaks with similar stacks for other services).  

Comment 11

[Louie Zhao]

Comment on attachment 107229 [details] [diff] [review]
patch v2

reviewed with minor change

>+{
>+  int i;
>+  for (i=0; i<dc->numDirs; i++) {

    for (i=0; i < dc->numDirs; i++) {

Comment 12

[Andrew Schultz]

>>+{
>>+  int i;
>>+  for (i=0; i<dc->numDirs; i++) {
>
>    for (i=0; i < dc->numDirs; i++) {

the lack of spacing there is consistent with other for loops in the file

the lack of leaking in comment 10 appears to have been because the profile was
too clean (didn't have it set to use FreeType fonts)

Comment 13

[Andrew Schultz]

Attachment: patch v3

this patch actually releases the sFcs FontCatalog service in nsFT2FontNode.cpp
and calls FreeGlobals from ~nsFT2FontCatalog (removed in bug 180473) so that
all the code in the patch actually gets used.  that exposed the fact that I had
been double-freeing mFontCatalog.

this patch also frees fc->fonts in GetFontCatalogEntries (the individual
"entries" are still being leaked by nsFT2FontNode.cpp and probably
nsFontMetricsPS.cpp)

Comment 14

[Brian Ryner (not reading)]

Comment on attachment 116513 [details] [diff] [review]
patch v3

The "destructor code" comment is fairly useless, but other than that looks
fine.

Comment 15

[Andrew Schultz]

Louie, can you check this in (without the destructor comment)?
thanks

Comment 16

[Louie Zhao]

check in the patch

Comment 17

[Brian Ryner (not reading)]

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