Closed Bug 1322537 Opened 4 years ago Closed 3 years ago
SVG: hang & memory exhaustion inside of mozilla::gfx::Path::Ensure
Flattened Path(), with animate Motion
99 bytes, text/html
8.13 KB, text/plain
351 bytes, text/html
322 bytes, text/html
59 bytes, text/x-review-board-request
No description provided.
Summary: SVG with memory exhaustion with animate motion → SVG: memory exhaustion with animate motion
Summary: SVG: memory exhaustion with animate motion → SVG: hang & memory exhaustion, with animateMotion
Summary: SVG: hang & memory exhaustion, with animateMotion → SVG: hang & memory exhaustion inside of mozilla::gfx::Path::EnsureFlattenedPath(), with animateMotion
Here's a backtrace from during the hang. We're inside of gfx::FlattenBezierCurveSegment's call to gfx::SplitBezier. I know we've had death-spirals in that code in the past, from trying to approximate a curve to a certain degree of precision and requiring zillions of tiny straight lines to do so...
Attachment #8817471 - Attachment description: test_case.html → testcase 1 (WARNING, hangs your browser)
CC'ing Bas & Tom Klein who have worked on this Bezier-flattening code before (in bug 1134549).
Here's a testcase with no SMIL, using the getTotalLength() API to trigger the same GFX path-flattening code.
The path in testcase 2 is: <path id="myPath" d="M8,0l69,97m45,-17592186044414A71,23 46,0,0 16382,98Q50,10 48,72T21,0Z"/> -17592186044414 is a 14 digit number - is that supported/expected to work? We don't want to crash even if it isn't supported of course.
The reduced path <path id="myPath" d="M45,-17592186044414A71,23 46,0,0 16382,98"/> with just a single arc also causes a hang.
Attachment #8817572 - Attachment mime type: image/svg+xml → text/html
Oh, that's what Grizzly is: https://en.wikipedia.org/wiki/Fuzz_testing (I guess?). 14 digits makes more sense then :-)
(In reply to Tom Klein from comment #4) > The path in testcase 2 is: > > <path id="myPath" d="M8,0l69,97m45,-17592186044414A71,23 46,0,0 > 16382,98Q50,10 48,72T21,0Z"/> > > -17592186044414 is a 14 digit number - is that supported/expected to work? > We don't want to crash even if it isn't supported of course. you can also use e notation to have numbers as large as a float will store; something around 1e38. In theory we're supposed to use doubles but that would double our memory cost.
See Also: → 1321012
This bug is coming up quite frequently while fuzzing.
Perhaps we could set some reasonable limit on how many splits we'll let ourselves make when flattening a bezier segment? e.g. maybe we could pick some arbitrary maximum loop count for this loop here: https://dxr.mozilla.org/mozilla-central/rev/6f8f10f48ace5692256efd91f011bd23054ee2ec/gfx/2d/Path.cpp#246,261 ...which is large enough for accuracy in common cases, but small enough to prevent us from letting a small testcase eat up all the memory.
(In reply to Daniel Holbert [:dholbert] from comment #9) > Perhaps we could set some reasonable limit on how many splits we'll let > ourselves make when flattening a bezier segment? > > e.g. maybe we could pick some arbitrary maximum loop count for this loop > here: > > https://dxr.mozilla.org/mozilla-central/rev/ > 6f8f10f48ace5692256efd91f011bd23054ee2ec/gfx/2d/Path.cpp#246,261 > ...which is large enough for accuracy in common cases, but small enough to > prevent us from letting a small testcase eat up all the memory. That seems like a fine idea, in practice it's impossible to achieve sufficient accuracy for every conceivable curve. So this seems like a good solution.
3 years ago
Assignee: nobody → kevin.hsieh
Status: NEW → ASSIGNED
I added a little bit of code to increase the tolerance slightly (but multiplicatively) every time the loop that Daniel mentioned gets executed. Reproducing my code comment regarding this change: // Increase tolerance every iteration to prevent this loop from executing // too many times. This approximates the length of large curves more // roughly. In practice, aTolerance is the constant kFlatteningTolerance // which has value 0.0001. With this value, it takes 6,932 splits to double // currentTolerance (to 0.0002) and 23,028 splits to increase // currentTolerance by an order of magnitude (to 0.001). With this, the test cases do not produce any noticeable hang or excessive memory usage. Hopefully this addition doesn't affect non-pathological SVG's, but maybe Bas can comment on that. I also added the first and third testcases to layout/svg/crashtests as 1322537-1.html and 1322537-2.html (skipping the second testcase since it uses the same path as the first testcase and the same API entry point as the third testcase).
Comment on attachment 8890621 [details] Bug 1322537 - Increase tolerance when splitting Bezier curves to prevent hang. https://reviewboard.mozilla.org/r/161776/#review167118 ::: layout/svg/crashtests/1322537-2.html:11 (Diff revision 2) > +<svg> > + <path id="myPath" d="M45,-17592186044414A71,23 46,0,0 16382,98"/> > +</svg> > +<body onload="go()"> Drive-by nit: you should put the <svg> inside of (i.e. after) the <body> here.
Comment on attachment 8890621 [details] Bug 1322537 - Increase tolerance when splitting Bezier curves to prevent hang. https://reviewboard.mozilla.org/r/161776/#review167800 ::: gfx/2d/Path.cpp:260 (Diff revision 3) > * then be approximated by a quadratic equation for which the maximum > * difference from a linear approximation can be much more easily determined. > */ > BezierControlPoints currentCP = aControlPoints; > > - double t = 0; > + double t = 0, currentTolerance = aTolerance; nit: Put this on a separate line. ::: layout/svg/crashtests/1322537-1.html:1 (Diff revision 3) > +<svg> I don't see this being added to the manifest file so it won't actually be run afaict :-).
Attachment #8890621 - Flags: review?(bas) → review+
This looks ready to land (review nits addressed, successful-looking try run, with new crashtests definitely running in the try run [which I verified by inspecting crashtest log]) --> triggered autoland.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/6df0dd7948ad Increase tolerance when splitting Bezier curves to prevent hang. r=bas
You need to log in before you can comment on or make changes to this bug.