Fix memory leaks in SetFont/ResolveFontDescription

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
 ==================

src/Canvas.cc

@@ -6,7 +6,11 @@
 
 #include <assert.h>
 #include <stdlib.h>
-#include <string.h>
+#include <cstring>
+#include <cctype>
+#include <string>
+#include <vector>
+#include <unordered_set>
 #include <node_buffer.h>
 #include <node_version.h>
 #include <glib.h>
@@ -35,6 +39,8 @@
 
 Nan::Persistent<FunctionTemplate> Canvas::constructor;
 
+std::vector<FontFace> 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<FontFace>::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<FontFace>
-_init_font_face_list() {
-  std::vector<FontFace> x;
-  return x;
-}
-
-std::vector<FontFace> Canvas::_font_face_list = _init_font_face_list();
-
 /*
  * Get a PangoStyle from a CSS string (like "italic")
  */
@@ -782,57 +775,57 @@ 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
  */
 
 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<FontFace>::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<string> 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;
 }

src/Canvas.h

@@ -13,7 +13,6 @@
 #include <node_object_wrap.h>
 #include <node_version.h>
 #include <pango/pangocairo.h>
-#include <vector>
 #include <cairo.h>
 #include <nan.h>
 
@@ -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<FontFace> _font_face_list;
 };
 
 #endif

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()

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<v8::Value> &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();
 

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<FunctionTemplate> ImageBackend::constructor;
 
-void ImageBackend::Initialize(Handle<Object> target)
-{
+void ImageBackend::Initialize(Handle<Object> target) {
 	Nan::HandleScope scope;
 
 	Local<FunctionTemplate> ctor = Nan::New<FunctionTemplate>(ImageBackend::New);

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();