[Spice-devel] [PATCH] red_worker: Fix reference counting for the current frame (drawable) of a stream

[Alon Levy]

On Wed, Jul 25, 2012 at 04:50:15PM +0300, Yonit Halperin wrote:
> After marshalling MSG_STREAM_CREATE, there is no need to ref and
> unref stream->current before and after completing the sending of the
> message (correspondingly). The referencing is unnecessary because all
> the data that is required from the drawable (the clipping), is copied
> during marshalling, and no field in the drawable is referenced (see
> spice_marshall_msg_display_stream_create).
> 
> Moreover, the referencing was bugous:
> While display_channel_hold_pipe_item and
> display_channel_client_release_item_after_push referenced and
> dereferenced, correspondingly, stream->current, stream->current might
> have changed in between these calls, and then we ended up with one drawable
> leaking, and one drawable released before its time has come (which
> of course led to critical errors).

Good one, ACK.

> ---
>  server/red_worker.c |   16 +---------------
>  1 files changed, 1 insertions(+), 15 deletions(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 9009462..9330fff 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -8552,7 +8552,7 @@ static void red_display_marshall_stream_start(RedChannelClient *rcc,
>  
>      agent->last_send_time = 0;
>      spice_assert(stream);
> -    red_channel_client_init_send_data(rcc, SPICE_MSG_DISPLAY_STREAM_CREATE, &agent->create_item);
> +    red_channel_client_init_send_data(rcc, SPICE_MSG_DISPLAY_STREAM_CREATE, NULL);
>      SpiceMsgDisplayStreamCreate stream_create;
>      SpiceClipRects clip_rects;
>  
> @@ -9955,13 +9955,6 @@ static void display_channel_hold_pipe_item(RedChannelClient *rcc, PipeItem *item
>      case PIPE_ITEM_TYPE_DRAW:
>          ref_drawable_pipe_item(SPICE_CONTAINEROF(item, DrawablePipeItem, dpi_pipe_item));
>          break;
> -    case PIPE_ITEM_TYPE_STREAM_CREATE: {
> -        StreamAgent *stream_agent = SPICE_CONTAINEROF(item, StreamAgent, create_item);
> -        if (stream_agent->stream->current) {
> -            stream_agent->stream->current->refs++;
> -        }
> -        break;
> -    }
>      case PIPE_ITEM_TYPE_STREAM_CLIP:
>          ((StreamClipItem *)item)->refs++;
>          break;
> @@ -9985,13 +9978,6 @@ static void display_channel_client_release_item_after_push(DisplayChannelClient
>      case PIPE_ITEM_TYPE_DRAW:
>          put_drawable_pipe_item(SPICE_CONTAINEROF(item, DrawablePipeItem, dpi_pipe_item));
>          break;
> -    case PIPE_ITEM_TYPE_STREAM_CREATE: {
> -        StreamAgent *stream_agent = SPICE_CONTAINEROF(item, StreamAgent, create_item);
> -        if (stream_agent->stream->current) {
> -            release_drawable(worker, stream_agent->stream->current);
> -        }
> -        break;
> -    }
>      case PIPE_ITEM_TYPE_STREAM_CLIP:
>          red_display_release_stream_clip(worker, (StreamClipItem *)item);
>          break;
> -- 
> 1.7.7.6
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel