Fix minikin_unittests

This CL fixes following test cases in minikin_tests
- FontFamilyTest.hasVariationSelectorTest
- HbFontCacheTest.getHbFontLockedTest
- HbFontCacheTest.purgeCacheTest

For the fix of FontFamilyTest.hasVariationSelectorTest, removing virtual
from GetUniqueId() in MinikinFont.  After [1], MinikinFont's destructor
started calling purgeHbCache() which calls virtual method,
MinikinFont::GetUniqueId().  Fortunately, the SkTypeface::uniqueID()
returns just internal value, so we can store it at the construction time
and use it instead of calling SkTypeface::uniqueID() every time.

This patch also changes purgeHbFont to purgeHbFontLocked, as all uses of
it were already under global mutex. This change avoids deadlock on
explicit unref, as when invoked by a Java finalizer from the Java object
that holds a reference to the font.

Some of the tests needed to change to using the ref counting protocol
rather than explicitly destructing font objects, as well.

[1] 1ea4165cef

Bug: 28105730
Bug: 28105688
Change-Id: Ie5983c4869147dacabdca81af1605066cd680b3f

engine/src/flutter/include/minikin/MinikinFont.h

@@ -99,6 +99,8 @@ typedef void (*MinikinDestroyFunc) (void* data);
 
 class MinikinFont : public MinikinRefCounted {
 public:
+    MinikinFont(int32_t uniqueId) : mUniqueId(uniqueId) {}
+
     virtual ~MinikinFont();
 
     virtual float GetHorizontalAdvance(uint32_t glyph_id,
@@ -125,12 +127,14 @@ public:
         return 0;
     }
 
-    virtual int32_t GetUniqueId() const = 0;
-
     static uint32_t MakeTag(char c1, char c2, char c3, char c4) {
         return ((uint32_t)c1 << 24) | ((uint32_t)c2 << 16) |
             ((uint32_t)c3 << 8) | (uint32_t)c4;
     }
+
+    int32_t GetUniqueId() const { return mUniqueId; }
+private:
+    const int32_t mUniqueId;
 };
 
 }  // namespace android

engine/src/flutter/include/minikin/MinikinFontFreeType.h

@@ -52,8 +52,6 @@ public:
 
     // TODO: provide access to raw data, as an optimization.
 
-    int32_t GetUniqueId() const;
-
     // Not a virtual method, as the protocol to access rendered
     // glyph bitmaps is probably different depending on the
     // backend.
@@ -64,7 +62,6 @@ public:
 
 private:
     FT_Face mTypeface;
-    int32_t mUniqueId;
     static int32_t sIdCounter;
 };
 

engine/src/flutter/include/minikin/MinikinRefCounted.h

@@ -37,6 +37,23 @@ private:
     int mRefcount_;
 };
 
+// An RAII container for reference counted objects.
+// Note: this is only suitable for clients which are _not_ holding the global lock.
+template <typename T>
+class MinikinAutoUnref {
+public:
+    MinikinAutoUnref(T* obj) : mObj(obj) {
+    }
+    ~MinikinAutoUnref() {
+        mObj->Unref();
+    }
+    T& operator*() const { return *mObj; }
+    T* operator->() const { return mObj; }
+    T* get() const { return mObj; }
+private:
+    T* mObj;
+};
+
 }
 
 #endif   // MINIKIN_REF_COUNTED_H

engine/src/flutter/libs/minikin/HbFontCache.cpp

@@ -91,8 +91,8 @@ void purgeHbFontCacheLocked() {
     getFontCacheLocked()->clear();
 }
 
-void purgeHbFont(const MinikinFont* minikinFont) {
+void purgeHbFontLocked(const MinikinFont* minikinFont) {
-    AutoMutex _l(gMinikinLock);
+    assertMinikinLocked();
     const int32_t fontId = minikinFont->GetUniqueId();
     getFontCacheLocked()->remove(fontId);
 }

engine/src/flutter/libs/minikin/HbFontCache.h

@@ -23,7 +23,7 @@ namespace android {
 class MinikinFont;
 
 void purgeHbFontCacheLocked();
-void purgeHbFont(const MinikinFont* minikinFont);
+void purgeHbFontLocked(const MinikinFont* minikinFont);
 hb_font_t* getHbFontLocked(MinikinFont* minikinFont);
 
 }  // namespace android

engine/src/flutter/libs/minikin/MinikinFont.cpp

@@ -20,7 +20,7 @@
 namespace android {
 
 MinikinFont::~MinikinFont() {
-    purgeHbFont(this);
+    purgeHbFontLocked(this);
 }
 
 }  // namespace android

engine/src/flutter/libs/minikin/MinikinFontFreeType.cpp

@@ -30,8 +30,8 @@ namespace android {
 int32_t MinikinFontFreeType::sIdCounter = 0;
 
 MinikinFontFreeType::MinikinFontFreeType(FT_Face typeface) : 
+    MinikinFont(sIdCounter++),
     mTypeface(typeface) {
-    mUniqueId = sIdCounter++;
 }
 
 MinikinFontFreeType::~MinikinFontFreeType() {
@@ -72,10 +72,6 @@ const void* MinikinFontFreeType::GetTable(uint32_t tag, size_t* size, MinikinDes
     return buf;
 }
 
-int32_t MinikinFontFreeType::GetUniqueId() const {
-    return mUniqueId;
-}
-
 bool MinikinFontFreeType::Render(uint32_t glyph_id, const MinikinPaint& /* paint */,
         GlyphBitmap *result) {
     FT_Error error;

engine/src/flutter/sample/MinikinSkia.cpp

@@ -7,6 +7,7 @@
 namespace android {
 
 MinikinFontSkia::MinikinFontSkia(SkTypeface *typeface) :
+    MinikinFont(typeface->uniqueID()),
     mTypeface(typeface) {
 }
 
@@ -67,8 +68,4 @@ SkTypeface *MinikinFontSkia::GetSkTypeface() {
     return mTypeface;
 }
 
-int32_t MinikinFontSkia::GetUniqueId() const {
-    return mTypeface->uniqueID();
-}
-
 }

engine/src/flutter/sample/MinikinSkia.h

@@ -14,8 +14,6 @@ public:
 
     const void* GetTable(uint32_t tag, size_t* size, MinikinDestroyFunc* destroy);
 
-    int32_t GetUniqueId() const;
-
     SkTypeface *GetSkTypeface();
 
 private:

engine/src/flutter/tests/FontFamilyTest.cpp

@@ -350,9 +350,9 @@ void expectVSGlyphs(FontFamily* family, uint32_t codepoint, const std::set<uint3
 }
 
 TEST_F(FontFamilyTest, hasVariationSelectorTest) {
-    MinikinFontForTest minikinFont(kVsTestFont);
+    MinikinAutoUnref<MinikinFontForTest> minikinFont(new MinikinFontForTest(kVsTestFont));
-    FontFamily family;
+    MinikinAutoUnref<FontFamily> family(new FontFamily);
-    family.addFont(&minikinFont);
+    family->addFont(minikinFont.get());
 
     AutoMutex _l(gMinikinLock);
 
@@ -365,24 +365,24 @@ TEST_F(FontFamilyTest, hasVariationSelectorTest) {
     const uint32_t kVS20 = 0xE0103;
 
     const uint32_t kSupportedChar1 = 0x82A6;
-    EXPECT_TRUE(family.getCoverage()->get(kSupportedChar1));
+    EXPECT_TRUE(family->getCoverage()->get(kSupportedChar1));
-    expectVSGlyphs(&family, kSupportedChar1, std::set<uint32_t>({kVS1, kVS17, kVS18, kVS19}));
+    expectVSGlyphs(family.get(), kSupportedChar1, std::set<uint32_t>({kVS1, kVS17, kVS18, kVS19}));
 
     const uint32_t kSupportedChar2 = 0x845B;
-    EXPECT_TRUE(family.getCoverage()->get(kSupportedChar2));
+    EXPECT_TRUE(family->getCoverage()->get(kSupportedChar2));
-    expectVSGlyphs(&family, kSupportedChar2, std::set<uint32_t>({kVS2, kVS18, kVS19, kVS20}));
+    expectVSGlyphs(family.get(), kSupportedChar2, std::set<uint32_t>({kVS2, kVS18, kVS19, kVS20}));
 
     const uint32_t kNoVsSupportedChar = 0x537F;
-    EXPECT_TRUE(family.getCoverage()->get(kNoVsSupportedChar));
+    EXPECT_TRUE(family->getCoverage()->get(kNoVsSupportedChar));
-    expectVSGlyphs(&family, kNoVsSupportedChar, std::set<uint32_t>());
+    expectVSGlyphs(family.get(), kNoVsSupportedChar, std::set<uint32_t>());
 
     const uint32_t kVsOnlySupportedChar = 0x717D;
-    EXPECT_FALSE(family.getCoverage()->get(kVsOnlySupportedChar));
+    EXPECT_FALSE(family->getCoverage()->get(kVsOnlySupportedChar));
-    expectVSGlyphs(&family, kVsOnlySupportedChar, std::set<uint32_t>({kVS3, kVS19, kVS20}));
+    expectVSGlyphs(family.get(), kVsOnlySupportedChar, std::set<uint32_t>({kVS3, kVS19, kVS20}));
 
     const uint32_t kNotSupportedChar = 0x845C;
-    EXPECT_FALSE(family.getCoverage()->get(kNotSupportedChar));
+    EXPECT_FALSE(family->getCoverage()->get(kNotSupportedChar));
-    expectVSGlyphs(&family, kNotSupportedChar, std::set<uint32_t>());
+    expectVSGlyphs(family.get(), kNotSupportedChar, std::set<uint32_t>());
 }
 
 TEST_F(FontFamilyTest, hasVSTableTest) {
@@ -403,12 +403,13 @@ TEST_F(FontFamilyTest, hasVSTableTest) {
                 "Font " + testCase.fontPath + " should have a variation sequence table." :
                 "Font " + testCase.fontPath + " shouldn't have a variation sequence table.");
 
-        MinikinFontForTest minikinFont(testCase.fontPath);
+        MinikinAutoUnref<MinikinFontForTest> minikinFont(new MinikinFontForTest(testCase.fontPath));
-        FontFamily family;
+        MinikinAutoUnref<FontFamily> family(new FontFamily);
-        family.addFont(&minikinFont);
+        family->addFont(minikinFont.get());
-        family.getCoverage();
+        AutoMutex _l(gMinikinLock);
+        family->getCoverage();
 
-        EXPECT_EQ(testCase.hasVSTable, family.hasVSTable());
+        EXPECT_EQ(testCase.hasVSTable, family->hasVSTable());
     }
 }
 

engine/src/flutter/tests/MinikinFontForTest.cpp

@@ -22,8 +22,14 @@
 
 #include <cutils/log.h>
 
-MinikinFontForTest::MinikinFontForTest(const std::string& font_path) : mFontPath(font_path) {
+MinikinFontForTest::MinikinFontForTest(const std::string& font_path) :
-    mTypeface = SkTypeface::CreateFromFile(font_path.c_str());
+    MinikinFontForTest(font_path, SkTypeface::CreateFromFile(font_path.c_str())) {
+}
+
+MinikinFontForTest::MinikinFontForTest(const std::string& font_path, SkTypeface* typeface) :
+    MinikinFont(typeface->uniqueID()),
+    mTypeface(typeface),
+    mFontPath(font_path) {
 }
 
 MinikinFontForTest::~MinikinFontForTest() {
@@ -55,7 +61,3 @@ const void* MinikinFontForTest::GetTable(uint32_t tag, size_t* size,
     *destroy = free;
     return buf;
 }
-
-int32_t MinikinFontForTest::GetUniqueId() const {
-    return mTypeface->uniqueID();
-}

engine/src/flutter/tests/MinikinFontForTest.h

@@ -24,6 +24,7 @@ class SkTypeface;
 class MinikinFontForTest : public android::MinikinFont {
 public:
     explicit MinikinFontForTest(const std::string& font_path);
+    MinikinFontForTest(const std::string& font_path, SkTypeface* typeface);
     ~MinikinFontForTest();
 
     // MinikinFont overrides.
@@ -31,7 +32,6 @@ public:
     void GetBounds(android::MinikinRect* bounds, uint32_t glyph_id,
             const android::MinikinPaint& paint) const;
     const void* GetTable(uint32_t tag, size_t* size, android::MinikinDestroyFunc* destroy);
-    int32_t GetUniqueId() const;
 
     const std::string& fontPath() const { return mFontPath; }
 private:

engine/src/flutter/tests/how_to_run.txt

@@ -1,5 +1,5 @@
 mmm -j8 frameworks/minikin/tests &&
 adb push $OUT/data/nativetest/minikin_tests/minikin_tests \
     /data/nativetest/minikin_tests/minikin_tests &&
-adb push frameworks/minikin/tests/data /data/nativetest/minikin_tests/data &&
+adb push frameworks/minikin/tests/data /data/nativetest/minikin_tests/ &&
 adb shell /data/nativetest/minikin_tests/minikin_tests