[webkit-reviews] review granted: [Bug 83405] REGRESSION(r105057): Infinite loop inside SVGTextLayoutEngine::currentLogicalCharacterMetrics : [Attachment 140752] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 83405: REGRESSION(r105057): Infinite loop inside
SVGTextLayoutEngine::currentLogicalCharacterMetrics
https://bugs.webkit.org/show_bug.cgi?id=83405

Attachment 140752: Patch
https://bugs.webkit.org/attachment.cgi?id=140752&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=140752&action=review


> Source/WebCore/rendering/svg/RenderSVGInlineText.cpp:86
> +    textRenderer->subtreeChildWasDestroyed(this, affectedAttributes);

This function doesn’t seem to be named right. As I understand it, at this
point, the subtreeChild was not yet destroyed. It won’t be destroyed until
after we return.

> Source/WebCore/rendering/svg/RenderSVGText.cpp:122
> +static inline void recursiveUpdateMetrics(RenderObject* start,
SVGTextLayoutAttributesBuilder& builder)
> +{
> +    if (start->isSVGInlineText()) {
> +	   builder.rebuildMetricsForTextRenderer(toRenderSVGInlineText(start));

> +	   return;
> +    }
> +
> +    for (RenderObject* child = start->firstChild(); child; child =
child->nextSibling())
> +	   recursiveUpdateMetrics(child, builder);
> +}

There is no need to write a function like this recursively. It can be written
iteratively using the nextInPreOrder and nextInPreOrderAfterChildren functions.


> Source/WebCore/rendering/svg/RenderSVGText.cpp:204
> +    unsigned position = m_layoutAttributes.find(currentLayoutAttributes);

This should be size_t, not unsigned.

> Source/WebCore/rendering/svg/RenderSVGText.cpp:205
> +    ASSERT(position != notFound);

This assertion will never fire on a 64-bit system because position is a 32-bit
unsigned and notFound is a 64-bit size_t constant.

> Source/WebCore/rendering/svg/RenderSVGText.cpp:221
> +static inline void recursiveCollectLayoutAttributes(RenderObject* start,
Vector<SVGTextLayoutAttributes*>& attributes)
> +{
> +    for (RenderObject* child = start->firstChild(); child; child =
child->nextSibling()) {
> +	   if (child->isSVGInlineText()) {
> +	      
attributes.append(toRenderSVGInlineText(child)->layoutAttributes());
> +	       continue;
> +	   }
> +
> +	   recursiveCollectLayoutAttributes(child, attributes);
> +    }
> +}

Same comment about the fact that this can be written non-recursively easily
using nextInPreOrder.

> Source/WebCore/rendering/svg/RenderSVGText.cpp:242
> +    unsigned size = affectedAttributes.size();

Should be size_t.