[webkit-reviews] review denied: [Bug 59136] Null deref when no use element exists for SVG element instance : [Attachment 90628] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Cris Neckar
<cdn at chromium.org>'s request for review:
Bug 59136: Null deref when no use element exists for SVG element instance
https://bugs.webkit.org/show_bug.cgi?id=59136

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=90628&action=review

Sorry Cris, I fear that involves a bit more work:

> Source/WebCore/svg/SVGAnimationElement.cpp:347
> +	       useElement->setNeedsStyleRecalc();

I'm confused a shadowTreeElement w/o correspondingUseElement, what _exactly_
forces us into such a state?
What happens if you completely disable the shadowTreeElement modifications, if
correspondingUseElement is 0, does the animation still work?

> LayoutTests/svg/custom/use-null-instanceroot-crash.svg:5
> +	  
document.getElementById("use_elem").instanceRoot.correspondingElement = 0;

Hm, that is needed? Or does it even trigger the bug?? In the code changes,
you're checking whether correspondingUseElement is zero, not
correspondingElement?
SVGElementInstance.idl defines: readonly attribute SVGElement
correspondingElement;
so this shouldn't take any affect at all, does it?

> LayoutTests/svg/custom/use-null-instanceroot-crash.svg:17
> +	   <animateTransform attributeName="transform" />

Why are two animations needed, why the transform attribute?
You definately also need a new testcase using the JS SVG Animation test
framework, see the other examples in LayoutTests/svg/animation.
We need to be sure that each of the animation runs properly, so you have to
setup a real animation, cy from 0 to 15, for example.

Basically we need to sample the animation at various times, and assure both
animations, the one of circleID, and the circleID clone through <use> work as
expected.

> LayoutTests/svg/custom/use-null-instanceroot-crash.svg:19
> +    <use id="use_elem" xlink:href="#circleID" />

You should use x="30", to translate the second circle, so when running the test
in the browser, you'll actually see two circles.