[cairo] [PATCH] Fix a 'memory leak' in the image compositor

[Uli Schlachter]

There is a global pixman_glyph_cache_t instance that is initialized on
first use and shows up in valgrind output as a relatively large leak (I
think it was about 200 KiB). The reason for this is that this cache is
not freed by cairo_debug_reset_static_data().

This commit wires up freeing the cache to
cairo_debug_reset_static_data().

This cache was introduced in commit 615205cf0729 from 2012.

Signed-off-by: Uli Schlachter <***@znc.in>
---
src/cairo-debug.c | 2 ++
src/cairo-image-compositor.c | 17 +++++++++++++++++
src/cairoint.h | 3 +++
3 files changed, 22 insertions(+)

diff --git a/src/cairo-debug.c b/src/cairo-debug.c
index 10933a673..6005060d4 100644
--- a/src/cairo-debug.c
+++ b/src/cairo-debug.c
@@ -86,6 +86,8 @@ cairo_debug_reset_static_data (void)

_cairo_image_reset_static_data ();

+ _cairo_image_compositor_reset_static_data ();
+
#if CAIRO_HAS_DRM_SURFACE
_cairo_drm_device_reset_static_data ();
#endif
diff --git a/src/cairo-image-compositor.c b/src/cairo-image-compositor.c
index 9f4e0adad..122a8ca42 100644
--- a/src/cairo-image-compositor.c
+++ b/src/cairo-image-compositor.c
@@ -820,6 +820,18 @@ get_glyph_cache (void)
return global_glyph_cache;
}

+void
+_cairo_image_compositor_reset_static_data (void)
+{
+ CAIRO_MUTEX_LOCK (_cairo_glyph_cache_mutex);
+
+ if (global_glyph_cache)
+ pixman_glyph_cache_destroy (global_glyph_cache);
+ global_glyph_cache = NULL;
+
+ CAIRO_MUTEX_UNLOCK (_cairo_glyph_cache_mutex);
+}
+
void
_cairo_image_scaled_glyph_fini (cairo_scaled_font_t *scaled_font,
cairo_scaled_glyph_t *scaled_glyph)
@@ -945,6 +957,11 @@ out_unlock:
return status;
}
#else
+void
+_cairo_image_compositor_reset_static_data (void)
+{
+}
+
void
_cairo_image_scaled_glyph_fini (cairo_scaled_font_t *scaled_font,
cairo_scaled_glyph_t *scaled_glyph)
diff --git a/src/cairoint.h b/src/cairoint.h
index 051e4f805..11f2c1eaf 100644
--- a/src/cairoint.h
+++ b/src/cairoint.h
@@ -1586,6 +1586,9 @@ _cairo_image_scaled_glyph_fini (cairo_scaled_font_t *scaled_font,
cairo_private void
_cairo_image_reset_static_data (void);

+cairo_private void
+_cairo_image_compositor_reset_static_data (void);
+
cairo_private cairo_surface_t *
_cairo_image_surface_create_with_pixman_format (unsigned char *data,
pixman_format_code_t pixman_format,

[Bryce Harrington]

Post by Uli Schlachter
There is a global pixman_glyph_cache_t instance that is initialized on
first use and shows up in valgrind output as a relatively large leak (I
think it was about 200 KiB). The reason for this is that this cache is
not freed by cairo_debug_reset_static_data().
This commit wires up freeing the cache to
cairo_debug_reset_static_data().
This cache was introduced in commit 615205cf0729 from 2012.

Offhand looks good to me. Have you run this against the test suite?

Post by Uli Schlachter
---
src/cairo-debug.c | 2 ++
src/cairo-image-compositor.c | 17 +++++++++++++++++
src/cairoint.h | 3 +++
3 files changed, 22 insertions(+)
diff --git a/src/cairo-debug.c b/src/cairo-debug.c
index 10933a673..6005060d4 100644
--- a/src/cairo-debug.c
+++ b/src/cairo-debug.c
@@ -86,6 +86,8 @@ cairo_debug_reset_static_data (void)
_cairo_image_reset_static_data ();
+ _cairo_image_compositor_reset_static_data ();
+
#if CAIRO_HAS_DRM_SURFACE
_cairo_drm_device_reset_static_data ();
#endif
diff --git a/src/cairo-image-compositor.c b/src/cairo-image-compositor.c
index 9f4e0adad..122a8ca42 100644
--- a/src/cairo-image-compositor.c
+++ b/src/cairo-image-compositor.c
@@ -820,6 +820,18 @@ get_glyph_cache (void)
return global_glyph_cache;
}
+void
+_cairo_image_compositor_reset_static_data (void)
+{
+ CAIRO_MUTEX_LOCK (_cairo_glyph_cache_mutex);
+
+ if (global_glyph_cache)
+ pixman_glyph_cache_destroy (global_glyph_cache);
+ global_glyph_cache = NULL;
+
+ CAIRO_MUTEX_UNLOCK (_cairo_glyph_cache_mutex);
+}
+
void
_cairo_image_scaled_glyph_fini (cairo_scaled_font_t *scaled_font,
cairo_scaled_glyph_t *scaled_glyph)
return status;
}
#else
+void
+_cairo_image_compositor_reset_static_data (void)
+{
+}
+
void
_cairo_image_scaled_glyph_fini (cairo_scaled_font_t *scaled_font,
cairo_scaled_glyph_t *scaled_glyph)
diff --git a/src/cairoint.h b/src/cairoint.h
index 051e4f805..11f2c1eaf 100644
--- a/src/cairoint.h
+++ b/src/cairoint.h
@@ -1586,6 +1586,9 @@ _cairo_image_scaled_glyph_fini (cairo_scaled_font_t *scaled_font,
cairo_private void
_cairo_image_reset_static_data (void);
+cairo_private void
+_cairo_image_compositor_reset_static_data (void);
+
cairo_private cairo_surface_t *
_cairo_image_surface_create_with_pixman_format (unsigned char *data,
pixman_format_code_t pixman_format,
--
2.15.1
--
cairo mailing list
https://lists.cairographics.org/mailman/listinfo/cairo

[Uli Schlachter]

Post by Bryce Harrington

Post by Uli Schlachter
There is a global pixman_glyph_cache_t instance that is initialized on
first use and shows up in valgrind output as a relatively large leak (I
think it was about 200 KiB). The reason for this is that this cache is
not freed by cairo_debug_reset_static_data().
This commit wires up freeing the cache to
cairo_debug_reset_static_data().
This cache was introduced in commit 615205cf0729 from 2012.

Offhand looks good to me. Have you run this against the test suite?

Not really, no. I think I only ran this against one specific test to see
if valgrind is content. Any reason that you expect this to go wrong?

Uli

[Uli Schlachter]

Hi Bryce,

Post by Uli Schlachter

Post by Bryce Harrington

Post by Uli Schlachter
There is a global pixman_glyph_cache_t instance that is initialized on
first use and shows up in valgrind output as a relatively large leak (I
think it was about 200 KiB). The reason for this is that this cache is
not freed by cairo_debug_reset_static_data().
This commit wires up freeing the cache to
cairo_debug_reset_static_data().
This cache was introduced in commit 615205cf0729 from 2012.

Offhand looks good to me. Have you run this against the test suite?

Not really, no. I think I only ran this against one specific test to see
if valgrind is content. Any reason that you expect this to go wrong?

Okay, I ran this against the test suite with CAIRO_TEST_TARGET=image.

Results with and without this patch on latest master:

491 Passed, 60 Failed [0 crashed, 14 expected], 27 Skipped
Preamble: 1 failed - font-variations
image (argb32): 43 failed - clip-operator clip-text coverage-rhombus
extended-blend-alpha extended-blend-mask extended-blend-alpha-mask
extended-blend-solid-alpha fallback halo halo-transform operator-clear
operator-source radial-gradient radial-gradient-source
radial-gradient-mask-source record-select-font-face
record-text-transform record1414x-select-font-face
record1414x-text-transform record2x-select-font-face
record2x-text-transform record90-select-font-face
record90-text-transform recordflip-whole-select-font-face
recordflip-whole-text-transform recordflip-select-font-face
recordflip-text-transform smask smask-text text-antialias-subpixel
text-antialias-subpixel-rgb text-antialias-subpixel-bgr
text-antialias-subpixel-vrgb text-antialias-subpixel-vbgr text-pattern
text-unhinted-metrics unbounded-operator user-font-proxy
user-font-rescale pthread-same-source pthread-show-text
ft-text-vertical-layout-type1 ft-text-antialias-none
image (rgb24): 45 failed - clip-operator clip-text coverage-rhombus
extended-blend extended-blend-alpha extended-blend-mask
extended-blend-alpha-mask extended-blend-solid
extended-blend-solid-alpha fallback halo halo-transform operator-clear
operator-source radial-gradient radial-gradient-source
radial-gradient-mask-source record-select-font-face
record-text-transform record1414x-select-font-face
record1414x-text-transform record2x-select-font-face
record2x-text-transform record90-select-font-face
record90-text-transform recordflip-whole-select-font-face
recordflip-whole-text-transform recordflip-select-font-face
recordflip-text-transform smask smask-text text-antialias-subpixel
text-antialias-subpixel-rgb text-antialias-subpixel-bgr
text-antialias-subpixel-vrgb text-antialias-subpixel-vbgr text-pattern
text-unhinted-metrics unbounded-operator user-font-proxy
user-font-rescale pthread-same-source pthread-show-text
ft-text-vertical-layout-type1 ft-text-antialias-none

Is this enough or do you want something more?

Cheers,
Uli

[Bryce Harrington]

Post by Uli Schlachter
Hi Bryce,

Post by Uli Schlachter

Post by Bryce Harrington

Post by Uli Schlachter
There is a global pixman_glyph_cache_t instance that is initialized on
first use and shows up in valgrind output as a relatively large leak (I
think it was about 200 KiB). The reason for this is that this cache is
not freed by cairo_debug_reset_static_data().
This commit wires up freeing the cache to
cairo_debug_reset_static_data().
This cache was introduced in commit 615205cf0729 from 2012.

Offhand looks good to me. Have you run this against the test suite?

Not really, no. I think I only ran this against one specific test to see
if valgrind is content. Any reason that you expect this to go wrong?

Okay, I ran this against the test suite with CAIRO_TEST_TARGET=image.
491 Passed, 60 Failed [0 crashed, 14 expected], 27 Skipped
Preamble: 1 failed - font-variations
image (argb32): 43 failed - clip-operator clip-text coverage-rhombus
extended-blend-alpha extended-blend-mask extended-blend-alpha-mask
extended-blend-solid-alpha fallback halo halo-transform operator-clear
operator-source radial-gradient radial-gradient-source
radial-gradient-mask-source record-select-font-face
record-text-transform record1414x-select-font-face
record1414x-text-transform record2x-select-font-face
record2x-text-transform record90-select-font-face
record90-text-transform recordflip-whole-select-font-face
recordflip-whole-text-transform recordflip-select-font-face
recordflip-text-transform smask smask-text text-antialias-subpixel
text-antialias-subpixel-rgb text-antialias-subpixel-bgr
text-antialias-subpixel-vrgb text-antialias-subpixel-vbgr text-pattern
text-unhinted-metrics unbounded-operator user-font-proxy
user-font-rescale pthread-same-source pthread-show-text
ft-text-vertical-layout-type1 ft-text-antialias-none
image (rgb24): 45 failed - clip-operator clip-text coverage-rhombus
extended-blend extended-blend-alpha extended-blend-mask
extended-blend-alpha-mask extended-blend-solid
extended-blend-solid-alpha fallback halo halo-transform operator-clear
operator-source radial-gradient radial-gradient-source
radial-gradient-mask-source record-select-font-face
record-text-transform record1414x-select-font-face
record1414x-text-transform record2x-select-font-face
record2x-text-transform record90-select-font-face
record90-text-transform recordflip-whole-select-font-face
recordflip-whole-text-transform recordflip-select-font-face
recordflip-text-transform smask smask-text text-antialias-subpixel
text-antialias-subpixel-rgb text-antialias-subpixel-bgr
text-antialias-subpixel-vrgb text-antialias-subpixel-vbgr text-pattern
text-unhinted-metrics unbounded-operator user-font-proxy
user-font-rescale pthread-same-source pthread-show-text
ft-text-vertical-layout-type1 ft-text-antialias-none
Is this enough or do you want something more?

Thanks Uli, yes lets go ahead and land this (let me know if you'd prefer
I land it). I didn't have any specific concerns just figured if there
were any issues the testsuite would (or at least should?) catch them.

Bryce