From fbd033d57235cfcfeb83226661da3777429bb4ce Mon Sep 17 00:00:00 2001 From: "reed@google.com" Date: Thu, 23 Feb 2012 16:15:58 +0000 Subject: [PATCH] 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 --- src/ports/SkFontHost_mac_coretext.cpp | 158 ++++++++++++++-------------------- tests/FontHostTest.cpp | 12 ++- 2 files changed, 74 insertions(+), 96 deletions(-) diff --git a/src/ports/SkFontHost_mac_coretext.cpp b/src/ports/SkFontHost_mac_coretext.cpp index e483250..914ceb0 100644 --- 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 --- a/tests/FontHostTest.cpp +++ b/tests/FontHostTest.cpp @@ -31,7 +31,9 @@ static void test_tables(skiatest::Reporter* reporter, SkTypeface* face) { SkAutoTMalloc 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); + } } } -- 2.7.4