[cairo] [PATCH 3/4] snapshot: fix the race condition. Protect the whole replaying function.

zhigang.gong at linux.intel.com zhigang.gong at linux.intel.com
Wed Jul 11 02:06:14 PDT 2012


From: Zhigang Gong <zhigang.gong at linux.intel.com>

Simply protect the whole replaying stage here, we may fine
tune the protect region in the future.

As now the accessing from the snapshot side must hold a rdlock,
then the surface's handling side is easier than before. We do
not need the snapshot side to touch the surface's reference counter
now, the wrlock at the COW function is enough to avoid race.

Slightly change the surface's pointer stealing. We set the _finishing
before call the cairo_surface_flush, then latter the first snapshot
will steal its pointer and then clear the bit, and the other snapshots
will have to create new buffer.

Signed-off-by: Zhigang Gong <zhigang.gong at linux.intel.com>
---
 src/cairo-image-source.c            |   33 ++-----------
 src/cairo-image-surface.c           |    1 +
 src/cairo-pdf-surface.c             |   18 ++-----
 src/cairo-ps-surface.c              |   16 ++-----
 src/cairo-recording-surface.c       |   91 +++++++++++++++++++++++++++++++++++
 src/cairo-script-surface.c          |    5 +-
 src/cairo-surface-private.h         |    1 -
 src/cairo-surface-snapshot-inline.h |    5 +--
 src/cairo-surface-snapshot.c        |   54 +++++++--------------
 src/cairo-surface.c                 |   11 +---
 10 files changed, 131 insertions(+), 104 deletions(-)

diff --git a/src/cairo-image-source.c b/src/cairo-image-source.c
index c5bd228..50e3e5e 100644
--- a/src/cairo-image-source.c
+++ b/src/cairo-image-source.c
@@ -432,13 +432,6 @@ _acquire_source_cleanup (pixman_image_t *pixman_image,
     free (data);
 }
 
-static void
-_defer_free_cleanup (pixman_image_t *pixman_image,
-		     void *closure)
-{
-    cairo_surface_destroy (closure);
-}
-
 static uint16_t
 expand_channel (uint16_t v, uint32_t bits)
 {
@@ -823,14 +816,11 @@ _pixman_image_for_surface (cairo_image_surface_t *dst,
 	(! is_mask || ! pattern->base.has_component_alpha ||
 	 (pattern->surface->content & CAIRO_CONTENT_COLOR) == 0))
     {
-	cairo_surface_t *defer_free = NULL;
 	cairo_image_surface_t *source = (cairo_image_surface_t *) pattern->surface;
 	cairo_surface_type_t type;
 
-	if (_cairo_surface_is_snapshot (&source->base)) {
-	    defer_free = _cairo_surface_snapshot_get_target (&source->base);
-	    source = (cairo_image_surface_t *) defer_free;
-	}
+	if (_cairo_surface_is_snapshot (&source->base))
+	    source = _cairo_surface_snapshot_get_target (&source->base);
 
 	type = source->base.backend->type;
 	if (type == CAIRO_SURFACE_TYPE_IMAGE) {
@@ -849,19 +839,15 @@ _pixman_image_for_surface (cairo_image_surface_t *dst,
 		    sample->x >= source->width ||
 		    sample->y >= source->height)
 		{
-		    if (extend == CAIRO_EXTEND_NONE) {
-			cairo_surface_destroy (defer_free);
+		    if (extend == CAIRO_EXTEND_NONE)
 			return _pixman_transparent_image ();
-		    }
 		}
 		else
 		{
 		    pixman_image = _pixel_to_solid (source,
 						    sample->x, sample->y);
-                    if (pixman_image) {
-			cairo_surface_destroy (defer_free);
+                    if (pixman_image)
                         return pixman_image;
-		    }
 		}
 	    }
 
@@ -872,7 +858,6 @@ _pixman_image_for_surface (cairo_image_surface_t *dst,
 						     pattern->base.filter,
 						     ix, iy))
 	    {
-		cairo_surface_destroy (defer_free);
 		return pixman_image_ref (source->pixman_image);
 	    }
 #endif
@@ -882,16 +867,8 @@ _pixman_image_for_surface (cairo_image_surface_t *dst,
 						     source->height,
 						     (uint32_t *) source->data,
 						     source->stride);
-	    if (unlikely (pixman_image == NULL)) {
-		cairo_surface_destroy (defer_free);
+	    if (unlikely (pixman_image == NULL))
 		return NULL;
-	    }
-
-	    if (defer_free) {
-		pixman_image_set_destroy_function (pixman_image,
-						   _defer_free_cleanup,
-						   defer_free);
-	    }
 	} else if (type == CAIRO_SURFACE_TYPE_SUBSURFACE) {
 	    cairo_surface_subsurface_t *sub;
 	    cairo_bool_t is_contained = FALSE;
diff --git a/src/cairo-image-surface.c b/src/cairo-image-surface.c
index 23e6ca6..116498f 100644
--- a/src/cairo-image-surface.c
+++ b/src/cairo-image-surface.c
@@ -768,6 +768,7 @@ _cairo_image_surface_snapshot (void *abstract_surface)
 	clone->color = image->color;
 
 	clone->owns_data = FALSE;
+	image->base._finishing = FALSE;
 	return &clone->base;
     }
 
diff --git a/src/cairo-pdf-surface.c b/src/cairo-pdf-surface.c
index eaa27f1..65a939d 100644
--- a/src/cairo-pdf-surface.c
+++ b/src/cairo-pdf-surface.c
@@ -1220,23 +1220,19 @@ _get_source_surface_size (cairo_surface_t         *source,
 	     *width  = extents->width;
 	     *height = extents->height;
 	} else {
-	    cairo_surface_t *free_me = NULL;
 	    cairo_rectangle_int_t surf_extents;
 	    cairo_box_t box;
 	    cairo_bool_t bounded;
 
 	    if (_cairo_surface_is_snapshot (source))
-		free_me = source = _cairo_surface_snapshot_get_target (source);
+		source = _cairo_surface_snapshot_get_target (source);
 
 	    status = _cairo_recording_surface_get_ink_bbox ((cairo_recording_surface_t *)source,
 							    &box, NULL);
-	    if (unlikely (status)) {
-		cairo_surface_destroy (free_me);
+	    if (unlikely (status))
 		return status;
-	    }
 
 	    bounded = _cairo_surface_get_extents (source, &surf_extents);
-	    cairo_surface_destroy (free_me);
 
 	    *width = surf_extents.width;
 	    *height = surf_extents.height;
@@ -2678,7 +2674,6 @@ _cairo_pdf_surface_emit_recording_surface (cairo_pdf_surface_t        *surface,
     cairo_box_double_t bbox;
     cairo_int_status_t status;
     int alpha = 0;
-    cairo_surface_t *free_me = NULL;
     cairo_surface_t *source;
     const cairo_rectangle_int_t *extents;
     int width;
@@ -2691,9 +2686,10 @@ _cairo_pdf_surface_emit_recording_surface (cairo_pdf_surface_t        *surface,
     height = pdf_source->hash_entry->height;
     is_subsurface = FALSE;
     source = pdf_source->surface;
-    if (_cairo_surface_is_snapshot (source)) {
-	free_me = source = _cairo_surface_snapshot_get_target (source);
-    } else if (source->backend->type == CAIRO_SURFACE_TYPE_SUBSURFACE) {
+
+    if (_cairo_surface_is_snapshot (source))
+	source = _cairo_surface_snapshot_get_target (source);
+    else if (source->backend->type == CAIRO_SURFACE_TYPE_SUBSURFACE) {
 	cairo_surface_subsurface_t *sub = (cairo_surface_subsurface_t *) source;
 
 	source = sub->target;
@@ -2751,9 +2747,7 @@ _cairo_pdf_surface_emit_recording_surface (cairo_pdf_surface_t        *surface,
 					  old_width,
 					  old_height);
     surface->paginated_mode = old_paginated_mode;
-
 err:
-    cairo_surface_destroy (free_me);
     return status;
 }
 
diff --git a/src/cairo-ps-surface.c b/src/cairo-ps-surface.c
index 877ba14..e6e5a88 100644
--- a/src/cairo-ps-surface.c
+++ b/src/cairo-ps-surface.c
@@ -1696,19 +1696,15 @@ _cairo_ps_surface_acquire_source_surface_from_pattern (cairo_ps_surface_t
 		*width  = sub->extents.width;
 		*height = sub->extents.height;
 	    } else {
-		cairo_surface_t *free_me = NULL;
 		cairo_recording_surface_t *recording_surface;
 		cairo_box_t bbox;
 		cairo_rectangle_int_t extents;
 
 		recording_surface = (cairo_recording_surface_t *) surf;
-		if (_cairo_surface_is_snapshot (&recording_surface->base)) {
-		    free_me = _cairo_surface_snapshot_get_target (&recording_surface->base);
-		    recording_surface = (cairo_recording_surface_t *) free_me;
-		}
+		if (_cairo_surface_is_snapshot (&recording_surface->base))
+		    recording_surface = _cairo_surface_snapshot_get_target (&recording_surface->base);
 
 		status = _cairo_recording_surface_get_bbox (recording_surface, &bbox, NULL);
-		cairo_surface_destroy (free_me);
 		if (unlikely (status))
 		    return status;
 
@@ -2869,7 +2865,6 @@ _cairo_ps_surface_emit_recording_surface (cairo_ps_surface_t *surface,
     cairo_matrix_t old_cairo_to_ps;
     cairo_content_t old_content;
     cairo_rectangle_int_t old_page_bbox;
-    cairo_surface_t *free_me = NULL;
     cairo_surface_clipper_t old_clipper;
     cairo_box_t bbox;
     cairo_int_status_t status;
@@ -2884,7 +2879,7 @@ _cairo_ps_surface_emit_recording_surface (cairo_ps_surface_t *surface,
 				 _cairo_ps_surface_clipper_intersect_clip_path);
 
     if (_cairo_surface_is_snapshot (recording_surface))
-	free_me = recording_surface = _cairo_surface_snapshot_get_target (recording_surface);
+	recording_surface = _cairo_surface_snapshot_get_target (recording_surface);
 
     status =
 	_cairo_recording_surface_get_bbox ((cairo_recording_surface_t *) recording_surface,
@@ -2951,7 +2946,6 @@ _cairo_ps_surface_emit_recording_surface (cairo_ps_surface_t *surface,
 						  &surface->cairo_to_ps);
 
 err:
-    cairo_surface_destroy (free_me);
     return status;
 }
 
@@ -2965,7 +2959,6 @@ _cairo_ps_surface_emit_recording_subsurface (cairo_ps_surface_t *surface,
     cairo_content_t old_content;
     cairo_rectangle_int_t old_page_bbox;
     cairo_surface_clipper_t old_clipper;
-    cairo_surface_t *free_me = NULL;
     cairo_int_status_t status;
 
     old_content = surface->content;
@@ -2996,7 +2989,7 @@ _cairo_ps_surface_emit_recording_subsurface (cairo_ps_surface_t *surface,
     _cairo_output_stream_printf (surface->stream, "  q\n");
 
     if (_cairo_surface_is_snapshot (recording_surface))
-	free_me = recording_surface = _cairo_surface_snapshot_get_target (recording_surface);
+	recording_surface = _cairo_surface_snapshot_get_target (recording_surface);
 
     if (recording_surface->content == CAIRO_CONTENT_COLOR) {
 	surface->content = CAIRO_CONTENT_COLOR;
@@ -3036,7 +3029,6 @@ _cairo_ps_surface_emit_recording_subsurface (cairo_ps_surface_t *surface,
 						  &surface->cairo_to_ps);
 
 err:
-    cairo_surface_destroy (free_me);
     return status;
 }
 
diff --git a/src/cairo-recording-surface.c b/src/cairo-recording-surface.c
index 02d8afd..af6f68a 100644
--- a/src/cairo-recording-surface.c
+++ b/src/cairo-recording-surface.c
@@ -89,6 +89,7 @@
 #include "cairo-recording-surface-inline.h"
 #include "cairo-surface-wrapper-private.h"
 #include "cairo-traps-private.h"
+#include "cairo-surface-snapshot-inline.h"
 
 typedef enum {
     CAIRO_RECORDING_REPLAY,
@@ -1287,6 +1288,56 @@ _cairo_recording_surface_get_visible_commands (cairo_recording_surface_t *surfac
     return num_visible;
 }
 
+static cairo_surface_snapshot_t *
+_cairo_pattern_get_snapshot (cairo_pattern_t *pattern)
+{
+    if (pattern->type == CAIRO_PATTERN_TYPE_SURFACE) {
+        cairo_surface_pattern_t *surface_pattern =
+            (cairo_surface_pattern_t *) pattern;
+        cairo_surface_t *surface = surface_pattern->surface;
+
+        if (_cairo_surface_is_snapshot(surface))
+	    return (cairo_surface_snapshot_t *) surface;
+    }
+    return NULL;
+}
+
+/**
+ * Begin to access the snapshot to replay on the target surface. To avoid race
+ * condition we need to get the rdlock. Thus other thread should not modify the
+ * source during we are accessing the original source.
+ *
+ * We need to check the target, if the target is source target of the snapshot, we
+ * need to break the self-copy here.
+ *
+ **/
+static void
+_cairo_snapshot_begin_access_target(cairo_surface_snapshot_t *snapshot,
+				    cairo_surface_t *target)
+{
+    assert(snapshot->target != target);
+    CAIRO_RWLOCK_RDLOCK(snapshot->rwlock);
+}
+
+static cairo_surface_snapshot_t *
+_cairo_pattern_begin_access_target(cairo_pattern_t *pattern,
+				   cairo_surface_t *target)
+{
+    cairo_surface_snapshot_t *snapshot;
+    snapshot = _cairo_pattern_get_snapshot(pattern);
+    if (snapshot == NULL)
+	return NULL;
+    _cairo_snapshot_begin_access_target(snapshot, target);
+    return snapshot;
+}
+
+static void
+_cairo_snapshot_finish_access_target(cairo_surface_snapshot_t *snapshot)
+{
+    if (snapshot)
+	CAIRO_RWLOCK_UNLOCK(snapshot->rwlock);
+}
+
 static cairo_status_t
 _cairo_recording_surface_replay_internal (cairo_recording_surface_t	*surface,
 					  const cairo_rectangle_int_t *surface_extents,
@@ -1306,6 +1357,7 @@ _cairo_recording_surface_replay_internal (cairo_recording_surface_t	*surface,
     cairo_bool_t use_indices = FALSE;
     const cairo_rectangle_int_t *r;
     int i, num_elements;
+    cairo_surface_snapshot_t *snapshot, *mask_snapshot;
 
     if (unlikely (surface->base.status))
 	return surface->base.status;
@@ -1344,6 +1396,9 @@ _cairo_recording_surface_replay_internal (cairo_recording_surface_t	*surface,
 	use_indices = TRUE;
     }
 
+    if (target->type == CAIRO_SURFACE_TYPE_RECORDING)
+	_cairo_recording_surface_break_self_copy_loop((cairo_recording_surface_t*)target);
+
     for (i = 0; i < num_elements; i++) {
 	cairo_command_t *command = elements[use_indices ? surface->indices[i] : i];
 
@@ -1355,21 +1410,32 @@ _cairo_recording_surface_replay_internal (cairo_recording_surface_t	*surface,
 
 	switch (command->header.type) {
 	case CAIRO_COMMAND_PAINT:
+	    snapshot = _cairo_pattern_begin_access_target(&command->paint.source.base, target);
 	    status = _cairo_surface_wrapper_paint (&wrapper,
 						   command->header.op,
 						   &command->paint.source.base,
 						   command->header.clip);
+	    _cairo_snapshot_finish_access_target(snapshot);
 	    break;
 
 	case CAIRO_COMMAND_MASK:
+            snapshot = _cairo_pattern_begin_access_target(&command->mask.source.base, target);
+	    mask_snapshot = _cairo_pattern_get_snapshot(&command->mask.mask.base);
+	    if (mask_snapshot && mask_snapshot != snapshot)
+	        _cairo_snapshot_begin_access_target(mask_snapshot, target);
+	    else
+		mask_snapshot = NULL;
 	    status = _cairo_surface_wrapper_mask (&wrapper,
 						  command->header.op,
 						  &command->mask.source.base,
 						  &command->mask.mask.base,
 						  command->header.clip);
+	    _cairo_snapshot_finish_access_target(mask_snapshot);
+	    _cairo_snapshot_finish_access_target(snapshot);
 	    break;
 
 	case CAIRO_COMMAND_STROKE:
+	    snapshot = _cairo_pattern_begin_access_target(&command->stroke.source.base, target);
 	    status = _cairo_surface_wrapper_stroke (&wrapper,
 						    command->header.op,
 						    &command->stroke.source.base,
@@ -1380,9 +1446,11 @@ _cairo_recording_surface_replay_internal (cairo_recording_surface_t	*surface,
 						    command->stroke.tolerance,
 						    command->stroke.antialias,
 						    command->header.clip);
+	    _cairo_snapshot_finish_access_target(snapshot);
 	    break;
 
 	case CAIRO_COMMAND_FILL:
+	    snapshot = _cairo_pattern_begin_access_target(&command->fill.source.base, target);
 	    status = CAIRO_INT_STATUS_UNSUPPORTED;
 	    if (_cairo_surface_wrapper_has_fill_stroke (&wrapper)) {
 		cairo_command_t *stroke_command;
@@ -1434,9 +1502,11 @@ _cairo_recording_surface_replay_internal (cairo_recording_surface_t	*surface,
 						      command->fill.antialias,
 						      command->header.clip);
 	    }
+	    _cairo_snapshot_finish_access_target(snapshot);
 	    break;
 
 	case CAIRO_COMMAND_SHOW_TEXT_GLYPHS:
+	    snapshot = _cairo_pattern_begin_access_target(&command->show_text_glyphs.source.base, target);
 	    status = _cairo_surface_wrapper_show_text_glyphs (&wrapper,
 							      command->header.op,
 							      &command->show_text_glyphs.source.base,
@@ -1446,6 +1516,7 @@ _cairo_recording_surface_replay_internal (cairo_recording_surface_t	*surface,
 							      command->show_text_glyphs.cluster_flags,
 							      command->show_text_glyphs.scaled_font,
 							      command->header.clip);
+	    _cairo_snapshot_finish_access_target(snapshot);
 	    break;
 
 	default:
@@ -1480,6 +1551,7 @@ _cairo_recording_surface_replay_one (cairo_recording_surface_t	*surface,
     cairo_surface_wrapper_t wrapper;
     cairo_command_t **elements, *command;
     cairo_int_status_t status;
+    cairo_surface_snapshot_t *snapshot, *mask_snapshot;
 
     if (unlikely (surface->base.status))
 	return surface->base.status;
@@ -1501,25 +1573,39 @@ _cairo_recording_surface_replay_one (cairo_recording_surface_t	*surface,
     if (index > surface->commands.num_elements)
 	return _cairo_error (CAIRO_STATUS_READ_ERROR);
 
+    if (target->type == CAIRO_SURFACE_TYPE_RECORDING)
+	_cairo_recording_surface_break_self_copy_loop((cairo_recording_surface_t*)target);
+
     elements = _cairo_array_index (&surface->commands, 0);
     command = elements[index];
     switch (command->header.type) {
     case CAIRO_COMMAND_PAINT:
+	snapshot = _cairo_pattern_begin_access_target(&command->paint.source.base, target);
 	status = _cairo_surface_wrapper_paint (&wrapper,
 					       command->header.op,
 					       &command->paint.source.base,
 					       command->header.clip);
+	_cairo_snapshot_finish_access_target(snapshot);
 	break;
 
     case CAIRO_COMMAND_MASK:
+        snapshot = _cairo_pattern_begin_access_target(&command->mask.source.base, target);
+	mask_snapshot = _cairo_pattern_get_snapshot(&command->mask.mask.base);
+	if (mask_snapshot && mask_snapshot != snapshot)
+	    _cairo_snapshot_begin_access_target(mask_snapshot, target);
+	else
+	    mask_snapshot = NULL;
 	status = _cairo_surface_wrapper_mask (&wrapper,
 					      command->header.op,
 					      &command->mask.source.base,
 					      &command->mask.mask.base,
 					      command->header.clip);
+	_cairo_snapshot_finish_access_target(mask_snapshot);
+	_cairo_snapshot_finish_access_target(snapshot);
 	break;
 
     case CAIRO_COMMAND_STROKE:
+	snapshot = _cairo_pattern_begin_access_target(&command->stroke.source.base, target);
 	status = _cairo_surface_wrapper_stroke (&wrapper,
 						command->header.op,
 						&command->stroke.source.base,
@@ -1530,9 +1616,11 @@ _cairo_recording_surface_replay_one (cairo_recording_surface_t	*surface,
 						command->stroke.tolerance,
 						command->stroke.antialias,
 						command->header.clip);
+	_cairo_snapshot_finish_access_target(snapshot);
 	break;
 
     case CAIRO_COMMAND_FILL:
+	snapshot = _cairo_pattern_begin_access_target(&command->fill.source.base, target);
 	status = _cairo_surface_wrapper_fill (&wrapper,
 					      command->header.op,
 					      &command->fill.source.base,
@@ -1541,9 +1629,11 @@ _cairo_recording_surface_replay_one (cairo_recording_surface_t	*surface,
 					      command->fill.tolerance,
 					      command->fill.antialias,
 					      command->header.clip);
+	_cairo_snapshot_finish_access_target(snapshot);
 	break;
 
     case CAIRO_COMMAND_SHOW_TEXT_GLYPHS:
+	snapshot = _cairo_pattern_begin_access_target(&command->show_text_glyphs.source.base, target);
 	status = _cairo_surface_wrapper_show_text_glyphs (&wrapper,
 							  command->header.op,
 							  &command->show_text_glyphs.source.base,
@@ -1553,6 +1643,7 @@ _cairo_recording_surface_replay_one (cairo_recording_surface_t	*surface,
 							  command->show_text_glyphs.cluster_flags,
 							  command->show_text_glyphs.scaled_font,
 							  command->header.clip);
+	_cairo_snapshot_finish_access_target(snapshot);
 	break;
 
     default:
diff --git a/src/cairo-script-surface.c b/src/cairo-script-surface.c
index 2149e7e..0852d2c 100644
--- a/src/cairo-script-surface.c
+++ b/src/cairo-script-surface.c
@@ -1549,7 +1549,7 @@ _emit_surface_pattern (cairo_script_surface_t *surface,
 {
     cairo_script_context_t *ctx = to_context (surface);
     cairo_surface_pattern_t *surface_pattern;
-    cairo_surface_t *source, *snapshot, *free_me = NULL;
+    cairo_surface_t *source, *snapshot;
     cairo_surface_t *take_snapshot = NULL;
     cairo_int_status_t status;
 
@@ -1568,7 +1568,7 @@ _emit_surface_pattern (cairo_script_surface_t *surface,
 	if (_cairo_surface_snapshot_is_reused (source))
 	    take_snapshot = source;
 
-	free_me = source = _cairo_surface_snapshot_get_target (source);
+	source = _cairo_surface_snapshot_get_target (source);
     }
 
     switch ((int) source->backend->type) {
@@ -1585,7 +1585,6 @@ _emit_surface_pattern (cairo_script_surface_t *surface,
 	status = _emit_image_surface_pattern (surface, source);
 	break;
     }
-    cairo_surface_destroy (free_me);
     if (unlikely (status))
 	return status;
 
diff --git a/src/cairo-surface-private.h b/src/cairo-surface-private.h
index 63202fb..56e3feb 100644
--- a/src/cairo-surface-private.h
+++ b/src/cairo-surface-private.h
@@ -64,7 +64,6 @@ struct _cairo_surface {
     unsigned int unique_id;
     unsigned int serial;
     cairo_damage_t *damage;
-
     unsigned _finishing : 1;
     unsigned finished : 1;
     unsigned is_clear : 1;
diff --git a/src/cairo-surface-snapshot-inline.h b/src/cairo-surface-snapshot-inline.h
index 48b24a4..2fe05ac 100644
--- a/src/cairo-surface-snapshot-inline.h
+++ b/src/cairo-surface-snapshot-inline.h
@@ -48,12 +48,9 @@ _cairo_surface_snapshot_is_reused (cairo_surface_t *surface)
 static inline cairo_surface_t *
 _cairo_surface_snapshot_get_target (cairo_surface_t *surface)
 {
-    cairo_surface_snapshot_t *snapshot = (cairo_surface_snapshot_t *) surface;
     cairo_surface_t *target;
 
-    CAIRO_RWLOCK_WRLOCK (snapshot->rwlock);
-    target = _cairo_surface_reference (snapshot->target);
-    CAIRO_RWLOCK_UNLOCK (snapshot->rwlock);
+    target = ((cairo_surface_snapshot_t *) surface)->target;
 
     return target;
 }
diff --git a/src/cairo-surface-snapshot.c b/src/cairo-surface-snapshot.c
index 30e58fd..bd7281e 100644
--- a/src/cairo-surface-snapshot.c
+++ b/src/cairo-surface-snapshot.c
@@ -67,15 +67,15 @@ static cairo_status_t
 _cairo_surface_snapshot_flush (void *abstract_surface)
 {
     cairo_surface_snapshot_t *surface = abstract_surface;
-    cairo_surface_t *target;
-    cairo_status_t status;
-
-    target = _cairo_surface_snapshot_get_target (&surface->base);
-    cairo_surface_flush (target);
-    status = target->status;
-    cairo_surface_destroy (target);
-
-    return status;
+    /**
+     * The following call implicitly touch the underlying source
+     * surface. As we already break the self-copy loop before we start
+     * the replaying process, this should not cause a recursive call of
+     * rwlock/rdlock.
+     **/ 
+    cairo_surface_flush (surface->target);
+
+    return surface->target->status;
 }
 
 static cairo_surface_t *
@@ -83,48 +83,31 @@ _cairo_surface_snapshot_source (void                    *abstract_surface,
 				cairo_rectangle_int_t *extents)
 {
     cairo_surface_snapshot_t *surface = abstract_surface;
-    return _cairo_surface_get_source (surface->target, extents); /* XXX racy */
+    /**
+     * As we protect the whole replaying phase, the following call should
+     * not cause racy problem now. 
+     * */
+    return _cairo_surface_get_source (surface->target, extents);
 }
 
-struct snapshot_extra {
-    cairo_surface_t *target;
-    void *extra;
-};
-
 static cairo_status_t
 _cairo_surface_snapshot_acquire_source_image (void                    *abstract_surface,
 					      cairo_image_surface_t  **image_out,
 					      void                   **extra_out)
 {
     cairo_surface_snapshot_t *surface = abstract_surface;
-    struct snapshot_extra *extra;
-    cairo_status_t status;
-
-    extra = malloc (sizeof (*extra));
-    if (unlikely (extra == NULL))
-	return _cairo_error (CAIRO_STATUS_NO_MEMORY);
-
-    extra->target = _cairo_surface_snapshot_get_target (&surface->base);
-    status =  _cairo_surface_acquire_source_image (extra->target, image_out, &extra->extra);
-    if (unlikely (status)) {
-	cairo_surface_destroy (extra->target);
-	free (extra);
-    }
 
-    *extra_out = extra;
-    return status;
+    return _cairo_surface_acquire_source_image (surface->target, image_out, extra_out);
 }
 
 static void
 _cairo_surface_snapshot_release_source_image (void                   *abstract_surface,
 					      cairo_image_surface_t  *image,
-					      void                   *_extra)
+					      void                   *extra)
 {
-    struct snapshot_extra *extra = _extra;
+    cairo_surface_snapshot_t *surface = abstract_surface;
 
-    _cairo_surface_release_source_image (extra->target, image, extra->extra);
-    cairo_surface_destroy (extra->target);
-    free (extra);
+    _cairo_surface_release_source_image (surface->target, image, extra);
 }
 
 static cairo_bool_t
@@ -137,7 +120,6 @@ _cairo_surface_snapshot_get_extents (void                  *abstract_surface,
 
     target = _cairo_surface_snapshot_get_target (&surface->base);
     bounded = _cairo_surface_get_extents (target, extents);
-    cairo_surface_destroy (target);
 
     return bounded;
 }
diff --git a/src/cairo-surface.c b/src/cairo-surface.c
index 6ab2140..751dd16 100644
--- a/src/cairo-surface.c
+++ b/src/cairo-surface.c
@@ -938,15 +938,8 @@ cairo_surface_destroy (cairo_surface_t *surface)
 
     if (! surface->finished) {
 	_cairo_surface_finish_snapshots (surface);
-	/* We may have been referenced by a snapshot prior to have
-	 * detaching it with the copy-on-write.
-	 */
-	if (CAIRO_REFERENCE_COUNT_GET_VALUE (&surface->ref_count))
-	    return;
 
 	_cairo_surface_finish (surface);
-	/* paranoid check that nobody took a reference whilst finishing */
-	assert (! CAIRO_REFERENCE_COUNT_HAS_REFERENCE (&surface->ref_count));
     }
 
     if (surface->damage)
@@ -961,6 +954,8 @@ cairo_surface_destroy (cairo_surface_t *surface)
     assert (surface->snapshot_of == NULL);
     assert (!_cairo_surface_has_snapshots (surface));
 
+    /* paranoid check that nobody took a reference whilst finishing */
+    assert (! CAIRO_REFERENCE_COUNT_HAS_REFERENCE (&surface->ref_count));
     free (surface);
 }
 slim_hidden_def(cairo_surface_destroy);
@@ -989,10 +984,10 @@ cairo_surface_get_reference_count (cairo_surface_t *surface)
 static void
 _cairo_surface_finish_snapshots (cairo_surface_t *surface)
 {
+    surface->_finishing = TRUE;
     cairo_surface_flush (surface);
 
     /* update the snapshots *before* we declare the surface as finished */
-    surface->_finishing = TRUE;
 
     _cairo_surface_detach_snapshots (surface);
     if (surface->snapshot_of != NULL)
-- 
1.7.4.4



More information about the cairo mailing list