Bug 316066

Well.  In fact, this description is wrong, because Cairo::Surface removes the surfaces itself. Still, something makes their Pixmaps not disappear on Tileset deletion.

Seems to me like a bug in cairo then, than cairo_surface destruction does not free the underlying pixmap.

Question: does the behavior persists if in Oxygen::StyleHelper::initializeRefSurface we use the second code path only (CAIRO_FORMAT_ARGB32) to create the ref surface ?

(I have not tried to reproduce yet, but am working on it)

Created attachment 77777 [details]
patch to test leak at tileset deletion

This patch forces all tileset cackes to a size of 1. 
It also adds a printout everytime the cache expunges some elements due to max_size being exceeded. 

With this patch applied I am not able to reproduce the leak mentioned above:
running xrestop in one terminal, then oxygen-gtk-demo in another and moving back-and-force between the first two tabs, I can see the number of allocated pixmaps remain constant at ~63.

@Ruslan: can you confirm/infirm the above observation ? 
How did you actually spot the leak mentionned above ? 

Note: pixmaps are not *always* freed when a tileset (or surface) is deleted. This is the case for instance when tileset or surface has been copied (somewhere else), in which case the same underlying pixmap is re-used by the copy (this is the _reference count in the surface equal-to operator) and freed only when then last copy is deleted. 

I plan to make further check on leaks by printing out the reference count of the cairo_surface_t objects (cairo_surface_get_reference_count), that counts the number of copies of the same surface. The pixmap is actually freed only when the cairo_surface_t that has reference_count = 1 is deleted.

Cheers,

Hugo

With this patch I can see constant increase in Total pixmap allocation every time I hover a tab, i.e. the prelight pixmaps get allocated but seem to not be unallocated.

> Note: pixmaps are not *always* freed when a tileset (or surface) is deleted. This is the case for instance when tileset or surface has been copied (somewhere else), in which case the same underlying pixmap is re-used by the copy (this is the _reference count in the surface equal-to operator) and freed only when then last copy is deleted. 

Where do we copy surfaces? AFAIR, we only use some original surface to create 'similar' ones.

(In reply to comment #4)
> With this patch I can see constant increase in Total pixmap allocation every
> time I hover a tab, i.e. the prelight pixmaps get allocated but seem to not
> be unallocated.
mmm. 

Can't reproduce. Has to be an upstream bug then ...
cairo, or X11, or even graphics driver.
My cairo is 1.12.8
X: libx11_6-1.4.99.1-4.mga2 (not sure that's the right version string)
graphics: tested on both intel and nvidia.

> 
> > Note: pixmaps are not *always* freed when a tileset (or surface) is deleted. This is the case for instance when tileset or surface has been copied (somewhere else), in which case the same underlying pixmap is re-used by the copy (this is the _reference count in the surface equal-to operator) and freed only when then last copy is deleted. 
> 
> Where do we copy surfaces? AFAIR, we only use some original surface to
> create 'similar' ones.

We do that via the implicit copy constructor of tilesets (that calls the "=" operator of Cairo::Surface), for instance in oxygenshadowhelper.

not sure there are other places where this guy is called. 
(mmm, in fact, possibly every time we insert a tileset into a cache)

Haaa. I've found one more place where something goes wrong: TileSet::initSurface(): 
                #if CAIRO_VERSION >= CAIRO_VERSION_ENCODE(1, 10, 0)                                                    |        // set metrics
                Cairo::Surface tile( cairo_surface_create_for_rectangle( source, sx, sy, sw, sh ) );                          |        _w3 = cairo_surface_get_width( surface ) - (w1 + w2);
                #else                                                                                                         |        _h3 = cairo_surface_get_height( surface ) - (h1 + h2);
                Cairo::Surface tile( cairo_surface_create_similar( source, CAIRO_CONTENT_COLOR_ALPHA, sw, sh ) );

Having your patch applied, if I change #if to #if false, I don't reproduce the pixmap increase with oxygen-gtk when moving mouse over lineedit controls on the first page. If I make it as is, then I do reproduce this.

Oops, should be this instead:
                #if false&&CAIRO_VERSION >= CAIRO_VERSION_ENCODE(1, 10, 0)                                                    
                Cairo::Surface tile( cairo_surface_create_for_rectangle( source, sx, sy, sw, sh ) );                          
                #else

... I don't have this issue. 
Maybe the solution is just to increase the version check in the incriminated #if.
Maybe the first version of cairo_surface_create_for_rectangle is buggy
(it was absent in earlier cairo versions, and is supposed to be faster than what's after the #if)

What's your cairo ?

> What's your cairo ?
1.12.3, so not that old. What's yours?

1.12.8 (see comment 5).
Any chance you can upgrade ? 
(Well I guess I can also try to downgrade)

Will try now

Created attachment 77783 [details]
Refcount test

I've installed version 1.12.14, newer than yours, but still the same issue.
Now, I'm attaching a new test for leaks. See typical output:
______________________________
source refcount1: 1
tile refcount: 1
source refcount2: 2
source refcount3: 2

Notice refcount2 equal refcount3, unequal refcount1. This means 'tile' removal doesn't decrease refcount for some reason (tile is removed when its scope ends, right?).
Do you confirm this?

Hmm, OK. It's not because of some bug, it's because source is used as source surface for Context.
Well, then I don't understand what's up...

Funny. I've done something, and now fail to reproduce this either. Even after downgrade.

when you upgrade/downgrade, you uninstall the old cairo version first ?

Created attachment 77784 [details]
New test

Great point, you were right. Now, here's what I get with this updated test with old cairo:
______________________________
source refcount1: 1
source refcount1.5: 1
source refcount3: 2
source refcount4: 1
______________________________
source refcount1: 1
source refcount1.5: 1
tile refcount: 1
source refcount2: 2
source refcount3: 2
source refcount4: 2
______________________________
source refcount1: 2
source refcount1.5: 2
source refcount3: 3
source refcount4: 2
______________________________
source refcount1: 2
source refcount1.5: 2
tile refcount: 1
source refcount2: 3
source refcount3: 3
source refcount4: 3

An more up to 6, sometimes increasing to 7. So, apparently, it's a bug in cairo up to at least 1.12.3, maybe something until 1.12.8.
BTW, do you confirm that refcount1==refcount4 with your cairo?

(In reply to comment #16)
> Created attachment 77784 [details]
> New test
> 
> Great point, you were right. Now, here's what I get with this updated test
> with old cairo:
> ______________________________
> source refcount1: 1
> source refcount1.5: 1
> source refcount3: 2
> source refcount4: 1
> ______________________________
> source refcount1: 1
> source refcount1.5: 1
> tile refcount: 1
> source refcount2: 2
> source refcount3: 2
> source refcount4: 2
> ______________________________
> source refcount1: 2
> source refcount1.5: 2
> source refcount3: 3
> source refcount4: 2
> ______________________________
> source refcount1: 2
> source refcount1.5: 2
> tile refcount: 1
> source refcount2: 3
> source refcount3: 3
> source refcount4: 3
> 
> An more up to 6, sometimes increasing to 7. So, apparently, it's a bug in
> cairo up to at least 1.12.3, maybe something until 1.12.8.
> BTW, do you confirm that refcount1==refcount4 with your cairo?

Yes. So this is good. And unlike your printout with earlier version (which definitly shows a leak)

So. We just bump version in the VERSION check, to 1.12.8, to be sure.
No big deal, the old code path is as valid as the first, and likely only a tiny bit less efficient.
You do that and push and close ?

I'll reverse-bisect cairo to determine first working version, add some warnings in case it breaks again (like refcount1!=refcount4->cerr) and bump the version.

(In reply to comment #18)
> I'll reverse-bisect cairo to determine first working version, add some
> warnings in case it breaks again (like refcount1!=refcount4->cerr) and bump
> the version.

Not sure the warning are necessary (and would clutter the code).
For one, we could likely have spotted it with valgrind (memchecker)
Also, it is unlikely to get broken again, once fixed.
What's more likely is that we uncover new issues elsewhere.
All in all, I'd rather avoid adding upstream checks in our code and rather rely on  upstream being "working" (we are not writing a cairo, or gtk debugger, after all :))

Note: version check update should go in all our "active" branches

Git commit c7e54e1e33b5634c5e5455253a2e3e364e1ffce6 by Ruslan Kabatsayev.
Committed on 05/03/2013 at 21:58.
Pushed by kabatsayev into branch '1.3'.

Bump needed cairo version for cairo_surface_create_for_rectangle()

M  +2    -1    src/oxygentileset.cpp

http://commits.kde.org/oxygen-gtk/c7e54e1e33b5634c5e5455253a2e3e364e1ffce6

I was lucky enough that I caught cairo before they released 1.12.4. Without this, we'll still have X resource leaks unreproduced. Now this one down.

Git commit d9884c293982d1c82025229d6b66388df2524b0c by Ruslan Kabatsayev.
Committed on 05/03/2013 at 21:58.
Pushed by kabatsayev into branch 'gtk3-1.1'.

Bump needed cairo version for cairo_surface_create_for_rectangle()

M  +2    -1    src/oxygentileset.cpp

http://commits.kde.org/oxygen-gtk/d9884c293982d1c82025229d6b66388df2524b0c

*** Bug 318002 has been marked as a duplicate of this bug. ***

*** Bug 318039 has been marked as a duplicate of this bug. ***