To keep track of glyphs that are available within a font, the font engine allocates a 64K bit array for each font. But the problem is that these maps are kept forever, and are not free'ed upon exiting from Mozilla. Erik has explained that because these maps are expensive to create, they are created once and cached as long as possible in a hash table. And that he is planning to add a "shutdown listener" that will free these maps just before shutdown. So I am filling this bug for the record, and as reminder to him. I think that there is also a plan to enable the sharing of identical maps between fonts, so this is worth reminding here as well.

Erik, in order to support measurement/drawing based on an array of glyph indices, rather than a standard string, I was forced to implement the GetGlyphIndices() function. It is based on the GetGlyph() function that was fortuitously in nsFontMetricsWin.cpp !! [lxr.mozilla.org/seamonkey/source/gfx/src/windows/nsFontMetricsWin.cpp#244] This GetGlyphIndices() function needs the CMAP itself. And so I have been forced to cache this CMAP as well! This GetGlyphIndices() function allow to fix a long-standing problem that have been affecting GetBoundingMetrics(). Because GetGlyphOutlineW doesn't exist on Win95/Win98, nsFontWinUnicode::GetBoundingMetrics() will systematically failing on some inputs. This problem is documented in the MS' Knowledge Base Q241358: "The GetGlyphOutlineW Function Fails on Windows 95 and Windows 98" http://support.microsoft.com/support/kb/articles/Q241/3/58.ASP With the GetGlyphIndices() function, it is now possible to use the work-around suggested there, i.e., use GetGlyphOutlineA in conjuction with the GGO_GLYPH_INDEX option. This, indeed, provides the desired metrics. I renamed GetGlyph() as GetGlyphIndex() and added a couple of changes to do the computations % 65536, as suggested in: Q241020 HOWTO: Translate Unicode Character Codes to TrueType Glyph http://support.microsoft.com/support/kb/articles/Q241/0/20.ASP Again, the main issue that is in correlation with this bug is that the CMAP itself has to be cached --for performance reasons. Currently, this is done lazily. If nsFontWinUnicode::GetBoundingMetrics() is not called, the CMAP is never kept --the first call causes the CMAP to be cached. This only happens for nsFontWinUnicode fonts (not for other variants). I know that you are particularly careful with nsFontWinUnicode fonts since these are the most used fonts. That's why I am expounding on all these details. I will submit the code (which is still in #ifdef MOZ_MATHML) to your perusal sometimes next week.

Prioritizing my bugs. This one is now M18.

reassign to yokoyama and mark it as M23. We should figure out what we should do with this . This may (or may not) help the embedding work.

Erik<-What is the best way to solve this problem? I would appreciate if you can help me to jump-start on this issue.

Erik<- I have a patch for this bug; but I noticed the following: 1) when you close the ProfileManager and start the browser, the ProfileManager calls to ~nsFontMetricsWin() via nsPresContext::PreferenceChanged(). Does this make sense? Does the FontMetrics Cache need to be flushed by changing the Preference?

Yes, this makes sense.

erik<- can you review the patch? ~nsFontMetricsWin() gets called on destroy and freeing the Cache. I did the pre-checkin tests as well.

Roy, in order to free both the hash table *and* its contents, you need to pass the 5th argument to PR_NewHashTable(). See how it is done in mozilla/modules/libpref/src/prefapi.c. Also, gGlobalFonts does not own the maps, so you shouldn't free those. The maps are owned by the gFontMaps hash table, so they should be freed when you free that hash table.

has anyone used Purify to look at the problem and patch?

No, I haven't. Roy, do you have Purify?

No, I don't. I made the correction to my patch and would like to test the code. Brian, if you have the purify, can I try it?

Erik: can you review the code for me to see if I am missing something else? I (with Brian's help) am in the process of getting the Purify; but I want to get your option. Thanks. :)

Updating the target milestone.

Hi Roy, please take another look at gFontMaps in nsFontMetricsWin.cpp. When entries are added to the hash table, we first create an nsFontInfo object using the "new" operator, so we should use "delete" to free it. Also, it contains a pointer to the 8K array (the map), which should also be freed. This pointer sometimes points to an object allocated using PR_Calloc(), so you need to use PR_Free() to free those, but it sometimes points to "emptyMap", which we only create once, so we should only delete it once (i.e. *not* in the hash table free routine, but in a separate call in FreeGlobals()). There may be other problems in your patch, but I'd like you to fix them before I take another look.

Erik: Thanks for the quick look. I modify the nsFontInfo constructor to the emptymap identifier making sure to free the emptymap only ONCE. nsFontInfo(... , BOOL aEmptymap=FALSE) Can you give a yet another quick look at the code so that I can check-in before mozilla0.8 milestone? Thanks:)

There is also a lack of error-checkin in case the allocs failed. These may have undesirable effects later, or at layers upper to GFX. Also when mCMAP is cached, it should be freed as well. This happens in MathML. So, the one-liner: + if (fontInfo) + { + if (!fontInfo->bEmptymap && fontInfo->mMap) + PR_Free(fontInfo->mMap); #ifdef MOZ_MATHML + if (fontInfo->mCMAP.mData) + PR_Free(fontInfo->mCMAP.mData); #endif + delete(fontInfo); +}

rbs< Thank you for your input. I'll definitely include your request.

Hi Roy, Adding a boolean member variable to nsFontInfo is not such a good idea because: 1. it increases the size of nsFontInfo 2. it is too much trouble to remember to pass the boolean to the constructor (a future programmer could easily forget to do that) Instead, you should just have some code that checks whether the map is the empty map before deleting it. For example: if (fontInfo->mMap && (fontInfo->mMap != gEmptyMap)) { PR_Free(fontInfo->mMap); } Note the 'g' in front of "gEmptyMap". It doesn't need to be inside a class, either.

Erik: Anything else you see I should do?

Hi Roy, emptyMap should be called gEmptyMap, and it should be set to nsnull after freeing it. Objects of type nsFontWeightEntry are created using "new", so they must be destroyed using "delete". mWeightTable is a PRUint16, so it should not be set to nsnull, which is reserved for pointers. Also, it is not necessary to set a PRUint16 to zero in a destructor. The keys and values in gFamilyNames are created using "new", so they should be destroyed using "delete". Finally, as rbs already mentioned, all of the allocations should be checked for failure. For example, gFontMaps is not checked after creation.

Erik: can you review it for me?

The keys in gFontMaps are created using "new", so they must be destroyed using "delete" after casting to nsString*. If the allocation for gEmptyMap fails, you need to destroy gFontMaps before returning nsnull. You would also need to set gInitializedFontMaps to zero. However, I believe it is cleaner to take the approach that we took in mozilla/gfx/src/gtk/nsFontMetricsGTK.cpp. There, we initialize all the globals in nsFontMetricsGTK::Init(), by calling InitGlobals(). This way, we can use a single variable instead of 3 separate ones (gInitializedFontMaps, gInitializedFamilyNames and gInitializedFontWeights). Sorry about all these comments. My, my isn't memory management in C++ fun!

The keys in gFontMaps are of type nsString*. In order to call nsString's destructor, you need to cast the pointer to nsString*, and then pass the result to delete. For example: delete (nsString*) (he->key); nsFontInfo's mMap is allocated using PR_Calloc, so it must be freed using PR_Free. You need to set gInitializedFontMaps to zero if gFontMaps allocation fails. Alternatively, you could set gInitializedFontMaps to 1 after gFontMaps and gEmptyMap allocation succeeds (or take the approach I took in nsFontMetricsGTK). When the allocation for nsFontInfo fails, you need to delete name and PR_Free(map) if map != gEmptyMap. Also, PL_HashTableAdd itself can fail (returning 0), so you need error checking there too. The keys in gFontWeights are of type nsFontWeightEntry*, so you need to cast to that type, and then call delete. There is also a variable called fontWeightEntry that is set but not used. You need to set gInitializedFontWeights to zero if the allocation for gFontWeights fails. The keys and values in gFamilyNames are of type nsString*, so you need to cast to that type, and then call delete. You need to delete one or both of name and winName when either or both of their allocations fail(s). Why did you add code for mSubsets? The added code looks wrong.

Since most (if not all) of these variables are needed before the code can run, having a single gInitialized flag and folding all the alloc/delete in a pair of InitGlobals()/FreeGlobals() like Erik did in nsFontMetricsGTK, things will be much easier to maintain and nicer for you Roy. I also filled bug 45737 where I alluded to such things. So if you bite the bullet on this one, that may help also in view of that bug.

Is there any particular reasons why we use PR_Calloc and new separately? Can we keep it consistant thru out the app/module? We have gFontMaps, gFamilyNames, gFontWeights, gGlobalFonts, gEmptyFont and others with different memory allocation methods.

There is no good reason to use "new" in some cases and PR_Calloc in others. If you want, I think it would be OK to change the PR_*alloc calls to new, but please be careful, because calloc and PR_Calloc write zeroes in the new memory after allocating it, while "new" doesn't (I think).

info = new nsFontInfo(NS_FONT_TYPE_NON_UNICODE, DEFAULT_CHARSET, map); if (info) { // XXX check to see if an identical map has already been added - PL_HashTableAdd(gFontMaps, name, info); + if (PL_HashTableAdd(gFontMaps, name, info)) + return map; + else { // error adding entry to the table + delete info; + delete name; + if (map != gEmptyMap) + PR_Free(map); + return nsnull; + } } - return map; + else { // error allocating FontInfo + delete name; + if (map != gEmptyMap) + PR_Free(map); + return nsnull; + } } becomes in a much compact way: info = new nsFontInfo(NS_FONT_TYPE_NON_UNICODE, DEFAULT_CHARSET, map); if (info) { // XXX check to see if an identical map has already been added PL_HashTableAdd(gFontMaps, name, info); if (PL_HashTableAdd(gFontMaps, name, info)) return map; } } // if we reach here, no map was added to the hashtable if (info) delete info; // error adding entry to the table delete name; if (map != gEmptyMap) PR_Free(map); return nsnull; [Same pattern elsewhere ...], for exemple at: + // Store it in font weight HashTable. + nsFontWeightEntry* fontWeightEntry = new nsFontWeightEntry; + if (fontWeightEntry) { + fontWeightEntry->mFontName = low; + fontWeightEntry->mWeightTable = weightTable; + PL_HashTableAdd(gFontWeights, fontWeightEntry, fontWeightEntry); + } + else { // error checking + return 0; + } becomes ... (note: you didn't check that the HT-add was ok] + nsFontWeightEntry* fontWeightEntry = new nsFontWeightEntry; + if (fontWeightEntry) { + fontWeightEntry->mFontName = low; + fontWeightEntry->mWeightTable = weightTable; if (PL_HashTableAdd(gFontWeights, fontWeightEntry, fontWeightEntry)) { return fontWeightEntry; } } // if we reach here, no fontWeightEntry was added to the hashtable if (fontWeightEntry) delete fontWeightEntry; return 0; [Same pattern there and elsewhere ...] + if (PL_HashTableAdd(gFamilyNames, name, (void*) winName) == 0) { becomes + if (PL_HashTableAdd(gFamilyNames, name, (void*) winName)) { ... r=rbs, assuming you get the suggested compact patterns.

+PR_STATIC_CALLBACK(void*) fontmap_AllocTable(void *pool, size_t size) +{ + return PR_MALLOC(size); +} + +PR_STATIC_CALLBACK(void) fontmap_FreeTable(void *pool, void *item) +{ + PR_Free(item); +} Nit: why PR_MALLOC but PR_Free? PR_Malloc is the matching function, PR_MALLOC is just a macro that calls PR_Malloc. More significant point: why not fuse the allocation of nsFontInfo with the allocation of its PLHashEntry? +PR_STATIC_CALLBACK(void) fontmap_FreeEntry(void *pool, PLHashEntry *he, PRUint32 flag) +{ + nsFontInfo *fontInfo = (nsFontInfo *) he->value; + if (fontInfo) + { + if (fontInfo->mMap && (fontInfo->mMap != nsFontMetricsWin::gEmptyMap)) + PR_Free(fontInfo->mMap); +#ifdef MOZ_MATHML + if (fontInfo->mCMAP.mData) + PR_Free(fontInfo->mCMAP.mData); +#endif + delete(fontInfo); + } + + if (flag == HT_FREE_ENTRY) { + delete (nsString *) (he->key); + PR_Free(he); + } +} + shows separate allocations for key, value, and entry, but since you are using PLHashAllocOps, you can easily combine two or possibly all three of those into one allocation, cutting down on malloc per-allocation overhead, cycles spent in malloc and free, and heap fragmentation. Instead of this: info = new nsFontInfo(NS_FONT_TYPE_NON_UNICODE, DEFAULT_CHARSET, map); if (info) { // XXX check to see if an identical map has already been added - PL_HashTableAdd(gFontMaps, name, info); + if (PL_HashTableAdd(gFontMaps, name, info)) + return map; + else { // error adding entry to the table + delete info; + delete name; + if (map != gEmptyMap) + PR_Free(map); + return nsnull; + } } - return map; + else { // error allocating FontInfo + delete name; + if (map != gEmptyMap) + PR_Free(map); + return nsnull; + } } [Note also that there is an else after return above, which is unnecessary and a bit confusing -- the return means *none* of the subsequent code in this method executes if the return is taken, yet only some of that subsequent code is braced in the else after the return.] how about this: PLHashEntry **hep, *he; PLHashNumber hash = HashKey(name); hep = PL_HashTableRawLookup(gFontMaps, hash, name); he = *hep; if (he) { // XXX an identical map has already been added??? } else { info = new nsFontInfo(NS_FONT_TYPE_NON_UNICODE, DEFAULT_CHARSET, map); if (info) { he = info; if (PL_HashTableRawAdd(gFontMaps, hep, he, name, info)) return map; // error adding entry to the table delete info; // fall through into "error allocating FontInfo" code } // error allocating FontInfo delete name; if (map != gEmptyMap) PR_Free(map); return nsnull; } and make class nsFontInfo : public PLHashEntry, of course? rbs's comment suggest almost the same thing, but I think I've eliminated all redundant tests of info in the cleanup code after the early 'return map;'. Ask me anything if this doesn't make sense. /be

Argh, I blew it in my attempt to type new code into this tiny textarea! Several errors in my last comment, both in not showing the revised functions needed for PLHashAllocOps that fuse nsFontInfo and PLHashEntry, and in the code I sketched that adds a new entry. Trying again, and using new/delete rather than PR_NEW/delete: PR_STATIC_CALLBACK(PLHashEntry*) fontmap_AllocEntry(void *pool, const void *key) { return new nsFontInfo(NS_FONT_TYPE_NON_UNICODE, DEFAULT_CHARSET, nsnull); } PR_STATIC_CALLBACK(void) fontmap_FreeEntry(void *pool, PLHashEntry *he, PRUint32 flag) { nsFontInfo *fontInfo = NS_STATIC_CAST(nsFontInfo *, he); if (fontInfo->mMap && (fontInfo->mMap != nsFontMetricsWin::gEmptyMap)) PR_Free(fontInfo->mMap); #ifdef MOZ_MATHML if (fontInfo->mCMAP.mData) PR_Free(fontInfo->mCMAP.mData); #endif if (flag == HT_FREE_ENTRY) { delete (nsString *) (he->key); delete fontInfo; } } Then the correct (revised from my last comment) code to add an entry, with some pseudo-code in the XXX case, is: PLHashEntry **hep, *he; PLHashNumber hash = HashKey(name); hep = PL_HashTableRawLookup(gFontMaps, hash, name); he = *hep; if (he) { // XXX an identical map has already been added info = he; reinitialize info->mMap, info->mCMAP, etc.; return map; } he = PL_HashTableRawAdd(gFontMaps, hep, hash, name, nsnull); if (he) { info = NS_STATIC_CAST(nsFontInfo*, he); he->value = info; // so PL_HashTableLookup returns an nsFontInfo* info->mMap = map; return map; } delete name; if (map != gEmptyMap) PR_Free(map); return nsnull; Notice how the entry, which is the superclass of the fontinfo class, does not get deleted and recreated if there is a duplication. Could that case arise? Sorry for the confusion; hope I have the above right. /be

That's a much compact and elegant way. I recollect the intention is that if there is an identical map, then a no-op follows. i.e., if the name of the font is the same (but some other attributes are different e.g., 12pt vs. 24pt), then they don't matter, there is a total re-use of the info stored in the hash because it doesn't depend on such attributes: if (he) { // an identical map has already been added info = he; init other OUT params of the function: *aFontType, *aCharset return info->map; } with the corresponding update: PR_STATIC_CALLBACK(void) fontmap_FreeEntry(void *pool, PLHashEntry *he, PRUint32 flag) { if (flag == HT_FREE_ENTRY) { nsFontInfo *fontInfo = NS_STATIC_CAST(nsFontInfo *, he); if (fontInfo->mMap && (fontInfo->mMap != nsFontMetricsWin::gEmptyMap)) PR_Free(fontInfo->mMap); #ifdef MOZ_MATHML if (fontInfo->mCMAP.mData) PR_Free(fontInfo->mCMAP.mData); #endif delete (nsString *) (he->key); delete fontInfo; } }

rbs: I'll buy that. Thanks for the out param pseudo-code fix! /be

Roger and Brendan, thanks for reviewing (IBM's bidi submission has been keeping me very busy), and Roy, thanks for taking this bug. Had a quick look at the most recent comments, and they look good.

Looks pretty good. There are a few else after return and else after break non-sequiturs visible in the patch, you might find that fixing those makes the code clearer, generally by making less exceptional cases (formerly in the unnecessary else clauses) less indented. Also, why not make gFontWeights fuse its value (which is also its key) with its PLHashEntry, since you're already specializing fontweight_HashAllocOps? Just new the nsFontWeightEntry in ..._allocEntry, and delete it in _freeEntry if HT_FREE_ENTRY. Use PL_HashTableRawLookup and PL_HashTableRawAdd similarly to the way they're used for gFontMaps. /be

brendan: I look at the code for exceptional cases and they seem reasonable. I wonder if the patch is acceptable for check-in.

Looks good, thanks. sr=brendan@mozilla.org. /be

r=rbs

checked-in

Marking verified in the May 30th build.