Fix a 'memory leak' in the image compositor

Submitted by Uli Schlachter on Jan. 13, 2018, 1:49 p.m.

Details

Message ID 20180113134952.22045-1-psychon@znc.in
State New
Headers show
Series "Fix a 'memory leak' in the image compositor" ( rev: 1 ) in Cairo

Not browsing as part of any series.

Commit Message

Uli Schlachter Jan. 13, 2018, 1:49 p.m.
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 <psychon@znc.in>
---
 src/cairo-debug.c            |  2 ++
 src/cairo-image-compositor.c | 17 +++++++++++++++++
 src/cairoint.h               |  3 +++
 3 files changed, 22 insertions(+)

Patch hide | download patch | download mbox

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,

Comments

On Sat, Jan 13, 2018 at 02:49:52PM +0100, Uli Schlachter wrote:
> 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 <psychon@znc.in>

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

Acked-by: Bryce Harrington <bryce@osg.samsung.com>

> ---
>  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,
> -- 
> 2.15.1
> 
> -- 
> cairo mailing list
> cairo@cairographics.org
> https://lists.cairographics.org/mailman/listinfo/cairo
On 19.01.2018 00:54, Bryce Harrington wrote:
> On Sat, Jan 13, 2018 at 02:49:52PM +0100, Uli Schlachter wrote:
>> 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 <psychon@znc.in>
> 
> 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
Hi Bryce,

On 10.02.2018 20:08, Uli Schlachter wrote:
> On 19.01.2018 00:54, Bryce Harrington wrote:
>> On Sat, Jan 13, 2018 at 02:49:52PM +0100, Uli Schlachter wrote:
>>> 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 <psychon@znc.in>
>>
>> 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
On Sun, Mar 11, 2018 at 10:18:47AM +0100, Uli Schlachter wrote:
> Hi Bryce,
> 
> On 10.02.2018 20:08, Uli Schlachter wrote:
> > On 19.01.2018 00:54, Bryce Harrington wrote:
> >> On Sat, Jan 13, 2018 at 02:49:52PM +0100, Uli Schlachter wrote:
> >>> 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 <psychon@znc.in>
> >>
> >> 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?

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