503924 - clean up Pango better so we can call FcFini

Status: NEW

(Core :: Widget: Gtk, defect, P5)

Description

[Zack Weinberg (:zwol)]

Firefox crashes on shutdown in my trunk debugging build -- either normal app shutdown or app restart from inside XRE_main.  Here's a gdb backtrace:

#0  0x00007ffbb0c71821 in nanosleep () from /lib/libc.so.6
#1  0x00007ffbb0c71647 in sleep () from /lib/libc.so.6
#2  0x00007ffbb53d5a58 in ah_crap_handler (signum=11)
    at /home/zack/src/mozilla/moz-central/toolkit/xre/nsSigHandlers.cpp:149
#3  0x00007ffbb53d64ef in nsProfileLock::FatalSignalHandler (signo=11)
    at nsProfileLock.cpp:216
#4  <signal handler called>
#5  IA__FcPatternDestroy (p=0x5a5a5a5a5a5a5a5a) at fcpat.c:286
#6  0x00007ffbb38dd942 in pango_fc_font_key_free (key=0x7ffb9a281a10)
    at /scratch/build-area/pango1.0-1.24.4/pango/pangofc-fontmap.c:580
#7  0x00007ffbb1f2e863 in IA__g_hash_table_foreach (hash_table=0x7ffb9a280f10, 
    func=0x7ffbb38dd970 <shutdown_font>, user_data=0x7ffb9a42bd70)
    at /tmp/buildd/glib2.0-2.20.4/glib/ghash.c:1210
#8  0x00007ffbb38dd803 in pango_fc_font_map_shutdown (fcfontmap=0x7ffb9a42bd70)
    at /scratch/build-area/pango1.0-1.24.4/pango/pangofc-fontmap.c:2015
#9  0x00007ffbb53c72f4 in MOZ_gdk_display_close (display=0x7ffbac971340)
    at /home/zack/src/mozilla/moz-central/toolkit/xre/nsAppRunner.cpp:2521
#10 0x00007ffbb53cb2ba in XRE_main (argc=<value optimized out>, 
    argv=<value optimized out>, aAppData=<value optimized out>)
    at /home/zack/src/mozilla/moz-central/toolkit/xre/nsAppRunner.cpp:3467
#11 0x0000000000401e2e in main (argc=2, argv=0x7fffaa218e18)
    at /home/zack/src/mozilla/moz-central/browser/app/nsBrowserApp.cpp:156

Clearly the problem here is that MOZ_gdk_display_close is trying to tear down the Pango font map but it is either already torn down or partially corrupted.  I don't have any idea why that might be, though.

Comment 1

[Zack Weinberg (:zwol)]

Setting a breakpoint on pango_fc_font_map_shutdown, starting firefox and then quitting it, first hit is here:

#0  pango_fc_font_map_shutdown (fcfontmap=0x7fffe67bb880)
    at /scratch/build-area/pango1.0-1.24.4/pango/pangofc-fontmap.c:2008
#1  0x00007fffecdf46bc in gfxPangoFontGroup::Shutdown ()
    at /net/heathwill/c/src/zack/mozilla/moz-frame-gc/gfx/thebes/src/gfxPangoFonts.cpp:2109
#2  0x00007fffecdfbf6e in ~gfxPlatformGtk (this=0x7fffebee9560, 
    __in_chrg=<value optimized out>)
    at /net/heathwill/c/src/zack/mozilla/moz-frame-gc/gfx/thebes/src/gfxPlatformGtk.cpp:134
#3  0x00007fffecdebf4e in gfxPlatform::Shutdown ()
    at /net/heathwill/c/src/zack/mozilla/moz-frame-gc/gfx/thebes/src/gfxPlatform.cpp:249
#4  0x00007ffff718847f in nsGenericModule::Release (this=0x7fffebebefb0)
    at nsGenericFactory.cpp:245
#5  0x00007ffff71cc4d7 in nsCOMPtr<nsIModule>::operator=(nsIModule*) ()
   from ./dist/bin/libxpcom_core.so
#6  0x00007ffff71cbc50 in nsNativeModuleLoader::ReleaserFunc (
    aHashedFile=<value optimized out>, aLoadData=...)
    at /net/heathwill/c/src/zack/mozilla/moz-frame-gc/xpcom/components/nsNativeComponentLoader.cpp:240
#7  0x00007ffff717c26b in PL_DHashTableEnumerate (table=0x7ffff278c200, 
    etor=0x7ffff71cc35e <nsBaseHashtable<nsHashableHashKey, nsNativeModuleLoader::NativeLoadData, nsNativeModuleLoader::NativeLoadData>::s_EnumStub(PLDHashTable
*, PLDHashEntryHdr*, unsigned int, void*)>, arg=0x7fffffffda00)
    at pldhash.c:754
#8  0x00007ffff71cc449 in nsBaseHashtable<nsHashableHashKey, nsNativeModuleLoader::NativeLoadData, nsNativeModuleLoader::NativeLoadData>::Enumerate(PLDHashOperator (*)(nsIHashable*, nsNativeModuleLoader::NativeLoadData&, void*), void*) ()
   from ./dist/bin/libxpcom_core.so
#9  0x00007ffff71cbc2f in nsNativeModuleLoader::UnloadLibraries (
    this=<value optimized out>)
    at /net/heathwill/c/src/zack/mozilla/moz-frame-gc/xpcom/components/nsNativeComponentLoader.cpp:281
#10 0x00007ffff71c7d2d in nsComponentManagerImpl::Shutdown (
    this=0x7ffff278c140)
    at /net/heathwill/c/src/zack/mozilla/moz-frame-gc/xpcom/components/nsComponentManager.cpp:747
#11 0x00007ffff718b0e7 in mozilla::ShutdownXPCOM (servMgr=0x7fffffffda80)
    at /net/heathwill/c/src/zack/mozilla/moz-frame-gc/xpcom/build/nsXPComInit.cpp:868
#12 0x00007ffff799c0e2 in ~ScopedXPCOMStartup (this=0x7fffffffdf90, 
    __in_chrg=<value optimized out>)
    at /net/heathwill/c/src/zack/mozilla/moz-frame-gc/toolkit/xre/nsAppRunner.cpp:1004
#13 0x00007ffff799f296 in XRE_main (argc=<value optimized out>, 
    argv=<value optimized out>, aAppData=<value optimized out>)
    at /net/heathwill/c/src/zack/mozilla/moz-frame-gc/toolkit/xre/nsAppRunner.cpp:3435
#14 0x0000000000401e4e in main (argc=1, argv=0x7fffffffe268)
    at /net/heathwill/c/src/zack/mozilla/moz-frame-gc/browser/app/nsBrowserApp.cpp:156

second hit is here:

#0  pango_fc_font_map_shutdown (fcfontmap=0x7ffff272e670)
    at /scratch/build-area/pango1.0-1.24.4/pango/pangofc-fontmap.c:2008
#1  0x00007ffff799b354 in MOZ_gdk_display_close (display=0x7ffff276b340)
    at /net/heathwill/c/src/zack/mozilla/moz-frame-gc/toolkit/xre/nsAppRunner.cpp:2526
#2  0x00007ffff799f72d in XRE_main (argc=<value optimized out>, 
    argv=<value optimized out>, aAppData=<value optimized out>)
    at /net/heathwill/c/src/zack/mozilla/moz-frame-gc/toolkit/xre/nsAppRunner.cpp:3506
#3  0x0000000000401e4e in main (argc=1, argv=0x7fffffffe268)
    at /net/heathwill/c/src/zack/mozilla/moz-frame-gc/browser/app/nsBrowserApp.cpp:156

Comment 2

[Zack Weinberg (:zwol)]

Further observations: The two calls to pango_fc_font_map_shutdown are attempting to deallocate separate fontmaps -- the first is the one used by Thebes text rendering, the second is used internally by GTK.  MOZ_gdk_display_close retrieves GTK's internal Pango context, then calls gdk_display_close, then calls pango_fc_font_map_shutdown and g_object_unref on the retrieved context.  This, in general, seems like a sketchy thing to do -- this is the library's data, not ours.  I wouldn't be surprised if gdk_display_close was doing at least some of this teardown for us, and that's why the crash is happening.

I note that http://library.gnome.org/devel/pango/stable/PangoFcFontMap.html#pango-fc-font-map-shutdown says "only intended to be called from backends", and that there are (less specific) nasty warnings in the GDK and GTK documentation to the effect that it is not necessary to tear down internal library state, there are atexit() registrations that do that for you.

System library versions are:

libfontconfig 2.6.0
libcairo      1.8.8
libpango      1.24.4
libglib       2.20.4
libgtk        2.16.4

Comment 3

[Karl Tomlinson (:karlt)]

This is http://bugzilla.gnome.org/show_bug.cgi?id=585806, which was fixed this morning:
http://git.gnome.org/cgit/pango/commit/?id=5a75ce1414f43a36a5e510cc5cfac085671bfdca

(In reply to comment #2)
> I note that
> http://library.gnome.org/devel/pango/stable/PangoFcFontMap.html#pango-fc-font-map-shutdown
> says "only intended to be called from backends",

Hmmm.  Perhaps pango_fc_font_map_cache_clear() might have been slightly better, though probably still only intended for backends.

Pango 1.22 and newer have pango_cairo_font_map_set_default(NULL), which is best for Pango 1.24 at least.  (I can't recall whether 1.22 still needed a pango-fc-font-map-shutdown to remove circular references.)

> and that there are (less specific) nasty warnings in the GDK and GTK
> documentation to the effect that it is not necessary to tear down internal
> library state, there are atexit() registrations that do that for you.

I'm not sure exactly what warnings you are referring to here.
I think the atexit registrations have been removed from GTK+/GDK, probably with good reason.  One reason might be that it is difficult to control the order of atexit handlers.

Most of the code here is only called in debug and leak test builds to check for leaks.  Without this code there are so many shutdown leaks that it makes it very difficult to notice when leak stats change.

The display close is done even in release builds to ensure that the Xserver connection is properly closed and synced before execing and creating another connection from the same process.

Comment 4

[Zack Weinberg (:zwol)]

Attachment: possible patch

So, perhaps something like this patch, then?  Apologies for the messy ifdefs, but presumably if *built* against pango <1.22 we mustn't call the function even in code that's never executed.

This fixes the crash for me, although I do get a bunch of warnings instead on shutdown:

GLib-GObject-WARNING **: IA__g_object_weak_unref: couldn't find weak ref 0x7f0414467290(0x7f03fc3d1ce0)

-- but that's really incidental, as far as I'm concerned.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 389582 [details] [diff] [review]
possible patch

Something like this, yes.

I checked Pango-1.22 and it no longer has the circular references, so pango_cairo_font_map_set_default(NULL) is fine without pango_fc_font_map_shutdown().

The main decision to make here is:
Should code compiled against Pango >= 1.22 run against Pango < 1.22?

Normally I'd assume yes, but as this is debug and memory analysis code only I think we may be OK with just the conditional compilation.

If builds against Pango >= 1.22 won't run against Pango < 1.22, then there is no need to compile the pango_fc_font_map_shutdown() code in those builds.

(If we want builds against Pango >= 1.22 to run against Pango < 1.22 then we'd need dlopen/dlsym.)

Comment 6

[Zack Weinberg (:zwol)]

(In reply to comment #5)
> I checked Pango-1.22 and it no longer has the circular references, so
> pango_cairo_font_map_set_default(NULL) is fine without
> pango_fc_font_map_shutdown().

Ok, so no need for the build/run distinction I was making, at least.

> The main decision to make here is:
> Should code compiled against Pango >= 1.22 run against Pango < 1.22?
> 
> Normally I'd assume yes, but as this is debug and memory analysis code only I
> think we may be OK with just the conditional compilation.

I'm not sure I want to make that call myself.  Sounds like a super-reviewer question ... who would you say is qualified?

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to comment #6)
> I'm not sure I want to make that call myself.  Sounds like a super-reviewer
> question ... who would you say is qualified?

This is toolkit/xre code so bsmedberg is probably a good person to ask.

CC'ing dbaron as ISTR some talk of having NS_BUILD_REFCNT_LOGGING in release builds.

Comment 8

[Zack Weinberg (:zwol)]

Unfortunately, with the current patch, "make reftest" crashes in FcFini():

firefox-bin: fccache.c:512: FcCacheFini: Assertion `fcCacheChains[i] == ((void *)0)' failed.

Program /home/zack/src/mozilla/obj-mozframegc/dist/bin/firefox-bin (pid = 328) received signal 6.
Stack:
UNKNOWN [/lib/libpthread.so.0 +0x0000E720]
gsignal+0x00000035 [/lib/libc.so.6 +0x00031D25]
abort+0x00000181 [/lib/libc.so.6 +0x00034DE1]
__assert_fail+0x000000E9 [/lib/libc.so.6 +0x0002AF99]
UNKNOWN [/usr/lib/libfontconfig.so.1 +0x00007F47]
FcFini+0x00000025 [/usr/lib/libfontconfig.so.1 +0x00011815]
UNKNOWN [/home/zack/src/mozilla/obj-mozframegc/dist/bin/libxul.so +0x0001B47C]
XRE_main+0x000016F7 [/home/zack/src/mozilla/obj-mozframegc/dist/bin/libxul.so +0x0001F86D]
UNKNOWN [/home/zack/src/mozilla/obj-mozframegc/dist/bin/firefox-bin +0x00001E4E]
__libc_start_main+0x000000E6 [/lib/libc.so.6 +0x0001E5C6]
UNKNOWN [/home/zack/src/mozilla/obj-mozframegc/dist/bin/firefox-bin +0x00001BA9]

(I don't know why our stack tracer is so bad at finding names...)

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to comment #8)
> firefox-bin: fccache.c:512: FcCacheFini: Assertion `fcCacheChains[i] == ((void
> *)0)' failed.

I'm not sure exactly what that assertion is, but the problem may be leaking fontconfig objects from bug 487971 for example.

> (I don't know why our stack tracer is so bad at finding names...)

http://mxr.mozilla.org/mozilla-central/source/tools/rb/fix-linux-stack.pl
is often useful.

Comment 10

[Zack Weinberg (:zwol)]

(In reply to comment #9)
> 
> I'm not sure exactly what that assertion is, but the problem may be leaking
> fontconfig objects from bug 487971 for example.

I do not see the assertion with your patch for 487971 in my tree, so that's hopeful.

Comment 11

[Zack Weinberg (:zwol)]

I was wrong.  If the browser instance has done enough work (the threshold is somewhere in between "load the minefield start page" and "run content/base/test reftests"), the assertion still happens.

Comment 12

[Zack Weinberg (:zwol)]

Er, I meant mochitests.

Comment 13

[Zack Weinberg (:zwol)]

Here's the fix-linux-stack.pl'ified trace -- doesn't really tell us anything we didn't know, though.

__restore_rt (/lib/libpthread.so.0)
raise (/lib/libc.so.6)
abort (/lib/libc.so.6)
__assert_fail (/lib/libc.so.6)
FcCacheFini (/build/buildd/fontconfig-2.6.0/src/fccache.c:513)
IA__FcFini (/build/buildd/fontconfig-2.6.0/src/fcinit.c:144)
MOZ_gdk_display_close (/home/zack/src/mozilla/moz-central/toolkit/xre/nsAppRunner.cpp:2541)
XRE_main (/home/zack/src/mozilla/moz-central/toolkit/xre/nsAppRunner.cpp:3528)
main (/home/zack/src/mozilla/moz-central/browser/app/nsBrowserApp.cpp:157)
__libc_start_main (/lib/libc.so.6)
_start (/build/buildd/eglibc-2.9/csu/../sysdeps/x86_64/elf/start.S:116)

Comment 14

[Zack Weinberg (:zwol)]

Attachment: another possible patch

Here's another possibility, which gets rid of the runtime check for Pango version and the call to FcFini() (which latter was crashing on an assertion inside libfontconfig).  Unfortunately it's not good enough -- we now crash on an assertion inside cairo_debug_reset_static_data.  GDB backtrace:

#4  <signal handler called>
#5  0x00007f7a6161ed25 in *__GI_raise (sig=<value optimized out>)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#6  0x00007f7a61621de1 in *__GI_abort () at abort.c:88
#7  0x00007f7a61617f99 in *__GI___assert_fail (
    assertion=0x7f7a6308e035 "hash_table->live_entries == 0", 
    file=0x7f7a6308df40 "/tmp/buildd/cairo-1.8.8/src/cairo-hash.c", line=199, 
    function=0x7f7a6308e330 "_cairo_hash_table_destroy") at assert.c:78
#8  0x00007f7a6303f891 in _cairo_hash_table_destroy (hash_table=0x7f7a4acb8ee0)
    at /tmp/buildd/cairo-1.8.8/src/cairo-hash.c:199
#9  0x00007f7a63050a97 in _cairo_scaled_font_map_destroy ()
    at /tmp/buildd/cairo-1.8.8/src/cairo-scaled-font.c:392
#10 0x00007f7a6303b209 in _cairo_font_face_reset_static_data ()
    at /tmp/buildd/cairo-1.8.8/src/cairo-font-face.c:767
#11 0x00007f7a6303b049 in cairo_debug_reset_static_data ()
    at /tmp/buildd/cairo-1.8.8/src/cairo-debug.c:64
#12 0x00007f7a65de0108 in MOZ_gdk_display_close (display=0x7f7a5d372340)
    at /home/zack/src/mozilla/moz-central/toolkit/xre/nsAppRunner.cpp:2549
#13 0x00007f7a65de44ef in XRE_main (argc=<value optimized out>, 
    argv=<value optimized out>, aAppData=<value optimized out>)
    at /home/zack/src/mozilla/moz-central/toolkit/xre/nsAppRunner.cpp:3514
#14 0x0000000000401e2e in main (argc=5, argv=0x7fff38702c08)
    at /home/zack/src/mozilla/moz-central/browser/app/nsBrowserApp.cpp:156

This is system cairo, as you can see, but the code in our copy is pretty close; it looks like something is still hanging onto a reference to a #cairo_scaled_font_t.  Digging into the hash table a little...

(gdb) p *hash_table
$2 = {keys_equal = 0x7f7a6304fd10 <_cairo_scaled_font_keys_equal>, 
  arrangement = 0x7f7a6308e060, entries = 0x7f7a5d3d73a0, live_entries = 2, 
  iterating = 0}
(gdb) p *hash_table->arrangement
$3 = {high_water_mark = 16, size = 43, rehash = 41}
(gdb) p hash_table->entries[0]@43
$4 = {0x0 <repeats 13 times>, 0x7f7a4ac899f0, 0x0 <repeats 12 times>, 
  0x7f7a3a4e9a10, 0x0 <repeats 16 times>}
(gdb) p *(cairo_scaled_font_t *)(hash_table->entries[13])
$6 = {hash_entry = {hash = 3518642326}, status = CAIRO_STATUS_SUCCESS, 
  ref_count = {ref_count = 2}, user_data = {size = 0, num_elements = 0, 
    element_size = 24, elements = 0x0, is_snapshot = 0}, 
  font_face = 0x7f7a4acaa0a0, font_matrix = {xx = 13.611328125, yx = 0, 
    xy = 0, yy = 13.611328125, x0 = 0, y0 = 0}, ctm = {xx = 1, yx = 0, xy = 0, 
    yy = 1, x0 = 0, y0 = 0}, options = {antialias = CAIRO_ANTIALIAS_SUBPIXEL, 
    subpixel_order = CAIRO_SUBPIXEL_ORDER_RGB, 
    hint_style = CAIRO_HINT_STYLE_FULL, hint_metrics = CAIRO_HINT_METRICS_ON}, 
  placeholder = 0, finished = 0, scale = {xx = 13.611328125, yx = 0, xy = 0, 
    yy = 13.611328125, x0 = 0, y0 = 0}, scale_inverse = {
    xx = 0.073468216386856078, yx = 0, xy = 0, yy = 0.073468216386856078, 
    x0 = -0, y0 = -0}, max_scale = 13.611328125, extents = {ascent = 13, 
    descent = 4, height = 16, max_x_advance = 24, max_y_advance = 0}, mutex = {
    __data = {__lock = 0, __count = 0, __owner = 0, __nusers = 0, __kind = 0, 
      __spins = 0, __list = {__prev = 0x0, __next = 0x0}}, 
    __size = '\0' <repeats 39 times>, __align = 0}, glyphs = 0x7f7a4acb8f70, 
  surface_backend = 0x0, surface_private = 0x0, backend = 0x7f7a632a5b40}

... and this is as far as I get; nowhere in this data structure can I find something which tells me which font this is or who might be hanging on to a reference. :-(

Comment 15

[Zack Weinberg (:zwol)]

I have now figured out that a human-readable name of the font is hiding in ((cairo_ft_scaled_font_t *)hash_table->entries[N])->unscaled->filename.

Unfortunately, the names of the two fonts still in this hash table are VeraSe.ttf and DejaVuSans.ttf; these are system default fonts.  As such, there is no obvious place to look for the stale references.  One of them has two references, the other has just one, and I don't know if the reference from the hashtable itself is counted in ->ref_count.

Comment 16

[Zack Weinberg (:zwol)]

Attachment: for reals patch

I think this is the right patch for the bug in nsAppRunner.cpp.  It does four things: first, if compiled against pango >=1.22, call pango_cairo_font_map_set_default(NULL) instead of pango_fc_font_map_shutdown; second, in the same circumstances, call FcFini() after calling cairo_debug_reset_static_data(); third, chainsaw out a bunch of obsolete bug workarounds since we no longer accept gtk < 2.10 or cairo < 1.6 (this is not strictly necessary, but makes the code a heck of a lot easier to read); fourth, observe that the Qt theme bug workaround amounts to skipping everything in this function if Qt is detected, so just return early (again, not strictly necessary but makes things easier to read).

With this patch applied I can run *most* of reftests and not crash on shutdown.  The problem I described in comment #14 still exists, but I've managed to figure out that it's caused by the test plugin, so I think that would best be treated as a separate bug (and will file that bug shortly).

Comment 17

[Zack Weinberg (:zwol)]

The problem with the test plugin is now bug 508149.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 392370 [details] [diff] [review]
for reals patch

(In reply to comment #16)
> third, chainsaw out a bunch of obsolete bug workarounds since we no longer
> accept [...] cairo < 1.6

Note that the cairo here is the cairo used by gtk and pango, about which Gecko
makes no requirements.  Our reference platform is CentOS 5.0, which has
(gtk+-2.10.4 but) cairo-1.2.4.  (configure only requires cairo-1.6 when using
--enable-system-cairo - and we actually need a newer version than that.)

http://vault.centos.org/5.0/os/i386/CentOS/

If it helps, pango-1.22.4 requires cairo-1.7.6, so no need to worry about old
cairos if USE_FONT_MAP_SET_DEFAULT.

>+#  include <pango/pangocairo.h>
>+#  include <pango/pangofc-fontmap.h>
>+#  if PANGO_VERSION_CHECK(1,22,0)
>+#    define USE_FONT_MAP_SET_DEFAULT 1
>+#  else
>+#    define USE_FONT_MAP_SET_DEFAULT 0
>+#  endif

This adds a dependency on pangocairo, which should really be added to
PKG_CHECK_MODULES here:

http://hg.mozilla.org/mozilla-central/file/8ea173d78d18/configure.in#l5145

>+// XXXX There shouldn't be two copies of the library to worry about.

I don't think this comment is helpful (without referencing a bug proposing how
to only have one library).  I think cairo development is still happening too
fast for us to rely on any particular versions to be available on the system,
and, provided use of the two cairo's is not mixed, the current situation is
not causing a problem.

>+  // has a default display.  Only do this if we're *not* using the 1.24
>+  // method.

1.22 method.

>+  if (CLEANUP_MEMORY && !USE_FONT_MAP_SET_DEFAULT)

>+  if (USE_FONT_MAP_SET_DEFAULT) {
>+    // If possible (Pango 1.24 and later) simply do this, which has
>+    // the effect of calling pango_fc_font_map_shutdown plus
>+    // releases Pango's reference to the destroyed fontmap.
>+    pango_cairo_font_map_set_default(NULL);
>+  } else if (pangoContext) {

>+  if (USE_FONT_MAP_SET_DEFAULT)

It would make more sense to use preprocessor conditionals rather than runtime C++
conditionals to test preprocessor identifiers.

1.24 -> 1.22.

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 392370 [details] [diff] [review]
for reals patch

>+  if (USE_FONT_MAP_SET_DEFAULT)
>+    FcFini();

I think we're only going to be able to call FcFini here with Pango-1.24.4 or newer.  (See bug 506135 comment 3.)

Comment 20

[Karl Tomlinson (:karlt)]

(In reply to comment #19)
> I think we're only going to be able to call FcFini here with Pango-1.24.4 or
> newer.  (See bug 506135 comment 3.)

Probably ignore this comment, sorry.  I was thinking that the fix for bug 487971 was needed for calling FcFini, but actually, with the way we use the font map involved in bug 487971, it looks like the pango_fc_font_map_shutdown in gfxPangoFontGroup::Shutdown() should be enough even if that font map gets leaked.
I guess shutting down cairo before called FcFini fixed the assert in FcFini.

Comment 21

[Zack Weinberg (:zwol)]

(In reply to comment #18)
> (From update of attachment 392370 [details] [diff] [review])
> (In reply to comment #16)
> > third, chainsaw out a bunch of obsolete bug workarounds since we no longer
> > accept [...] cairo < 1.6
> 
> Note that the cairo here is the cairo used by gtk and pango, about which Gecko
> makes no requirements.  Our reference platform is CentOS 5.0, which has
> (gtk+-2.10.4 but) cairo-1.2.4.  (configure only requires cairo-1.6 when using
> --enable-system-cairo - and we actually need a newer version than that.)
> 
> http://vault.centos.org/5.0/os/i386/CentOS/

(checks) And CentOS 5.0 isn't even that old.  Alright, I'll put the workaround back.  What do you think about just not bothering to do this teardown with system cairo <1.4?  It's debugging code only, and it would make the control flow a lot simpler.

> If it helps, pango-1.22.4 requires cairo-1.7.6, so no need to worry about old
> cairos if USE_FONT_MAP_SET_DEFAULT.

Not really; the two workarounds are orthogonal.
 
> >+#  include <pango/pangocairo.h>
> >+#  include <pango/pangofc-fontmap.h>
> >+#  if PANGO_VERSION_CHECK(1,22,0)
> >+#    define USE_FONT_MAP_SET_DEFAULT 1
> >+#  else
> >+#    define USE_FONT_MAP_SET_DEFAULT 0
> >+#  endif
> 
> This adds a dependency on pangocairo, which should really be added to
> PKG_CHECK_MODULES here:

That's kinda awkward, since the header-file dependency is (well, could be) conditional on pango-core >=1.22 anyway, I don't know how to express that to PKG_CHECK_MODULES.  And it works for me without messing with the link...  

(Which version would be required?)

> >+// XXXX There shouldn't be two copies of the library to worry about.
> 
> I don't think this comment is helpful (without referencing a bug proposing how
> to only have one library).  I think cairo development is still happening too
> fast for us to rely on any particular versions to be available on the system,
> and, provided use of the two cairo's is not mixed, the current situation is
> not causing a problem.

Was just grumping, will remove.

> >+  // has a default display.  Only do this if we're *not* using the 1.24
> >+  // method.
> 
> 1.22 method.

corrected.

> >+  if (USE_FONT_MAP_SET_DEFAULT)
> 
> It would make more sense to use preprocessor conditionals rather than runtime
> C++ conditionals to test preprocessor identifiers.

I disagree here; a mishmash of preprocessor and runtime conditionals is very hard to read.  We can't get rid of all the runtime conditionals in this function, so it is better (more readable) to convert the preprocessor conditionals to regular if statements.  The compiler will optimize them out since their controlling expressions are constant.  (This is why I take care to #define the constant in both branches of the #if at the top of the file.)

> 1.24 -> 1.22.

also corrected.

Comment 22

[Zack Weinberg (:zwol)]

Attachment: patch v2

I take back what I said about mixing #if with if() ... we need preprocessor conditionals for CLEANUP_MEMORY.  When CLEANUP_MEMORY==0, the pango headers are not included, so the code conditional on CLEANUP_MEMORY would get compile errors if I kept using regular if().

This revision puts that back and also restores the cairo <1.4.0 hack (sort of ... we call gdk_display_close, but we then skip all the rest of the cleanup).

I haven't done anything about adding libpangocairo to configure.in because I'm not sure how to do that, see previous comments.

Comment 23

[Zack Weinberg (:zwol)]

> > If it helps, pango-1.22.4 requires cairo-1.7.6, so no need to worry
> > about old cairos if USE_FONT_MAP_SET_DEFAULT.
>
> Not really; the two workarounds are orthogonal.

What I meant by that was, the code for the cairo <1.4.0 workaround and the code for the pango <1.22.4 cleanup path don't overlap much, so it wouldn't help to move one inside the other.

Comment 24

[Zack Weinberg (:zwol)]

Attachment: patch v2 (really this time)

forgot to hg qrefresh before uploading patch, sorry about that.

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 392556 [details] [diff] [review]
patch v2 (really this time)

(In reply to comment #21)
> What do you think about just not bothering to do this teardown with
> system cairo <1.4?  It's debugging code only, and it would make the control
> flow a lot simpler.

This debugging code gets run on our leak test machines.  Skipping
cairo_debug_reset_static_data() would make our Lk stats increase
considerably, which I'd prefer to avoid.

> > >+#  include <pango/pangocairo.h>
> > 
> > This adds a dependency on pangocairo, which should really be added to
> > PKG_CHECK_MODULES here:
> 
> That's kinda awkward, since the header-file dependency is (well, could be)
> conditional on pango-core >=1.22 anyway,

(I see the header inclusion is now conditional.)
It's probably not worth worrying about making pangocairo conditional on 1.22
in configure.in, but it's more correct to add the dependency.  (It probably
works because gtk dependencies pull in pangocairo, but better to be explicit.)

> I don't know how to express that to PKG_CHECK_MODULES.
 
> (Which version would be required?)

Just add "pangocairo >= $PANGO_VERSION" to the PKG_CHECK_MODULES line, like it
was before this change:

http://hg.mozilla.org/mozilla-central/diff/71004cbe3d8a/configure.in

We actually need it when 'test "$MOZ_PANGO"' fails too
(so the MOZ_PANGO_* logic actually doesn't need to be in the conditional).

>+#  include <pango/pango.h>

What is this for?

>+#  if PANGO_VERSION_CHECK(1,22,4)

AFAIK PANGO_VERSION_CHECK(1,22,0) was fine here (and in comments).  Sorry if I
confused things with my unhelpful comment 19.

Comment 26

[Zack Weinberg (:zwol)]

With the very latest non-experimental Pango (1.24.5) this crash has gone away even without this patch -- but this patch still triggers the crash described in bug 508149.

We should still do this patch someday, since it does let us call FcFini(), but it's not a priority for me anymore.  karlt thinks there might be Pango bugs that provoke bug 508149 -- maybe we need to condition the use of pango_cairo_font_map_set_default on some future Pango version...

Comment 27

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1552613