[webkit-reviews] review denied: [Bug 62914] Incorrectly placed SVG gradients can cause crashes when referenced : [Attachment 97823] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Jeffrey Pfau <jeffrey at endrift.com>'s
request for review:
Bug 62914: Incorrectly placed SVG gradients can cause crashes when referenced
https://bugs.webkit.org/show_bug.cgi?id=62914

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=97823&action=review

The fix is not correct, see comments below.

> LayoutTests/svg/custom/invalid-gradient-with-xlink.svg:22
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?>
> +<svg
> +   xmlns="http://www.w3.org/2000/svg"
> +   xmlns:xlink="http://www.w3.org/1999/xlink"
> +   version="1.0"
> +   width="200"
> +   height="200">
> +<script><![CDATA[
> +	 if (window.layoutTestController)
> +	   layoutTestController.dumpAsText();
> +]]></script>
> +<defs>
> +  <linearGradient id="linearGradient1" xlink:href="#linearGradient0" />
> +</defs>
> +<path d="M 0,0 C 0,1 1,1 1,0 L 0,0 z " style="fill:url(#linearGradient1);" 
id="path0" />
> +<path id="path1">
> +  <linearGradient id="linearGradient0">
> +    <stop />
> +  </linearGradient>
> +</path>
> +<text y="20" x="10">PASS: The test did not crash.</text>
> +</svg>

Given the fact that we should fill the rect with a gradient, a dumpAsText test
is not enough. We need a pixel test. The style of the SVG does not make it self
explaining. Use something more simple like that:

<svg xmlns="http://www.w3.org/2000/svg"
xmlns:xlink="http://www.w3.org/1999/xlink">
<defs>
  <linearGradient id="gradient1" xlink:href="#gradient2" />
</defs>
<rect width="200" height="200" fill="url(#gradient1)">
  <linearGradient id="gradient2">
    <stop stop-color="green"/>
  </linearGradient>
</rect>
</svg>

The green indicates that we should draw fill the rect with the gradient. On
pass you'd see a green rect.

> Source/WebCore/ChangeLog:8
> +	   Added check to ensure gradient stops have the correct properties
before attempting to access them.

The patch is not correct. We should draw the gradient, independent where it is
located. In a <defs> section or not does not matter.