Re: [PATCH 7/9] core: Protect browse/search/query specs before invoking source operation

[Iago Toral <itoral igalia com>]

This patch has a bug (in the search method you ref/ref instead of ref/unref).

Also, implementation of virtual methods can be async, however, you always unref 
after invoking the method, is that alright?

Also, please check that we are not introducing any leaks / corruption before 
pushing this, I suggest that you add traces to the unref functions and check 
that resources are freed only once and that they are freed when they should.

Iago

On Fri,  9 Jul 2010 18:54:04 +0200, "Juan A. Suarez Romero"
<jasuarez igalia com> wrote:
> So far, specs were destroyed when sources invoke callback with remaining == 0.
> 
> This has the drawback that after it, spec is no longer valid, and sources
> should avoid to use them. This means that sources must copy spec content if
> they want to use them after the callback.
> 
> To avoid this problem, before invoking the source operation ref_counter in the
> spec is increased, and then decreased after returning. This has the advantage
> that when source invokes callback with remaining == 0, spec is not inmediatly
> destroyed, but after source returns from the operation. So source is safe to
> continue accessing the spec content.
> 
> Even more, if source, for any reason, needs to keep the spec even after
> returning (could be the spec is in turn wrapped in another structure handled in
> an idle function), it can ref() the spec, so it is not destroyed in the return,
> but later, when source calls the appropriate unref() function.
> ---
>  src/grl-media-source.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/src/grl-media-source.c b/src/grl-media-source.c
> index 8b66005..0d811dd 100644
> --- a/src/grl-media-source.c
> +++ b/src/grl-media-source.c
> @@ -420,7 +420,9 @@ browse_idle (gpointer user_data)
>    GrlMediaSourceBrowseSpec *bs = (GrlMediaSourceBrowseSpec *) user_data;
>    /* Check if operation was cancelled even before the idle kicked in */
>    if (!operation_is_cancelled (bs->source, bs->browse_id)) {
> +    grl_media_source_browse_spec_ref (bs);
>      GRL_MEDIA_SOURCE_GET_CLASS (bs->source)->browse (bs->source, bs);
> +    grl_media_source_browse_spec_unref (bs);
>    } else {
>      g_debug ("  operation was cancelled");
>      bs->callback (bs->source, bs->browse_id, NULL, 0, bs->user_data, NULL);
> @@ -435,7 +437,9 @@ search_idle (gpointer user_data)
>    GrlMediaSourceSearchSpec *ss = (GrlMediaSourceSearchSpec *) user_data;
>    /* Check if operation was cancelled even before the idle kicked in */
>    if (!operation_is_cancelled (ss->source, ss->search_id)) {
> +    grl_media_source_search_spec_ref (ss);
>      GRL_MEDIA_SOURCE_GET_CLASS (ss->source)->search (ss->source, ss);
> +    grl_media_source_search_spec_ref (ss);
>    } else {
>      g_debug ("  operation was cancelled");
>      ss->callback (ss->source, ss->search_id, NULL, 0, ss->user_data, NULL);
> @@ -449,7 +453,9 @@ query_idle (gpointer user_data)
>    g_debug ("query_idle");
>    GrlMediaSourceQuerySpec *qs = (GrlMediaSourceQuerySpec *) user_data;
>    if (!operation_is_cancelled (qs->source, qs->query_id)) {
> +    grl_media_source_query_spec_ref (qs);
>      GRL_MEDIA_SOURCE_GET_CLASS (qs->source)->query (qs->source, qs);
> +    grl_media_source_query_spec_unref (qs);
>    } else {
>      g_debug ("  operation was cancelled");
>      qs->callback (qs->source, qs->query_id, NULL, 0, qs->user_data, NULL);