Fix memory leak in GetTableData() and add unittests for it

Review URL: https://codereview.appspot.com/5693048

git-svn-id: http://skia.googlecode.com/svn/trunk@3239 2bbb7eff-a529-9590-31e7-b0007b416f81

diff --git a/src/ports/SkFontHost_mac_coretext.cpp b/src/ports/SkFontHost_mac_coretext.cpp
index e483250..914ceb0 100644 (file)
--- a/src/ports/SkFontHost_mac_coretext.cpp
+++ b/src/ports/SkFontHost_mac_coretext.cpp
@@ -31,6 +31,20 @@
 
 class SkScalerContext_Mac;
 
+static void CFSafeRelease(CFTypeRef obj) {
+    if (obj) {
+        CFRelease(obj);
+    }
+}
+
+class AutoCFRelease : SkNoncopyable {
+public:
+    AutoCFRelease(CFTypeRef obj) : fObj(obj) {}
+    ~AutoCFRelease() { CFSafeRelease(fObj); }
+    
+private:
+    CFTypeRef fObj;
+};
 
 // inline versions of these rect helpers
 
@@ -248,23 +262,6 @@ static SkScalar getFontScale(CGFontRef cgFont) {
     return SkScalarInvert(SkIntToScalar(unitsPerEm));
 }
 
-//============================================================================
-//      Macros
-//----------------------------------------------------------------------------
-// Release a CFTypeRef
-#ifndef CFSafeRelease
-#define CFSafeRelease(_object)                                      \
-    do                                                              \
-        {                                                           \
-        if ((_object) != NULL)                                      \
-            {                                                       \
-            CFRelease((CFTypeRef) (_object));                       \
-            (_object) = NULL;                                       \
-            }                                                       \
-        }                                                           \
-    while (false)
-#endif
-
 ///////////////////////////////////////////////////////////////////////////////
 
 #define BITMAP_INFO_RGB     (kCGImageAlphaNoneSkipFirst | kCGBitmapByteOrder32Host)
@@ -1888,50 +1885,34 @@ void SkFontHost::FilterRec(SkScalerContext::Rec* rec) {
 ///////////////////////////////////////////////////////////////////////////
 
 int SkFontHost::CountTables(SkFontID fontID) {
-    int             numTables;
-    CFArrayRef      cfArray;
-    CTFontRef       ctFont;
-
-
-    // Get the state we need
-    ctFont    = GetFontRefFromFontID(fontID);
-    cfArray   = CTFontCopyAvailableTables(ctFont, kCTFontTableOptionNoOptions);
-    numTables = 0;
-
-
-    // Get the table count
-    if (cfArray != NULL)
-        {
-        numTables = CFArrayGetCount(cfArray);
-        CFSafeRelease(cfArray);
-        }
-
-    return(numTables);
+    CTFontRef ctFont = GetFontRefFromFontID(fontID);
+    CFArrayRef cfArray = CTFontCopyAvailableTables(ctFont,
+                                                   kCTFontTableOptionNoOptions);
+    if (NULL == cfArray) {
+        return 0;
+    }
+    
+    AutoCFRelease ar(cfArray);
+    return CFArrayGetCount(cfArray);
 }
 
-int SkFontHost::GetTableTags(SkFontID fontID, SkFontTableTag tags[])
-{   int             n, numTables;
-    CFArrayRef      cfArray;
-    CTFontRef       ctFont;
-
-
-    // Get the state we need
-    ctFont    = GetFontRefFromFontID(fontID);
-    cfArray   = CTFontCopyAvailableTables(ctFont, kCTFontTableOptionNoOptions);
-    numTables = 0;
-
+int SkFontHost::GetTableTags(SkFontID fontID, SkFontTableTag tags[]) {
+    CTFontRef ctFont = GetFontRefFromFontID(fontID);
+    CFArrayRef cfArray = CTFontCopyAvailableTables(ctFont,
+                                                   kCTFontTableOptionNoOptions);
+    if (NULL == cfArray) {
+        return 0;
+    }
 
-    // Get the table tags
-    if (cfArray != NULL)
-        {
-        numTables = CFArrayGetCount(cfArray);
-        for (n = 0; n < numTables; n++)
-            tags[n] = (SkFontTableTag) ((uintptr_t) CFArrayGetValueAtIndex(cfArray, n));
+    AutoCFRelease ar(cfArray);
 
-        CFSafeRelease(cfArray);
+    int count = CFArrayGetCount(cfArray);
+    if (tags) {
+        for (int i = 0; i < count; ++i) {
+            tags[i] = (SkFontTableTag)CFArrayGetValueAtIndex(cfArray, i);
         }
-
-    return(numTables);
+    }
+    return count;
 }
 
 // If, as is the case with web fonts, the CTFont data isn't available,
@@ -1939,7 +1920,7 @@ int SkFontHost::GetTableTags(SkFontID fontID, SkFontTableTag tags[])
 // right result, leave the CTFont code path to minimize disruption.
 static CFDataRef copyTableFromFont(CTFontRef ctFont, SkFontTableTag tag) {
     CFDataRef data = CTFontCopyTable(ctFont, (CTFontTableTag) tag,
-            kCTFontTableOptionNoOptions);
+                                     kCTFontTableOptionNoOptions);
     if (NULL == data) {
         CGFontRef cgFont = CTFontCopyGraphicsFont(ctFont, NULL);
         data = CGFontCopyTableForTag(cgFont, tag);
@@ -1948,51 +1929,38 @@ static CFDataRef copyTableFromFont(CTFontRef ctFont, SkFontTableTag tag) {
     return data;
 }
 
-size_t SkFontHost::GetTableSize(SkFontID fontID, SkFontTableTag tag)
-{   size_t      theSize;
-    CTFontRef   ctFont;
-    CFDataRef   cfData;
-
-
-    // Get the state we need
-    ctFont  = GetFontRefFromFontID(fontID);
-    cfData  = copyTableFromFont(ctFont, tag);
-    theSize = 0;
-
-
-    // Get the data size
-    if (cfData != NULL)
-        {
-        theSize = CFDataGetLength(cfData);
-        CFSafeRelease(cfData);
-        }
+size_t SkFontHost::GetTableSize(SkFontID fontID, SkFontTableTag tag) {
+    CTFontRef ctFont = GetFontRefFromFontID(fontID);
+    CFDataRef srcData = copyTableFromFont(ctFont, tag);
+    if (NULL == srcData) {
+        return 0;
+    }
 
-    return(theSize);
+    AutoCFRelease ar(srcData);
+    return CFDataGetLength(srcData);
 }
 
 size_t SkFontHost::GetTableData(SkFontID fontID, SkFontTableTag tag,
-                                size_t offset, size_t length, void* data)
-{   size_t          theSize;
-    CTFontRef       ctFont;
-    CFDataRef       cfData;
-
-
-    // Get the state we need
-    ctFont  = GetFontRefFromFontID(fontID);
-    cfData  = copyTableFromFont(ctFont, tag);
-    theSize = 0;
-
+                                size_t offset, size_t length, void* dst) {
+    CTFontRef ctFont = GetFontRefFromFontID(fontID);
+    CFDataRef srcData = copyTableFromFont(ctFont, tag);
+    if (NULL == srcData) {
+        return 0;
+    }
 
-    // Get the data
-    if (cfData != NULL)
-        theSize = CFDataGetLength(cfData);
+    AutoCFRelease ar(srcData);
 
-    if (offset >= theSize)
+    size_t srcSize = CFDataGetLength(srcData);
+    if (offset >= srcSize) {
         return 0;
+    }
 
-    if ((offset + length) > theSize)
-        length = theSize - offset;
+    if ((offset + length) > srcSize) {
+        length = srcSize - offset;
+    }
 
-    memcpy(data, CFDataGetBytePtr(cfData) + offset, length);
-    return(length);
+    if (dst) {
+        memcpy(dst, CFDataGetBytePtr(srcData) + offset, length);
+    }
+    return length;
 }

diff --git a/tests/FontHostTest.cpp b/tests/FontHostTest.cpp
index 2b83658..8ab7ad3 100644 (file)
--- a/tests/FontHostTest.cpp
+++ b/tests/FontHostTest.cpp
@@ -31,7 +31,9 @@ static void test_tables(skiatest::Reporter* reporter, SkTypeface* face) {
 
     SkAutoTMalloc<SkFontTableTag> storage(count);
     SkFontTableTag* tags = storage.get();
-    SkFontHost::GetTableTags(fontID, tags);
+
+    int count2 = SkFontHost::GetTableTags(fontID, tags);
+    REPORTER_ASSERT(reporter, count2 == count);
 
     for (int i = 0; i < count; ++i) {
         size_t size = SkFontHost::GetTableSize(fontID, tags[i]);
@@ -52,6 +54,14 @@ static void test_tables(skiatest::Reporter* reporter, SkTypeface* face) {
                 REPORTER_ASSERT(reporter, gKnownTableSizes[j].fSize == size);
             }
         }
+        
+        // do we get the same size from GetTableData and GetTableSize
+        {
+            SkAutoMalloc data(size);
+            size_t size2 = SkFontHost::GetTableData(fontID, tags[i], 0, size,
+                                                    data.get());
+            REPORTER_ASSERT(reporter, size2 == size);
+        }
     }
 }