293007 - nsFontXft objects are not freed on exit

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Denis Vlasenko]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.7) Gecko/20050505 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.7) Gecko/20050505 Firefox/1.0.3

Some background. I am trying to find memory leaks in Firefox using valgrind.
Firefox leaves many unfreed dynamically allocated objects at exit.
It is not a bug per se, but makes hunting for real leaks difficult.
I think we shall free everything we ever allocated.

Let me start from the most visible ones. I opened five tabs with
a local page:

<html>
<a href="Titan IV.mpg">Titan IV.mpg (size: 2318887 bytes)</a>
</html>

then I closed them one-by-one, then closed the browser.

Many small and two of three biggest unfreed allocations
in valgrind log seem to result from unfreed nsFontXft objects:

141924 bytes in 4 blocks are still reachable in loss record 142 of 144
   at 0x341462B4: malloc (vg_replace_malloc.c:130)
   by 0x34A7CEF3: XftFontOpenInfo (in $X/lib/libXft.so.2.1)
   by 0x34A7DC7A: XftFontOpenPattern (in $X/lib/libXft.so.2.1)
   by 0x3537829D: nsFontXft::GetXftFont() (in $moz/components/libgfx_gtk.so)
   by 0x35375D15: nsFontMetricsXft::FindFont(unsigned) (in
$moz/components/libgfx_gtk.so)
   by 0x35375610: nsFontMetricsXft::RealizeFont() (in $moz/components/libgfx_gtk.so)
   by 0x35374F2D: (within $moz/components/libgfx_gtk.so)
   by 0x3515E3E5: nsFontCache::GetMetricsFor(nsFont const&, nsIAtom*,
nsIFontMetrics*&) (in $moz/libgkgfx.so)
   by 0x3515D740: DeviceContextImpl::GetMetricsFor(nsFont const&, nsIAtom*,
nsIFontMetrics*&) (in $moz/libgkgfx.so)
   by 0x3573FC99: (within $moz/components/libgklayout.so)
   by 0x3573FDD3: nsHTMLReflowState::CalcLineHeight(nsIPresContext*,
nsIRenderingContext*, nsIFrame*) (in $moz/components/libgklayout.so)
   by 0x35716FC1: nsBlockReflowState::nsBlockReflowState(nsHTMLReflowState
const&, nsIPresContext*, nsBlockFrame*, nsHTMLReflowMetrics const&, int) (in
$moz/components/libgklayout.so)

486394 bytes in 286 blocks are still reachable in loss record 144 of 144
   at 0x341462B4: malloc (vg_replace_malloc.c:130)
   by 0x34A956F6: ft_alloc (in $X/lib/libfreetype.so.6.3.3)
   by 0x34A8D1C9: FT_Alloc (in $X/lib/libfreetype.so.6.3.3)
   by 0x34A90DBF: FT_CMap_New (in $X/lib/libfreetype.so.6.3.3)
   by 0x34AA7B3E: PCF_Face_Init (in $X/lib/libfreetype.so.6.3.3)
   by 0x34A902B5: open_face (in $X/lib/libfreetype.so.6.3.3)
   by 0x34A9056B: FT_Open_Face (in $X/lib/libfreetype.so.6.3.3)
   by 0x34A90772: FT_New_Face (in $X/lib/libfreetype.so.6.3.3)
   by 0x34A7C8A6: _XftLockFile (in $X/lib/libXft.so.2.1)
   by 0x34A7CDCB: XftFontOpenInfo (in $X/lib/libXft.so.2.1)
   by 0x34A7DC7A: XftFontOpenPattern (in $X/lib/libXft.so.2.1)
   by 0x3537829D: nsFontXft::GetXftFont() (in $moz/components/libgfx_gtk.so)

Last one is the biggest. Allocation:

XftFont *nsFontXft::GetXftFont(void) {
    if (!mXftFont) {
        FcPattern *pat = FcFontRenderPrepare(0, mPattern, mFontName);
        if (!pat)
            return nsnull;
...
        mXftFont = XftFontOpenPattern(GDK_DISPLAY(), pat);  <===== HERE
        if (!mXftFont)
            FcPatternDestroy(pat);
    }
    return mXftFont;
}

It is never leaked AFAIKS. ~nsFontXft() destructor
have code which frees mXftFont. I conclude that some
nsFontXft objects are not destroyed at exit.

Being totally unfamiliar with source tree, I did not track
where freeing can (should?) be added.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Denis Vlasenko]

gfx/src/gtk/nsFontMetricsXft.cpp:

mLoadedFonts is a collection of void* ptrs
which are really nsFontXft*. We need to
cast and delete them, but it seems we
are forgetting to do it here (2 places):

    PRInt32 i = 1;
    if (removeFirstFont) {
        // The first font was bad, so remove it (see below).  But do this
        // after |DoMatch| since otherwise it will get re-added.
        mLoadedFonts.RemoveElementAt(0);  <====================
        i = 0;
    }

    // Now check the remaining fonts
    for (; i < mLoadedFonts.Count(); ++i) {
        nsFontXft *font = (nsFontXft *)mLoadedFonts.ElementAt(i);
        if (font->HasChar(aChar)) {
            if (font->GetXftFont())
                return font;
            // This is a bad font, so remove it from mLoadedFonts.  This
            // could happen if it's in fc.cache-1 but the font doesn't exist
            // (https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=111973)
            // or isn't readable.
            mLoadedFonts.RemoveElementAt(i--); <=======================
        }
    }

BTW why mLoadedFonts is a nsVoidArray and not a properly typed array?

Comment 2

[Denis Vlasenko]

Nah, this is not where it leaks fonts...
Mozilla does free 'em but Xft caches freed fonts!
Grrr...
somehow we do not call XCloseDisplay ->
X extension close handlers do not run ->
fonts are not freed...

Comment 3

[Denis Vlasenko]

nsAppRunner.cpp:main() initializes gtk with gtk_init.
It turns out that gtk_init leaks GdkDisplay * internally!
This is my dirty workaround:

extern "C" int //gboolean
gtk_parse_args (int *argc, char ***argv);

// This one is copied from gtk source,
// modified to NOT leak GdkDisplay*
static
GdkDisplay* //gboolean
vda_gtk_init_check (int *argc, char ***argv)
{
  if (!gtk_parse_args (argc, argv))
    return FALSE;

  return gdk_display_open_default_libgtk_only ();// != NULL;
}

int main(int argc, char* argv[])
...
  // Initialize GTK+1/2 here for splash
#if defined(MOZ_WIDGET_GTK)
  gtk_set_locale();
#endif
  GdkDisplay *vda_dpy = vda_gtk_init_check(&argc, &argv); //vda
...
  nsresult mainResult = main1(argc, argv, nativeApp ? (nsISupports*)nativeApp :
(nsISupports*)splash);

#if defined(MOZ_WIDGET_GTK) || defined(MOZ_WIDGET_GTK2)
// gtk_init() -> gtk_init_check():
// return gdk_display_open_default_libgtk_only () != NULL;
// thus we lost GdkDisplay* inside gtk_init! Work around:
  gdk_display_close(vda_dpy);
#endif /* MOZ_WIDGET_GTK || MOZ_WIDGET_GTK2 */

  /* if main1() didn't succeed, then don't bother trying to shut down clipboard,
etc */
...

(not in patch format, sorry...)

This will segfault in gtk because there are more bugs in gtk
on gdk_display_close() path. I tracked them down.
I will submit them to gnome bugzilla.

Comment 4

[Denis Vlasenko]

Gnome bugs submitted: http://bugzilla.gnome.org/show_bug.cgi?id=304608

Comment 5

[Denis Vlasenko]

With this fix I am down from >1000000 unfree()d bytes
on exit to ~80000 bytes. Just opening and closing "about:blank"
page. Most of that memory is in fonts cached by Xft.

Comment 6

[Denis Vlasenko]

I moved this code to the very end of main()
(just before return statement):

#if defined(MOZ_WIDGET_GTK) || defined(MOZ_WIDGET_GTK2)
  gdk_display_close(vda_dpy);
#endif /* MOZ_WIDGET_GTK || MOZ_WIDGET_GTK2 */

Unfortunately even some destructors expect that gtk
has default display opened. Valgrind shows this trace to segfault
(definitely happens after main() has returned):

Invalid write of size 4
   at 0x3588DCE9: __static_initialization_and_destruction_0(int, int) (in
$moz/components/libgfx_gtk.so)
   by 0x3588DD5A: _GLOBAL__D_NSGetModule (in $moz/components/libgfx_gtk.so)
   by 0x3588390E: (within $moz/components/libgfx_gtk.so)
   by 0x358A1C49: (within $moz/components/libgfx_gtk.so)
   by 0x34133B8B: _dl_fini (in $glibc/ld-2.3.2.so)
   by 0x34A81A15: exit (in $glibc/libc-2.3.2.so)
   by 0x34A6DFFB: __libc_start_main (in $glibc/libc-2.3.2.so)
   by 0x804A020: (within $moz/mozilla-bin)
 Address 0x36115B70 is not stack'd, malloc'd or (recently) free'd

Invalid read of size 4
   at 0x341CFAFF: nsAString::~nsAString() (in $moz/libxpcom_core.so)
   by 0x34E99B18: nsFont::~nsFont() (in $moz/libgkgfx.so)
   by 0x3588DCF6: __static_initialization_and_destruction_0(int, int) (in
$moz/components/libgfx_gtk.so)
   by 0x3588DD5A: _GLOBAL__D_NSGetModule (in $moz/components/libgfx_gtk.so)
   by 0x3588390E: (within $moz/components/libgfx_gtk.so)
   by 0x358A1C49: (within $moz/components/libgfx_gtk.so)
   by 0x34133B8B: _dl_fini (in $glibc/ld-2.3.2.so)
   by 0x34A81A15: exit (in $glibc/libc-2.3.2.so)
   by 0x34A6DFFB: __libc_start_main (in $glibc/libc-2.3.2.so)
   by 0x804A020: (within $moz/mozilla-bin)
 Address 0x36115B74 is not stack'd, malloc'd or (recently) free'd

Invalid read of size 4
   at 0x341CB553: nsSubstring::Finalize() (in $moz/libxpcom_core.so)
   by 0x341CFB11: nsAString::~nsAString() (in $moz/libxpcom_core.so)
   by 0x34E99B18: nsFont::~nsFont() (in $moz/libgkgfx.so)
   by 0x3588DCF6: __static_initialization_and_destruction_0(int, int) (in
$moz/components/libgfx_gtk.so)
   by 0x3588DD5A: _GLOBAL__D_NSGetModule (in $moz/components/libgfx_gtk.so)
   by 0x3588390E: (within $moz/components/libgfx_gtk.so)
   by 0x358A1C49: (within $moz/components/libgfx_gtk.so)
   by 0x34133B8B: _dl_fini (in $glibc/ld-2.3.2.so)
   by 0x34A81A15: exit (in $glibc/libc-2.3.2.so)
   by 0x34A6DFFB: __libc_start_main (in $glibc/libc-2.3.2.so)
   by 0x804A020: (within $moz/mozilla-bin)
 Address 0x36115B80 is not stack'd, malloc'd or (recently) free'd

Invalid read of size 4
   at 0x341CB556: nsSubstring::Finalize() (in $moz/libxpcom_core.so)
   by 0x341CFB11: nsAString::~nsAString() (in $moz/libxpcom_core.so)
   by 0x34E99B18: nsFont::~nsFont() (in $moz/libgkgfx.so)
   by 0x3588DCF6: __static_initialization_and_destruction_0(int, int) (in
$moz/components/libgfx_gtk.so)
   by 0x3588DD5A: _GLOBAL__D_NSGetModule (in $moz/components/libgfx_gtk.so)
   by 0x3588390E: (within $moz/components/libgfx_gtk.so)
   by 0x358A1C49: (within $moz/components/libgfx_gtk.so)
   by 0x34133B8B: _dl_fini (in $glibc/ld-2.3.2.so)
   by 0x34A81A15: exit (in $glibc/libc-2.3.2.so)
   by 0x34A6DFFB: __libc_start_main (in $glibc/libc-2.3.2.so)
   by 0x804A020: (within $moz/mozilla-bin)
 Address 0x36115B78 is not stack'd, malloc'd or (recently) free'd
...

Comment 7

[Denis Vlasenko]

Disregard last comment, destructor segv'ing was my fault.
I added some debug code and introduced a bug myself :)

Closing gtk's default GdkDisplay just before returning from main()
works without any problems (if you fix gtk, tho:
http://bugzilla.gnome.org/show_bug.cgi?id=304608)

Opening and closing about:blank with this change
frees ~700k of X resources. ~300k is still left unfreed.
Top remaining offender is glib's iconv cache (it has no way to
be flushed, see http://bugzilla.gnome.org/show_bug.cgi?id=305037)

Comment 8

[Denis Vlasenko]

Aha, GdkDisplay* can be freed with much simpler code.
nsAppRunner.cpp:main():

   rv = NS_ShutdownXPCOM(nsnull);
     NS_ASSERTION(NS_SUCCEEDED(rv), "NS_ShutdownXPCOM failed");
   #endif
+
+#if defined(MOZ_WIDGET_GTK) || defined(MOZ_WIDGET_GTK2)
+  gdk_display_close(gdk_display_get_default());
+#endif /* MOZ_WIDGET_GTK || MOZ_WIDGET_GTK2 */

   return TranslateReturnValue(mainResult);
 }

That's all. Please apply sometime after 'official' gtk will
be patched per http://bugzilla.gnome.org/show_bug.cgi?id=85715

Comment 9

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 10

[Gervase Markham [:gerv]]

This bug has been automatically resolved after a period of inactivity (see above
comment). If anyone thinks this is incorrect, they should feel free to reopen it.