[cairo] Crash in glyph cache

[Behdad Esfahbod]

Hi,

If you grab fontconfig master and pango master, the resulting pangocairo
library is supposed to be threadsafe. However, I'm seeing crashes when I run
pango/tests/test-pangocairo-threads with arguments "200 10000", which means
create 200 threads, and in each one, render a certain line of text at 10000
different font sizes.

When catching the crashes in gdb, the last few times I've seen this backtrace
in cairo:

#0 0x00007ffff6b65425 in __GI_raise (sig=<optimized out>)
at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1 0x00007ffff6b68b8b in __GI_abort () at abort.c:91
#2 0x00007ffff6b5e0ee in __assert_fail_base (fmt=<optimized out>,
assertion=0x7ffff774dbf6 "!\"reached\"", file=0x7ffff774faf8 "cairo-hash.c",
line=<optimized out>, function=<optimized out>) at assert.c:94
#3 0x00007ffff6b5e192 in __GI___assert_fail (assertion=0x7ffff774dbf6
"!\"reached\"",
file=0x7ffff774faf8 "cairo-hash.c", line=506,
function=0x7ffff774fd80 <__PRETTY_FUNCTION__.10233>
"_cairo_hash_table_lookup_exact_key")
at assert.c:103
#4 0x00007ffff76b13a0 in _cairo_hash_table_lookup_exact_key (key=<optimized
out>,
hash_table=<optimized out>) at cairo-hash.c:506
#5 _cairo_hash_table_remove (hash_table=<optimized out>, key=<optimized out>)
at cairo-hash.c:523
#6 0x00007ffff76e1148 in _cairo_scaled_glyph_page_destroy (closure=0x127c52860)
at cairo-scaled-font.c:463
#7 0x00007ffff76a31ec in _cairo_cache_remove_random (
cache=0x7ffff7982700 <cairo_scaled_glyph_page_cache>) at cairo-cache.c:223
#8 _cairo_cache_shrink_to_accommodate (cache=0x7ffff7982700
<cairo_scaled_glyph_page_cache>,
additional=0) at cairo-cache.c:243
#9 0x00007ffff76e16c2 in _cairo_scaled_font_thaw_cache (scaled_font=0x1108fc310)
at cairo-scaled-font.c:795
#10 0x00007ffff76e43cc in INT_cairo_scaled_font_glyph_extents
(scaled_font=0x1108fc310,
glyphs=0x7fffd3bf46a0, num_glyphs=<optimized out>, extents=0x7fffd3bf4640)
at cairo-scaled-font.c:1635
#11 0x00007ffff798a389 in compute_glyph_extents (entry=0x13239ea08, glyph=55,
cf_priv=0x1d86bba8) at ../../pango/pangocairo-font.c:765


Now, it's possible that there's memory corruption going on somewhere else, but
that's not hugely likely. Anyone feels like / competent enough to look into this?

Cheers,

[Chris Wilson]

If we need to reap the global cache, this will call back into the scaled
font to free the glyph page. We therefore need to be careful not to run
concurrently with a user adding to the glyph page, ergo we need locking.
To complicate matters we need to be wary of a lock-inversion as we hold
the scaled_font lock whilst thawing the global cache. We prevent the
deadlock by careful ordering of the thaw-unlock and by inspecting the
current frozen state of the scaled-font before releasing the glyph
page.

Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
---
src/cairo-scaled-font.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index dc6a6fd..e1cb095 100644
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -449,6 +449,7 @@ _cairo_scaled_font_map_destroy (void)
CLEANUP_MUTEX_LOCK:
CAIRO_MUTEX_UNLOCK (_cairo_scaled_font_map_mutex);
}
+
static void
_cairo_scaled_glyph_page_destroy (void *closure)
{
@@ -459,11 +460,16 @@ _cairo_scaled_glyph_page_destroy (void *closure)
assert (! cairo_list_is_empty (&page->link));

scaled_font = (cairo_scaled_font_t *) page->cache_entry.hash;
+ assert (!scaled_font->cache_frozen);
+ assert (!scaled_font->global_cache_frozen);
+
+ CAIRO_MUTEX_LOCK(scaled_font->mutex);
for (n = 0; n < page->num_glyphs; n++) {
_cairo_hash_table_remove (scaled_font->glyphs,
&page->glyphs[n].hash_entry);
_cairo_scaled_glyph_fini (scaled_font, &page->glyphs[n]);
}
+ CAIRO_MUTEX_UNLOCK(scaled_font->mutex);

cairo_list_del (&page->link);

@@ -788,16 +794,14 @@ _cairo_scaled_font_freeze_cache (cairo_scaled_font_t *scaled_font)
void
_cairo_scaled_font_thaw_cache (cairo_scaled_font_t *scaled_font)
{
- scaled_font->cache_frozen = FALSE;
-
if (scaled_font->global_cache_frozen) {
CAIRO_MUTEX_LOCK (_cairo_scaled_glyph_page_cache_mutex);
_cairo_cache_thaw (&cairo_scaled_glyph_page_cache);
CAIRO_MUTEX_UNLOCK (_cairo_scaled_glyph_page_cache_mutex);
-
scaled_font->global_cache_frozen = FALSE;
}

+ scaled_font->cache_frozen = FALSE;
CAIRO_MUTEX_UNLOCK (scaled_font->mutex);
}

[Adrian Johnson]

I tested the patch with the test case in bug 54950 and got this:

cairo-scaled-font.c:464: _cairo_scaled_glyph_page_destroy: Assertion
`!scaled_font->global_cache_frozen' failed.
Aborted

Post by Chris Wilson
diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index dc6a6fd..e1cb095 100644
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -449,6 +449,7 @@ _cairo_scaled_font_map_destroy (void)
CAIRO_MUTEX_UNLOCK (_cairo_scaled_font_map_mutex);
}
+
static void
_cairo_scaled_glyph_page_destroy (void *closure)
{
@@ -459,11 +460,16 @@ _cairo_scaled_glyph_page_destroy (void *closure)
assert (! cairo_list_is_empty (&page->link));
scaled_font = (cairo_scaled_font_t *) page->cache_entry.hash;
+ assert (!scaled_font->cache_frozen);
+ assert (!scaled_font->global_cache_frozen);
+
+ CAIRO_MUTEX_LOCK(scaled_font->mutex);
for (n = 0; n < page->num_glyphs; n++) {
_cairo_hash_table_remove (scaled_font->glyphs,
&page->glyphs[n].hash_entry);
_cairo_scaled_glyph_fini (scaled_font, &page->glyphs[n]);
}
+ CAIRO_MUTEX_UNLOCK(scaled_font->mutex);
cairo_list_del (&page->link);
@@ -788,16 +794,14 @@ _cairo_scaled_font_freeze_cache (cairo_scaled_font_t *scaled_font)
void
_cairo_scaled_font_thaw_cache (cairo_scaled_font_t *scaled_font)
{
- scaled_font->cache_frozen = FALSE;
-
if (scaled_font->global_cache_frozen) {
CAIRO_MUTEX_LOCK (_cairo_scaled_glyph_page_cache_mutex);
_cairo_cache_thaw (&cairo_scaled_glyph_page_cache);
CAIRO_MUTEX_UNLOCK (_cairo_scaled_glyph_page_cache_mutex);
-
scaled_font->global_cache_frozen = FALSE;
}
+ scaled_font->cache_frozen = FALSE;
CAIRO_MUTEX_UNLOCK (scaled_font->mutex);
}
-- 1.7.10.4

[Chris Wilson]

Post by Adrian Johnson
cairo-scaled-font.c:464: _cairo_scaled_glyph_page_destroy: Assertion
`!scaled_font->global_cache_frozen' failed.
Aborted

Which turned out to be a different bug, where we leaked the frozen_count
on the global glyph cache, effectively allowing us to hold open all
glyphs for all fonts.

commit 44a093eb95c950b0e8f2d7d1cdb9719cb8a550f7
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Tue Jan 8 14:58:41 2013 +0000

scaled-font: Always hold the mutex even for single glyph probes

Then the deadlock was indeed a recursion through a recording_surface
beneath the glyph:

commit 4d4bf8fddff49d349e03282ffa827f6f4659e3fe
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Tue Jan 8 14:56:07 2013 +0000

scaled-font: Free the cached glyphs from the font before taking the
global lock

And I added more assertions to make sure we don't leak the global glyph
cache again. Thanks,
-Chris