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
--- 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 @@
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 @@
///////////////////////////////////////////////////////////////////////////
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;
+int SkFontHost::GetTableTags(SkFontID fontID, SkFontTableTag tags[]) {
+ CTFontRef ctFont = GetFontRefFromFontID(fontID);
+ CFArrayRef cfArray = CTFontCopyAvailableTables(ctFont,
+ kCTFontTableOptionNoOptions);
+ if (NULL == cfArray) {
+ return 0;
+ }
+ AutoCFRelease ar(cfArray);
- // Get the state we need
- ctFont = GetFontRefFromFontID(fontID);
- cfArray = CTFontCopyAvailableTables(ctFont, kCTFontTableOptionNoOptions);
- numTables = 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));
-
- 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 @@
// 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 @@
return data;
}
-size_t SkFontHost::GetTableSize(SkFontID fontID, SkFontTableTag tag)
-{ size_t theSize;
- CTFontRef ctFont;
- CFDataRef cfData;
+size_t SkFontHost::GetTableSize(SkFontID fontID, SkFontTableTag tag) {
+ CTFontRef ctFont = GetFontRefFromFontID(fontID);
+ CFDataRef srcData = copyTableFromFont(ctFont, tag);
+ if (NULL == srcData) {
+ return 0;
+ }
-
- // 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);
- }
-
- 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;
-
-
- // Get the data
- if (cfData != NULL)
- theSize = CFDataGetLength(cfData);
-
- if (offset >= theSize)
+ size_t offset, size_t length, void* dst) {
+ CTFontRef ctFont = GetFontRefFromFontID(fontID);
+ CFDataRef srcData = copyTableFromFont(ctFont, tag);
+ if (NULL == srcData) {
return 0;
+ }
- if ((offset + length) > theSize)
- length = theSize - offset;
+ AutoCFRelease ar(srcData);
- memcpy(data, CFDataGetBytePtr(cfData) + offset, length);
- return(length);
+ size_t srcSize = CFDataGetLength(srcData);
+ if (offset >= srcSize) {
+ return 0;
+ }
+
+ if ((offset + length) > srcSize) {
+ length = srcSize - offset;
+ }
+
+ 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 @@
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 @@
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);
+ }
}
}