Bug 978608: Shut down pango's fontmap cleanly and shut down fontconfig, #if CLEANUP_MEMORY. r=karlt
authorL. David Baron <dbaron@dbaron.org>
Mon, 03 Mar 2014 00:54:38 -0800
changeset 171765 c66cd5bf1aadd254aefa2078ef3d3f319e006a53
parent 171764 2275e85c9ad070c25bba736fa4ea3ed3afb2bf5e
child 171766 8a72d26209182966111fcc0a6a721c65ac3160ec
push id270
push userpvanderbeken@mozilla.com
push dateThu, 06 Mar 2014 09:24:21 +0000
reviewerskarlt
bugs978608
milestone30.0a1
Bug 978608: Shut down pango's fontmap cleanly and shut down fontconfig, #if CLEANUP_MEMORY. r=karlt This bumps the minimum required pango version to 1.22, released 29 Sep 2008.
configure.in
gfx/thebes/gfxPlatform.cpp
gfx/thebes/gfxPlatformGtk.cpp
toolkit/xre/nsAppRunner.cpp
--- a/configure.in
+++ b/configure.in
@@ -55,17 +55,17 @@ MOZPNG=10609
 NSPR_VERSION=4
 NSS_VERSION=3
 
 dnl Set the minimum version of toolkit libs used by mozilla
 dnl ========================================================
 GLIB_VERSION=1.2.0
 PERL_VERSION=5.006
 CAIRO_VERSION=1.10
-PANGO_VERSION=1.14.0
+PANGO_VERSION=1.22.0
 GTK2_VERSION=2.10.0
 GTK3_VERSION=3.0.0
 WINDRES_VERSION=2.14.90
 W32API_VERSION=3.14
 GNOMEVFS_VERSION=2.0
 GNOMEUI_VERSION=2.2.0
 GCONF_VERSION=1.2.1
 GIO_VERSION=2.20
--- a/gfx/thebes/gfxPlatform.cpp
+++ b/gfx/thebes/gfxPlatform.cpp
@@ -529,24 +529,16 @@ gfxPlatform::~gfxPlatform()
     // Cairo objects e.g. through SkCairoFTTypeface
     SkGraphics::Term();
 #endif
 
 #if MOZ_TREE_CAIRO
     cairo_debug_reset_static_data();
 #endif
 #endif
-
-#if 0
-    // It would be nice to do this (although it might need to be after
-    // the cairo shutdown that happens in ~gfxPlatform).  It even looks
-    // idempotent.  But it has fatal assertions that fire if stuff is
-    // leaked, and we hit them.
-    FcFini();
-#endif
 }
 
 bool
 gfxPlatform::PreferMemoryOverShmem() const {
   MOZ_ASSERT(!CompositorParent::IsInCompositorThread());
   return mLayersPreferMemoryOverShmem;
 }
 
--- a/gfx/thebes/gfxPlatformGtk.cpp
+++ b/gfx/thebes/gfxPlatformGtk.cpp
@@ -70,24 +70,16 @@ gfxPlatformGtk::gfxPlatformGtk()
 }
 
 gfxPlatformGtk::~gfxPlatformGtk()
 {
     gfxFontconfigUtils::Shutdown();
     sFontconfigUtils = nullptr;
 
     gfxPangoFontGroup::Shutdown();
-
-#if 0
-    // It would be nice to do this (although it might need to be after
-    // the cairo shutdown that happens in ~gfxPlatform).  It even looks
-    // idempotent.  But it has fatal assertions that fire if stuff is
-    // leaked, and we hit them.
-    FcFini();
-#endif
 }
 
 already_AddRefed<gfxASurface>
 gfxPlatformGtk::CreateOffscreenSurface(const IntSize& size,
                                        gfxContentType contentType)
 {
     nsRefPtr<gfxASurface> newSurface;
     bool needsClear = true;
--- a/toolkit/xre/nsAppRunner.cpp
+++ b/toolkit/xre/nsAppRunner.cpp
@@ -218,16 +218,17 @@ static char **gQtOnlyArgv;
 #define PANGO_ENABLE_BACKEND
 #include <pango/pangofc-fontmap.h>
 #endif
 #include <gtk/gtk.h>
 #ifdef MOZ_X11
 #include <gdk/gdkx.h>
 #endif /* MOZ_X11 */
 #include "nsGTKToolkit.h"
+#include <fontconfig/fontconfig.h>
 #endif
 #include "BinaryPath.h"
 
 #ifdef MOZ_LINKER
 extern "C" MFBT_API bool IsSignalHandlingBroken();
 #endif
 
 namespace mozilla {
@@ -2610,38 +2611,47 @@ static void MOZ_gdk_display_close(GdkDis
     // like Pango and cairo. But if cairo shutdown is buggy, we should
     // shut down cairo first otherwise it may crash because of dangling
     // references to Display objects (see bug 469831).
     if (!theme_is_qt)
       gdk_display_close(display);
   }
 
 #if CLEANUP_MEMORY
+  // Clean up PangoCairo's default fontmap.
+  // This pango_fc_font_map_shutdown call (and the associated code to
+  // get the font map) really shouldn't be needed anymore, except that
+  // it's needed to avoid having cairo_debug_reset_static_data fatally
+  // assert if we've leaked other things that hold on to the fontmap,
+  // which is something that currently happens in mochitest-plugins.
+  // Even if it didn't happen in mochitest-plugins, we probably want to
+  // avoid the crash-on-leak problem since it makes it harder to use
+  // many of our leak tools to debug leaks.
+
   // This doesn't take a reference.
   PangoFontMap *fontmap = pango_context_get_font_map(pangoContext);
   // Do some shutdown of the fontmap, which releases the fonts, clearing a
   // bunch of circular references from the fontmap through the fonts back to
   // itself.  The shutdown that this does is much less than what's done by
   // the fontmap's finalize, though.
   if (PANGO_IS_FC_FONT_MAP(fontmap))
       pango_fc_font_map_shutdown(PANGO_FC_FONT_MAP(fontmap));
   g_object_unref(pangoContext);
-  // PangoCairo still holds a reference to the fontmap.
-  // Now that we have finished with GTK and Pango, we could unref fontmap,
-  // which would allow us to call FcFini, but removing what is really
-  // Pango's ref feels a bit evil.  Pango-1.22 will have support for
-  // pango_cairo_font_map_set_default(nullptr), which would release the
-  // reference on the old fontmap.
+
+  // Tell PangoCairo to release its default fontmap.
+  pango_cairo_font_map_set_default(nullptr);
 
   // cairo_debug_reset_static_data() is prototyped through cairo.h included
   // by gtk.h.
 #ifdef cairo_debug_reset_static_data
 #error "Looks like we're including Mozilla's cairo instead of system cairo"
 #endif
   cairo_debug_reset_static_data();
+  // FIXME: Do we need to call this in non-GTK2 cases as well?
+  FcFini();
 #endif // CLEANUP_MEMORY
 
   if (buggyCairoShutdown) {
     if (!theme_is_qt)
       gdk_display_close(display);
   }
 }
 #endif // MOZ_WIDGET_GTK2