RESOLVED FIXED 6890
SVG - invalid polyline data causes hang
https://bugs.webkit.org/show_bug.cgi?id=6890
Summary SVG - invalid polyline data causes hang
Mark Vakoc
Reported 2006-01-28 09:33:58 PST
The attached SVG will cause WebKit (lastest nightly ~1/27/2006) to hang indefinitely. It has invalid polyline data (INF as one of the coordinates). Firefox and Adobe/IE report the problem, but WebKit goes to pinwheel. WARNING: Viewing this actually causes the entire computer to hang, and had to manually power off (perhaps another issue for apple?), so I wouldn't view it outside a debugger.
Attachments
sample svg with invalid polyline data (393 bytes, image/svg+xml)
2006-01-28 09:36 PST, Mark Vakoc
no flags
testcases for 6890 and 6951 (2.23 KB, patch)
2006-02-13 12:21 PST, Alexander Kellett
no flags
fix (2.09 KB, patch)
2006-02-13 12:22 PST, Alexander Kellett
eric: review-
cleaned version of the method (2.12 KB, patch)
2006-03-02 16:09 PST, Alexander Kellett
eric: review+
Mark Vakoc
Comment 1 2006-01-28 09:36:46 PST
Created attachment 6051 [details] sample svg with invalid polyline data
Eric Seidel (no email)
Comment 2 2006-01-28 13:12:02 PST
Reproducible crasher = SVG HitList. Addign Keyword. This shoudl be simple to fix.
Darin Adler
Comment 3 2006-02-10 09:31:53 PST
Reproducible crashers, even in SVG, are P1.
Alexander Kellett
Comment 4 2006-02-13 12:21:53 PST
Created attachment 6463 [details] testcases for 6890 and 6951
Alexander Kellett
Comment 5 2006-02-13 12:22:28 PST
Eric Seidel (no email)
Comment 6 2006-02-14 03:21:27 PST
Comment on attachment 6464 [details] fix A couple comments: 1. I think it's better to declare optr on its own line. (make the declaration less confusing). 2. I think optr deserves a better name. Honestly, I think ptr deserves a better name... The code is hard enough to read as is... giving the variables cryptic names only hurts :( 3. While you're in there, you might as well remove the commented out //std::cout lines. Three little nits, if you were landing your own patches you could just fix them as you landed, but since someone else has to land this, I think it's best to get one more round of patches.
Eric Seidel (no email)
Comment 7 2006-02-14 03:24:34 PST
Comment on attachment 6463 [details] testcases for 6890 and 6951 These are fine. Ideally we would remove more of the extra junk when reducing, such as: xmlns:my="http://mark.vakoc.com/" xmlns:fn="http://www.w3.org/2005/02/xpath-functions", etc. Also no need to have svg:polyline, just polyline The second test is another example. You could remove the <![CDATA[ block as well as the xlink namespace. the tests look fine otherwise. Whoever lands these will have to actually generate the final expected results.
Eric Seidel (no email)
Comment 8 2006-02-15 01:44:27 PST
Comment on attachment 6463 [details] testcases for 6890 and 6951 Removing review flag from test cases. They're fine to land. Waiting still for revised patch.
Alexander Kellett
Comment 9 2006-03-02 16:09:41 PST
Created attachment 6815 [details] cleaned version of the method
Eric Seidel (no email)
Comment 10 2006-03-03 01:24:16 PST
Comment on attachment 6815 [details] cleaned version of the method svgPolyTo(xPos, xPos, segmentNum++); looks wrong. It should be svgPolyTo(xPos, yPos, segmentNum++); I think. Otherwise this looks fine. Please be sure to add the layout tests, results, and change log when landing. With that fixed, this is good to land. r=me.
Darin Adler
Comment 11 2006-03-06 09:11:38 PST
Comment on attachment 6815 [details] cleaned version of the method This code looks to me like it can look at a character past the end of the string. Where's the check that currSegment < eoString before looking at *currSegment?
Darin Adler
Comment 12 2006-03-06 15:08:30 PST
Alex landed this change already.
Note You need to log in before you can comment on or make changes to this bug.