Summary: | SVG - invalid polyline data causes hang | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Vakoc <mark> | ||||||||||
Component: | SVG | Assignee: | Alexander Kellett <a> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap | ||||||||||
Priority: | P1 | Keywords: | SVGHitList | ||||||||||
Version: | 420+ | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 6951 | ||||||||||||
Attachments: |
|
Description
Mark Vakoc
2006-01-28 09:33:58 PST
Created attachment 6051 [details]
sample svg with invalid polyline data
Reproducible crasher = SVG HitList. Addign Keyword. This shoudl be simple to fix. Reproducible crashers, even in SVG, are P1. Created attachment 6463 [details]
testcases for 6890 and 6951
Created attachment 6464 [details]
fix
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.
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. 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.
Created attachment 6815 [details]
cleaned version of the method
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.
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?
Alex landed this change already. |