Bug 73021

Summary: Divide by zero for zero-length arcs
Product: WebKit Reporter: Stephen Chenney <schenney>
Component: SVGAssignee: Stephen Chenney <schenney>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dglazkov, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 71820, 73677    
Attachments:
Description Flags
Test case
none
Patch
none
Patch
schenney: review-, schenney: commit-queue-
Patch
none
Patch none

Stephen Chenney
Reported 2011-11-23 07:33:35 PST
Created attachment 116356 [details] Test case The code in SVGPathParser::decomposeArcToCubic performs a divide by zero when the start and endpoint of the arc are the same. This value happens to result in nothing being drawn, but it is poor behavior when linecaps are defined. Specifically, for consistency with other paths and continuity in animation, non-butt linecaps should be drawn for zero-length arcs. See http://lists.w3.org/Archives/Public/www-svg/2011Nov/0129.html I propose catching the zero length arc case and converting it to a zero length line. This has the advantage of matching behavior for the zero arc radius case.
Attachments
Test case (1003 bytes, image/svg+xml)
2011-11-23 07:33 PST, Stephen Chenney 		no flags
Details
Patch (12.66 KB, patch)
2011-11-23 08:41 PST, Stephen Chenney 		no flags
DetailsFormatted DiffDiff
Patch (41.28 KB, patch)
2011-11-23 12:30 PST, Stephen Chenney 		schenney: review-
schenney: commit-queue-
DetailsFormatted DiffDiff
Patch (41.16 KB, patch)
2011-11-23 13:35 PST, Stephen Chenney 		no flags
DetailsFormatted DiffDiff
Patch (24.94 KB, patch)
2011-12-02 11:59 PST, Stephen Chenney 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Stephen Chenney
Comment 1 2011-11-23 08:41:15 PST
Created attachment 116367 [details] Patch Do not submit this patch - needs mac expectations
WebKit Review Bot
Comment 2 2011-11-23 09:26:43 PST
Comment on attachment 116367 [details] Patch Attachment 116367 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10608399 New failing tests: svg/as-background-image/svg-as-background-4.html
Stephen Chenney
Comment 3 2011-11-23 12:30:56 PST
Created attachment 116398 [details] Patch This patch is ready for commit.
Stephen Chenney
Comment 4 2011-11-23 12:46:56 PST
Comment on attachment 116398 [details] Patch This has problems - need to fix.
Stephen Chenney
Comment 5 2011-11-23 13:35:38 PST
Created attachment 116413 [details] Patch This patch is really ready for commit.
Alexey Proskuryakov
Comment 6 2011-11-23 14:25:02 PST
This test looks like it should have cross-platform expectations. What makes them different?
Stephen Chenney
Comment 7 2011-11-23 14:30:56 PST
The platforms differ by one pixel (or so) in the radius of the circle drawn for linecaps in CoreGraphics when compared to skia. The expectations are set up so that anything using CoreGraphcis catches the mac expectations, while chromium skia on mac catches the chromium-mac expectations and linux and windows catch win expectations. I have verified linux, mac, chromium-mac-skia and chromium-mac-cg.
Alexey Proskuryakov
Comment 8 2011-11-23 14:50:10 PST
Thank you for the explanation. Should this be considered a bug in either implementation?
Stephen Chenney
Comment 9 2011-11-23 14:57:14 PST
The bug manifested in all the platforms I tested the file on (before any fix). That was Safari, Chrome on Mac and Linux, and the Linux desktop file previewer (which I think is the gtk port). The code that is modified by the patch is common to all platforms, so I presume it affects all platforms. I suspect that expectations will be required for the Qt port, but I have no way of generating those. The Qt gardener should be able to update expectations once the patch is in.
Stephen Chenney
Comment 10 2011-11-28 17:10:18 PST
Comment on attachment 116413 [details] Patch It occurs to me that I need to test an arc that has the same start and end point but the large sweep (i.e a circle). This should not be replaced with a line.
Alexey Proskuryakov
Comment 11 2011-11-28 17:19:25 PST
Re comment 9 - my question was whether the one pixel difference between CG and Skia is a bug in either of those, or if it's something we don't intend to be compatible.
Stephen Chenney
Comment 12 2011-11-29 07:12:15 PST
Comment on attachment 116413 [details] Patch On second thought, an arc with the same starting and ending point is ill-defined as a circle, because it is impossible to come up with a center. So this patch stands.
Stephen Chenney
Comment 13 2011-11-29 07:13:36 PST
(In reply to comment #11) > Re comment 9 - my question was whether the one pixel difference between CG and Skia is a bug in either of those, or if it's something we don't intend to be compatible. The platforms are free to make different assumptions and different rounding errors and different anti-aliasing behavior. So we do not force them to all be the same. It's just not practical.
Nikolas Zimmermann
Comment 14 2011-12-02 11:08:35 PST
Comment on attachment 116413 [details] Patch Looks good to me! r=me.
Stephen Chenney
Comment 15 2011-12-02 11:33:17 PST
Comment on attachment 116413 [details] Patch Reverting commit-queue while expectations are sorted out
Stephen Chenney
Comment 16 2011-12-02 11:59:06 PST
Created attachment 117664 [details] Patch
WebKit Review Bot
Comment 17 2011-12-02 19:12:49 PST
Comment on attachment 117664 [details] Patch Clearing flags on attachment: 117664 Committed r101895: <http://trac.webkit.org/changeset/101895>
WebKit Review Bot
Comment 18 2011-12-02 19:12:56 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.