Closed Bug 1322537 Opened 7 years ago Closed 7 years ago

SVG: hang & memory exhaustion inside of mozilla::gfx::Path::EnsureFlattenedPath(), with animateMotion

Categories

(Core :: SVG, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox53 --- affected
firefox56 --- fixed

People

(Reporter: tsmith, Assigned: kevin.hsieh)

References

(Blocks 2 open bugs)

Details

(Keywords: hang, perf, testcase, Whiteboard: [fuzzblocker])

Attachments

(5 files)

      No description provided.
Summary: SVG with memory exhaustion with animate motion → SVG: memory exhaustion with animate motion
Keywords: hang
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)
Severity: major → critical
Priority: -- → P3
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.
Flags: needinfo?(dholbert)
Whiteboard: [fuzzblocker]
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.
Flags: needinfo?(dholbert) → needinfo?(bas)
(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.
Flags: needinfo?(bas)
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 dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6df0dd7948ad
Increase tolerance when splitting Bezier curves to prevent hang. r=bas
https://hg.mozilla.org/mozilla-central/rev/6df0dd7948ad
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 963223
You need to log in before you can comment on or make changes to this bug.