[FreeType] ASSERTION FAILED: !lookupForWriting(Extractor::extract(entry)).second...
authorcarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Apr 2016 09:57:49 +0000 (09:57 +0000)
committercarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Apr 2016 09:57:49 +0000 (09:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=157132

Reviewed by Darin Adler.

I've noticed that some tests fail randomly in the GTK+ debug bot due to an assertion in HashMap when getting
vertical data from the FontCache. I don't know exactly what's wrong, but looks like a problem with the
FontVerticalDataCache hash traits implementation. Looking at the code, I've realized that we could simplify
everything by reusing the FontDataCache hash and traits, since we are actually using the
FontPlatformData::hash() in the end in both cases. Also, I don't see why we need to get the vertical data from
the FontPlatformData while it's actually cached by the font cache. We could just use the FontCache directly
passing only the FontPlatformData. These changes seem to fix the crashes and make the code a lot simpler.

* platform/graphics/Font.cpp:
(WebCore::Font::Font): Use FontCache::verticalData().
* platform/graphics/FontCache.cpp:
(WebCore::fontVerticalDataCache):
(WebCore::FontCache::verticalData):
(WebCore::FontCache::purgeInactiveFontData): Also remove the cached vertical data when removing a font.
(WebCore::FontCache::invalidate): Clear also the vertical data.
* platform/graphics/FontCache.h:
* platform/graphics/FontPlatformData.h:
* platform/graphics/freetype/FontPlatformDataFreeType.cpp:
(WebCore::FontPlatformData::openTypeTable): Deleted.
* platform/graphics/opentype/OpenTypeVerticalData.h: Remove the m_inFontCache member that is now unused.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@200237 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/Font.cpp
Source/WebCore/platform/graphics/FontCache.cpp
Source/WebCore/platform/graphics/FontCache.h
Source/WebCore/platform/graphics/FontPlatformData.h
Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp
Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h

index a85ae0c..537f91e 100644 (file)
@@ -1,3 +1,31 @@
+2016-04-29  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [FreeType] ASSERTION FAILED: !lookupForWriting(Extractor::extract(entry)).second in FontCache::getVerticalData()
+        https://bugs.webkit.org/show_bug.cgi?id=157132
+
+        Reviewed by Darin Adler.
+
+        I've noticed that some tests fail randomly in the GTK+ debug bot due to an assertion in HashMap when getting
+        vertical data from the FontCache. I don't know exactly what's wrong, but looks like a problem with the
+        FontVerticalDataCache hash traits implementation. Looking at the code, I've realized that we could simplify
+        everything by reusing the FontDataCache hash and traits, since we are actually using the
+        FontPlatformData::hash() in the end in both cases. Also, I don't see why we need to get the vertical data from
+        the FontPlatformData while it's actually cached by the font cache. We could just use the FontCache directly
+        passing only the FontPlatformData. These changes seem to fix the crashes and make the code a lot simpler.
+
+        * platform/graphics/Font.cpp:
+        (WebCore::Font::Font): Use FontCache::verticalData().
+        * platform/graphics/FontCache.cpp:
+        (WebCore::fontVerticalDataCache):
+        (WebCore::FontCache::verticalData):
+        (WebCore::FontCache::purgeInactiveFontData): Also remove the cached vertical data when removing a font.
+        (WebCore::FontCache::invalidate): Clear also the vertical data.
+        * platform/graphics/FontCache.h:
+        * platform/graphics/FontPlatformData.h:
+        * platform/graphics/freetype/FontPlatformDataFreeType.cpp:
+        (WebCore::FontPlatformData::openTypeTable): Deleted.
+        * platform/graphics/opentype/OpenTypeVerticalData.h: Remove the m_inFontCache member that is now unused.
+
 2016-04-29  Youenn Fablet  <youenn.fablet@crf.canon.fr>
 
         Remove UsePointersEvenForNonNullableObjectArguments keyword
index 72c77f0..f0f50bb 100644 (file)
@@ -72,7 +72,7 @@ Font::Font(const FontPlatformData& platformData, bool isCustomFont, bool isLoadi
     platformCharWidthInit();
 #if ENABLE(OPENTYPE_VERTICAL)
     if (platformData.orientation() == Vertical && !isTextOrientationFallback) {
-        m_verticalData = platformData.verticalData();
+        m_verticalData = FontCache::singleton().verticalData(platformData);
         m_hasVerticalGlyphs = m_verticalData.get() && m_verticalData->hasVerticalMetrics();
     }
 #endif
index 9fbc6c7..80496fe 100644 (file)
@@ -267,62 +267,6 @@ FontPlatformData* FontCache::getCachedFontPlatformData(const FontDescription& fo
     return it->value.get();
 }
 
-#if ENABLE(OPENTYPE_VERTICAL)
-struct FontVerticalDataCacheKeyHash {
-    static unsigned hash(const FontCache::FontFileKey& fontFileKey)
-    {
-        return PtrHash<const FontCache::FontFileKey*>::hash(&fontFileKey);
-    }
-
-    static bool equal(const FontCache::FontFileKey& a, const FontCache::FontFileKey& b)
-    {
-        return a == b;
-    }
-
-    static const bool safeToCompareToEmptyOrDeleted = true;
-};
-
-struct FontVerticalDataCacheKeyTraits : WTF::GenericHashTraits<FontCache::FontFileKey> {
-    static const bool emptyValueIsZero = true;
-    static const bool needsDestruction = true;
-    static const FontCache::FontFileKey& emptyValue()
-    {
-        static NeverDestroyed<FontCache::FontFileKey> key = nullAtom;
-        return key;
-    }
-    static void constructDeletedValue(FontCache::FontFileKey& slot)
-    {
-        new (NotNull, &slot) FontCache::FontFileKey(HashTableDeletedValue);
-    }
-    static bool isDeletedValue(const FontCache::FontFileKey& value)
-    {
-        return value.isHashTableDeletedValue();
-    }
-};
-
-typedef HashMap<FontCache::FontFileKey, RefPtr<OpenTypeVerticalData>, FontVerticalDataCacheKeyHash, FontVerticalDataCacheKeyTraits> FontVerticalDataCache;
-
-FontVerticalDataCache& fontVerticalDataCacheInstance()
-{
-    static NeverDestroyed<FontVerticalDataCache> fontVerticalDataCache;
-    return fontVerticalDataCache;
-}
-
-PassRefPtr<OpenTypeVerticalData> FontCache::getVerticalData(const FontFileKey& key, const FontPlatformData& platformData)
-{
-    FontVerticalDataCache& fontVerticalDataCache = fontVerticalDataCacheInstance();
-    FontVerticalDataCache::iterator result = fontVerticalDataCache.find(key);
-    if (result != fontVerticalDataCache.end())
-        return result.get()->value;
-
-    RefPtr<OpenTypeVerticalData> verticalData = OpenTypeVerticalData::create(platformData);
-    if (!verticalData->isOpenType())
-        verticalData = nullptr;
-    fontVerticalDataCache.set(key, verticalData);
-    return verticalData;
-}
-#endif
-
 struct FontDataCacheKeyHash {
     static unsigned hash(const FontPlatformData& platformData)
     {
@@ -362,6 +306,25 @@ static FontDataCache& cachedFonts()
     return cache;
 }
 
+#if ENABLE(OPENTYPE_VERTICAL)
+typedef HashMap<FontPlatformData, RefPtr<OpenTypeVerticalData>, FontDataCacheKeyHash, FontDataCacheKeyTraits> FontVerticalDataCache;
+
+FontVerticalDataCache& fontVerticalDataCache()
+{
+    static NeverDestroyed<FontVerticalDataCache> fontVerticalDataCache;
+    return fontVerticalDataCache;
+}
+
+RefPtr<OpenTypeVerticalData> FontCache::verticalData(const FontPlatformData& platformData)
+{
+    auto addResult = fontVerticalDataCache().add(platformData, nullptr);
+    if (addResult.isNewEntry) {
+        RefPtr<OpenTypeVerticalData> data = OpenTypeVerticalData::create(platformData);
+        addResult.iterator->value = data->isOpenType() ? WTFMove(data) : nullptr;
+    }
+    return addResult.iterator->value;
+}
+#endif
 
 #if PLATFORM(IOS)
 const unsigned cMaxInactiveFontData = 120;
@@ -440,6 +403,9 @@ void FontCache::purgeInactiveFontData(unsigned purgeCount)
         for (auto& font : fontsToDelete) {
             bool success = cachedFonts().remove(font->platformData());
             ASSERT_UNUSED(success, success);
+#if ENABLE(OPENTYPE_VERTICAL)
+            fontVerticalDataCache().remove(font->platformData());
+#endif
         }
     };
 
@@ -452,30 +418,6 @@ void FontCache::purgeInactiveFontData(unsigned purgeCount)
     for (auto& key : keysToRemove)
         fontPlatformDataCache().remove(key);
 
-#if ENABLE(OPENTYPE_VERTICAL)
-    FontVerticalDataCache& fontVerticalDataCache = fontVerticalDataCacheInstance();
-    if (!fontVerticalDataCache.isEmpty()) {
-        // Mark & sweep unused verticalData
-        for (auto& verticalData : fontVerticalDataCache.values()) {
-            if (verticalData)
-                verticalData->m_inFontCache = false;
-        }
-        for (auto& font : cachedFonts().values()) {
-            auto* verticalData = const_cast<OpenTypeVerticalData*>(font->verticalData());
-            if (verticalData)
-                verticalData->m_inFontCache = true;
-        }
-        Vector<FontFileKey> keysToRemove;
-        keysToRemove.reserveInitialCapacity(fontVerticalDataCache.size());
-        for (auto& it : fontVerticalDataCache) {
-            if (!it.value || !it.value->m_inFontCache)
-                keysToRemove.append(it.key);
-        }
-        for (auto& key : keysToRemove)
-            fontVerticalDataCache.remove(key);
-    }
-#endif
-
     platformPurgeInactiveFontData();
 }
 
@@ -531,6 +473,9 @@ void FontCache::invalidate()
     }
 
     fontPlatformDataCache().clear();
+#if ENABLE(OPENTYPE_VERTICAL)
+    fontVerticalDataCache().clear();
+#endif
     invalidateFontCascadeCache();
 
     gGeneration++;
index fdbb1f7..e88dea4 100644 (file)
@@ -209,8 +209,7 @@ public:
 #endif
 
 #if ENABLE(OPENTYPE_VERTICAL)
-    typedef AtomicString FontFileKey;
-    PassRefPtr<OpenTypeVerticalData> getVerticalData(const FontFileKey&, const FontPlatformData&);
+    RefPtr<OpenTypeVerticalData> verticalData(const FontPlatformData&);
 #endif
 
 private:
index e81d945..827f540 100644 (file)
@@ -162,7 +162,6 @@ public:
 #if USE(FREETYPE)
     HarfBuzzFace* harfBuzzFace() const;
     bool hasCompatibleCharmap() const;
-    PassRefPtr<OpenTypeVerticalData> verticalData() const;
     FcFontSet* fallbacks() const;
 #endif
 
index c6bd535..bf7a56f 100644 (file)
@@ -356,12 +356,6 @@ bool FontPlatformData::hasCompatibleCharmap() const
         && FT_Select_Charmap(freeTypeFace, ft_encoding_apple_roman));
 }
 
-PassRefPtr<OpenTypeVerticalData> FontPlatformData::verticalData() const
-{
-    ASSERT(hash());
-    return FontCache::singleton().getVerticalData(String::number(hash()), *this);
-}
-
 RefPtr<SharedBuffer> FontPlatformData::openTypeTable(uint32_t table) const
 {
     CairoFtFaceLocker cairoFtFaceLocker(m_scaledFont.get());
index eba1abc..fc32d5d 100644 (file)
@@ -65,9 +65,6 @@ private:
     Vector<int16_t> m_topSideBearings;
     int16_t m_defaultVertOriginY;
     HashMap<Glyph, int16_t> m_vertOriginY;
-
-    friend class FontCache;
-    bool m_inFontCache; // for mark & sweep in FontCache::purgeInactiveFontData()
 };
 
 } // namespace WebCore