Crash when getting font bounding rect
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Aug 2016 18:12:56 +0000 (18:12 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Aug 2016 18:12:56 +0000 (18:12 +0000)
https://bugs.webkit.org/show_bug.cgi?id=161202
<rdar://problem/27986981>

Reviewed by Myles C. Maxfield.

We should never store GlyphData objects for later use, because they contain raw pointers to Font elements
contained in caches, and those font caches get periodically purged.

Instead, we should hold onto the ‘key’ representing the GlyphData, and simply ask the system for the
GlyphData the next time it is needed.

Tested by existing MathML tests under ASAN and GuardMalloc.

* rendering/mathml/RenderMathMLToken.cpp:
(WebCore::RenderMathMLToken::RenderMathMLToken): Clean up constructors.
(WebCore::RenderMathMLToken::computePreferredLogicalWidths): Use keys to get correct GlyphData when needed.
(WebCore::RenderMathMLToken::updateMathVariantGlyph): Ditto.
(WebCore::RenderMathMLToken::firstLineBaseline): Ditto.
(WebCore::RenderMathMLToken::layoutBlock): Ditto.
(WebCore::RenderMathMLToken::paint): Ditto.
(WebCore::RenderMathMLToken::paintChildren): Ditto.
* rendering/mathml/RenderMathMLToken.h:

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

Source/WebCore/ChangeLog
Source/WebCore/rendering/mathml/RenderMathMLToken.cpp
Source/WebCore/rendering/mathml/RenderMathMLToken.h

index 4335cb39643e83e9df35bf5d432019cae726a8ef..ead5456ffbc174b042f2bbe687eb09d78707fa12 100644 (file)
@@ -1,3 +1,29 @@
+2016-08-25  Brent Fulgham  <bfulgham@apple.com>
+
+        Crash when getting font bounding rect
+        https://bugs.webkit.org/show_bug.cgi?id=161202
+        <rdar://problem/27986981>
+
+        Reviewed by Myles C. Maxfield.
+
+        We should never store GlyphData objects for later use, because they contain raw pointers to Font elements
+        contained in caches, and those font caches get periodically purged.
+
+        Instead, we should hold onto the ‘key’ representing the GlyphData, and simply ask the system for the
+        GlyphData the next time it is needed.
+
+        Tested by existing MathML tests under ASAN and GuardMalloc.
+
+        * rendering/mathml/RenderMathMLToken.cpp:
+        (WebCore::RenderMathMLToken::RenderMathMLToken): Clean up constructors.
+        (WebCore::RenderMathMLToken::computePreferredLogicalWidths): Use keys to get correct GlyphData when needed.
+        (WebCore::RenderMathMLToken::updateMathVariantGlyph): Ditto.
+        (WebCore::RenderMathMLToken::firstLineBaseline): Ditto.
+        (WebCore::RenderMathMLToken::layoutBlock): Ditto.
+        (WebCore::RenderMathMLToken::paint): Ditto.
+        (WebCore::RenderMathMLToken::paintChildren): Ditto.
+        * rendering/mathml/RenderMathMLToken.h:
+
 2016-08-26  Chris Dumez  <cdumez@apple.com>
 
         HTMLAreaElement's coords attributes parsing does not comply with the HTML specification
index 1179884b17569194ecdc43ac7b6dc97e95d3d531..6398059d2595e4290502a168b2e87ffc2be18f4f 100644 (file)
@@ -1,6 +1,7 @@
 /*
  * Copyright (C) 2014 Frédéric Wang (fred.wang@free.fr). All rights reserved.
  * Copyright (C) 2016 Igalia S.L.
+ * Copyright (C) 2016 Apple Inc.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -42,15 +43,11 @@ using namespace MathMLNames;
 
 RenderMathMLToken::RenderMathMLToken(Element& element, RenderStyle&& style)
     : RenderMathMLBlock(element, WTFMove(style))
-    , m_mathVariantGlyph()
-    , m_mathVariantGlyphDirty(false)
 {
 }
 
 RenderMathMLToken::RenderMathMLToken(Document& document, RenderStyle&& style)
     : RenderMathMLBlock(document, WTFMove(style))
-    , m_mathVariantGlyph()
-    , m_mathVariantGlyphDirty(false)
 {
 }
 
@@ -503,10 +500,13 @@ void RenderMathMLToken::computePreferredLogicalWidths()
     if (m_mathVariantGlyphDirty)
         updateMathVariantGlyph();
 
-    if (m_mathVariantGlyph.font) {
-        m_maxPreferredLogicalWidth = m_minPreferredLogicalWidth = m_mathVariantGlyph.font->widthForGlyph(m_mathVariantGlyph.glyph);
-        setPreferredLogicalWidthsDirty(false);
-        return;
+    if (m_mathVariantCodePoint) {
+        auto mathVariantGlyph = style().fontCascade().glyphDataForCharacter(m_mathVariantCodePoint.value(), m_mathVariantIsMirrored);
+        if (mathVariantGlyph.font) {
+            m_maxPreferredLogicalWidth = m_minPreferredLogicalWidth = mathVariantGlyph.font->widthForGlyph(mathVariantGlyph.glyph);
+            setPreferredLogicalWidthsDirty(false);
+            return;
+        }
     }
 
     RenderMathMLBlock::computePreferredLogicalWidths();
@@ -516,7 +516,7 @@ void RenderMathMLToken::updateMathVariantGlyph()
 {
     ASSERT(m_mathVariantGlyphDirty);
 
-    m_mathVariantGlyph = GlyphData();
+    m_mathVariantCodePoint = Nullopt;
     m_mathVariantGlyphDirty = false;
 
     // Early return if the token element contains RenderElements.
@@ -532,8 +532,10 @@ void RenderMathMLToken::updateMathVariantGlyph()
         if (mathvariant == MathMLElement::MathVariant::None)
             mathvariant = tokenElement.hasTagName(MathMLNames::miTag) ? MathMLElement::MathVariant::Italic : MathMLElement::MathVariant::Normal;
         UChar32 transformedCodePoint = mathVariant(codePoint.value(), mathvariant);
-        if (transformedCodePoint != codePoint.value())
-            m_mathVariantGlyph = style().fontCascade().glyphDataForCharacter(transformedCodePoint, !style().isLeftToRightDirection());
+        if (transformedCodePoint != codePoint.value()) {
+            m_mathVariantCodePoint = mathVariant(codePoint.value(), mathvariant);
+            m_mathVariantIsMirrored = !style().isLeftToRightDirection();
+        }
     }
 }
 
@@ -551,8 +553,11 @@ void RenderMathMLToken::updateFromElement()
 
 Optional<int> RenderMathMLToken::firstLineBaseline() const
 {
-    if (m_mathVariantGlyph.font)
-        return Optional<int>(static_cast<int>(lroundf(-m_mathVariantGlyph.font->boundsForGlyph(m_mathVariantGlyph.glyph).y())));
+    if (m_mathVariantCodePoint) {
+        auto mathVariantGlyph = style().fontCascade().glyphDataForCharacter(m_mathVariantCodePoint.value(), m_mathVariantIsMirrored);
+        if (mathVariantGlyph.font)
+            return Optional<int>(static_cast<int>(lroundf(-mathVariantGlyph.font->boundsForGlyph(mathVariantGlyph.glyph).y())));
+    }
     return RenderMathMLBlock::firstLineBaseline();
 }
 
@@ -563,7 +568,11 @@ void RenderMathMLToken::layoutBlock(bool relayoutChildren, LayoutUnit pageLogica
     if (!relayoutChildren && simplifiedLayout())
         return;
 
-    if (!m_mathVariantGlyph.font) {
+    GlyphData mathVariantGlyph;
+    if (m_mathVariantCodePoint)
+        mathVariantGlyph = style().fontCascade().glyphDataForCharacter(m_mathVariantCodePoint.value(), m_mathVariantIsMirrored);
+
+    if (!mathVariantGlyph.font) {
         RenderMathMLBlock::layoutBlock(relayoutChildren, pageLogicalHeight);
         return;
     }
@@ -571,8 +580,8 @@ void RenderMathMLToken::layoutBlock(bool relayoutChildren, LayoutUnit pageLogica
     for (auto* child = firstChildBox(); child; child = child->nextSiblingBox())
         child->layoutIfNeeded();
 
-    setLogicalWidth(m_mathVariantGlyph.font->widthForGlyph(m_mathVariantGlyph.glyph));
-    setLogicalHeight(m_mathVariantGlyph.font->boundsForGlyph(m_mathVariantGlyph.glyph).height());
+    setLogicalWidth(mathVariantGlyph.font->widthForGlyph(mathVariantGlyph.glyph));
+    setLogicalHeight(mathVariantGlyph.font->boundsForGlyph(mathVariantGlyph.glyph).height());
 
     clearNeedsLayout();
 }
@@ -582,22 +591,30 @@ void RenderMathMLToken::paint(PaintInfo& info, const LayoutPoint& paintOffset)
     RenderMathMLBlock::paint(info, paintOffset);
 
     // FIXME: Instead of using DrawGlyph, we may consider using the more general TextPainter so that we can apply mathvariant to strings with an arbitrary number of characters and preserve advanced CSS effects (text-shadow, etc).
-    if (info.context().paintingDisabled() || info.phase != PaintPhaseForeground || style().visibility() != VISIBLE || !m_mathVariantGlyph.font)
+    if (info.context().paintingDisabled() || info.phase != PaintPhaseForeground || style().visibility() != VISIBLE || !m_mathVariantCodePoint)
+        return;
+
+    auto mathVariantGlyph = style().fontCascade().glyphDataForCharacter(m_mathVariantCodePoint.value(), m_mathVariantIsMirrored);
+    if (!mathVariantGlyph.font)
         return;
 
     GraphicsContextStateSaver stateSaver(info.context());
     info.context().setFillColor(style().visitedDependentColor(CSSPropertyColor));
 
     GlyphBuffer buffer;
-    buffer.add(m_mathVariantGlyph.glyph, m_mathVariantGlyph.font, m_mathVariantGlyph.font->widthForGlyph(m_mathVariantGlyph.glyph));
-    LayoutUnit glyphAscent = static_cast<int>(lroundf(-m_mathVariantGlyph.font->boundsForGlyph(m_mathVariantGlyph.glyph).y()));
-    info.context().drawGlyphs(style().fontCascade(), *m_mathVariantGlyph.font, buffer, 0, 1, paintOffset + location() + LayoutPoint(0, glyphAscent));
+    buffer.add(mathVariantGlyph.glyph, mathVariantGlyph.font, mathVariantGlyph.font->widthForGlyph(mathVariantGlyph.glyph));
+    LayoutUnit glyphAscent = static_cast<int>(lroundf(-mathVariantGlyph.font->boundsForGlyph(mathVariantGlyph.glyph).y()));
+    info.context().drawGlyphs(style().fontCascade(), *mathVariantGlyph.font, buffer, 0, 1, paintOffset + location() + LayoutPoint(0, glyphAscent));
 }
 
 void RenderMathMLToken::paintChildren(PaintInfo& paintInfo, const LayoutPoint& paintOffset, PaintInfo& paintInfoForChild, bool usePrintRect)
 {
-    if (m_mathVariantGlyph.font)
-        return;
+    if (m_mathVariantCodePoint) {
+        auto mathVariantGlyph = style().fontCascade().glyphDataForCharacter(m_mathVariantCodePoint.value(), m_mathVariantIsMirrored);
+        if (mathVariantGlyph.font)
+            return;
+    }
+
     RenderMathMLBlock::paintChildren(paintInfo, paintOffset, paintInfoForChild, usePrintRect);
 }
 
index 9a71b66bb55ea1ff3064ef2201117d0c2148141e..7298be64d8586168243f5abf62d10cfb663cb4ac 100644 (file)
@@ -1,6 +1,7 @@
 /*
  * Copyright (C) 2014 Frédéric Wang (fred.wang@free.fr). All rights reserved.
  * Copyright (C) 2016 Igalia S.L.
+ * Copyright (C) 2016 Apple Inc.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -62,8 +63,9 @@ private:
         m_mathVariantGlyphDirty = true;
         setNeedsLayoutAndPrefWidthsRecalc();
     }
-    GlyphData m_mathVariantGlyph;
-    bool m_mathVariantGlyphDirty;
+    Optional<UChar32> m_mathVariantCodePoint { Nullopt };
+    bool m_mathVariantIsMirrored { false };
+    bool m_mathVariantGlyphDirty { false };
 };
 
 } // namespace WebCore