[cairo] _cairo_pattern_acquire_surface_for_solid saga continues...

[Behdad Esfahbod]

Chris,

I hate to have to write this, but seems like that function is, umm,
broken again.  Happens for example with xrender missing or disabled.

I believe at least one bug is that here:

    /* Choose a surface to repaint/evict */
    surface = NULL;
    if (solid_surface_cache.size == MAX_SURFACE_CACHE_SIZE) {
        i = rand () % MAX_SURFACE_CACHE_SIZE;
        surface = solid_surface_cache.cache[i].surface;

        if (CAIRO_REFERENCE_COUNT_GET_VALUE (&surface->ref_count) == 1 &&
            _cairo_surface_is_similar (surface, dst, pattern->content))
        {
            status = _cairo_surface_reset (surface);
            if (status)
                goto UNLOCK;

            status = _cairo_surface_paint (surface,
                                           CAIRO_OPERATOR_SOURCE,
                                           &pattern->base);
            if (status)
                goto UNLOCK;
        } else {
            /* Unable to reuse, evict */
            cairo_surface_destroy (surface);

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
            surface = NULL;
        }
    }

You destroy the cached surface but can later bail out without erasing
the pointer from the cache.

I tried moving the destroy to where cache surface is assigned, but that
didn't quite help.  I now get a segfault instead of assert failure.

I wanted to debug further, but found it hard to motivate myself to fix
this caching scheme.  The reason is, the scheme was nice and good for
what it was designed for, which was "sharing" solid pattern surfaces.
But as time passed it became obvious that sharing is impossible, so the
current cache is pretty much just a "holdover" cache, but is not
implemented that way.  For example, surfaces remain in the cache even
when they are being returned.  Their ref count jumps up and they are
never used by the cache again until all non-cache references are
dropped, but they still take a slot in the cache.

I think we should rewrite it to be a simple holdover cache instead:
remove from the cache when using, and put it in the cache when not using
anymore.


Cheers,

-- 
behdad
http://behdad.org/

"Those who would give up Essential Liberty to purchase a little
 Temporary Safety, deserve neither Liberty nor Safety."
        -- Benjamin Franklin, 1759