[webkit-reviews] review granted: [Bug 209773] Hit test with clipPath referencing parent element causes infinite recursion (when referenced by use element) : [Attachment 395225] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Doug Kelly
<dougk at apple.com>'s request for review:
Bug 209773: Hit test with clipPath referencing parent element causes infinite
recursion (when referenced by use element)
https://bugs.webkit.org/show_bug.cgi?id=209773

Attachment 395225: Patch

https://bugs.webkit.org/attachment.cgi?id=395225&action=review




--- Comment #4 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 395225
  --> https://bugs.webkit.org/attachment.cgi?id=395225
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395225&action=review

> Source/WebCore/ChangeLog:3
> +	   Hit test with clipPath referencing parent element causes infinite
recursion (when referenced by use element)

I think the cyclic referencing can happen without the use element. So maybe you
need to change the title to something like: SVG cyclic referencing causes
hit-testing a clipPath to recurse infinitely.

> Source/WebCore/ChangeLog:12
> +	   visiting the same element twice, and thus breaking any cycles which
might occur in the SVG document.  By
> +	   storing the data at the top of the SVG document, this keeps a finite
scope for the set of renderers, and

The visited elements are stored now in the RenderSVGRoot.

> Source/WebCore/ChangeLog:19
> +
> +

One extra line.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:405
> +    ASSERT(m_visitedElements.isEmpty());

Since you made m_visitedElements accessible from the methods of this class
expect in this place, can't we add a new function and name isVisitingAny() or
something similar and replace this assertion by 

ASSERT(!isVisitingAny());

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:458
> +    // We should never be visiting an element more than once.

This comment adds no value. The assertion below says the same thing.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:460
> +    ASSERT(!m_visitedElements.contains(&element));
> +    m_visitedElements.add(&element);

Another way to write this and save one HashSet traversal is:

auto result = m_visitedElements.add(&element);
ASSERT_UNUSED(result, result.isNewEntry);

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:466
> +    ASSERT(m_visitedElements.contains(&element));
> +    m_visitedElements.remove(&element);

Another way to write this is:

bool result = m_visitedElements.remove(&element);
ASSERT_UNUSED(result, result);

> Source/WebCore/rendering/svg/SVGRenderSupport.h:105
> +    WeakPtr<RenderElement> m_element;

I am puzzled by this WeakPtr. m_visitedElements is supposed to keep renderers
while they are alive in the calling stack. So there is no possibility at any
point that any of them will be freed before it is removed from
m_visitedElements. So I think we do not need m_element and it is fine to have
m_visitedElements be HashSet of raw pointers.