Closed Bug 1322537 Opened 4 years ago Closed 4 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.
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.
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 email@example.com: 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.