Bug 34912

To quote the commit message from 12e41acf9c2361:

    The only downside is that the cache enforced a limit of 16 MiB of pixel data
    that was used for source surfaces on the server. After this commit the Picture
    will be kept alive until the snapshot is detached. If this becomes a problem
    memory-wise, a new solution will have to be invented...

I did a quick awk script that parses the output of xtrace and counts the number of CreateFoo and FreeFoo requests and the number of live Pictures and Pixmaps goes through the roof. Removing the call to _cairo_surface_attach_snapshot() from _cairo_xcb_surface_picture() solves this problem (and AFAIK this causes the same behavior as with cairo-xlib).

Any ideas how we could limit the number of living cairo_xcb_picture_t without having a cache that keeps them alive longer than needed? (And especially not longer than cairo_surface_finish())

Created attachment 44018 [details] [review]
Workaround which makes the problem go away

We should be able to keep around the Picture for a Pixmap, we just need to make sure that doing so doesn't incur a reference.

However, go for simplicity replace the attach_snapshot with a comment:
/* XXX Consider keeping the Picture associated with this source until
 * it is modified or destroyed; a snapshot. However, it must not
 * increase the lifetime of the underlying source.
 */

Turns out the problem lies deeper than this. Thanks to Andrea, I dug a little deeper and found a cyclic reference:

- _cairo_xcb_surface_picture() is called for a XCB surface
- This calls _copy_to_picture(source, FALSE) to make use of the existing Picture
- Let's assume the source owns its pixmap, then this calls _cairo_xcb_picture_copy()
- This creates a surface with: surface->owner = cairo_surface_reference (&target->base); 
  (-> The _cairo_xcb_picture_t has a reference to the "source" surface)
-  _cairo_xcb_surface_picture() now calls _cairo_surface_attach_snapshot() which causes the reference count for "source" to be increased.

So the result is:
"source" has the _cairo_xcb_picture_t as a snapshot and owns a reference for it.
At the same time, _cairo_xcb_picture_t owns a reference for the "source" since it co-uses its Picture.

(valgrind won't detect this because cairo_device_finish for the xcb device "cleans up" by finishing all surfaces which are still alive)

Easiest fix would be to remove _cairo_xcb_picture_copy.

Created attachment 44036 [details] [review]
Disable use of _cairo_xcb_picture_copy

(In reply to comment #5)
> Created an attachment (id=44036) [details]
> Disable use of _cairo_xcb_picture_copy

Test in cmmmit c0dc933efda7672b07e188a1195821340f911a66 with above patch,it works fine.

Fixed in master by removing _cairo_xcb_picture_copy (f1d313e042).

Verified with the commit f1d313e042af89b2f5f5d09d3eb1703d0517ecd7,it works fine when run firefox-talos-gfx.trace.