memleak in cairo_select_font_face

[PandorasFox]

I've squashed all the memleaks in my fork so far except for the small handful that are in libfontconfig.so, which are also . They all seem to be coming from the calls to cairo_select_font_face, which is still apparently just a 'toy' function.

I'll be poking at using cairo glyphs for the text instead, which should be a bit more robust (and not have memory leaks), and should have a fix soonish, if that's alright.

[Airblader]

I'm sorry, I'm a bit confused. What's the issue in i3lock here?

[PandorasFox]

ASAN complains some about memory errors in fontconfig due to cairo's show_text and select_font_face functions being toy/non-production functions. (I think that with a regular version of fontconfig, only ~3 memory leaks are reported, but with a version compiled with ASAN there's... A few thousand).

Additionally, since i3lock keeps setting the font face through cairo's toy functions on each redraw, it's a bit inefficient and hits up fontconfig on each tick. Depending on the version of fontconfig at runtime, this might cause a memory error on each tick.

[Airblader]

Thanks for the explanation; so the i3lock issue here is the unnecessary calls, right? The memory errors seem to be an issue of cairo if I understand you correctly. :-)

[PandorasFox]

Pretty much. The real issue with i3lock is using the "toy" Cairo functions, which cause the memory leaks.

I'm awful at naming things, including issues :)

[Airblader]

I haven't looked into those functions, but if they don't do anything we can also remove the calls.

[PandorasFox]

They're currently necessary for displaying the "Verifying..." /"Incorrect" /etc text, but they're basically the quick/inefficient way to get text done with Cairo.

The more correct way is for i3lock to lookup the font, then supply it to Cairo and cache that, to avoid using the toy functions and repeated lookups, since the toy functions don't include reference counting on the fonts they return/don't guarantee longevity.

[Airblader]

If the efforts to do so are reasonable, and I assume they are, I think it'd make sense to change it to what you described. PRs welcome, but I think you already mentioned you're preparing for one, right? :-)

[PandorasFox]

I made #168 last night :)

[ghost]

Is #168 going to be merged soon?

[psychon]

Additionally, since i3lock keeps setting the font face through cairo's toy functions on each redraw, it's a bit inefficient and hits up fontconfig on each tick.

Are you sure? From a quick look at cairo's code, there is a hash table caching things. Also, do you know about cairo_debug_reset_static_data()? (That clears all of cairo's caches and most likely explodes spectacularly if you still have a reference to anything cairo-y)

Also, the above comments sound like you believe there is a memory leak in cairo. Could you provide some details? If you want to make me really happy, please provide a self-contained C program showing the leak (which calls cairo_debug_reset_static_data() at the end).

The more correct way is for i3lock to lookup the font, then supply it to Cairo and cache that, to avoid using the toy functions and repeated lookups, since the toy functions don't include reference counting on the fonts they return/don't guarantee longevity.

First member of cairo_toy_font_face_t is a cairo_font_face_t and third member of that is a cairo_reference_count_t ref_count;. Also, cairo-toy-font-face.c has a function _cairo_toy_font_face_hash_table_lock() that returns a pointer to the hash table used for caching toy font instances.

The "don't guarantee longevity" seems to be correct. Toy font faces are destroyed when their last reference is dropped, while freetype font faces have lots of complicated magic to keep alive already-destroyed instances. So, just caching the toy font face might be enough for what you want to do?

[PandorasFox]

Also, do you know about cairo_debug_reset_static_data()? (That clears all of cairo's caches and most likely explodes spectacularly if you still have a reference to anything cairo-y)

I didn't know about this, and it does fix the problem for i3lock, apparently. I feel like just clearing Cairo's cache to avoid ASAN's warnings shouldn't be the solution - i3lock should just use its own, then.

So, just caching the toy font face might be enough for what you want to do?

I tried that and couldn't get it to work, although I may have just been unaware of Cairo's internals to do it the right way.

[psychon]

Cairo also caches freetype font faces. Cairo even caches the allocation for a cairo_t instances. Really, without cairo_debug_reset_static_data() you should get lots of false-positive memory leak reports. No idea why "something from freetype" is special according to ASAN.

[Vanclief]

Is there any update to this?

[PandorasFox]

Issues seem to have resolved themselves - it was an issue with a bad fontconfig update.