SkFontHost_FreeType constructor to correctly release resources.

src/ports/SkFontHost_FreeType.cpp

 /*
  * Copyright 2006 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkAdvancedTypefaceMetrics.h"
 #include "SkBitmap.h"
 #include "SkCanvas.h"
 #include "SkColorPriv.h"
 #include "SkDescriptor.h"
 #include "SkFDot6.h"
 #include "SkFontDescriptor.h"
 #include "SkFontHost_FreeType_common.h"
 #include "SkGlyph.h"
 #include "SkMask.h"
 #include "SkMaskGamma.h"
 #include "SkMatrix22.h"
 #include "SkMutex.h"
 #include "SkOTUtils.h"
 #include "SkPath.h"
 #include "SkScalerContext.h"
 #include "SkStream.h"
 #include "SkString.h"
 #include "SkTemplates.h"
 #include "SkTypes.h"
+#include "SkUniquePtr.h"
 
 #if defined(SK_CAN_USE_DLOPEN)
 #include <dlfcn.h>
 #endif
 #include <ft2build.h>
 #include FT_ADVANCES_H
 #include FT_BITMAP_H
 #include FT_FREETYPE_H
 #include FT_LCD_FILTER_H
 #include FT_MODULE_H
@@ skipping 103 matching lines @@
     // Android >= Gingerbread (good)
     typedef FT_Error (*FT_Library_SetLcdFilterWeightsProc)(FT_Library, unsigned char*);
 };
 
 struct SkFaceRec;
 
 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.
 static bool ref_ft_library() {
     gFTMutex.assertHeld();
     SkASSERT(gFTCount >= 0);
 
     if (0 == gFTCount) {
         SkASSERT(nullptr == gFTLibrary);
         gFTLibrary = new FreeTypeLibrary;
     }
     ++gFTCount;
     return gFTLibrary->library();
 }
 
 // Caller must lock gFTMutex before calling this function.
 static void unref_ft_library() {
     gFTMutex.assertHeld();
     SkASSERT(gFTCount > 0);
 
     --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;)
     }
 }
 
 class SkScalerContext_FreeType : public SkScalerContext_FreeType_Base {
 public:
     SkScalerContext_FreeType(SkTypeface*, const SkDescriptor* desc);
     virtual ~SkScalerContext_FreeType();
@@ skipping 484 matching lines @@
         delete c;
         c = nullptr;
     }
     return c;
 }
 
 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);
     }
 
     if (isLCD(*rec)) {
         // TODO: re-work so that FreeType is set-up and selected by the SkFontMgr.
         SkAutoMutexAcquire ama(gFTMutex);
         ref_ft_library();
         if (!gFTLibrary->isLCDSupported()) {
             // If the runtime Freetype library doesn't support LCD, disable it here.
@@ skipping 91 matching lines @@
             SkDEBUGF(("FT_Select_Size(%s, %d) returned 0x%x\n", face->family_name,
                       chosenStrikeIndex, err));
             chosenStrikeIndex = -1;
         }
     }
     return chosenStrikeIndex;
 }
 
 SkScalerContext_FreeType::SkScalerContext_FreeType(SkTypeface* typeface, const SkDescriptor* desc)
     : SkScalerContext_FreeType_Base(typeface, desc)
+    , fFace(nullptr)
+    , fFTSize(nullptr)
+    , fStrikeIndex(-1)
 {
     SkAutoMutexAcquire  ac(gFTMutex);
 
     if (!ref_ft_library()) {
         sk_throw();
     }
 
     // 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;
     }
 
     fRec.computeMatrices(SkScalerContextRec::kFull_PreMatrixScale, &fScale, &fMatrix22Scalar);
     fMatrix22Scalar.setSkewX(-fMatrix22Scalar.getSkewX());
     fMatrix22Scalar.setSkewY(-fMatrix22Scalar.getSkewY());
 
     fScaleX = SkScalarToFDot6(fScale.fX);
     fScaleY = SkScalarToFDot6(fScale.fY);
     fMatrix22.xx = SkScalarToFixed(fMatrix22Scalar.getScaleX());
@@ skipping 65 matching lines @@
         // Use vertical layout if requested.
         if (fRec.fFlags & SkScalerContext::kVertical_Flag) {
             loadFlags |= FT_LOAD_VERTICAL_LAYOUT;
         }
 
         loadFlags |= FT_LOAD_COLOR;
 
         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;
-        return;
-    }
-    err = FT_Activate_Size(fFTSize);
-    if (err != 0) {
-        SkDEBUGF(("FT_Activate_Size(%08x, 0x%x, 0x%x) returned 0x%x\n", fFace, fScaleX, fScaleY,
-                  err));
-        fFTSize = 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;
     }
 
-    if (FT_IS_SCALABLE(fFace)) {
-        err = FT_Set_Char_Size(fFace, fScaleX, fScaleY, 72, 72);
+    FT_Error err = FT_Activate_Size(ftSize.get());
+    if (err != 0) {
+        SkDEBUGF(("FT_Activate_Size(%08x, 0x%x, 0x%x) returned 0x%x\n",
+                         ftFace.get(), fScaleX,   fScaleY,       err));
+        return;
+    }
+
+    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;
 
             // FreeType documentation says:
             // FT_LOAD_NO_BITMAP -- Ignore bitmap strikes when loading.
             // Bitmap-only fonts ignore this flag.
             //
             // However, in FreeType 2.5.1 color bitmap only fonts do not ignore this flag.
             // Force this flag off for bitmap only fonts.
             fLoadGlyphFlags &= ~FT_LOAD_NO_BITMAP;
         }
     } else {
         SkDEBUGF(("unknown kind of font \"%s\" size %f?\n",
                             fFace->family_name,     SkFDot6ToScalar(fScaleY)));
     }
 
+    fFTSize = ftSize.release();
+    fFace = ftFace.release();
     fDoLinearMetrics = linearMetrics;
 }
 
 SkScalerContext_FreeType::~SkScalerContext_FreeType() {
     SkAutoMutexAcquire  ac(gFTMutex);
 
     if (fFTSize != nullptr) {
         FT_Done_Size(fFTSize);
     }
 
@@ skipping 849 matching lines @@
                 SkDEBUGF(("Requested font axis not found: %s '%c%c%c%c'\n",
                           name.c_str(),
                           (skTag >> 24) & 0xFF,
                           (skTag >> 16) & 0xFF,
                           (skTag >>  8) & 0xFF,
                           (skTag)       & 0xFF));
             }
         }
     )
 }