Permalink
Browse files

Remove crash causes my what is likely a Fontconfig bug and no GC issue

Not sure why, but when we don't immediately use the fpattern variable we
received from Fontconfig, later calls on it smash memory which makes it
impossible for the GC to free the memory later (from FreeType, not
Fontconfig! - which makes no sense), but works when freeing it
explicitly.

I currently assume that these issues are all multithreading-related and
due to Fontconfig being absolutely non-threadsafe.
It doesn't really make sense for these weird bugs to occur, neither does
it make sense that this change fixes the issues. But since others use
Fontconfig the same way, and with these changes I didn't see any crash
in 30 runs, this patch seems to reliably fix the bug.
  • Loading branch information...
1 parent f955629 commit 6ab455ed9c33c2e014a52d8c9539ba5169a6131d @ximion committed Sep 24, 2016
Showing with 56 additions and 77 deletions.
  1. +56 −70 source/font.d
  2. +0 −7 source/handlers/fonthandler.d
View
@@ -76,9 +76,6 @@ private:
FT_Library library;
FT_Face fface;
- FcConfig *fconfig;
- FcPattern *fpattern;
-
string[] languages_;
string sampleText_;
string sampleIconText_;
@@ -96,34 +93,26 @@ public:
if (err != 0)
throw new Exception ("Unable to load font face from file. Error code: %s".format (err));
- loadFontConfig (fname);
+ loadFontConfigData (fname);
}
}
this (const(ubyte)[] data, string fileBaseName)
{
import std.stdio : File;
- synchronized {
- initFreeType ();
- // we unfortunately need to create a stupid temporary file here, otherwise Fontconfig
- // does not work and we can not determine the right demo strings for this font.
- // (FreeType itself could load from memory)
- auto cacheRoot = Config.get ().cacheRootDir;
- if (!std.file.exists (cacheRoot))
- cacheRoot = "/tmp/";
- immutable fname = buildPath (cacheRoot, fileBaseName);
- auto f = File (fname, "w");
- f.rawWrite (data);
- f.close ();
-
- FT_Error err;
- err = FT_New_Face (library, fname.toStringz (), 0, &fface);
- if (err != 0)
- throw new Exception ("Unable to load font face from file. Error code: %s".format (err));
-
- loadFontConfig (fname);
- }
+ // we unfortunately need to create a stupid temporary file here, otherwise Fontconfig
+ // does not work and we can not determine the right demo strings for this font.
+ // (FreeType itself could load from memory)
+ auto cacheRoot = Config.get ().cacheRootDir;
+ if (!std.file.exists (cacheRoot))
+ cacheRoot = "/tmp/";
+ immutable fname = buildPath (cacheRoot, fileBaseName);
+ auto f = File (fname, "w");
+ f.rawWrite (data);
+ f.close ();
+
+ this (fname);
}
~this ()
@@ -140,19 +129,14 @@ public:
FT_Done_Face (fface);
if (library !is null)
FT_Done_Library (library);
- if (fconfig !is null) {
- //! FcConfigAppFontClear (fconfig); // FIXME: This crashes...
- FcConfigDestroy (fconfig);
- }
fface = null;
library = null;
- fconfig = null;
}
private bool ready ()
{
- return fface !is null && library !is null && fconfig !is null;
+ return fface !is null && library !is null;
}
private void initFreeType ()
@@ -166,10 +150,14 @@ public:
throw new Exception ("Unable to load FreeType. Error code: %s".format (err));
}
- private void loadFontConfig (string fname)
+ private void loadFontConfigData (string fname)
{
// create a new fontconfig configuration
- fconfig = FcConfigCreate ();
+ auto fconfig = FcConfigCreate ();
+ scope (exit) {
+ FcConfigAppFontClear (fconfig);
+ FcConfigDestroy (fconfig);
+ }
// ensure that default configuration and fonts are not loaded
FcConfigSetCurrent (fconfig);
@@ -180,7 +168,42 @@ public:
if (fonts is null || fonts.fonts is null) {
throw new Exception ("FcConfigGetFonts failed (for %s)".format (fname.baseName));
}
- fpattern = fonts.fonts[0];
+ auto fpattern = fonts.fonts[0];
+
+ // initialize our icon-text map globally
+ initIconTextMap ();
+
+ // load supported locale
+ auto fcRc = FcResult.Match;
+ FcValue fcValue;
+ auto res = appender!(string[]);
+
+ auto anyAdded = false;
+ for (uint i = 0; fcRc == FcResult.Match; i++) {
+ FcLangSet *ls;
+
+ fcRc = FcPatternGetLangSet (fpattern, FC_LANG, i, &ls);
+ if (fcRc == FcResult.Match) {
+ auto langs = FcLangSetGetLangs (ls);
+ auto list = FcStrListCreate (langs);
+ scope (exit) {
+ FcStrListDone (list);
+ FcStrSetDestroy (langs);
+ }
+
+ char *tmp;
+ FcStrListFirst (list);
+ while ((tmp = FcStrListNext (list)) !is null) {
+ res ~= to!string (tmp.fromStringz);
+ anyAdded = true;
+ }
+ }
+ }
+
+ // assume 'en' is available
+ if (!anyAdded)
+ res ~= "en";
+ languages_ = res.data;
}
@property
@@ -230,43 +253,6 @@ public:
@property
string[] languages ()
{
- // check if we have cached this result
- if (!languages_.empty)
- return languages_;
- assert (ready ());
-
- initIconTextMap ();
-
- auto fcRc = FcResult.Match;
- FcValue fcValue;
- auto res = appender!(string[]);
-
- auto anyAdded = false;
- for (uint i = 0; fcRc == FcResult.Match; i++) {
- FcLangSet *ls;
-
- fcRc = FcPatternGetLangSet (fpattern, FC_LANG, i, &ls);
- if (fcRc == FcResult.Match) {
- auto langs = FcLangSetGetLangs (ls);
- auto list = FcStrListCreate (langs);
- scope (exit) {
- FcStrListDone (list);
- FcStrSetDestroy (langs);
- }
-
- char *tmp;
- FcStrListFirst (list);
- while ((tmp = FcStrListNext (list)) !is null) {
- res ~= to!string (tmp.fromStringz);
- anyAdded = true;
- }
- }
- }
-
- // assume 'en' is available
- if (!anyAdded)
- res ~= "en";
- languages_ = res.data;
return languages_;
}
@@ -118,11 +118,6 @@ void processFontDataInternal (GeneratorResult gres, Component cpt, string mediaE
// TODO: Catch errors
auto font = new Font (fdata, fontFile.baseName);
- // FIXME: For some reason we need to ensure that *we* free the resources
- // owned by the Font object, and not the GC.
- // Otherwise this will crash with SIGBUS in FreeType.
- scope (exit) font.release ();
-
// add language information
foreach (ref lang; font.languages) {
cpt.addLanguage (lang, 100);
@@ -156,8 +151,6 @@ private bool renderFontIcon (GeneratorResult gres, Font font, string fontFile, i
immutable iconName = format ("%s_%s.png", gres.pkgname, fid);
immutable iconStoreLocation = buildPath (path, iconName);
-
-
if (!std.file.exists (iconStoreLocation)) {
// we didn't create an icon yet - render it
auto cv = new Canvas (size.width, size.height);

0 comments on commit 6ab455e

Please sign in to comment.