Changeset 116498 in webkit


Ignore:
Timestamp:
May 9, 2012 12:17:09 AM (12 years ago)
Author:
Nikolas Zimmermann
Message:

REGRESSION(r105057): Infinite loop inside SVGTextLayoutEngine::currentLogicalCharacterMetrics
https://bugs.webkit.org/show_bug.cgi?id=83405

Reviewed by Darin Adler.

Source/WebCore:

Dynamically adding tspans carrying position information in the x/y/dx/dy/rotate lists is broken.
To avoid mistakes like this in future, simplify the calling code in RenderSVGInlineText and centralize
the managment of all caches (text positioning element cache / metrics map / layout attributes) in
RenderSVGText. This avoids the hack in SVGRootInlineBox::computePerCharacterLayoutInformation() which
called textRoot->rebuildLayoutAttributes(), which was used to fix previous security issues with this code.
Instead correctly handle destruction of RenderSVGInlineText in RenderSVGText, keeping the m_layoutAttributes
synchronized with the current state of the render tree. Fixes highcharts problems.

Tests: svg/text/add-tspan-position-bug.html

svg/text/modify-tspan-position-bug.html

  • rendering/svg/RenderSVGInline.cpp:

(WebCore::RenderSVGInline::addChild):

  • rendering/svg/RenderSVGInlineText.cpp:

(WebCore::RenderSVGInlineText::willBeDestroyed):
(WebCore::RenderSVGInlineText::setTextInternal):
(WebCore::RenderSVGInlineText::styleDidChange):

  • rendering/svg/RenderSVGText.cpp:

(WebCore::recursiveUpdateMetrics):
(WebCore::RenderSVGText::subtreeChildAdded):
(WebCore::RenderSVGText::subtreeChildWillBeDestroyed):
(WebCore::recursiveCollectLayoutAttributes):
(WebCore::checkLayoutAttributesConsistency):
(WebCore::RenderSVGText::subtreeChildWasDestroyed):
(WebCore::RenderSVGText::subtreeStyleChanged):
(WebCore::RenderSVGText::subtreeTextChanged):
(WebCore::RenderSVGText::layout):
(WebCore::RenderSVGText::addChild):
(WebCore::RenderSVGText::rebuildAllLayoutAttributes):
(WebCore::RenderSVGText::rebuildLayoutAttributes):

  • rendering/svg/RenderSVGText.h:

(WebCore::RenderSVGText::layoutAttributes):

  • rendering/svg/SVGRootInlineBox.cpp:

(WebCore::SVGRootInlineBox::computePerCharacterLayoutInformation):

  • rendering/svg/SVGTextLayoutAttributesBuilder.cpp:

(WebCore::SVGTextLayoutAttributesBuilder::buildLayoutAttributes):

LayoutTests:

Add two new testcases covering the problem.

  • svg/text/add-tspan-position-bug-expected.html: Added.
  • svg/text/add-tspan-position-bug.html: Added.
  • svg/text/modify-tspan-position-bug-expected.html: Added.
  • svg/text/modify-tspan-position-bug.html: Added.
Location:
trunk
Files:
4 added
8 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r116491 r116498  
     12012-05-09  Nikolas Zimmermann  <nzimmermann@rim.com>
     2
     3        REGRESSION(r105057): Infinite loop inside SVGTextLayoutEngine::currentLogicalCharacterMetrics
     4        https://bugs.webkit.org/show_bug.cgi?id=83405
     5
     6        Reviewed by Darin Adler.
     7
     8        Add two new testcases covering the problem.
     9
     10        * svg/text/add-tspan-position-bug-expected.html: Added.
     11        * svg/text/add-tspan-position-bug.html: Added.
     12        * svg/text/modify-tspan-position-bug-expected.html: Added.
     13        * svg/text/modify-tspan-position-bug.html: Added.
     14
    1152012-05-08  Kent Tamura  <tkent@chromium.org>
    216
  • trunk/Source/WebCore/ChangeLog

    r116497 r116498  
     12012-05-09  Nikolas Zimmermann  <nzimmermann@rim.com>
     2
     3        REGRESSION(r105057): Infinite loop inside SVGTextLayoutEngine::currentLogicalCharacterMetrics
     4        https://bugs.webkit.org/show_bug.cgi?id=83405
     5
     6        Reviewed by Darin Adler.
     7
     8        Dynamically adding tspans carrying position information in the x/y/dx/dy/rotate lists is broken.
     9        To avoid mistakes like this in future, simplify the calling code in RenderSVGInlineText and centralize
     10        the managment of all caches (text positioning element cache / metrics map / layout attributes) in
     11        RenderSVGText. This avoids the hack in SVGRootInlineBox::computePerCharacterLayoutInformation() which
     12        called textRoot->rebuildLayoutAttributes(), which was used to fix previous security issues with this code.
     13        Instead correctly handle destruction of RenderSVGInlineText in RenderSVGText, keeping the m_layoutAttributes
     14        synchronized with the current state of the render tree. Fixes highcharts problems.
     15
     16        Tests: svg/text/add-tspan-position-bug.html
     17               svg/text/modify-tspan-position-bug.html
     18
     19        * rendering/svg/RenderSVGInline.cpp:
     20        (WebCore::RenderSVGInline::addChild):
     21        * rendering/svg/RenderSVGInlineText.cpp:
     22        (WebCore::RenderSVGInlineText::willBeDestroyed):
     23        (WebCore::RenderSVGInlineText::setTextInternal):
     24        (WebCore::RenderSVGInlineText::styleDidChange):
     25        * rendering/svg/RenderSVGText.cpp:
     26        (WebCore::recursiveUpdateMetrics):
     27        (WebCore::RenderSVGText::subtreeChildAdded):
     28        (WebCore::RenderSVGText::subtreeChildWillBeDestroyed):
     29        (WebCore::recursiveCollectLayoutAttributes):
     30        (WebCore::checkLayoutAttributesConsistency):
     31        (WebCore::RenderSVGText::subtreeChildWasDestroyed):
     32        (WebCore::RenderSVGText::subtreeStyleChanged):
     33        (WebCore::RenderSVGText::subtreeTextChanged):
     34        (WebCore::RenderSVGText::layout):
     35        (WebCore::RenderSVGText::addChild):
     36        (WebCore::RenderSVGText::rebuildAllLayoutAttributes):
     37        (WebCore::RenderSVGText::rebuildLayoutAttributes):
     38        * rendering/svg/RenderSVGText.h:
     39        (WebCore::RenderSVGText::layoutAttributes):
     40        * rendering/svg/SVGRootInlineBox.cpp:
     41        (WebCore::SVGRootInlineBox::computePerCharacterLayoutInformation):
     42        * rendering/svg/SVGTextLayoutAttributesBuilder.cpp:
     43        (WebCore::SVGTextLayoutAttributesBuilder::buildLayoutAttributes):
     44
    1452012-05-08  Dongwoo Im  <dw.im@samsung.com>
    246
  • trunk/Source/WebCore/rendering/svg/RenderSVGInline.cpp

    r115981 r116498  
    125125    RenderInline::addChild(child, beforeChild);
    126126    if (RenderSVGText* textRenderer = RenderSVGText::locateRenderSVGTextAncestor(this))
    127         textRenderer->layoutAttributesChanged(child);
     127        textRenderer->subtreeChildAdded(child);
    128128}
    129129
  • trunk/Source/WebCore/rendering/svg/RenderSVGInlineText.cpp

    r115215 r116498  
    8282
    8383    Vector<SVGTextLayoutAttributes*> affectedAttributes;
    84     textRenderer->layoutAttributesWillBeDestroyed(this, affectedAttributes);
    85 
     84    textRenderer->subtreeChildWillBeDestroyed(this, affectedAttributes);
    8685    RenderText::willBeDestroyed();
    87     if (affectedAttributes.isEmpty())
    88         return;
    89 
    90     if (!documentBeingDestroyed())
    91         textRenderer->rebuildLayoutAttributes(affectedAttributes);
     86    textRenderer->subtreeChildWasDestroyed(this, affectedAttributes);
    9287}
    9388
     
    9590{
    9691    RenderText::setTextInternal(text);
    97 
    98     // When the underlying text content changes, call both textDOMChanged() & layoutAttributesChanged()
    99     // The former will clear the SVGTextPositioningElement cache, which depends on the textLength() of
    100     // the RenderSVGInlineText objects, and thus needs to be rebuild. The latter will assure that the
    101     // SVGTextLayoutAttributes associated with the RenderSVGInlineText will be updated.
    102     if (RenderSVGText* textRenderer = RenderSVGText::locateRenderSVGTextAncestor(this)) {
    103         textRenderer->invalidateTextPositioningElements();
    104         textRenderer->layoutAttributesChanged(this);
    105     }
     92    if (RenderSVGText* textRenderer = RenderSVGText::locateRenderSVGTextAncestor(this))
     93        textRenderer->subtreeTextChanged(this);
    10694}
    10795
     
    128116    // The text metrics may be influenced by style changes.
    129117    if (RenderSVGText* textRenderer = RenderSVGText::locateRenderSVGTextAncestor(this))
    130         textRenderer->layoutAttributesChanged(this);
     118        textRenderer->subtreeStyleChanged(this);
    131119}
    132120
  • trunk/Source/WebCore/rendering/svg/RenderSVGText.cpp

    r115981 r116498  
    111111}
    112112
    113 static inline void recursiveUpdateLayoutAttributes(RenderObject* start, SVGTextLayoutAttributesBuilder& builder)
    114 {
    115     if (start->isSVGInlineText()) {
    116         builder.buildLayoutAttributesForTextRenderer(toRenderSVGInlineText(start));
    117         return;
    118     }
    119 
    120     for (RenderObject* child = start->firstChild(); child; child = child->nextSibling())
    121         recursiveUpdateLayoutAttributes(child, builder);
    122 }
    123 
    124 void RenderSVGText::layoutAttributesChanged(RenderObject* child)
     113void RenderSVGText::subtreeChildAdded(RenderObject* child)
    125114{
    126115    ASSERT(child);
    127116    if (m_needsPositioningValuesUpdate)
    128117        return;
     118
     119    // The positioning elements cache doesn't include the new 'child' yet. Clear the
     120    // cache, as the next buildLayoutAttributesForTextRenderer() call rebuilds it.
     121    invalidateTextPositioningElements();
     122
    129123    FontCachePurgePreventer fontCachePurgePreventer;
    130     recursiveUpdateLayoutAttributes(child, m_layoutAttributesBuilder);
     124    for (RenderObject* descendant = child; descendant; descendant = descendant->nextInPreOrder(child)) {
     125        if (descendant->isSVGInlineText())
     126            m_layoutAttributesBuilder.buildLayoutAttributesForTextRenderer(toRenderSVGInlineText(descendant));
     127    }
     128
    131129    rebuildLayoutAttributes();
    132130}
     
    163161}
    164162
    165 void RenderSVGText::layoutAttributesWillBeDestroyed(RenderSVGInlineText* text, Vector<SVGTextLayoutAttributes*>& affectedAttributes)
     163void RenderSVGText::subtreeChildWillBeDestroyed(RenderSVGInlineText* text, Vector<SVGTextLayoutAttributes*>& affectedAttributes)
    166164{
    167165    ASSERT(text);
     166
     167    // The positioning elements cache depends on the size of each text renderer in the
     168    // subtree. If this changes, clear the cache. It's going to be rebuilt below.
     169    invalidateTextPositioningElements();
     170
    168171    if (m_needsPositioningValuesUpdate)
    169172        return;
    170173
     174    // This logic requires that the 'text' child is still inserted in the tree.
    171175    bool stopAfterNext = false;
    172176    SVGTextLayoutAttributes* previous = 0;
     
    177181    if (next)
    178182        affectedAttributes.append(next);
     183
     184    SVGTextLayoutAttributes* currentLayoutAttributes = text->layoutAttributes();
     185
     186    size_t position = m_layoutAttributes.find(currentLayoutAttributes);
     187    ASSERT(position != notFound);
     188    m_layoutAttributes.remove(position);
     189
     190    ASSERT(!m_layoutAttributes.contains(currentLayoutAttributes));
     191}
     192
     193static inline void recursiveCollectLayoutAttributes(RenderObject* start, Vector<SVGTextLayoutAttributes*>& attributes)
     194{
     195    for (RenderObject* child = start->firstChild(); child; child = child->nextSibling()) {
     196        if (child->isSVGInlineText()) {
     197            attributes.append(toRenderSVGInlineText(child)->layoutAttributes());
     198            continue;
     199        }
     200
     201        recursiveCollectLayoutAttributes(child, attributes);
     202    }
     203}
     204
     205static inline void checkLayoutAttributesConsistency(RenderSVGText* text, Vector<SVGTextLayoutAttributes*>& expectedLayoutAttributes)
     206{
     207#ifndef NDEBUG
     208    Vector<SVGTextLayoutAttributes*> newLayoutAttributes;
     209    recursiveCollectLayoutAttributes(text, newLayoutAttributes);
     210    ASSERT(newLayoutAttributes == expectedLayoutAttributes);
     211#else
     212    UNUSED_PARAM(text);
     213    UNUSED_PARAM(expectedLayoutAttributes);
     214#endif
     215}
     216
     217void RenderSVGText::subtreeChildWasDestroyed(RenderSVGInlineText*, Vector<SVGTextLayoutAttributes*>& affectedAttributes)
     218{
     219    if (documentBeingDestroyed() || affectedAttributes.isEmpty())
     220        return;
     221
     222    checkLayoutAttributesConsistency(this, m_layoutAttributes);
     223
     224    size_t size = affectedAttributes.size();
     225    for (size_t i = 0; i < size; ++i)
     226        m_layoutAttributesBuilder.rebuildMetricsForTextRenderer(affectedAttributes[i]->context());
     227}
     228
     229void RenderSVGText::subtreeStyleChanged(RenderSVGInlineText* text)
     230{
     231    ASSERT(text);
     232    if (m_needsPositioningValuesUpdate)
     233        return;
     234
     235    // Only update the metrics cache, but not the text positioning element cache
     236    // nor the layout attributes cached in the leaf #text renderers.
     237    FontCachePurgePreventer fontCachePurgePreventer;
     238    for (RenderObject* descendant = text; descendant; descendant = descendant->nextInPreOrder(text)) {
     239        if (descendant->isSVGInlineText())
     240            m_layoutAttributesBuilder.rebuildMetricsForTextRenderer(toRenderSVGInlineText(descendant));
     241    }
     242}
     243
     244void RenderSVGText::subtreeTextChanged(RenderSVGInlineText* text)
     245{
     246    ASSERT(text);
     247
     248    // The positioning elements cache depends on the size of each text renderer in the
     249    // subtree. If this changes, clear the cache. It's going to be rebuilt below.
     250    invalidateTextPositioningElements();
     251
     252    if (m_needsPositioningValuesUpdate)
     253        return;
     254
     255    FontCachePurgePreventer fontCachePurgePreventer;
     256    for (RenderObject* descendant = text; descendant; descendant = descendant->nextInPreOrder(text)) {
     257        if (descendant->isSVGInlineText())
     258            m_layoutAttributesBuilder.buildLayoutAttributesForTextRenderer(toRenderSVGInlineText(descendant));
     259    }
    179260}
    180261
     
    186267    // do not always cause the position elements to be marked invalid before use.
    187268    m_layoutAttributesBuilder.clearTextPositioningElements();
    188 }
    189 
    190 static inline void recursiveUpdateScaledFont(RenderObject* start)
    191 {
    192     for (RenderObject* child = start->firstChild(); child; child = child->nextSibling()) {
    193         if (child->isSVGInlineText()) {
    194             toRenderSVGInlineText(child)->updateScaledFont();
    195             continue;
    196         }
    197 
    198         recursiveUpdateScaledFont(child);
    199     }
    200269}
    201270
     
    216285    // or the transform to the root context has changed then recompute the on-screen font size.
    217286    if (m_needsTextMetricsUpdate || SVGRenderSupport::findTreeRootObject(this)->isLayoutSizeChanged()) {
    218         recursiveUpdateScaledFont(this);
    219         rebuildLayoutAttributes(true);
     287        for (RenderObject* descendant = this; descendant; descendant = descendant->nextInPreOrder(this)) {
     288            if (descendant->isSVGInlineText())
     289                toRenderSVGInlineText(descendant)->updateScaledFont();
     290        }
     291
     292        rebuildAllLayoutAttributes();
    220293        updateCachedBoundariesInParents = true;
    221294        m_needsTextMetricsUpdate = false;
     
    365438{
    366439    RenderSVGBlock::addChild(child, beforeChild);
    367     layoutAttributesChanged(child);
     440    subtreeChildAdded(child);
    368441}
    369442
     
    381454}
    382455
    383 static inline void recursiveCollectLayoutAttributes(RenderObject* start, Vector<SVGTextLayoutAttributes*>& attributes)
    384 {
    385     for (RenderObject* child = start->firstChild(); child; child = child->nextSibling()) {
    386         if (child->isSVGInlineText()) {
    387             attributes.append(toRenderSVGInlineText(child)->layoutAttributes());
    388             continue;
    389         }
    390 
    391         recursiveCollectLayoutAttributes(child, attributes);
    392     }
    393 }
    394 
    395 void RenderSVGText::rebuildLayoutAttributes(bool performFullRebuild)
    396 {
    397     if (performFullRebuild)
    398         m_layoutAttributes.clear();
    399 
     456void RenderSVGText::rebuildAllLayoutAttributes()
     457{
     458    m_layoutAttributes.clear();
     459    recursiveCollectLayoutAttributes(this, m_layoutAttributes);
     460    if (m_layoutAttributes.isEmpty())
     461        return;
     462
     463    m_layoutAttributesBuilder.rebuildMetricsForWholeTree(this);
     464}
     465
     466void RenderSVGText::rebuildLayoutAttributes()
     467{
    400468    if (m_layoutAttributes.isEmpty()) {
    401         recursiveCollectLayoutAttributes(this, m_layoutAttributes);
    402         if (m_layoutAttributes.isEmpty() || !performFullRebuild)
    403             return;
    404 
    405         m_layoutAttributesBuilder.rebuildMetricsForWholeTree(this);
    406         return;
    407     }
    408 
    409     Vector<SVGTextLayoutAttributes*> affectedAttributes;
    410     rebuildLayoutAttributes(affectedAttributes);
    411 }
    412 
    413 void RenderSVGText::rebuildLayoutAttributes(Vector<SVGTextLayoutAttributes*>& affectedAttributes)
    414 {
     469        rebuildAllLayoutAttributes();
     470        return;
     471    }
     472
    415473    // Detect changes in layout attributes and only measure those text parts that have changed!
    416474    Vector<SVGTextLayoutAttributes*> newLayoutAttributes;
     
    421479    }
    422480
    423     // Compare m_layoutAttributes with newLayoutAttributes to figure out which attributes got added/removed.
     481    // Compare m_layoutAttributes with newLayoutAttributes to figure out which attributes got added.
    424482    size_t size = newLayoutAttributes.size();
    425483    for (size_t i = 0; i < size; ++i) {
     
    429487    }
    430488
    431     size = affectedAttributes.size();
    432     for (size_t i = 0; i < size; ++i)
    433         m_layoutAttributesBuilder.rebuildMetricsForTextRenderer(affectedAttributes[i]->context());
    434 
    435489    m_layoutAttributes = newLayoutAttributes;
    436490}
  • trunk/Source/WebCore/rendering/svg/RenderSVGText.h

    r115981 r116498  
    4949
    5050    bool needsReordering() const { return m_needsReordering; }
     51    Vector<SVGTextLayoutAttributes*>& layoutAttributes() { return m_layoutAttributes; }
     52
     53    void subtreeChildAdded(RenderObject*);
     54    void subtreeChildWillBeDestroyed(RenderSVGInlineText*, Vector<SVGTextLayoutAttributes*>& affectedAttributes);
     55    void subtreeChildWasDestroyed(RenderSVGInlineText*, Vector<SVGTextLayoutAttributes*>& affectedAttributes);
     56    void subtreeStyleChanged(RenderSVGInlineText*);
     57    void subtreeTextChanged(RenderSVGInlineText*);
    5158
    5259    // Call this method when either the children of a DOM text element have changed, or the length of
    5360    // the text in any child element has changed.
    5461    void invalidateTextPositioningElements();
    55 
    56     void layoutAttributesChanged(RenderObject*);
    57     void layoutAttributesWillBeDestroyed(RenderSVGInlineText*, Vector<SVGTextLayoutAttributes*>& affectedAttributes);
    58     void rebuildLayoutAttributes(bool performFullRebuild = false);
    59     void rebuildLayoutAttributes(Vector<SVGTextLayoutAttributes*>& affectedAttributes);
    60 
    61     Vector<SVGTextLayoutAttributes*>& layoutAttributes() { return m_layoutAttributes; }
    6262
    6363private:
     
    9292    virtual void updateFirstLetter();
    9393
     94    void rebuildAllLayoutAttributes();
     95    void rebuildLayoutAttributes();
     96
    9497    bool m_needsReordering : 1;
    9598    bool m_needsPositioningValuesUpdate : 1;
  • trunk/Source/WebCore/rendering/svg/SVGRootInlineBox.cpp

    r110285 r116498  
    7474    ASSERT(textRoot);
    7575
    76     textRoot->rebuildLayoutAttributes();
    7776    Vector<SVGTextLayoutAttributes*>& layoutAttributes = textRoot->layoutAttributes();
    7877    if (layoutAttributes.isEmpty())
  • trunk/Source/WebCore/rendering/svg/SVGTextLayoutAttributesBuilder.cpp

    r107862 r116498  
    2727#include "SVGTextPositioningElement.h"
    2828
    29 // Set to a value > 0 to dump the text layout attributes
    30 #define DUMP_TEXT_LAYOUT_ATTRIBUTES 0
    31 
    3229namespace WebCore {
    3330
     
    174171    for (unsigned i = 0; i < size; ++i)
    175172        fillCharacterDataMap(m_textPositions[i]);
    176 
    177 #if DUMP_TEXT_LAYOUT_ATTRIBUTES > 0
    178     fprintf(stderr, "\nDumping ALL layout attributes for RenderSVGText, renderer=%p, node=%p (m_textLength: %i)\n", textRoot, textRoot->node(), m_textLength);
    179     m_characterDataMap.dump();
    180 #endif
    181173}
    182174
Note: See TracChangeset for help on using the changeset viewer.