SkFontHost_FreeType constructor to correctly release resources.

src/ports/SkFontHost_FreeType.cpp

Index: src/ports/SkFontHost_FreeType.cpp
diff --git a/src/ports/SkFontHost_FreeType.cpp b/src/ports/SkFontHost_FreeType.cpp
index 181116a15e09d4b1853a17abeb0da73a261e6d16..9230507662974fd729d2fd20d66828a1eac1a1c6 100644
--- a/src/ports/SkFontHost_FreeType.cpp
+++ b/src/ports/SkFontHost_FreeType.cpp
@@ -25,6 +25,7 @@
 #include "SkString.h"
 #include "SkTemplates.h"
 #include "SkTypes.h"
+#include "SkUniquePtr.h"
 
 #if defined(SK_CAN_USE_DLOPEN)
 #include <dlfcn.h>
@@ -148,7 +149,7 @@ SK_DECLARE_STATIC_MUTEX(gFTMutex);
 static FreeTypeLibrary* gFTLibrary;
 static SkFaceRec* gFaceRecHead;
 
-// Private to RefFreeType and UnrefFreeType
+// Private to ref_ft_library and unref_ft_library
 static int gFTCount;
 
 // Caller must lock gFTMutex before calling this function.
@@ -171,6 +172,7 @@ static void unref_ft_library() {
 
     --gFTCount;
     if (0 == gFTCount) {
+        SkASSERT(nullptr == gFaceRecHead);

[bungeman-skia, 2016/03/01 22:28:06]

Adding this assert would have helped find this a bit faster. It also documents
something assumed anyway, which is that if the library is going away then all
the faces should be done.

         SkASSERT(nullptr != gFTLibrary);
         delete gFTLibrary;
         SkDEBUGCODE(gFTLibrary = nullptr;)
@@ -675,6 +677,8 @@ void SkTypeface_FreeType::onFilterRec(SkScalerContextRec* rec) const {
     //BOGUS: http://code.google.com/p/chromium/issues/detail?id=121119
     //Cap the requested size as larger sizes give bogus values.
     //Remove when http://code.google.com/p/skia/issues/detail?id=554 is fixed.
+    //Note that this also currently only protects against large text size requests,
+    //the total matrix is not taken into account here.

[bungeman-skia, 2016/03/01 22:28:06]

All of the 'onFilterRec' implementations should be using this opportunity to
call computeMatrices and normalize everything. This would allow for better
checking here, as well as result in more cache hits.

     if (rec->fTextSize > SkIntToScalar(1 << 14)) {
         rec->fTextSize = SkIntToScalar(1 << 14);
     }
@@ -786,6 +790,9 @@ static FT_Int chooseBitmapStrike(FT_Face face, FT_F26Dot6 scaleY) {
 
 SkScalerContext_FreeType::SkScalerContext_FreeType(SkTypeface* typeface, const SkDescriptor* desc)
     : SkScalerContext_FreeType_Base(typeface, desc)
+    , fFace(nullptr)
+    , fFTSize(nullptr)
+    , fStrikeIndex(-1)
 {
     SkAutoMutexAcquire  ac(gFTMutex);
 
@@ -794,10 +801,10 @@ SkScalerContext_FreeType::SkScalerContext_FreeType(SkTypeface* typeface, const S
     }
 
     // load the font file
-    fStrikeIndex = -1;
-    fFTSize = nullptr;
-    fFace = ref_ft_face(typeface);
-    if (nullptr == fFace) {
+    using UnrefFTFace = SkFunctionWrapper<void, skstd::remove_pointer_t<FT_Face>, unref_ft_face>;
+    skstd::unique_ptr<skstd::remove_pointer_t<FT_Face>, UnrefFTFace> ftFace(ref_ft_face(typeface));
+    if (nullptr == ftFace) {
+        SkDEBUGF(("Could not create FT_Face.\n"));
         return;
     }
 
@@ -883,34 +890,41 @@ SkScalerContext_FreeType::SkScalerContext_FreeType(SkTypeface* typeface, const S
         fLoadGlyphFlags = loadFlags;
     }
 
-    FT_Error err = FT_New_Size(fFace, &fFTSize);
-    if (err != 0) {
-        SkDEBUGF(("FT_New_Size returned %x for face %s\n", err, fFace->family_name));
-        fFace = nullptr;
+    using DoneFTSize = SkFunctionWrapper<FT_Error, skstd::remove_pointer_t<FT_Size>, FT_Done_Size>;
+    skstd::unique_ptr<skstd::remove_pointer_t<FT_Size>, DoneFTSize> ftSize([&ftFace]() -> FT_Size {
+        FT_Size size;
+        FT_Error err = FT_New_Size(ftFace.get(), &size);
+        if (err != 0) {
+            SkDEBUGF(("FT_New_Size returned %x for face %s\n", err, ftFace->family_name));
+            return nullptr;
+        }
+        return size;

[dogben, 2016/03/02 00:55:22]

ignorable nit: Seems like it would be simpler to default-construct ftSize,
change the lambda to a block, and make this line ftSize.reset(size). Then also
remove the duplicate nullptr check below.

+    }());
+    if (nullptr == ftSize) {
+        SkDEBUGF(("Could not create FT_Size.\n"));
         return;
     }
-    err = FT_Activate_Size(fFTSize);
+
+    FT_Error err = FT_Activate_Size(ftSize.get());
     if (err != 0) {
-        SkDEBUGF(("FT_Activate_Size(%08x, 0x%x, 0x%x) returned 0x%x\n", fFace, fScaleX, fScaleY,
-                  err));
-        fFTSize = nullptr;
+        SkDEBUGF(("FT_Activate_Size(%08x, 0x%x, 0x%x) returned 0x%x\n",
+                         ftFace.get(), fScaleX,   fScaleY,       err));
         return;
     }
 
-    if (FT_IS_SCALABLE(fFace)) {
-        err = FT_Set_Char_Size(fFace, fScaleX, fScaleY, 72, 72);
+    if (FT_IS_SCALABLE(ftFace)) {
+        err = FT_Set_Char_Size(ftFace.get(), fScaleX, fScaleY, 72, 72);
         if (err != 0) {
             SkDEBUGF(("FT_Set_CharSize(%08x, 0x%x, 0x%x) returned 0x%x\n",
-                                    fFace, fScaleX, fScaleY,      err));
-            fFace = nullptr;

[bungeman-skia, 2016/03/01 22:28:06]

The related bug was because we went down this path. The fFace is being set to
nullptr (so that this->success() would return false) but unref_ft_face(fFace) is
not being called. This leaves the face cache dirty.

+                               ftFace.get(), fScaleX, fScaleY,      err));
             return;
         }
-        FT_Set_Transform(fFace, &fMatrix22, nullptr);
-    } else if (FT_HAS_FIXED_SIZES(fFace)) {
-        fStrikeIndex = chooseBitmapStrike(fFace, fScaleY);
+        FT_Set_Transform(ftFace.get(), &fMatrix22, nullptr);
+    } else if (FT_HAS_FIXED_SIZES(ftFace)) {
+        fStrikeIndex = chooseBitmapStrike(ftFace.get(), fScaleY);
         if (fStrikeIndex == -1) {
             SkDEBUGF(("no glyphs for font \"%s\" size %f?\n",
-                            fFace->family_name,       SkFDot6ToScalar(fScaleY)));
+                            ftFace->family_name,      SkFDot6ToScalar(fScaleY)));
         } else {
             // FreeType does no provide linear metrics for bitmap fonts.
             linearMetrics = false;
@@ -928,6 +942,8 @@ SkScalerContext_FreeType::SkScalerContext_FreeType(SkTypeface* typeface, const S
                             fFace->family_name,     SkFDot6ToScalar(fScaleY)));
     }
 
+    fFTSize = ftSize.release();
+    fFace = ftFace.release();
     fDoLinearMetrics = linearMetrics;
 }