Bug 56981 - regression in cairo_surface_t refcounting when using cairo_surface_unmap_image
regression in cairo_surface_t refcounting when using cairo_surface_unmap_image
Status: RESOLVED FIXED
Product: cairo
Classification: Unclassified
Component: general
1.12.8
Other Linux (All)
: medium normal
Assigned To: Chris Wilson
cairo-bugs mailing list
:
Depends on:
Blocks: cairo-1.14
  Show dependency treegraph
 
Reported: 2012-11-11 14:51 UTC by Richard Hughes
Modified: 2014-09-23 09:25 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Hughes 2012-11-11 14:51:39 UTC
Using cairo 1.12.2 this code returns correctly with no warnings or errors:

//gcc -o cairo-test2 cairo-test2.c `pkg-config --cflags --libs cairo-pdf` && ./cairo-test2
#include <glib.h>
#include <cairo.h>
int
main (int argc, char *argv[])
{
	cairo_surface_t *surface;
	cairo_surface_t *s2;
	surface = cairo_pdf_surface_create ("/tmp/dave.pdf", 300, 300);
	s2 = cairo_surface_map_to_image (surface, NULL);
	cairo_surface_unmap_image (surface, s2);
	cairo_surface_destroy (surface);
	return 0;
}

When testing with the more recent 1.12.8 I get:

cairo-test2: cairo-surface.c:930: cairo_surface_destroy: Assertion `((*&(&surface->ref_count)->ref_count) > 0)' failed.

This can be worked around by doing something like this:

	cairo_surface_reference (surface); // <--- THIS SHOULD NOT BE REQUIRED!!!
	cairo_surface_unmap_image (surface, s2);

The documentation says: "The content of the image will be uploaded to the target surface. Afterwards, the image is destroyed." It doesn't say anything about unreffing the parent surface and this is quite unexpected.

Any help welcome, thanks.
Comment 1 Uli Schlachter 2012-11-15 21:19:18 UTC
git bisect start
# bad: [5a6e1d680a5bf1c4091e74f999abd611abd92334] type1-subset: restore correct callothersub behavior
git bisect bad 5a6e1d680a5bf1c4091e74f999abd611abd92334
# good: [dbc0efad7e565558a3abf7f69d7675efddc4688d] version: bump for cairo-1.12.2 release
git bisect good dbc0efad7e565558a3abf7f69d7675efddc4688d
# bad: [652c632fb211cede74cef3813c7d6e8099d02089] gl: Fallback for copy_boxes if src/dst do not belong to the same device
git bisect bad 652c632fb211cede74cef3813c7d6e8099d02089
# good: [37c5c2dbe55f64ee84ab248eb0072d5d7c5145cc] surface: Only use non-NULL extents for internal mapping
git bisect good 37c5c2dbe55f64ee84ab248eb0072d5d7c5145cc
# bad: [615205cf072935401dac46813b597e70bc8f0a8c] Use the new pixman_glyph_cache_t API that will be in pixman 0.28.0
git bisect bad 615205cf072935401dac46813b597e70bc8f0a8c
# good: [7d8d98b91ccf7165be853c36e6d5ef0714f4a986] image: Upconvert glyphs through a WHITE source when adding to the glyph mask
git bisect good 7d8d98b91ccf7165be853c36e6d5ef0714f4a986
# bad: [f1b546b1a2b3c4a87ca00ce0d6fa6ce88c84d20c] Erradicate internal use of cairo_surface_get_type()
git bisect bad f1b546b1a2b3c4a87ca00ce0d6fa6ce88c84d20c
# good: [9e933d4b8790f0f8309bdd980f4558d51ccec168] gl: Add missing cairo-private to _cairo_gl_composite_with_clip
git bisect good 9e933d4b8790f0f8309bdd980f4558d51ccec168
# bad: [4b5d3436a36e7a2fe29131dff58b50999cd972bb] image: Fix bugs related to glyph mask creation
git bisect bad 4b5d3436a36e7a2fe29131dff58b50999cd972bb
# bad: [c0a92bf8329c5a8aee76ac96034435d4fce043dc] surface: replace map-to-image clone's use of user_data with parent pointer
git bisect bad c0a92bf8329c5a8aee76ac96034435d4fce043dc

c0a92bf8329c5a8aee76ac96034435d4fce043dc is the first bad commit
commit c0a92bf8329c5a8aee76ac96034435d4fce043dc
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu May 31 18:30:58 2012 +0100

    surface: replace map-to-image clone's use of user_data with parent pointer
    
    Removes an another undeclared PLT entry and prevents mixing of user_data
    with internal state.
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

However, no clue yet why this would make a difference...
Comment 2 Uli Schlachter 2012-11-15 21:33:07 UTC
Oh, that was easy. cairo_surface_unmap_image() finishes the image surface. This does the following in _cairo_image_surface_finish():

	cairo_surface_destroy (surface->parent);

However, that extra reference was never taken. Possible fixes are:

- Add a call to cairo_surface_reference() in _cairo_image_surface_set_parent() [This might possibly fix a similar bug in cairo-win32, however, no clue how that parent-magic in cairo-win32 works exactly]
- Add that call in _cairo_image_surface_clone_subimage()
- Revert the faulty commit and go back to set_user_data()

I'll let Chris decide about this since he might know how this is supposed to work while I don't (and cairo-win32 is too much black magic for my taste).
Comment 3 Bryce Harrington 2014-09-22 20:45:56 UTC
Dropping the 1.14 block on this, because the last comment was two years ago, and a simple workaround is available.
Comment 4 Uli Schlachter 2014-09-23 09:25:49 UTC
This was fixed one and a half year ago with my second proposal ("Add that call in _cairo_image_surface_clone_subimage()"):

commit 22b7fae0368ba6cff23b2ebdf58bd7d1bfdfbd6f
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Jan 31 14:19:53 2013 +0000

    image: Add a reference for the clone's parent image
    
    We use the parent as a flag during map-to-image/umap-image that the
    resultant image came from a fallback rather than as direct call
    to the backend's map_to_image(). Whilst we use it as a simple flag,
    we need to make sure the parent surface obeys the reference counting
    semantics and is consistent for all callers.
    
    Unlike other users of the parent pointer, there is no resource sharing
    between the two surfaces.
    
    Reported-by: Henry Song <henry.song@samsung.com>
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

diff --git a/src/cairo-image-surface.c b/src/cairo-image-surface.c
index 3fe6e43..20a1c03 100644
--- a/src/cairo-image-surface.c
+++ b/src/cairo-image-surface.c
@@ -1240,7 +1240,14 @@ _cairo_image_surface_clone_subimage (cairo_surface_t             *surface,
     if (unlikely (status))
        goto error;
 
-    _cairo_image_surface_set_parent (to_image_surface (image), surface);
+    /* We use the parent as a flag during map-to-image/umap-image that the
+     * resultant image came from a fallback rather than as direct call
+     * to the backend's map_to_image(). Whilst we use it as a simple flag,
+     * we need to make sure the parent surface obeys the reference counting
+     * semantics and is consistent for all callers.
+     */
+    _cairo_image_surface_set_parent (to_image_surface (image),
+                                    cairo_surface_reference (surface));
 
     return to_image_surface (image);