487971 - gfxPangoFcFont::NewFont leaks reference to fc_font->fontmap (shutdown leak)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

gfxPangoFcFont::NewFont has code:
        // PangoFcFont::get_coverage wants an FcFontMap.  (PangoFcFontMap
        // usually sets this after calling PangoFcFontMap::create_font().)
        PangoFcFont *fc_font = &font->parent_instance;
        NS_ASSERTION(!fc_font->fontmap, "already have fontmap");
        fc_font->fontmap = GetPangoFontMap();
        g_object_ref(fc_font->fontmap);
which gives ownership of a reference to fc_font->fontmap.

gfx_pango_fc_font_finalize has the comment:
    // The parent class removes the reference to parent_instance->fontmap.

However, in the pango source code used on Ubuntu 8.10, pango1.0-1.22.2, pango_fc_font_finalize does:
  if (fcfont->fontmap)
    _pango_fc_font_map_remove (PANGO_FC_FONT_MAP (fcfont->fontmap), fcfont);
which is essentially clearing a weak back-pointer, rather than doing any releasing.  (It's possible this changed over pango versions.)


It seems like there are two problems here:
 (1) we leak a reference to our global font map (gPangoFontMap) for each font we create, so we never free it at shutdown, which clutters the leak statistics with the font map object and I think a good bit of coverage stuff
 (2) our fontmap object doesn't actually know about the fonts that reference it, as I think fontmaps are supposed to.  I'm not sure if this would cause any problems.

Comment 1

[Karl Tomlinson (:karlt)]

Yes, there was a change from strong to weak references between Pango-1.20 and 1.22.

The change is a problem to us because Mozilla overrides PangoFcFontMap::load_fontset, which sets (or used to set) PangoFcFont::fontmap but not PangoFcFont::finalize, which unsets PangoFcFont::fontmap.

If Mozilla were to override PangoFcFont::finalize, it should really implement the whole PangoFcFont class, but doing that before switching to harfbuzz doesn't seem so appealing.

Pango-1.24 sets PangoFcFont::fontmap through a property on the PangoFcFont.
Using this mechanism would be much better as it would let the PangoFcFont handle any referencing and unreferencing so that Mozilla would not need to care.

For older versions, it looks like we'd be safe just to unref and set to null PangoFcFont::fontmap in gfx_pango_fc_font_finalize (but I need to test that).

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: fix for recent Pango

This is a half fix.  It fixes the problem for Pango-1.24.x, and leaves the behavior the same for earlier versions.

Pango-1.22.x would really need a different fix, but it doesn't seem worth the extra code and time to get it right, given that this version is going to be used less and less in the future.

(In reply to comment #1)
> For older versions, it looks like we'd be safe just to unref and set to null
> PangoFcFont::fontmap in gfx_pango_fc_font_finalize (but I need to test that).

That would work most of the time, but we'd need to actually check that we added the reference before removing it, and I don't think its worth trying to do that for a problem with only Pango-1.22.x that most people won't notice.

Comment 3

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/a55505d20efd

(I know this doesn't fix the problem as originally reported with Pango-1.22, but this is fixed in the sense that recent versions of Pango now how a better way of handling this, and we use that method when available.)