Bug 6890

Summary: SVG - invalid polyline data causes hang
Product: WebKit Reporter: Mark Vakoc <mark>
Component: SVGAssignee: Alexander Kellett <a>
Severity: Normal CC: ap
Priority: P1 Keywords: SVGHitList
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Bug Depends on:    
Bug Blocks: 6951    
Description Flags
sample svg with invalid polyline data
testcases for 6890 and 6951
eric: review-
cleaned version of the method eric: review+

Description Mark Vakoc 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.
Comment 1 Mark Vakoc 2006-01-28 09:36:46 PST
Created attachment 6051 [details]
sample svg with invalid polyline data
Comment 2 Eric Seidel (no email) 2006-01-28 13:12:02 PST
Reproducible crasher = SVG HitList.  Addign Keyword.  This shoudl be simple to fix.
Comment 3 Darin Adler 2006-02-10 09:31:53 PST
Reproducible crashers, even in SVG, are P1.
Comment 4 Alexander Kellett 2006-02-13 12:21:53 PST
Created attachment 6463 [details]
testcases for 6890 and 6951
Comment 5 Alexander Kellett 2006-02-13 12:22:28 PST
Created attachment 6464 [details]
Comment 6 Eric Seidel (no email) 2006-02-14 03:21:27 PST
Comment on attachment 6464 [details]

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 7 Eric Seidel (no email) 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.
Comment 8 Eric Seidel (no email) 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.
Comment 9 Alexander Kellett 2006-03-02 16:09:41 PST
Created attachment 6815 [details]
cleaned version of the method
Comment 10 Eric Seidel (no email) 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.
Comment 11 Darin Adler 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?
Comment 12 Darin Adler 2006-03-06 15:08:30 PST
Alex landed this change already.