From 744ccae74681f76f03a478e2099616146f48fd69 Mon Sep 17 00:00:00 2001 From: Zach Bjornson Date: Sat, 5 Jan 2019 00:23:12 -0800 Subject: [PATCH 1/2] Fix memory leak in SetFont/ResolveFontDescription Also about 55% faster. Fixes #1202 --- CHANGELOG.md | 3 +- src/Canvas.cc | 95 ++++++++++++++++++++++++--------------------------- src/Canvas.h | 6 ++-- 3 files changed, 48 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35cfcc457..502d237d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,8 @@ project adheres to [Semantic Versioning](http://semver.org/). ### Fixed * Don't crash when font string is invalid (bug since 2.2.0) (#1328) -* Fix memory leak in toBuffer +* Fix memory leak in `canvas.toBuffer()` (#1202, #1296) +* Fix memory leak in `ctx.font=` (#1202) 2.2.0 ================== diff --git a/src/Canvas.cc b/src/Canvas.cc index 46af72730..c629ff915 100644 --- a/src/Canvas.cc +++ b/src/Canvas.cc @@ -6,7 +6,11 @@ #include #include -#include +#include +#include +#include +#include +#include #include #include #include @@ -35,6 +39,8 @@ Nan::Persistent Canvas::constructor; +std::vector font_face_list; + /* * Initialize Canvas. */ @@ -671,23 +677,18 @@ NAN_METHOD(Canvas::RegisterFont) { pango_font_description_set_style(user_desc, Canvas::GetStyleFromCSSString(style)); pango_font_description_set_family(user_desc, family); - std::vector::iterator it = _font_face_list.begin(); - FontFace *already_registered = NULL; - - for (; it != _font_face_list.end() && !already_registered; ++it) { - if (pango_font_description_equal(it->sys_desc, sys_desc)) { - already_registered = &(*it); - } - } - - if (already_registered) { - pango_font_description_free(already_registered->user_desc); - already_registered->user_desc = user_desc; + auto found = std::find_if(font_face_list.begin(), font_face_list.end(), [&](FontFace& f) { + return pango_font_description_equal(f.sys_desc, sys_desc); + }); + + if (found != font_face_list.end()) { + pango_font_description_free(found->user_desc); + found->user_desc = user_desc; } else if (register_font((unsigned char *) *filePath)) { FontFace face; face.user_desc = user_desc; face.sys_desc = sys_desc; - _font_face_list.push_back(face); + font_face_list.push_back(face); } else { pango_font_description_free(user_desc); Nan::ThrowError("Could not load font to the system's font host"); @@ -720,14 +721,6 @@ Canvas::~Canvas() { } } -std::vector -_init_font_face_list() { - std::vector x; - return x; -} - -std::vector Canvas::_font_face_list = _init_font_face_list(); - /* * Get a PangoStyle from a CSS string (like "italic") */ @@ -782,6 +775,12 @@ Canvas::GetWeightFromCSSString(const char *weight) { return w; } +bool streq_casein(std::string& str1, std::string& str2) { + return str1.size() == str2.size() && std::equal(str1.begin(), str1.end(), str2.begin(), [](char& c1, char& c2) { + return c1 == c2 || std::toupper(c1) == std::toupper(c2); + }); +} + /* * Given a user description, return a description that will select the * font either from the system or @font-face @@ -789,50 +788,44 @@ Canvas::GetWeightFromCSSString(const char *weight) { PangoFontDescription * Canvas::ResolveFontDescription(const PangoFontDescription *desc) { - FontFace best; - PangoFontDescription *ret = NULL; - // One of the user-specified families could map to multiple SFNT family names // if someone registered two different fonts under the same family name. // https://drafts.csswg.org/css-fonts-3/#font-style-matching - char **families = g_strsplit(pango_font_description_get_family(desc), ",", -1); - GHashTable *seen_families = g_hash_table_new(g_str_hash, g_str_equal); - GString *resolved_families = g_string_new(""); - - for (int i = 0; families[i]; ++i) { - GString *renamed_families = g_string_new(""); - std::vector::iterator it = _font_face_list.begin(); - - 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)); - bool unseen = g_hash_table_lookup(seen_families, name) == NULL; + FontFace best; + istringstream families(pango_font_description_get_family(desc)); + unordered_set seen_families; + string resolved_families; + bool first = true; + + for (string family; getline(families, family, ','); ) { + string renamed_families; + for (auto& ff : font_face_list) { + string pangofamily = string(pango_font_description_get_family(ff.user_desc)); + if (streq_casein(family, pangofamily)) { + const char* sys_desc_family_name = pango_font_description_get_family(ff.sys_desc); + bool unseen = seen_families.find(sys_desc_family_name) == seen_families.end(); // Avoid sending duplicate SFNT font names due to a bug in Pango for macOS: // https://bugzilla.gnome.org/show_bug.cgi?id=762873 if (unseen) { - g_hash_table_replace(seen_families, name, name); - if (renamed_families->len) g_string_append(renamed_families, ","); - g_string_append(renamed_families, name); + seen_families.insert(sys_desc_family_name); + if (renamed_families.size()) renamed_families += ','; + renamed_families += sys_desc_family_name; } - if (i == 0 && (best.user_desc == NULL || pango_font_description_better_match(desc, best.user_desc, it->user_desc))) { - best = *it; + if (first && (best.user_desc == nullptr || pango_font_description_better_match(desc, best.user_desc, ff.user_desc))) { + best = ff; } } } - if (resolved_families->len) g_string_append(resolved_families, ","); - g_string_append(resolved_families, renamed_families->len ? renamed_families->str : families[i]); - g_string_free(renamed_families, true); + if (resolved_families.size()) resolved_families += ','; + resolved_families += renamed_families.size() ? renamed_families : family; + first = false; } - ret = pango_font_description_copy(best.sys_desc ? best.sys_desc : desc); - pango_font_description_set_family_static(ret, resolved_families->str); - - g_strfreev(families); - g_string_free(resolved_families, false); - g_hash_table_destroy(seen_families); + PangoFontDescription* ret = pango_font_description_copy(best.sys_desc ? best.sys_desc : desc); + pango_font_description_set_family(ret, resolved_families.c_str()); return ret; } diff --git a/src/Canvas.h b/src/Canvas.h index adaac1554..fe97dc5a8 100644 --- a/src/Canvas.h +++ b/src/Canvas.h @@ -13,7 +13,6 @@ #include #include #include -#include #include #include @@ -39,8 +38,8 @@ using namespace v8; */ class FontFace { public: - PangoFontDescription *sys_desc = NULL; - PangoFontDescription *user_desc = NULL; + PangoFontDescription *sys_desc = nullptr; + PangoFontDescription *user_desc = nullptr; }; /* @@ -88,7 +87,6 @@ class Canvas: public Nan::ObjectWrap { private: ~Canvas(); Backend* _backend; - static std::vector _font_face_list; }; #endif From e04e57607a9d5e2fa2def6bd76551f5b51e4e71d Mon Sep 17 00:00:00 2001 From: Zach Bjornson Date: Sat, 5 Jan 2019 14:40:20 -0800 Subject: [PATCH 2/2] fix incorrect external memory reporting Moves the AdjustExternalMemory calls to always be adjacent to cairo_image_surface_create/destroy so they can't be used incorrectly. See https://github.com/Automattic/node-canvas/issues/1202#issuecomment-451445282 --- src/backend/Backend.cc | 3 --- src/backend/Backend.h | 6 +++--- src/backend/ImageBackend.cc | 40 +++++++++++-------------------------- src/backend/ImageBackend.h | 3 +-- 4 files changed, 16 insertions(+), 36 deletions(-) diff --git a/src/backend/Backend.cc b/src/backend/Backend.cc index 34ef541f5..66b6af84d 100644 --- a/src/backend/Backend.cc +++ b/src/backend/Backend.cc @@ -1,12 +1,9 @@ #include "Backend.h" - Backend::Backend(string name, int width, int height) : name(name) , width(width) , height(height) - , surface(NULL) - , canvas(NULL) {} Backend::~Backend() diff --git a/src/backend/Backend.h b/src/backend/Backend.h index 75ae37aad..f310d9769 100644 --- a/src/backend/Backend.h +++ b/src/backend/Backend.h @@ -25,8 +25,8 @@ class Backend : public Nan::ObjectWrap protected: int width; int height; - cairo_surface_t* surface; - Canvas* canvas; + cairo_surface_t* surface = nullptr; + Canvas* canvas = nullptr; Backend(string name, int width, int height); static void init(const Nan::FunctionCallbackInfo &info); @@ -41,7 +41,7 @@ class Backend : public Nan::ObjectWrap virtual cairo_surface_t* recreateSurface(); DLL_PUBLIC cairo_surface_t* getSurface(); - void destroySurface(); + virtual void destroySurface(); DLL_PUBLIC string getName(); diff --git a/src/backend/ImageBackend.cc b/src/backend/ImageBackend.cc index b9d169af8..dfae3dd13 100644 --- a/src/backend/ImageBackend.cc +++ b/src/backend/ImageBackend.cc @@ -6,14 +6,6 @@ ImageBackend::ImageBackend(int width, int height) : Backend("image", width, height) {} -ImageBackend::~ImageBackend() -{ - if (surface) { - destroySurface(); - Nan::AdjustExternalMemory(-approxBytesPerPixel() * width * height); - } -} - Backend *ImageBackend::construct(int width, int height){ return new ImageBackend(width, height); } @@ -39,27 +31,20 @@ int32_t ImageBackend::approxBytesPerPixel() { } } -cairo_surface_t* ImageBackend::createSurface() -{ - assert(!this->surface); - this->surface = cairo_image_surface_create(this->format, width, height); - assert(this->surface); +cairo_surface_t* ImageBackend::createSurface() { + assert(!surface); + surface = cairo_image_surface_create(format, width, height); + assert(surface); Nan::AdjustExternalMemory(approxBytesPerPixel() * width * height); - - return this->surface; + return surface; } -cairo_surface_t* ImageBackend::recreateSurface() -{ - // Re-surface - if (this->surface) { - int old_width = cairo_image_surface_get_width(this->surface); - int old_height = cairo_image_surface_get_height(this->surface); - this->destroySurface(); - Nan::AdjustExternalMemory(-approxBytesPerPixel() * old_width * old_height); - } - - return createSurface(); +void ImageBackend::destroySurface() { + if (surface) { + cairo_surface_destroy(surface); + surface = nullptr; + Nan::AdjustExternalMemory(-approxBytesPerPixel() * width * height); + } } cairo_format_t ImageBackend::getFormat() { @@ -72,8 +57,7 @@ void ImageBackend::setFormat(cairo_format_t _format) { Nan::Persistent ImageBackend::constructor; -void ImageBackend::Initialize(Handle target) -{ +void ImageBackend::Initialize(Handle target) { Nan::HandleScope scope; Local ctor = Nan::New(ImageBackend::New); diff --git a/src/backend/ImageBackend.h b/src/backend/ImageBackend.h index c245565e0..4c002abe4 100644 --- a/src/backend/ImageBackend.h +++ b/src/backend/ImageBackend.h @@ -11,12 +11,11 @@ class ImageBackend : public Backend { private: cairo_surface_t* createSurface(); - cairo_surface_t* recreateSurface(); + void destroySurface(); cairo_format_t format = DEFAULT_FORMAT; public: ImageBackend(int width, int height); - ~ImageBackend(); static Backend *construct(int width, int height); cairo_format_t getFormat();