User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0 Build ID: 20150125222008 Steps to reproduce: Open test case at http://svg.cc/posts/textPath_crashes_ff.svg Actual results: Firefox crashes with out of memory: 0x00000000C0000000 bytes requested Expected results: A blue line and a green line with text "Eisack" along the green line should be displayed

The crash is probably caused by the SVGTextPathElement in combination with large coordinates and the relative curveto command ("c") in the referenced path with id="tp_crash". If you replace the relative curveto command with a simple path (id="tp_fine") the browser does not crash. See the markup in plain text: http://svg.cc/posts/textPath_crashes_ff.svg.txt

This looks to be an infinite loop in FlattenBezierCurveSegment caused by accumulating floating point roundoffs. The attached reduced testcase contains a subpath of Klaus's path that winds up in an infinite loop in FlattenBezierCurveSegment on the values t=0.0110847075 and currentCP={(699499.875,-177724.891), (699501.375,-177725.594), (699501.688,-177726.297), (699499,-177727)}. With those values the splitBezier call returns the same path (i.e. it doesn't split, because of float sums like 699499.875 + 0.0166270602 = 699499.875) and we run out of memory as the same LineTo op gets written to aSink over and over. (Also note that that path is _not_ a subpath of the original path, so (presumably) repeated float errors in SplitBezier have been deforming the original curve as the FlattenBezierCurveSegment loop runs.) If I hack up FlattenBezierCurveSegment and SplitBezier to keep doubles instead of floats everything works. If that's the desired solution let me know and I can work on a patch.

Go for it I say.

The cleanest solution seems to be to just have everything called from FlattenBezier use doubles instead of Floats, but if that's too much let me know and I can submit a patch affecting just FlattenBezierCurveSegment and SplitBezier.

You should add your reduced crashtest as an actual crashtest to the codebase i.e. include it in the patch

Comment on attachment 8607230 [details] [diff] [review] bug1134549.diff Nice work. Bas wrote this code, so I'm going to punt the review to him.

Added the crashtest.

Comment on attachment 8607549 [details] [diff] [review] bug1134549.diff Review of attachment 8607549 [details] [diff] [review]: ----------------------------------------------------------------- I'm okay with this I suppose. It will cause somewhat of a performance hit though, but this doesn't need to be a big problem. I believe technically our SVG implementation only needs to support floats though and not doubles, is this assumption correct?

(In reply to Bas Schouten (:bas.schouten) from comment #8) > I believe technically our > SVG implementation only needs to support floats though and not doubles, is > this assumption correct? There is no technical requirement in the specification to use double precision (although it is recommended). However, we have on occasion been bitten by conversions to using single precision.

Thanks for the patch Tom. I assume you don't have commit access yet so I pushed it for you.

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9998263&repo=mozilla-inbound - thats also the failure from the try run so i guess we need a new try run before landing

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/55b78f481396

Any ideas Tom?

It works on linux, but is definitely still broken on Windows, where I see the same behavior with the patch on as I got without the patch (comment 2). The screenshot is with the patch applied, with the computation for cp1a.x in splitBezier broken out into separate computations. If the debug values are to believed, we're getting, all as doubles: cp1a.x = 699499.875 + 0.016627062112092972 --> 699499.87500000000 (which is the same result as in the float case, only this time with doubles...). (If I compute the sum "699499.875 + 0.016627062112092972" with literals I get the expected 699499.89162706211.) I'm not sure what to make of that - linux shows the computational behavior I would expect with doubles: as the loop runs you get random looking aControlPoint values with 11 "random" decimal digits, whereas on Windows the control point values in aControlPoints never show more than five non-zero decimal places (at least as long as I watched). I'll try some more stuff tomorrow, but right now I'm stumped!

On Windows, if I artificially add the following in splitBezier (just for context): double d1 = 699499.; double d2 = .01; I get double sum = d1 + d2 --> 699499.00000000000 so I would say something floating point is definitely "broken" (it works correctly in a vanilla non-mozilla msvc project) - my impression is that it's doing double addition at floating point precision. The actual splitBezier computations follow a similar pattern, as described in comment 16. If I add an _fpreset(); call at the start of splitBezier then reducedCrash.svg doesn't hang (i.e. the crashtest loads correctly and passes) and all of the artificial computations look correct. So is there some MS-specific precision setting happening somewhere that's being applied to the code in Path.cpp? (I'm assuming not all firefox doubles additions are working this way...) I've never dealt with _fpreset before, and I'm not sure how those types of settings are handled in firefox - has anybody run into this before?

PathD2D::StreamToSink makes the D2D system call mGeometry->Simplify to stream a bezier curve to a StreamingGeometrySink, and from within the Simplify call, the virtual AddBeziers gets called on the StreamingGeometrySink. AddBeziers calls BezierTo, which calls FlattenBezier. But the system Simplify call evidently changes the floating point computation settings internally, because outside of Simplify, _controlfp_s(control_word, 0, 0) returns 0x9001f, which corresponds to a "Precision control" of 53 bits, but in any function called from within the Simplify call it returns 0xa001f, which corresponds to 24 bits. (See https://msdn.microsoft.com/en-us/library/c9676k6h.aspx for _controlfp_s.) The sum failures of comment 17 correspond exactly with that change in precision control. The attached patch resets the precision control to a default value in AddBeziers before calling BezierTo, and then restores it to whatever setting Simplify had it on before returning. This is my first experience with "Precision control", so I'm not sure if that's reasonable or not, but it does fix the bug on Windows.

Comment on attachment 8609719 [details] [diff] [review] bug1134549.diff Review of attachment 8609719 [details] [diff] [review]: ----------------------------------------------------------------- Hrm, -if- we do this, we should do it for the other calls as well. However this makes me feel a little iffy. What do you think Jeff?

If it's going to be in more than one place then its probably worth writing it as a RAII class AutoRestoreFP perhaps which gets the control flag in its constructor and restores it in its destructor.

Comment on attachment 8609719 [details] [diff] [review] bug1134549.diff Review of attachment 8609719 [details] [diff] [review]: ----------------------------------------------------------------- It seems like we should restore the floating point mode to what Gecko expects and we should do it in all of the callbacks.

Created AutoRestoreFP (thanks for the suggestion Robert) and added it to StreamingGeometrySink overrides that operate on mSink (hope that's the correct interpretation of "all").

Comment on attachment 8621744 [details] [diff] [review] 1134549.diff +class AutoRestoreFP Should be + class MOZ_STACK_CLASS AutoRestoreFP to ensure it's used correctly.

Thanks again Robert.

Here's a possibly related bug where FF is returning NaN from Path.getTotalLength(). Tom, does your patch fix this bug also? If not, I will create a new bug report. http://stackoverflow.com/questions/31111192/why-will-firefox-not-measure-the-length-of-this-svg-path Here's a simplified test case: <svg viewBox="0 0 1200 900"> <path id="trouble" d="M0,0 C0.8-7.2,7.2-10.7,12.2-4.4"/> </svg> // JS: var path = document.getElementById('trouble'); var length = path.getTotalLength(); alert("length: ", length); http://jsfiddle.net/fh81dqgp/1/

Bitrot update.

(In reply to Paul LeBeau from comment #25) The jsfiddle works for me in ff38 if I change the ',' to a '+' in the alert; the codepen example doesn't work in ff38 but it does work in Nightly with this patch applied (I get a length of 113.704...).

(In reply to Tom Klein from comment #27) Apologies. I switched from console.log() to alert() part way through the simplification process and confused myself. Here is the corrected test case. Would you mind testing this in your patched version please? <svg viewBox="0 0 1200 900"> <path id="trouble" d="M1051.19,607.23 c0.8-7.2,7.2-10.7,12.2-4.4"/> </svg> http://jsfiddle.net/fh81dqgp/2/ (Originated as a StackOverflow question: http://stackoverflow.com/questions/31111192/why-will-firefox-not-measure-the-length-of-this-svg-path )

(In reply to Paul LeBeau from comment #28) Yes, it works on Nightly with the patch applied.

(In reply to Tom Klein from comment #29) Thanks Tom

Bas, this crash-fix has been awaiting review for a while -- do you know when you'll be able to take a look? (Or, if you don't have cycles to get to it soon, could you suggest someone else to review gfx/2d/Path.cpp & PathD2D.cpp changes?)

(In reply to Daniel Holbert [:dholbert] from comment #31) > Bas, this crash-fix has been awaiting review for a while -- do you know when > you'll be able to take a look? > > (Or, if you don't have cycles to get to it soon, could you suggest someone > else to review gfx/2d/Path.cpp & PathD2D.cpp changes?) Are we really sure supporting these high precision coordinates in our implementation is worth the performance hits? We could choose to avoid the crash without switching to Double but just suffer glitches/inaccuracies.

(In reply to Bas Schouten (:bas.schouten) from comment #32) > Are we really sure supporting these high precision coordinates in our > implementation is worth the performance hits? We could choose to avoid the > crash without switching to Double but just suffer glitches/inaccuracies. In complex files with long paths the glitches/inaccuracies can be pretty painful. E.g. animating stroke-dashoffset to a path length that isn't really the exact path length can leave little segments at the endpoints where there should be nothing rendered. Personally, as an author, I would basically always prefer a small performance hit over a rendering error.

For what it's worth, bug 1066556 (which itself has 2 dups), bug 1044355, bug 1044372, and bug 1044449 are also fixed by this patch.

Bas: that depends on how IE/Chrome behave. Tom: can you shed any light on that? If you hack up a patch to avoid the infinite loop (by checking that we don't recurse with the same values, or whatever), do we end up with the testcases in the bugs you mention in comment 34 giving noticeably different results to IE/Chrome?

[canceling my needinfo from comment 32 since I don't know the answer and it sounds like others are already discussing it]

(In reply to Jonathan Watt [:jwatt] from comment #35) > Tom: can you shed any light on that? If you hack up a patch to avoid the > infinite loop (by checking that we don't recurse with the same values, or > whatever), do we end up with the testcases in the bugs you mention in > comment 34 giving noticeably different results to IE/Chrome? There are already some cases where the loop finishes but returns the wrong number (so presumably we'd have to live with those errors), and I would say our results in those cases are noticeably different to IE/Chrome. See, for example, the testcases in bug 1044449, bug 1044372 (both the reporter's original testcase and Robert's reduced version), bug 1066592, and bug 972515 (where we return NaN in all testcases). Let me know if you'd still like to see what happens in the infinite loop cases if we stop the loop, I'd be happy to check.

I tried limiting the number of iterations in FlattenBezierCurveSegment to 100 (should be higher or lower? no idea) which fixes the crash in both test cases and the result seems to be identical to what IE and Edge displays. I think avoiding crashes is paramount so something must be done to that effect. About the patch which increases precision... I am personally in favour of it because as Tom Klein pointed out in comment 34 it fixes an assortment of what I think looks like commonplace examples that simply shouldn't be broken, somewhat unlike the rather obscure example in this bug. Without the precision patch and with the number of iterations limited to 100, Nightly produces results in at least bug 1044372 even farther from IE than with unlimited iterations. But I suppose that could be remedied by increasing the maximum number of iterations, given that the exact number is not important, the point (I guess) is that it always will terminate. Should I look into how this bug and the ones in comment 34 behave when I apply the precision patch and cap the number of iterations at a higher number?

Bas: I agree that we should be able to render the various examples correctly as Chrome/IE do. Can you review this any time soon?

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e73fc0dfb01e190959b396525586096f68853fb Bug 1134549 - Switch FlattenBezier from floats to doubles. r=bas

Got a report on #firefox about http://beta.nbrc.ac.in/newweb and it links back to this bug. It affects 47.0.1. We might want to do an uplift of this fix?

(In reply to Cork from comment #43) > Got a report on #firefox about http://beta.nbrc.ac.in/newweb and it links > back to this bug. It affects 47.0.1. We might want to do an uplift of this > fix? I'm not quite sure this bug is severe enough to justify uplifting to a dot release.. but I'll leave that up to release management.

I don't know, what is the volume in term of crashes?