Fix memory leaks in overhaul branch

[springmeyer]

The overhaul branch no longer uses C++ wrappers around Freetype objects. @lbud one side effect of this is that we lost the RAII benefit of those classes.

To solve this I propose we use scope guards (http://en.wikibooks.org/wiki/More_C++_Idioms/Scope_Guard). Scope gaurds make sense because in our used of FT objects is now scoped purely to the async methods. Scope guards will help use ensure that the Freetype objects get destructed the same no matter if the function throws an error or returns early.

Here are the current leaks:

• FT_Library is leaked here
• FT_Glyph is leaked here and here and here

For reference I found the memory leaks by:

• booting a linux VM (since OS X does not support valgrind and iprofiler -leaks is not as good as valgrind)
• built overhaul branch of node-fontnik in debug mode
• ran valgrind --leak-check=full node ./node_modules/.bin/_mocha test/
• paid attention to the sections that originated from node-fontnik function calls and said blocks are definitely lost and ignored all others

Valgrind results that showed the memory leaks were:

==5575== 320 bytes in 4 blocks are definitely lost in loss record 230 of 309
==5575==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5575==    by 0x7B5CC6F: ft_mem_qalloc (in /usr/lib/x86_64-linux-gnu/libfreetype.so.6.11.1)
==5575==    by 0x7B5CCB7: ft_mem_alloc (in /usr/lib/x86_64-linux-gnu/libfreetype.so.6.11.1)
==5575==    by 0x7B63177: ??? (in /usr/lib/x86_64-linux-gnu/libfreetype.so.6.11.1)
==5575==    by 0x7B634CE: FT_Get_Glyph (in /usr/lib/x86_64-linux-gnu/libfreetype.so.6.11.1)
==5575==    by 0x78C7FC5: node_fontnik::RenderSDF(node_fontnik::glyph_info&, int, int, float, FT_FaceRec_*) (glyphs.cpp:522)
==5575==    by 0x78C69B7: node_fontnik::RangeAsync(uv_work_s*) (glyphs.cpp:290)
==5575==    by 0x9E0E2F: worker (threadpool.c:74)
==5575==    by 0x9D6640: uv__thread_start (uv-common.c:323)
==5575==    by 0x5A6B181: start_thread (pthread_create.c:312)
==5575==    by 0x5D7BEFC: clone (clone.S:111)

And

==5575== 2,128 (128 direct, 2,000 indirect) bytes in 1 blocks are definitely lost in loss record 273 of 309
==5575==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5575==    by 0x7B5CC6F: ft_mem_qalloc (in /usr/lib/x86_64-linux-gnu/libfreetype.so.6.11.1)
==5575==    by 0x7B5CCB7: ft_mem_alloc (in /usr/lib/x86_64-linux-gnu/libfreetype.so.6.11.1)
==5575==    by 0x7B61431: FT_Add_Module (in /usr/lib/x86_64-linux-gnu/libfreetype.so.6.11.1)
==5575==    by 0x7B5856B: FT_Add_Default_Modules (in /usr/lib/x86_64-linux-gnu/libfreetype.so.6.11.1)
==5575==    by 0x7B585C8: FT_Init_FreeType (in /usr/lib/x86_64-linux-gnu/libfreetype.so.6.11.1)
==5575==    by 0x78C5D40: node_fontnik::LoadAsync(uv_work_s*) (glyphs.cpp:175)
==5575==    by 0x9E0E2F: worker (threadpool.c:74)
==5575==    by 0x9D6640: uv__thread_start (uv-common.c:323)
==5575==    by 0x5A6B181: start_thread (pthread_create.c:312)
==5575==    by 0x5D7BEFC: clone (clone.S:111)

[springmeyer]

@lbud - here's what a scope guard could look like:

diff --git a/src/glyphs.cpp b/src/glyphs.cpp
index 55cca87..a1d6c7b 100644
--- a/src/glyphs.cpp
+++ b/src/glyphs.cpp
@@ -168,10 +168,24 @@ NAN_METHOD(Range) {
     NanReturnUndefined();
 }

+struct ft_library_guard
+{
+    ft_library_guard(FT_Library lib) :
+      library_(lib) {}
+
+    ~ft_library_guard()
+    {
+        if (library_) FT_Done_FreeType(library_);
+    }
+
+    FT_Library library_;
+};
+
 void LoadAsync(uv_work_t* req) {
     LoadBaton* baton = static_cast<LoadBaton*>(req->data);

     FT_Library library = nullptr;
+    ft_library_guard library_guard(library);
     FT_Error error = FT_Init_FreeType(&library);
     if (error) {
         /* LCOV_EXCL_START */
@@ -205,7 +219,6 @@ void LoadAsync(uv_work_t* req) {
             FT_Done_Face(ft_face);
         }
     }
-    FT_Done_FreeType(library);

Want to run this out and see if you can replicate the memory leaks and try to fix them?

Note: iprofiler -leaks might also be able to find them, but as you saw on Friday my Instruments.app is broken (crashes when I try to open it), so I was unable to confirm that iprofiler sampling could pick up the leaks. I would recommend you learn both iprofiler/instruments.app and valgrind as you have time, but overall put more trust in valgrind on linux (sadly it does not run on recent OS X).

[lbud]

@springmeyer fwiw this is all I've been able to get out of Instruments so far:

That's only profiling fontnik.range. Profiling fontnik.load shows no leaks in Instruments. (So I may be doing it wrong but iprofiler does seem to be less effective as valgrind.)

[springmeyer]

Okay, good at least that it caught one of two. I wonder if iprofiler/instruments could catch the FT_Library leak if you ran this test in a loop for a few seconds. I think iprofiler works by sampling on intervals so it may be that the time between the function starting and the leak happening is so short that it is missed. Valgrind instruments and tracks all allocations so it should not miss anything, but is also very very slow (sometimes prohibitively slow).

[lbud]

Hm, I tried changing bench.js to run invalid buffers or running iprofiler on the tests, but it always either hangs in saving results or doesn't show leaks. But I brew installed valgrind and was able to reproduce.

1b408bd was my attempt to fix these, but I'm having a few issues. Full valgrind output is here.

• There are a lot of issues around the uninitialized value here, which i also get a compiler warning about:

../src/glyphs.cpp:545:32: warning: variable 'ft_glyph' is uninitialized when used here [-Wuninitialized]
    ft_glyph_guard glyph_guard(ft_glyph);
                               ^~~~~~~~
../src/glyphs.cpp:544:22: note: initialize the variable 'ft_glyph' to silence this warning
    FT_Glyph ft_glyph;
                     ^
                      = nullptr

Is there any way I can initialize that or is this just an inevitable error because of the FT library?

• I'm still getting

==38475== 22,224 (392 direct, 21,832 indirect) bytes in 1 blocks are definitely lost in loss record 1,020 of 1,037
==38475==    at 0x1007BA59B: malloc (in /usr/local/Cellar/valgrind/HEAD/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==38475==    by 0x10A16670D: ft_mem_alloc (in /usr/local/Cellar/freetype/2.5.5/lib/libfreetype.6.dylib)
==38475==    by 0x10A16A648: FT_New_Library (in /usr/local/Cellar/freetype/2.5.5/lib/libfreetype.6.dylib)
==38475==    by 0x10A1654EF: FT_Init_FreeType (in /usr/local/Cellar/freetype/2.5.5/lib/libfreetype.6.dylib)
==38475==    by 0x10A001722: node_fontnik::LoadAsync(uv_work_s*) (in /Users/laurenbudorick/src/node-fontnik/lib/fontnik.node)
==38475==    by 0x100139148: worker (in /usr/local/bin/node)
==38475==    by 0x10012F935: uv__thread_start (in /usr/local/bin/node)
==38475==    by 0x1034E42FB: _pthread_body (in /usr/lib/system/libsystem_pthread.dylib)
==38475==    by 0x1034E4278: _pthread_start (in /usr/lib/system/libsystem_pthread.dylib)
==38475==    by 0x1034E24B0: thread_start (in /usr/lib/system/libsystem_pthread.dylib)
==38475== 
==38475== 44,448 (128 direct, 44,320 indirect) bytes in 2 blocks are definitely lost in loss record 1,028 of 1,037
==38475==    at 0x1007BA59B: malloc (in /usr/local/Cellar/valgrind/HEAD/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==38475==    by 0x10A16670D: ft_mem_alloc (in /usr/local/Cellar/freetype/2.5.5/lib/libfreetype.6.dylib)
==38475==    by 0x10A169FAE: FT_Add_Module (in /usr/local/Cellar/freetype/2.5.5/lib/libfreetype.6.dylib)
==38475==    by 0x10A16551D: FT_Init_FreeType (in /usr/local/Cellar/freetype/2.5.5/lib/libfreetype.6.dylib)
==38475==    by 0x10A001722: node_fontnik::LoadAsync(uv_work_s*) (in /Users/laurenbudorick/src/node-fontnik/lib/fontnik.node)
==38475==    by 0x100139148: worker (in /usr/local/bin/node)
==38475==    by 0x10012F935: uv__thread_start (in /usr/local/bin/node)
==38475==    by 0x1034E42FB: _pthread_body (in /usr/lib/system/libsystem_pthread.dylib)
==38475==    by 0x1034E4278: _pthread_start (in /usr/lib/system/libsystem_pthread.dylib)
==38475==    by 0x1034E24B0: thread_start (in /usr/lib/system/libsystem_pthread.dylib)

😕

[springmeyer]

Ah, okay try doing:

+FT_Glyph ft_glyph = nullptr;
ft_glyph_guard glyph_guard(ft_glyph);

To make sure the ft_glyph is initialized to something. We are already doing this for the library.

[springmeyer]

running iprofiler on the tests, but it always either hangs in saving results or doesn't show leaks

Yes, i've seen that iprofiler generates such a large amount of metadata that will may hang or crash for tests runs that are long. So, its most viable for short test runs (like test/test.js rather than bench/bench.js) which of course is lame if it cannot catch the leaks. Great that you figured out how to get valgrind going on OS X - I just followed your lead and installed it and am testing.

[springmeyer]

done in #83