| Summary: | regression in cairo_surface_t refcounting when using cairo_surface_unmap_image | ||
|---|---|---|---|
| Product: | cairo | Reporter: | Richard Hughes <richard> |
| Component: | general | Assignee: | Chris Wilson <chris> |
| Status: | RESOLVED FIXED | QA Contact: | cairo-bugs mailing list <cairo-bugs> |
| Severity: | normal | ||
| Priority: | medium | CC: | erik-freedesktop-bugzilla, jjardon |
| Version: | 1.12.8 | ||
| Hardware: | Other | ||
| OS: | Linux (All) | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Bug Depends on: | |||
| Bug Blocks: | 68382 | ||
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... 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). Dropping the 1.14 block on this, because the last comment was two years ago, and a simple workaround is available. 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);
|
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
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.