[Spice-devel] [spice-gtk] If replace me, should nod do refcount plus one

[Frediano Ziglio]

> 
> > 
> > From: 乐义华 <yueyihua at os-easy.com>
> > 
> > https://gitlab.freedesktop.org/spice/spice-gtk/issues/72
> > ---
> >  src/channel-display.c     | 5 ++++-
> >  src/spice-channel-cache.h | 8 ++++++++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index 44ba043..138cd8c 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -788,7 +788,10 @@ static void image_put_lossy(SpiceImageCache *cache,
> > uint64_t id,
> >  static void image_replace_lossy(SpiceImageCache *cache, uint64_t id,
> >                                  pixman_image_t *surface)
> >  {
> > -    image_put(cache, id, surface);
> > +    SpiceDisplayChannelPrivate *c =
> > +        SPICE_CONTAINEROF(cache, SpiceDisplayChannelPrivate, image_cache);
> > +
> > +    cache_replace_lossy(c->images, id, pixman_image_ref(surface), FALSE);
> >  }
> >  
> 
> Maybe just add a cache_remove before image_put?
> image_put always uses a cache_add which add a reference so creating the leak
> as apparently image_replace_lossy is supposed to replace, not add a reference
> to the cache.
> In theory if the reference count of that item is already, for instance, 2
> calling g_hash_table_replace directly will ignore the reference counting.
> Adding the call to cache_remove and then image_put will keep the
> reference count to 2.
> 
> >  static pixman_image_t* image_get_lossless(SpiceImageCache *cache, uint64_t
> >  id)
> > diff --git a/src/spice-channel-cache.h b/src/spice-channel-cache.h
> > index 75cc2cd..1df8f34 100644
> > --- a/src/spice-channel-cache.h
> > +++ b/src/spice-channel-cache.h
> > @@ -101,6 +101,14 @@ static inline void cache_add_lossy(display_cache
> > *cache,
> > uint64_t id,
> >      g_hash_table_replace(cache->table, item, value);
> >  }
> >  
> > +static inline void cache_replace_lossy(display_cache *cache, uint64_t id,
> > +                                   gpointer value, gboolean lossy)
> > +{
> > +    display_cache_item *item = cache_item_new(id, lossy);
> > +
> > +    g_hash_table_replace(cache->table, item, value);
> > +}
> > +
> >  static inline void cache_add(display_cache *cache, uint64_t id, gpointer
> >  value)
> >  {
> >      cache_add_lossy(cache, id, value, FALSE);
> 

Do we really need to use the reference counting on the image cache?
Looking at protocol and spice-gtk source I don't see the reason.
cache_add always call a g_hash_table_replace while cache_remove is only
called when server requires specific invalidate. I cannot imagine a
sequence of commands leading to a cache_remove that should not remove
the id from the cache.
After server send a CACHE_ME of an already existing ID all following
images with that ID are referring to last image inserted with that ID
so why server should send a INVAL_LIST with the ID referring to the old
ID and not the new one?

Frediano