Do not leak FamilyData in Android SkFontMgr.

src/ports/SkFontConfigParser_android.cpp

 /*
  * Copyright 2011 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 "SkFontConfigParser_android.h"
 #include "SkTDArray.h"
 #include "SkTSearch.h"
@@ skipping 40 matching lines @@
  * can read these variables that are relevant to the current parsing.
  */
 struct FamilyData {
     FamilyData(XML_Parser parserRef, SkTDArray<FontFamily*> &familiesRef) :
         parser(parserRef),
         families(familiesRef),
         currentFamily(NULL),
         currentFontInfo(NULL),
         currentTag(NO_TAG) {};
 
-    XML_Parser parser;                 // The expat parser doing the work, owned by caller
-    SkTDArray<FontFamily*>& families;  // The array to append families, owned by caller
-    FontFamily* currentFamily;         // The current family being created, owned by this
-    FontFileInfo* currentFontInfo;     // The current fontInfo being created, owned by currentFamily
-    int currentTag;                    // A flag to indicate whether we're in nameset/fileset tags
+    XML_Parser parser;                       // The expat parser doing the work, owned by caller
+    SkTDArray<FontFamily*>& families;        // The array to append families, owned by caller

[mtklein, 2015/01/30 23:07:41]

Who cleans up things we add here?

[bungeman-skia, 2015/01/30 23:14:56]

This is completely owned by the caller (this is the families passe into
GetFontFamilies). Currently, the caller of that is also leaking these, I think,
but that's another CL I'm working on.

+    SkAutoTDelete<FontFamily> currentFamily; // The family being created, owned by this

[mtklein, 2015/01/30 23:07:41]

So, uh.... we were just never deleting this?  I can't even find a "delete" in
here.

[bungeman-skia, 2015/01/30 23:14:56]

This is the 'working' FontFamily. When we finish building it, it is appended to
families. This simplifies things, because it may be possible to have a 'bad'
family which should never end up in the list ever. However, it was possible in
error conditions that one of these could just never end up being freed (it's
still possible, just less likely now, that's another CL).

+    FontFileInfo* currentFontInfo;           // The fontInfo being created, owned by currentFamily
+    int currentTag;                          // Flag to indicate when we're in nameset/fileset tags
 };
 
 /** http://www.w3.org/TR/html-markup/datatypes.html#common.data.integer.non-negative-def */
 template <typename T> static bool parseNonNegativeInteger(const char* s, T* value) {
     SK_COMPILE_ASSERT(std::numeric_limits<T>::is_integer, T_must_be_integer);
     const T nMax = std::numeric_limits<T>::max() / 10;
     const T dMax = std::numeric_limits<T>::max() - (nMax * 10);
     T n = 0;
     for (; *s; ++s) {
         // Check if digit
@@ skipping 106 matching lines @@
     }
 
     // Assumes that the named family is already declared
     FontFamily* targetFamily = findFamily(familyData, to.c_str());
     if (!targetFamily) {
         SkDebugf("---- Font alias target %s (NOT FOUND)", to.c_str());
         return;
     }
 
     if (weight) {
         FontFamily* family = new FontFamily();

[mtklein, 2015/01/30 23:07:41]

E.g. I'm curious who calls delete on this family.

[bungeman-skia, 2015/01/30 23:14:56]

This is added to the set of families being appended (see line 200). As a result,
it is up to the caller. This is the same situation as outlined above. 

         family->fNames.push_back().set(aliasName);
 
         for (int i = 0; i < targetFamily->fFonts.count(); i++) {
             if (targetFamily->fFonts[i].fWeight == weight) {
                 family->fFonts.push_back(targetFamily->fFonts[i]);
             }
         }
         *familyData->families.append() = family;
     } else {
         targetFamily->fNames.push_back().set(aliasName);
     }
 }
 
 void startElementHandler(void* data, const char* tag, const char** attributes) {
     FamilyData* familyData = (FamilyData*) data;
     size_t len = strlen(tag);
     if (len == 6 && !strncmp(tag, "family", len)) {
-        familyData->currentFamily = new FontFamily();
+        familyData->currentFamily.reset(new FontFamily());
         familyElementHandler(familyData->currentFamily, attributes);
     } else if (len == 4 && !strncmp(tag, "font", len)) {
         FontFileInfo* file = &familyData->currentFamily->fFonts.push_back();
         familyData->currentFontInfo = file;
         fontElementHandler(familyData->parser, file, attributes);
     } else if (len == 5 && !strncmp(tag, "alias", len)) {
         aliasElementHandler(familyData, attributes);
     }
 }
 
 void endElementHandler(void* data, const char* tag) {
     FamilyData* familyData = (FamilyData*) data;
     size_t len = strlen(tag);
     if (len == 6 && strncmp(tag, "family", len) == 0) {
-        *familyData->families.append() = familyData->currentFamily;
-        familyData->currentFamily = NULL;
+        *familyData->families.append() = familyData->currentFamily.detach();
     } else if (len == 4 && !strncmp(tag, "font", len)) {
         XML_SetCharacterDataHandler(familyData->parser, NULL);
     }
 }
 
 } // lmpParser
 
 namespace jbParser {
 
 /**
  * Handler for arbitrary text. This is used to parse the text inside each name
  * or file tag. The resulting strings are put into the fNames or FontFileInfo arrays.
  */
 static void textHandler(void* data, const char* s, int len) {
     FamilyData* familyData = (FamilyData*) data;
     // Make sure we're in the right state to store this name information
-    if (familyData->currentFamily &&
+    if (familyData->currentFamily.get() &&
             (familyData->currentTag == NAMESET_TAG || familyData->currentTag == FILESET_TAG)) {
         switch (familyData->currentTag) {
         case NAMESET_TAG: {
             SkAutoAsciiToLC tolc(s, len);
             familyData->currentFamily->fNames.push_back().set(tolc.lc(), len);
             break;
         }
         case FILESET_TAG:
             if (familyData->currentFontInfo) {
                 familyData->currentFontInfo->fFileName.set(s, len);
@@ skipping 73 matching lines @@
             if (nameLen == 7 && strncmp(atts[i], "version", nameLen)) continue;
             const char* valueString = atts[i+1];
             int version;
             if (parseNonNegativeInteger(valueString, &version) && (version >= 21)) {
                 XML_SetElementHandler(familyData->parser,
                                       lmpParser::startElementHandler,
                                       lmpParser::endElementHandler);
             }
         }
     } else if (len == 6 && strncmp(tag, "family", len) == 0) {
-        familyData->currentFamily = new FontFamily();
+        familyData->currentFamily.reset(new FontFamily());
         // The Family tag has an optional "order" attribute with an integer value >= 0
         // If this attribute does not exist, the default value is -1
         for (size_t i = 0; atts[i] != NULL &&
                            atts[i+1] != NULL; i += 2) {
             const char* valueString = atts[i+1];
             int value;
             if (parseNonNegativeInteger(valueString, &value)) {
                 familyData->currentFamily->fOrder = value;
             }
         }
@@ skipping 12 matching lines @@
 
 /**
  * Handler for the end of tags. We only care about family, nameset, fileset,
  * name, and file.
  */
 static void endElementHandler(void* data, const char* tag) {
     FamilyData* familyData = (FamilyData*) data;
     size_t len = strlen(tag);
     if (len == 6 && strncmp(tag, "family", len)== 0) {
         // Done parsing a Family - store the created currentFamily in the families array
-        *familyData->families.append() = familyData->currentFamily;
-        familyData->currentFamily = NULL;
+        *familyData->families.append() = familyData->currentFamily.detach();
     } else if (len == 7 && strncmp(tag, "nameset", len) == 0) {
         familyData->currentTag = NO_TAG;
     } else if (len == 7 && strncmp(tag, "fileset", len) == 0) {
         familyData->currentTag = NO_TAG;
     } else if ((len == 4 &&
                 strncmp(tag, "name", len) == 0 &&
                 familyData->currentTag == NAMESET_TAG) ||
                (len == 4 &&
                 strncmp(tag, "file", len) == 0 &&
                 familyData->currentTag == FILESET_TAG)) {
@@ skipping 12 matching lines @@
 
     FILE* file = fopen(filename, "r");
 
     // Some of the files we attempt to parse (in particular, /vendor/etc/fallback_fonts.xml)
     // are optional - failure here is okay because one of these optional files may not exist.
     if (NULL == file) {
         return;
     }
 
     XML_Parser parser = XML_ParserCreate(NULL);
-    FamilyData* familyData = new FamilyData(parser, families);
-    XML_SetUserData(parser, familyData);
+    FamilyData familyData(parser, families);
+    XML_SetUserData(parser, &familyData);
     // Start parsing oldschool; switch these in flight if we detect a newer version of the file.
     XML_SetElementHandler(parser, jbParser::startElementHandler, jbParser::endElementHandler);
 
     char buffer[512];
     bool done = false;
     while (!done) {
         fgets(buffer, sizeof(buffer), file);
         size_t len = strlen(buffer);
         if (feof(file) != 0) {
             done = true;
@@ skipping 138 matching lines @@
     const char* tag = fTag.c_str();
 
     // strip off the rightmost "-.*"
     const char* parentTagEnd = strrchr(tag, '-');
     if (parentTagEnd == NULL) {
         return SkLanguage();
     }
     size_t parentTagLen = parentTagEnd - tag;
     return SkLanguage(tag, parentTagLen);
 }