From 4841666ebe6aba70a957ee47ed26dbc3e63d189c Mon Sep 17 00:00:00 2001
From: Raph Levien <raph@google.com>
Date: Mon, 19 May 2014 13:21:21 -0700
Subject: [PATCH] Fix incomplete refcounting and locking

These changes were supposed to be committed in the previous patch
"Better refcounting and locking" but seem to have gotten lost in a
rebase. It fixes a memory leak and some possible race conditions.

Change-Id: I54ca1e37500ec49756fe317cc6d6d03da9911501
---
 engine/src/flutter/include/minikin/FontFamily.h    |  2 ++
 engine/src/flutter/include/minikin/Layout.h        |  6 ++++++
 engine/src/flutter/libs/minikin/FontCollection.cpp |  2 ++
 engine/src/flutter/libs/minikin/FontFamily.cpp     | 11 +++++++++--
 engine/src/flutter/libs/minikin/Layout.cpp         |  7 +++++++
 5 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/engine/src/flutter/include/minikin/FontFamily.h b/engine/src/flutter/include/minikin/FontFamily.h
index 3b59017631a..82fcfe9af87 100644
--- a/engine/src/flutter/include/minikin/FontFamily.h
+++ b/engine/src/flutter/include/minikin/FontFamily.h
@@ -56,6 +56,8 @@ public:
     MinikinFont* getFont(size_t index) const;
     FontStyle getStyle(size_t index) const;
 private:
+    void addFontLocked(MinikinFont* typeface, FontStyle style);
+
     class Font {
     public:
         Font(MinikinFont* typeface, FontStyle style) :
diff --git a/engine/src/flutter/include/minikin/Layout.h b/engine/src/flutter/include/minikin/Layout.h
index 896478b219a..4fe9d8772fa 100644
--- a/engine/src/flutter/include/minikin/Layout.h
+++ b/engine/src/flutter/include/minikin/Layout.h
@@ -55,8 +55,14 @@ struct LayoutGlyph {
     float y;
 };
 
+// Lifecycle and threading assumptions for Layout:
+// The object is assumed to be owned by a single thread; multiple threads
+// may not mutate it at the same time.
+// The lifetime of the FontCollection set through setFontCollection must
+// extend through the lifetime of the Layout object.
 class Layout {
 public:
+    ~Layout();
 
     void dump() const;
     void setFontCollection(const FontCollection* collection);
diff --git a/engine/src/flutter/libs/minikin/FontCollection.cpp b/engine/src/flutter/libs/minikin/FontCollection.cpp
index 18e528efb05..cd7b19e0096 100644
--- a/engine/src/flutter/libs/minikin/FontCollection.cpp
+++ b/engine/src/flutter/libs/minikin/FontCollection.cpp
@@ -19,6 +19,7 @@
 #define LOG_TAG "Minikin"
 #include <cutils/log.h>
 
+#include "MinikinInternal.h"
 #include <minikin/CmapCoverage.h>
 #include <minikin/FontCollection.h>
 
@@ -33,6 +34,7 @@ static inline T max(T a, T b) {
 
 FontCollection::FontCollection(const vector<FontFamily*>& typefaces) :
     mMaxChar(0) {
+    AutoMutex _l(gMinikinLock);
     vector<uint32_t> lastChar;
     size_t nTypefaces = typefaces.size();
 #ifdef VERBOSE_DEBUG
diff --git a/engine/src/flutter/libs/minikin/FontFamily.cpp b/engine/src/flutter/libs/minikin/FontFamily.cpp
index d8525ffa6de..d818f77eec2 100644
--- a/engine/src/flutter/libs/minikin/FontFamily.cpp
+++ b/engine/src/flutter/libs/minikin/FontFamily.cpp
@@ -19,6 +19,8 @@
 #include <cutils/log.h>
 #include <stdlib.h>
 #include <stdint.h>
+
+#include "MinikinInternal.h"
 #include <minikin/MinikinFont.h>
 #include <minikin/AnalyzeStyle.h>
 #include <minikin/FontFamily.h>
@@ -35,6 +37,7 @@ FontFamily::~FontFamily() {
 }
 
 bool FontFamily::addFont(MinikinFont* typeface) {
+    AutoMutex _l(gMinikinLock);
     const uint32_t os2Tag = MinikinFont::MakeTag('O', 'S', '/', '2');
     size_t os2Size = 0;
     bool ok = typeface->GetTable(os2Tag, NULL, &os2Size);
@@ -47,7 +50,7 @@ bool FontFamily::addFont(MinikinFont* typeface) {
     if (analyzeStyle(os2Data.get(), os2Size, &weight, &italic)) {
         //ALOGD("analyzed weight = %d, italic = %s", weight, italic ? "true" : "false");
         FontStyle style(weight, italic);
-        addFont(typeface, style);
+        addFontLocked(typeface, style);
         return true;
     } else {
         ALOGD("failed to analyze style");
@@ -56,7 +59,11 @@ bool FontFamily::addFont(MinikinFont* typeface) {
 }
 
 void FontFamily::addFont(MinikinFont* typeface, FontStyle style) {
-    typeface->RefLocked();
+    AutoMutex _l(gMinikinLock);
+    addFontLocked(typeface, style);
+}
+
+void FontFamily::addFontLocked(MinikinFont* typeface, FontStyle style) {    typeface->RefLocked();
     mFonts.push_back(Font(typeface, style));
 }
 
diff --git a/engine/src/flutter/libs/minikin/Layout.cpp b/engine/src/flutter/libs/minikin/Layout.cpp
index 3a0be6abefc..f32e9f4e4f9 100644
--- a/engine/src/flutter/libs/minikin/Layout.cpp
+++ b/engine/src/flutter/libs/minikin/Layout.cpp
@@ -161,6 +161,7 @@ hb_font_funcs_t* getHbFontFuncs() {
 hb_font_t* create_hb_font(MinikinFont* minikinFont, MinikinPaint* minikinPaint) {
     hb_face_t* face = hb_face_create_for_tables(referenceTable, minikinFont, NULL);
     hb_font_t* font = hb_font_create(face);
+    hb_face_destroy(face);
     hb_font_set_funcs(font, getHbFontFuncs(), minikinPaint, 0);
     // TODO: manage ownership of face
     return font;
@@ -176,6 +177,12 @@ static hb_position_t HBFloatToFixed(float v)
     return scalbnf (v, +8);
 }
 
+Layout::~Layout() {
+    for (size_t ix = 0; ix < mHbFonts.size(); ix++) {
+        hb_font_destroy(mHbFonts[ix]);
+    }
+}
+
 void Layout::dump() const {
     for (size_t i = 0; i < mGlyphs.size(); i++) {
         const LayoutGlyph& glyph = mGlyphs[i];