From 7d07812fa678b19dbd3f90a0689796881b588dc1 Mon Sep 17 00:00:00 2001 From: Michael Carlberg Date: Wed, 14 Dec 2016 05:13:59 +0100 Subject: [PATCH] fix(font_manager): Memory leak --- include/x11/fonts.hpp | 49 ++++++------- src/CMakeLists.txt | 5 +- src/components/renderer.cpp | 14 ++-- src/x11/fonts.cpp | 137 +++++++++++++++++++++--------------- 4 files changed, 113 insertions(+), 92 deletions(-) diff --git a/include/x11/fonts.hpp b/include/x11/fonts.hpp index 1ff24a51..0d1e5090 100644 --- a/include/x11/fonts.hpp +++ b/include/x11/fonts.hpp @@ -13,29 +13,25 @@ POLYBAR_NS class connection; class logger; -#define XFT_MAXCHARS (1 << 16) -extern array xft_widths; -extern array xft_chars; - struct fonttype { - fonttype() {} - XftFont* xft; - xcb_font_t ptr; - int offset_y = 0; - int ascent = 0; - int descent = 0; - int height = 0; - int width = 0; - uint16_t char_max = 0; - uint16_t char_min = 0; - vector width_lut; + explicit fonttype() = default; + XftFont* xft{nullptr}; + xcb_font_t ptr{XCB_NONE}; + int offset_y{0}; + int ascent{0}; + int descent{0}; + int height{0}; + int width{0}; + uint16_t char_max{0}; + uint16_t char_min{0}; + vector width_lut{}; }; struct fonttype_deleter { void operator()(fonttype* f); }; -using font_t = unique_ptr; +using fonttype_pointer = unique_ptr; class font_manager { public: @@ -49,22 +45,23 @@ class font_manager { void set_preferred_font(int8_t index); - font_t& match_char(uint16_t chr); - uint8_t char_width(font_t& font, uint16_t chr); + fonttype_pointer& match_char(uint16_t chr); + uint8_t char_width(fonttype_pointer& font, uint16_t chr); - XftColor xftcolor(); + XftColor* xftcolor(); XftDraw* xftdraw(); - XftDraw* create_xftdraw(xcb_pixmap_t pm, xcb_colormap_t cm); + + void create_xftdraw(xcb_pixmap_t pm); void destroy_xftdraw(); - void allocate_color(uint32_t color, bool initial_alloc = false); - void allocate_color(XRenderColor color, bool initial_alloc = false); + void allocate_color(uint32_t color); + void allocate_color(XRenderColor color); void set_gcontext_font(xcb_gcontext_t gc, xcb_font_t font); protected: - bool open_xcb_font(font_t& fontptr, string fontname); - bool has_glyph(font_t& font, uint16_t chr); + bool open_xcb_font(fonttype_pointer& fontptr, string fontname); + bool has_glyph(fonttype_pointer& font, uint16_t chr); private: connection& m_connection; @@ -74,10 +71,10 @@ class font_manager { Visual* m_visual{nullptr}; Colormap m_colormap{}; - std::map m_fonts; + std::map m_fonts; int8_t m_fontindex{DEFAULT_FONT_INDEX}; - XftColor m_xftcolor{}; + XftColor* m_xftcolor{nullptr}; XftDraw* m_xftdraw{nullptr}; }; diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 237ea5dc..12b85a04 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -49,14 +49,13 @@ find_package(X11_XCB REQUIRED) find_package(PkgConfig) pkg_check_modules(FONTCONFIG REQUIRED fontconfig) -set(APP_LIBRARIES ${APP_LIBRARIES} ${BOOST_LIBRARIES}) set(APP_LIBRARIES ${APP_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}) set(APP_LIBRARIES ${APP_LIBRARIES} ${X11_Xft_LIB}) +set(APP_LIBRARIES ${APP_LIBRARIES} ${FONTCONFIG_LIBRARIES}) -set(APP_INCLUDE_DIRS ${APP_INCLUDE_DIRS} ${BOOST_INCLUDE_DIR}) +set(APP_INCLUDE_DIRS ${APP_INCLUDE_DIRS} ${Boost_INCLUDE_DIR}) set(APP_INCLUDE_DIRS ${APP_INCLUDE_DIRS} ${FONTCONFIG_INCLUDE_DIRS}) set(APP_INCLUDE_DIRS ${APP_INCLUDE_DIRS} ${PROJECT_SOURCE_DIR}/include) -set(APP_INCLUDE_DIRS ${APP_INCLUDE_DIRS} ${PROJECT_SOURCE_DIR}/lib/boost/include) set(APP_INCLUDE_DIRS ${APP_INCLUDE_DIRS} ${PROJECT_SOURCE_DIR}/lib/concurrentqueue/include) set(APP_INCLUDE_DIRS ${APP_INCLUDE_DIRS} ${CMAKE_CURRENT_BINARY_DIR}) diff --git a/src/components/renderer.cpp b/src/components/renderer.cpp index 66c5eb95..2f1cedd5 100644 --- a/src/components/renderer.cpp +++ b/src/components/renderer.cpp @@ -3,8 +3,8 @@ #include "components/types.hpp" #include "errors.hpp" #include "events/signal.hpp" -#include "events/signal_emitter.hpp" #include "events/signal.hpp" +#include "events/signal_emitter.hpp" #include "x11/connection.hpp" #include "x11/draw.hpp" #include "x11/fonts.hpp" @@ -137,7 +137,7 @@ renderer::renderer(connection& conn, signal_emitter& emitter, const logger& logg if (!fonts_loaded && !m_fontmanager->load("fixed")) { throw application_error("Unable to load fonts"); } - m_fontmanager->allocate_color(m_bar.foreground, true); + m_fontmanager->allocate_color(m_bar.foreground); } } @@ -171,7 +171,7 @@ void renderer::begin() { m_attributes = 0; m_actions.clear(); - m_fontmanager->create_xftdraw(m_pixmap, m_colormap); + m_fontmanager->create_xftdraw(m_pixmap); } /** @@ -368,8 +368,7 @@ void renderer::draw_character(uint16_t character) { auto y = m_rect.height / 2 + font->height / 2 - font->descent + font->offset_y; if (font->xft != nullptr) { - auto color = m_fontmanager->xftcolor(); - XftDrawString16(m_fontmanager->xftdraw(), &color, font->xft, x, y, &character, 1); + XftDrawString16(m_fontmanager->xftdraw(), m_fontmanager->xftcolor(), font->xft, x, y, &character, 1); } else { uint16_t ucs = ((character >> 8) | (character << 8)); draw_util::xcb_poly_text_16_patched(m_connection, m_pixmap, m_gcontexts.at(gc::FG), x, y, 1, &ucs); @@ -409,9 +408,8 @@ void renderer::draw_textstring(const char* text, size_t len) { auto y = m_rect.height / 2 + font->height / 2 - font->descent + font->offset_y; if (font->xft != nullptr) { - auto color = m_fontmanager->xftcolor(); - const FcChar16* drawchars = static_cast(chars.data()); - XftDrawString16(m_fontmanager->xftdraw(), &color, font->xft, x, y, drawchars, chars.size()); + XftDrawString16(m_fontmanager->xftdraw(), m_fontmanager->xftcolor(), font->xft, x, y, + static_cast(chars.data()), chars.size()); } else { for (unsigned short& i : chars) { i = ((i >> 8) | (i << 8)); diff --git a/src/x11/fonts.cpp b/src/x11/fonts.cpp index a84fb2ac..4f5cb76a 100644 --- a/src/x11/fonts.cpp +++ b/src/x11/fonts.cpp @@ -1,6 +1,7 @@ #include #include "components/logger.hpp" +#include "errors.hpp" #include "utils/color.hpp" #include "utils/factory.hpp" #include "utils/memory.hpp" @@ -11,6 +12,11 @@ POLYBAR_NS +#define XFT_MAXCHARS (1 << 16) + +array g_xft_widths; +array g_xft_chars; + /** * Create instance */ @@ -18,27 +24,34 @@ font_manager::make_type font_manager::make() { return factory_util::unique(connection::make(), logger::make()); } -array xft_widths; -array xft_chars; - void fonttype_deleter::operator()(fonttype* f) { if (f->xft != nullptr) { XftFontClose(xlib::get_display(), f->xft); - } else { - xcb_close_font(xutils::get_connection(), f->ptr); + free(f->xft); } + if (f->ptr != XCB_NONE) { + connection::make().close_font(f->ptr); + } + delete f; } font_manager::font_manager(connection& conn, const logger& logger) : m_connection(conn), m_logger(logger) { - m_display = xlib::get_display(); + if ((m_display = xlib::get_display()) == nullptr) { + throw application_error("Failed to get X display"); + } m_visual = xlib::get_visual(conn.default_screen()); m_colormap = xlib::create_colormap(conn.default_screen()); } font_manager::~font_manager() { - XftColorFree(m_display, m_visual, m_colormap, &m_xftcolor); - XFreeColormap(m_display, m_colormap); - m_fonts.clear(); + if (m_display != nullptr) { + if (m_xftcolor != nullptr) { + XftColorFree(m_display, m_visual, m_colormap, m_xftcolor); + free(m_xftcolor); + } + destroy_xftdraw(); + XFreeColormap(m_display, m_colormap); + } } bool font_manager::load(const string& name, int8_t fontindex, int8_t offset_y) { @@ -52,23 +65,37 @@ bool font_manager::load(const string& name, int8_t fontindex, int8_t offset_y) { m_logger.trace("font_manager: Add font '%s' to index '%i'", name, fontindex); } - m_fonts.emplace(make_pair(fontindex, font_t{new fonttype(), fonttype_deleter{}})); - m_fonts[fontindex]->offset_y = offset_y; - m_fonts[fontindex]->ptr = 0; - m_fonts[fontindex]->xft = nullptr; + fonttype_pointer f{new fonttype_pointer::element_type{}, fonttype_deleter{}}; + f->offset_y = offset_y; - if (open_xcb_font(m_fonts[fontindex], name)) { - m_logger.trace("font_manager: Successfully loaded X font '%s'", name); - } else if ((m_fonts[fontindex]->xft = XftFontOpenName(m_display, 0, name.c_str())) != nullptr) { - m_fonts[fontindex]->ptr = 0; - m_fonts[fontindex]->ascent = m_fonts[fontindex]->xft->ascent; - m_fonts[fontindex]->descent = m_fonts[fontindex]->xft->descent; - m_fonts[fontindex]->height = m_fonts[fontindex]->ascent + m_fonts[fontindex]->descent; - m_logger.trace("font_manager: Successfully loaded Freetype font '%s'", name); - } else { + if (open_xcb_font(f, name)) { + m_logger.info("Loaded font (xlfd=%s)", name); + } else if (f->ptr != XCB_NONE) { + m_connection.close_font_checked(f->ptr); + f->ptr = XCB_NONE; + } + + if (f->ptr == XCB_NONE && (f->xft = XftFontOpenName(m_display, m_connection.default_screen(), name.c_str())) != nullptr) { + f->ascent = f->xft->ascent; + f->descent = f->xft->descent; + f->height = f->ascent + f->descent; + + if (f->xft->pattern != nullptr) { + FcChar8* file{nullptr}; + FcPatternGetString(f->xft->pattern, "file", 0, &file); + m_logger.info("Loaded font (pattern=%s, file=%s)", name, file); + free(file); + } else { + m_logger.info("Loaded font (pattern=%s)", name); + } + } + + if (f->ptr == XCB_NONE && f->xft == nullptr) { return false; } + m_fonts.emplace(make_pair(fontindex, move(f))); + int max_height = 0; for (auto& iter : m_fonts) { @@ -98,8 +125,8 @@ void font_manager::set_preferred_font(int8_t index) { } } -font_t& font_manager::match_char(uint16_t chr) { - static font_t notfound; +fonttype_pointer& font_manager::match_char(uint16_t chr) { + static fonttype_pointer notfound; if (!m_fonts.empty()) { if (m_fontindex != DEFAULT_FONT_INDEX && size_t(m_fontindex) <= m_fonts.size()) { auto iter = m_fonts.find(m_fontindex); @@ -116,7 +143,7 @@ font_t& font_manager::match_char(uint16_t chr) { return notfound; } -uint8_t font_manager::char_width(font_t& font, uint16_t chr) { +uint8_t font_manager::char_width(fonttype_pointer& font, uint16_t chr) { if (!font) { return 0; } @@ -130,27 +157,27 @@ uint8_t font_manager::char_width(font_t& font, uint16_t chr) { } auto index = chr % XFT_MAXCHARS; - while (xft_chars[index] != 0 && xft_chars[index] != chr) { + while (g_xft_chars[index] != 0 && g_xft_chars[index] != chr) { index = (index + 1) % XFT_MAXCHARS; } - if (!xft_chars[index]) { + if (!g_xft_chars[index]) { XGlyphInfo gi; - FT_UInt glyph = XftCharIndex(m_display, font->xft, (FcChar32)chr); + FT_UInt glyph = XftCharIndex(m_display, font->xft, static_cast(chr)); XftFontLoadGlyphs(m_display, font->xft, FcFalse, &glyph, 1); XftGlyphExtents(m_display, font->xft, &glyph, 1, &gi); XftFontUnloadGlyphs(m_display, font->xft, &glyph, 1); - xft_chars[index] = chr; - xft_widths[index] = gi.xOff; + g_xft_chars[index] = chr; + g_xft_widths[index] = gi.xOff; return gi.xOff; - } else if (xft_chars[index] == chr) { - return xft_widths[index]; + } else if (g_xft_chars[index] == chr) { + return g_xft_widths[index]; } return 0; } -XftColor font_manager::xftcolor() { +XftColor* font_manager::xftcolor() { return m_xftcolor; } @@ -158,30 +185,36 @@ XftDraw* font_manager::xftdraw() { return m_xftdraw; } -XftDraw* font_manager::create_xftdraw(xcb_pixmap_t pm, xcb_colormap_t cm) { - m_xftdraw = XftDrawCreate(xlib::get_display(), pm, xlib::get_visual(), cm); - return m_xftdraw; +void font_manager::create_xftdraw(xcb_pixmap_t pm) { + destroy_xftdraw(); + m_xftdraw = XftDrawCreate(m_display, pm, m_visual, m_colormap); } void font_manager::destroy_xftdraw() { - XftDrawDestroy(m_xftdraw); + if (m_xftdraw != nullptr) { + XftDrawDestroy(m_xftdraw); + m_xftdraw = nullptr; + } } -void font_manager::allocate_color(uint32_t color, bool initial_alloc) { +void font_manager::allocate_color(uint32_t color) { XRenderColor x; x.red = color_util::red_channel(color); x.green = color_util::green_channel(color); x.blue = color_util::blue_channel(color); x.alpha = color_util::alpha_channel(color); - allocate_color(x, initial_alloc); + allocate_color(x); } -void font_manager::allocate_color(XRenderColor color, bool initial_alloc) { - if (!initial_alloc) { - XftColorFree(m_display, m_visual, m_colormap, &m_xftcolor); +void font_manager::allocate_color(XRenderColor color) { + if (m_xftcolor != nullptr) { + XftColorFree(m_display, m_visual, m_colormap, m_xftcolor); + free(m_xftcolor); } - if (!XftColorAllocValue(m_display, m_visual, m_colormap, &color, &m_xftcolor)) { + m_xftcolor = static_cast(malloc(sizeof(XftColor))); + + if (!XftColorAllocValue(m_display, m_visual, m_colormap, &color, m_xftcolor)) { m_logger.err("Failed to allocate color"); } } @@ -191,14 +224,15 @@ void font_manager::set_gcontext_font(xcb_gcontext_t gc, xcb_font_t font) { m_connection.change_gc(gc, XCB_GC_FONT, values); } -bool font_manager::open_xcb_font(font_t& fontptr, string fontname) { +bool font_manager::open_xcb_font(fonttype_pointer& fontptr, string fontname) { try { - font xfont(m_connection, m_connection.generate_id()); + uint32_t font_id{m_connection.generate_id()}; + m_connection.open_font_checked(font_id, fontname); - m_connection.open_font_checked(xfont, fontname); m_logger.trace("Found X font '%s'", fontname); + fontptr->ptr = font_id; - auto query = m_connection.query_font(xfont); + auto query = m_connection.query_font(font_id); if (query->char_infos_len == 0) { m_logger.warn("X font '%s' does not contain any characters... (Verify the XLFD string)", fontname); return false; @@ -214,14 +248,7 @@ bool font_manager::open_xcb_font(font_t& fontptr, string fontname) { for (auto it = chars.begin(); it != chars.end(); it++) { fontptr->width_lut.emplace_back(forward(*it)); } - - fontptr->ptr = xfont; - return true; - } catch (const xpp::x::error::name& e) { - m_logger.trace("font_manager: Could not find X font '%s'", fontname); - } catch (const shared_ptr& e) { - m_logger.trace("font_manager: Could not find X font '%s'", fontname); } catch (const std::exception& e) { m_logger.trace("font_manager: Could not find X font '%s' (what: %s)", fontname, e.what()); } @@ -229,7 +256,7 @@ bool font_manager::open_xcb_font(font_t& fontptr, string fontname) { return false; } -bool font_manager::has_glyph(font_t& font, uint16_t chr) { +bool font_manager::has_glyph(fonttype_pointer& font, uint16_t chr) { if (font->xft != nullptr) { return static_cast(XftCharExists(m_display, font->xft, (FcChar32)chr)); } else {