Fix memory leaks in SetFont/ResolveFontDescription #1340
Conversation
|
Will review soon, do you know which reference wasn't being freed? |
| pango_font_description_set_family_static(ret, resolved_families->str); | ||
|
|
||
| g_strfreev(families); | ||
| g_string_free(resolved_families, false); |
There was a problem hiding this comment.
I forget exactly, but this might have been one leak (perhaps only when setting the font to a value that has already been used before). I don't think resolved_families's character data is ever free'd.
There was a problem hiding this comment.
Yep. The false is because due to pango_font_description_set_family_static, I wanted to share the same pointer, but forgot to free it somewhere.
|
|
||
| for (; it != _font_face_list.end(); ++it) { | ||
| if (g_ascii_strcasecmp(families[i], pango_font_description_get_family(it->user_desc)) == 0) { | ||
| char *name = g_strdup(pango_font_description_get_family(it->sys_desc)); |
There was a problem hiding this comment.
And I think this was the other. g_hash_table_destroy doesn't do anything to free its contained data unless the table is created with key/value_destroy_funcs.
src/Canvas.cc
Outdated
|
|
||
| std::vector<FontFace> Canvas::_font_face_list = _init_font_face_list(); | ||
|
|
||
| std::vector<FontFace> Canvas::_font_face_list; |
There was a problem hiding this comment.
I'm a little worried I made _init_font_face_list for some esoteric reason but it doesn't look necessary. That said do we even need the green line?
There was a problem hiding this comment.
Yeah I was wondering if at some point _init_font_face_list was being called more than once, but couldn't find evidence of that.
The green line is necessary (static members have to be initialized in the source and not the header so they're not redefined).
There was a problem hiding this comment.
Although I don't really see a reason why it needs to be a member of Canvas; it could probably just be in the source file unless you know of a reason it might be needed externally?
There was a problem hiding this comment.
It could be in the source file yeah, as long as there's one list per process since that's how system fonts get registered. Will merge as-is or with that change if you want it.
| g_string_free(renamed_families, true); | ||
| if (resolved_families.size()) resolved_families += ','; | ||
| resolved_families += renamed_families.size() ? renamed_families : family; | ||
| first = false; |
There was a problem hiding this comment.
Great job preserving all of the existing logic 👍
| pango_font_description_set_family_static(ret, resolved_families->str); | ||
|
|
||
| g_strfreev(families); | ||
| g_string_free(resolved_families, false); |
There was a problem hiding this comment.
Yep. The false is because due to pango_font_description_set_family_static, I wanted to share the same pointer, but forgot to free it somewhere.
Also about 55% faster. Fixes Automattic#1202
Moves the AdjustExternalMemory calls to always be adjacent to cairo_image_surface_create/destroy so they can't be used incorrectly. See Automattic#1202 (comment)
Also about 55% faster and 9 fewer LOC.
Fixes #1202