Title: [207271] branches/safari-602-branch/Source/WebCore
Revision
207271
Author
matthew_han...@apple.com
Date
2016-10-12 19:28:46 -0700 (Wed, 12 Oct 2016)

Log Message

Merge r205031. rdar://problem/28216249

    2016-08-25  Brent Fulgham  <bfulg...@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:

Patch by Brent Fulgham <bfulg...@apple.com> on 2016-08-25

Modified Paths

Diff

Modified: branches/safari-602-branch/Source/WebCore/ChangeLog (207270 => 207271)


--- branches/safari-602-branch/Source/WebCore/ChangeLog	2016-10-13 02:28:42 UTC (rev 207270)
+++ branches/safari-602-branch/Source/WebCore/ChangeLog	2016-10-13 02:28:46 UTC (rev 207271)
@@ -1,3 +1,33 @@
+2016-08-25  Brent Fulgham  <bfulg...@apple.com>
+
+        Merge r205031. rdar://problem/28216249
+
+    2016-08-25  Brent Fulgham  <bfulg...@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-10-12  Matthew Hanson  <matthew_han...@apple.com>
 
         Merge r206975. rdar://problem/28545012

Modified: branches/safari-602-branch/Source/WebCore/rendering/mathml/RenderMathMLToken.cpp (207270 => 207271)


--- branches/safari-602-branch/Source/WebCore/rendering/mathml/RenderMathMLToken.cpp	2016-10-13 02:28:42 UTC (rev 207270)
+++ branches/safari-602-branch/Source/WebCore/rendering/mathml/RenderMathMLToken.cpp	2016-10-13 02:28:46 UTC (rev 207271)
@@ -1,6 +1,7 @@
 /*
  * Copyright (C) 2014 Frédéric Wang (fred.w...@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
@@ -40,15 +41,11 @@
 
 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)
 {
 }
 
@@ -496,10 +493,13 @@
     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();
@@ -509,7 +509,7 @@
 {
     ASSERT(m_mathVariantGlyphDirty);
 
-    m_mathVariantGlyph = GlyphData();
+    m_mathVariantCodePoint = Nullopt;
     m_mathVariantGlyphDirty = false;
 
     // Early return if the token element contains RenderElements.
@@ -527,8 +527,10 @@
         if (mathvariant == MathMLStyle::None)
             mathvariant = tokenElement.hasTagName(MathMLNames::miTag) ? MathMLStyle::Italic : MathMLStyle::Normal;
         UChar32 transformedCodePoint = mathVariant(codePoint, mathvariant);
-        if (transformedCodePoint != codePoint)
-            m_mathVariantGlyph = style().fontCascade().glyphDataForCharacter(transformedCodePoint, !style().isLeftToRightDirection());
+        if (transformedCodePoint != codePoint) {
+            m_mathVariantCodePoint = mathVariant(codePoint, mathvariant);
+            m_mathVariantIsMirrored = !style().isLeftToRightDirection();
+        }
     }
 }
 
@@ -546,8 +548,11 @@
 
 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();
 }
 
@@ -558,7 +563,11 @@
     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;
     }
@@ -566,8 +575,8 @@
     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();
 }
@@ -577,22 +586,30 @@
     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);
 }
 

Modified: branches/safari-602-branch/Source/WebCore/rendering/mathml/RenderMathMLToken.h (207270 => 207271)


--- branches/safari-602-branch/Source/WebCore/rendering/mathml/RenderMathMLToken.h	2016-10-13 02:28:42 UTC (rev 207270)
+++ branches/safari-602-branch/Source/WebCore/rendering/mathml/RenderMathMLToken.h	2016-10-13 02:28:46 UTC (rev 207271)
@@ -1,6 +1,7 @@
 /*
  * Copyright (C) 2014 Frédéric Wang (fred.w...@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
@@ -61,8 +62,9 @@
         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
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to