Move to use cairo glyphs instead of text

[PandorasFox]

Requires linking fontconfig now, but that was already required at runtime for cairo anyways.

Fixes the memory leaks caused by using cairo_select_font_face, as mentioned in #167

[Airblader]

@stapelberg I can offer a review in terms of checking the code, but given the changes to the dependencies I'd assume you would want to look at this, too.

[stapelberg]

I don’t mind the explicit dependency on fontconfig.

Once we’re happy with the code, we should get someone with cairo knowledge to review. Perhaps psychon?

[Airblader]

Yes, that sounds like a good idea. At least as a sanity check. I agree that we should first in a shape where we're happy with the PR.

[PandorasFox]

I'll make those changes tomorrow morning. I think I developed this patch on this laptop (I'm traveling for the holidays), so I should have the test programs I made to figure out how clean up the fonctconfig structs properly.

[PandorasFox]

Alright, I added some comments and made the changes with variable handling/duplicate lines.

I couldn't find the files I used for reference when working on this, so I'm assuming they're on my computer back home. I'll keep poking around to see if I can find a gist or something.

[stapelberg]

I’m not entirely sure why the travis check doesn’t complain, but formatting the code with clang-format-3.5 -i *.[ch] shows differences for me. Before pushing, could you please run that command to ensure consistent formatting? Thanks!

[PandorasFox]

Before pushing, could you please run that command to ensure consistent formatting? Thanks!

Should be good now!

[stapelberg]

@psychon, would you have a minute to review this pull request? AFAIR, you’re somewhat familiar with the cairo APIs :) Thanks in advance!

[psychon]

Okay, one last thing:

i3lock: version 2.9.1 (2017-06-21) © 2010 Michael Stapelberg

I'm not sure how many leaks were fixes since this version, but from a quick scroll through the output of valgrind --leak-check=full --show-reachable=yes i3lock, most of the leaks are through libxkbcommon, cairo_xcb_surface_create, xcb internals and dynamic linker internals. However, I did not check everything since the report is way too long:

==8310== LEAK SUMMARY:
==8310==    definitely lost: 256 bytes in 1 blocks
==8310==    indirectly lost: 107 bytes in 4 blocks
==8310==      possibly lost: 9,652 bytes in 113 blocks
==8310==    still reachable: 741,020 bytes in 5,901 blocks
==8310==         suppressed: 0 bytes in 0 blocks

Is it just me, or are ASAN and valgrind reporting wildly different results?

[PandorasFox]

Yeah, I get wildly different stuff with ASAN and valgrind, but I think that's due to an ASAN-compiled program running with non-ASAN libs. I was doing some debugging for another project and ended up compiling fontconfig with ASAN and ended up getting much more similar outputs after then.

I'm guessing it's just due to how ASAN gets built into the binary.

[psychon]

This patch makes valgrind go from

==15126== LEAK SUMMARY:
==15126==    definitely lost: 256 bytes in 1 blocks
==15126==    indirectly lost: 107 bytes in 4 blocks
==15126==      possibly lost: 9,652 bytes in 113 blocks
==15126==    still reachable: 740,708 bytes in 5,896 blocks
==15126==         suppressed: 0 bytes in 0 blocks

to

==17259== LEAK SUMMARY:
==17259==    definitely lost: 144 bytes in 3 blocks
==17259==    indirectly lost: 0 bytes in 0 blocks
==17259==      possibly lost: 0 bytes in 0 blocks
==17259==    still reachable: 272,496 bytes in 6 blocks
==17259==         suppressed: 0 bytes in 0 blocks

Remaining leaks are:

• An actual bug in cairo (cairo_debug_reset_static_data misses one cache which has 262,176 bytes and thus accounts for most of the remaining memory; thanks for making me spot this).
• Weird stuff called from _dl_init which I will just ignore.
• One timer that is still pending and cannot easily be freed (see patch; this is also where the definite lost comes from).

I'm not quite sure what my point here is, but it should be something like "there is no actual memory leak in cairo". I don't know why ASAN manages to find some leaks, but ignores all the other ones that valgrind finds, but... well, perhaps it is a good idea to figure out why ASAN only complains about some particular leak before adding 100 lines of code to make ASAN stop complaining.

[stapelberg]

@PandorasFox Sorry for letting this PR linger for so long. Could you resolve the conflicts please? We can go ahead with merging it then.

[PandorasFox]

I'll look into that later. I've been meaning to dig into the fontconfig errors some more and figure out if there's really a consistent way to resolve its errors (or if there's been a subsequent fontconfig release that's fixed them).

I noticed this commit, along with several others (checking the 2.13.1 changelog, I'm seeing a lot of commits that should fix the warnings/errors that I was seeing before) and am unsure if the rewrite/extra complexity is really needed anymore.

[PandorasFox]

I'm currently on a long flight and my laptop died shortly after I pushed that rebase. I'll do more testing on this later this weekend.

[PandorasFox]

I built fontconfig with -g -fsanitize=address -fno-omit-frame-pointer from this commit. When I ran the current i3lock master pulling in my local fontconfig build, which produced no ASAN output:

LD_PRELOAD=/usr/lib/libasan.so:/usr/local/lib/libfontconfig.so i3lock --nofork -> no output. Running i3lock --nofork (using Arch's fontconfig 2.13.1+12+g5f5ec56-1) produces leak warnings.

When running my fork vs the current master against Arch's fontconfig, the current master produces some ASAN leak warnings in the older fontconfig:

=================================================================
==22665==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 256 byte(s) in 1 object(s) allocated from:
    #0 0x7f7b94b2f019 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:86
    #1 0x7f7b92e40345  (/usr/lib/libfontconfig.so.1+0x21345)

Indirect leak of 96 byte(s) in 3 object(s) allocated from:
    #0 0x7f7b94b2f231 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:95
    #1 0x7f7b92e40989  (/usr/lib/libfontconfig.so.1+0x21989)

Indirect leak of 11 byte(s) in 1 object(s) allocated from:
    #0 0x7f7b94a77f01 in __interceptor_strdup /build/gcc/src/gcc/libsanitizer/asan/asan_interceptors.cc:405
    #1 0x7f7b92e3fc25 in FcValueSave (/usr/lib/libfontconfig.so.1+0x20c25)

SUMMARY: AddressSanitizer: 363 byte(s) leaked in 5 allocation(s).

So - it definitely seems that the overall issue was with fontconfig and this pull only really resolves the symptoms of it. I don't really feel that this should be necessary for correctness on i3lock's part, but I guess it might still be nice to have so that font handling is broken out and a bit easier to audit (maybe?) instead of relying on cairo functions. I'm personally leaning towards closing this.

[stapelberg]

So - it definitely seems that the overall issue was with fontconfig and this pull only really resolves the symptoms of it. I don't really feel that this should be necessary for correctness on i3lock's part

Let’s close this, then. The less code we have, the better :).

Thanks for looking into this!