Bug 205217 – Nullptr crash if SVG element if element parent becomes document node

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Sunny He]

Add appropriate null checking to ensure that we don't crash in certain edge cases if an SVG element's parent is set to document.

Crash if the element has a transform:
rdar://49890457

Crash if the element is an SVG <text>
rdar://57075206

[Sunny He]

Created attachment 385640 [details]
Patch

[Darin Adler]

Comment on attachment 385640 [details]
Patch

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

> Source/WebCore/rendering/svg/RenderSVGText.cpp:375
> -    } else if (m_needsTextMetricsUpdate || SVGRenderSupport::findTreeRootObject(*this)->isLayoutSizeChanged()) {
> +    } else if (m_needsTextMetricsUpdate || (SVGRenderSupport::findTreeRootObject(*this) && SVGRenderSupport::findTreeRootObject(*this)->isLayoutSizeChanged())) {

The word "find" in the function name here is supposed to be a hint that the operation is nontrivial, so I suggest we restructure slightly so we can use a local variable and don’t need to call the function twice. Or rename the function if "find" is a guaranteed-inexpensive operation.

[Sunny He]

Created attachment 385646 [details]
Patch

[Darin Adler]

Comment on attachment 385646 [details]
Patch

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

> Source/WebCore/rendering/svg/RenderSVGText.cpp:377
> +    } else if (m_needsTextMetricsUpdate) {
> +        RenderSVGRoot* rootObj = SVGRenderSupport::findTreeRootObject(*this);
> +        if (rootObj && rootObj->isLayoutSizeChanged()) {

This implements &&, but in the old code it was ||.

[Sunny He]

Created attachment 385667 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 385667 [details]
Patch

Looks sane to me.

[WebKit Commit Bot]

Comment on attachment 385667 [details]
Patch

Clearing flags on attachment: 385667

Committed r253521: <https://trac.webkit.org/changeset/253521>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

Comment on attachment 385667 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Test: svg/dom/replaceChild-document-crash.html

Where is this test? I do not see it in this patch. It also did not land in r253521.

[Sunny He]

(In reply to Said Abou-Hallawa from comment #9)
> Comment on attachment 385667 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385667&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Test: svg/dom/replaceChild-document-crash.html
> 
> Where is this test? I do not see it in this patch. It also did not land in
> r253521.

Strange, my local git commit shows that file, but it's missing from the uploaded patch. Created new bug 205282 to track getting these tests in.

[Ryosuke Niwa]

*** Bug 207030 has been marked as a duplicate of this bug. ***