357779 - gfxTextRunCache not cleaning up memory on shutdown

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Andrew Schultz]

nye bloat tbox showed a RLk change from 2KB to 22KB while it was down.  Relevant chunk from the log is

TOTAL                                         22528     939.11%
nsStringBuffer                                 9800   17400.00%
gfxTextRun                                     6624          -
gfxFont                                        2072          -
gfxFontGroup                                   1920          -

I verified that the leaks are due to bug 356235

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I don't think that these are real leaks; this is the the textrun cache not cleaning things up after itself on shutdown -- it's just a global object that gets allocated on the heap and is never freed.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So the problem with doing this is that this cache lives in a static lib, not in a module... ;(

I've hacked my layout module to delete the cache on shutdown so I can debug other leaks, for the time being.  But that's not good enough for checkin.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Note that Roc's new textframe patch does not use the textRunCache.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Propoposed fix

OK, roc says he's not planning to actually remove this cache.  So we need to fix this bug so we have reasonable leak numbers.

This patch makes consumers of the static lib actually init and shut down the cache if they want it to work.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 252257 [details] [diff] [review]
Propoposed fix

r=me, thanks!

Comment 6

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I know I reviewed this, but is there a reason why we didn't just add the textrun cache Init/Shutdown to nsThebesGfxFactory? The module destructor does get called, right?

Comment 8

[Boris Zbarsky [:bzbarsky]]

We could do that, sure.  The point was that we needed it in _some_ module.  And only one module.  But one guaranteed to be instantiating this cache, which lives in a non-module lib....