If an element has a very big width or height of 1000000000000000000 (on qt more than ~500,000,000,000,000,000) webkit gets a overflow and tell us that width/height is negative.
Created attachment 30132 [details] SVG with a big sized rect A test case for the overflow.
It looks like the biggest number for real values is 2^31 -1 don't know why we just support 31bit numbers.
Created attachment 51467 [details] Patch to use floating point in SVG number parser I've made a little patch and test case for this. The patch prevents the overflow (positive into negative and negative into positive) at the parser. This doesn't solve the problem entirely since the number returned by the parser may now be too big for the later rendering code, but it's a first step. This bug appears to be the cause of http://crbug.com/825.
Hi David, did you notice the -expected.txt results don't make much sense? Can you investigate the cause? 123456789 is correct, and afterwards it's broken... 5 RenderPath {path} at (0,0) size 0x0 [stroke={[type=SOLID] [color=#0000FF]}] [data="M-1000.00,12345679395506094080.00 L200.00,200.00"] 6 RenderPath {path} at (0,0) size 0x0 [stroke={[type=SOLID] [color=#00FF00]}] [data="M600.00,400.00 L1000.00,-98765435164048752640.00"] CC'ing Darin, who might have more comments here!
(In reply to comment #4) > Hi David, > > did you notice the -expected.txt results don't make much sense? Can you > investigate the cause? > 123456789 is correct, and afterwards it's broken... > > 5 RenderPath {path} at (0,0) size 0x0 [stroke={[type=SOLID] > [color=#0000FF]}] [data="M-1000.00,12345679395506094080.00 L200.00,200.00"] > 6 RenderPath {path} at (0,0) size 0x0 [stroke={[type=SOLID] > [color=#00FF00]}] [data="M600.00,400.00 L1000.00,-98765435164048752640.00"] > > CC'ing Darin, who might have more comments here! The results are like that because of FloatType's (lack of) precision. The numbers are 1.234568e+19 and 9.876543e+19. When these are formatted for printing (using a format string of "%.2f"), that is how they appear. I checked the output on Mac and Linux, and perhaps surprisingly they are the same. If we want to prettify these up, we'll have to change the dump render tree code, which will involve different changes on Mac and Linux (since the Linux one uses Skia). On the other hand, we could go to a higher precision representation of floats. Changing the definition of FloatType, however, will have repercussions beyond just this bug. The question is then whether around 7 decimal digits of precision (float) is enough, and if switching to double (around 16 digits) is worth it. It's probable that we will rarely even need 7 significant digits on numbers sufficiently large that precision becomes a problem. But that's a separate (though related) bug from overflow.
By the way, the bug can be "solved" entirely by clamping the output from the parser to std::limits::max()-1 and std::limits::min()+1. With the above suggested patch, the renderer will fail to display anything because the numbers are too big, but if the clamping hack is put in, it will display something that visually appears to be correct (because the coordinates are so big they are effectively the same from the view port). This "fix" is of course a bad idea as transforms may later push the coordinates beyond the limits of integer storage again. However, you may want to put such code in for debugging purposes.
(In reply to comment #5) > I checked the output on Mac and Linux, and perhaps surprisingly they are > the same. The reason I chose 123... and 987... is because they're easy to spot while debugging. When I noticed the output was different than the input, I thought about using other numbers, but any number large enough to cause integer overflow is also large enough to cause floating point imprecision. For example, 1000... changes to 999... and so on, and at least with the current choice of numbers the first few digits remain the same.
Problems with large number output precision are also discussed here: https://bugs.webkit.org/show_bug.cgi?id=23927
There's also some discussion of large number round-tripping through CSS in bug 20674.
Comment on attachment 51467 [details] Patch to use floating point in SVG number parser > template <typename FloatType> static bool _parseNumber(const UChar*& ptr, const UChar* end, FloatType& number, bool skip) > { > - int integer, exponent; > - FloatType decimal, frac; > + int exponent; > + FloatType integer, decimal, frac; I'm not sure I agree with this. Why not use 'long long' for integer and exponent?
(In reply to comment #10) > (From update of attachment 51467 [details]) > > > > template <typename FloatType> static bool _parseNumber(const UChar*& ptr, const UChar* end, FloatType& number, bool skip) > > { > > - int integer, exponent; > > - FloatType decimal, frac; > > + int exponent; > > + FloatType integer, decimal, frac; > > I'm not sure I agree with this. Why not use 'long long' for integer and > exponent? The test case in the patch actually overflows 'long long' as well, so it will return the wrong result. Also, I'm not sure that 'long long' for the exponent is realistic (even though we should aim to handle anything). Even for enormous numbers, the exponent is relatively small (fits into an int).
I'd argue that dimensions that overflow a long long are not going to be encountered in normal content. Agreed on the size of exponent.
I wonder if ConformingHighQualitySVGViewers: http://www.w3.org/TR/SVG11/conform.html#ConformingHighQualitySVGViewers has any relation to anything here.
(In reply to comment #12) > I'd argue that dimensions that overflow a long long are not going to be > encountered in normal content. > Agreed on the size of exponent. Nothing is really gained by using long long for the representation of the integer portion of the coordinate. I think any precision gained in doing so is lost in the subsequent conversion back into FloatType. (In reply to comment #13) > I wonder if ConformingHighQualitySVGViewers: > http://www.w3.org/TR/SVG11/conform.html#ConformingHighQualitySVGViewers > has any relation to anything here. The relevant part would be: "Both a Conforming High-Quality Static SVG Viewer and a Conforming High-Quality Dynamic SVG Viewer must support the following additional features: ... At least double-precision floating point computation on coordinate system transformation numerical calculations." Does WebKit aim to be a Conforming "High-Quality" SVG Viewer? If so, then FloatType needs to be double rather than float, which would affect many things other than this bug.
Created attachment 53973 [details] SVG file of tan(), all values in range of int
Are there any further thoughts on David's proposed patch? It seems like a reasonable approach, and it would be nice to get a fix for the _parseNumber() function, which is clearly broken at present. That being said, there are further issues, as it is easy to create a sample SVG file that works with the current parser (and even stays in the range of 'int'), but still fails to render in the current release of WebKit under Chrome (see attached).
Created attachment 53974 [details] SVG of tan() function, int's only Sorry, here's the attachment.
Simon, can you recomment given you had concerns about the patch? Very busy atm, but we should work towards resolving the issues :-)
Comment on attachment 51467 [details] Patch to use floating point in SVG number parser Rejecting patch 51467 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12715 test cases. svg/custom/mask-excessive-malloc.svg -> failed Exiting early after 1 failures. 10198 tests run. 245.44s total testing time 10197 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 10 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/1821054
Davi, can you have a look at the patch? It fails with commit queue.
(In reply to comment #20) > Davi, can you have a look at the patch? It fails with commit queue. I've taken over this bug from David. I'm looking at the failed test case now to see where the problem lies.
Created attachment 55237 [details] Revised patch to fix SVG number parsing, large number handling, fix 2 layout test outputs Here is a revised patch for discussion. I apologize for the length of this post, but there are several inter-related issues to be considered. I would appreciate feedback on whether the proposed patch is reasonable (at least in the short term), and thoughts on the larger issue of unsafe float-to-int conversions elsewhere in the code base. *** Issue: The previous patch failed a layout test (svg/custom/mask-excessive-malloc.svg) *** Summary of Discussion points: 1) The layout tests svg/custom/mask-excessive-malloc.svg svg/custom/pattern-excessive-malloc.svg both appear to have errors in their expected output files. 2) The revised patch handles this test now (with the corrected layout test). 3) The original patch would also have failed svg/custom/pattern-excessive-malloc.svg. The revised patch passes the corrected version of this layout test. 4) Similar float-to-int conversion errors exist elsewhere in WebKit. *** Detailed Discussion Points: 1) It appears two of the svg layout tests have errors in their expected outputs. platform/mac/svg/custom/mask-excessive-malloc-expected.txt show a RenderPath object (child of RenderSVGResourceMasker) with a size of 0x0. I believe this should actually be 800x600 (the cause for this will be discussed in (2)). Note that in the similar case of pattern-excessive-malloc.svg, the RenderPath size is 800x600. platform/mac/svg/custom/pattern-excessive-malloc-expected.txt shows multiple point coordinates of 1215752192.00 when the correct value should be 99999997952.00 . In the original file (pattern-excessive-malloc.svg) the coordinate value is 100000000000. This value can be exactly represented by a double, but the closest representation as a float is 99999997952. The value 1215752192 arises when we truncate the leading bytes of the integer representation. The proposed patch includes corrections for both expected output files. 2) The reason the original patch failed is that, although 2147483647 cannot be represented exactly by float, the closest representable value of 2147483648 will not be obtained when building digit-by-digit using float. This proposed patch uses double rather than float for building the representation, which is then cast to float. While similar problems could also occur with double, fixing that would require a complete overhaul of the number parser. 3) The original patch also failed on pattern-excessive-malloc.svg, in that its render tree dump shows a RenderPath with size 0x0 (not unlike the expected output file for mask-excessive-malloc.txt). This occurs due to an incorrect conversion from int to float in FloatRect::enclosingIntRect(). The problem arises when doing float Xf = <value larger than 2147483647> int Xi = static_cast<int>(Xf); // now Xi = -2147483648 When the negative value is returned, later processing detects a rect with negative height & width, and changes them to 0. A simple, although not entirely elegant, fix is to merely check to see if the value is above 2147483647.0f and if it is, return 2147483647 (thus clipping the value to the max representable by an int). Similarly for values below -2147483647. 4) While hunting down this bug, I noticed at least two other places where unsafe float-to-int conversions where taking place. The proposed patch does not address these, although they should be fixed. It seems likely that a concerted effort would uncover more problematic conversions. The constructor IntRect::IntRect(FloatRect &) makes the same float-to-int conversion error as does FloatRect::enclosingIntRect(). In SVGRenderStyle::inflateForShadow(FloatRect &) internal computations are performed by (i) converting the FLoatRect to an IntRect, (ii) making the required changes, (iii) storing the result back into the FloatRect. This incurs the same error as mentioned above.
(In reply to comment #22) > 1) It appears two of the svg layout tests have errors in their expected > outputs. ... > The proposed patch includes corrections for both expected output files. Sounds good. > 2) The reason the original patch failed is that, although 2147483647 cannot be > represented exactly by float, the closest representable value of 2147483648 > will not be obtained when building digit-by-digit using float. > > This proposed patch uses double rather than float for building the > representation, which is then cast to float. While similar problems could also > occur with double, fixing that would require a complete overhaul of the number > parser. Looks reasonable. > 3) The original patch also failed on pattern-excessive-malloc.svg, in that its > render tree dump shows a RenderPath with size 0x0 (not unlike the expected > output file for mask-excessive-malloc.txt). This occurs due to an incorrect > conversion from int to float in FloatRect::enclosingIntRect(). The problem > arises when doing > > float Xf = <value larger than 2147483647> > int Xi = static_cast<int>(Xf); // now Xi = -2147483648 > > When the negative value is returned, later processing detects a rect with > negative height & width, and changes them to 0. > > A simple, although not entirely elegant, fix is to merely check to see if the > value is above 2147483647.0f and if it is, return 2147483647 (thus clipping the > value to the max representable by an int). Similarly for values below > -2147483647. Please file a new bug on this issue. > 4) While hunting down this bug, I noticed at least two other places where > unsafe float-to-int conversions where taking place. The proposed patch does not > address these, although they should be fixed. It seems likely that a concerted > effort would uncover more problematic conversions. > > The constructor IntRect::IntRect(FloatRect &) makes the same float-to-int > conversion error as does FloatRect::enclosingIntRect(). > > In SVGRenderStyle::inflateForShadow(FloatRect &) internal computations are > performed by (i) converting the FLoatRect to an IntRect, (ii) making the > required changes, (iii) storing the result back into the FloatRect. This incurs > the same error as mentioned above. Please file another new bug on these instances.
(In reply to comment #23) > (In reply to comment #22) > > 2) The reason the original patch failed is that, although 2147483647 cannot be > > represented exactly by float, the closest representable value of 2147483648 > > will not be obtained when building digit-by-digit using float. > > > > This proposed patch uses double rather than float for building the > > representation, which is then cast to float. While similar problems could also > > occur with double, fixing that would require a complete overhaul of the number > > parser. > > Looks reasonable. The same issue can exist with numbers at the extreme range of what double can represent, and in the case of double using a larger floating point number is not practical. One way to fix this is to build digit by digit in the opposite direction after counting digits. This is what the constant mantissaOverflowLowerBound and parseIntOverflow in JSGlobalObjectFunctions.h are written to deal with. We might want to take a similar approach for float.
(In reply to comment #24) > > The same issue can exist with numbers at the extreme range of what double can > represent, and in the case of double using a larger floating point number is > not practical. One way to fix this is to build digit by digit in the opposite > direction after counting digits. This is what the constant > mantissaOverflowLowerBound and parseIntOverflow in JSGlobalObjectFunctions.h > are written to deal with. We might want to take a similar approach for float. I agree that a right-to-left parsing of the number is appropriate. It's also more effort, so the questions in my mind are: 1) Should we implement a double-based short-term fix before doing a full-blown parser (based on the notion that we will do the full-blown parser, but it will take longer)? and 2) In the full blown parser, what rules are appropriate for those numbers that cannot be represented (too large, too much precision, etc.)? I would think any compiler has standard code to solve this type of problem.
(In reply to comment #23) > > A simple, although not entirely elegant, fix is to merely check to see if the > > value is above 2147483647.0f and if it is, return 2147483647 (thus clipping the > > value to the max representable by an int). Similarly for values below > > -2147483647. > > Please file a new bug on this issue. If we file this as a separate bug, I presume we would then resolve the new bug before fixing bug 25645? The proposed fixes for 25645 won't pass the layout tests without a fix for this issue.
(In reply to comment #26) > If we file this as a separate bug, I presume we would then resolve the new bug > before fixing bug 25645? It's fine for it to be "Depends on" this bug. That's what those fields are for.
(In reply to comment #24) > > The same issue can exist with numbers at the extreme range of what double can represent, and in the case of double using a larger floating point number is not practical. One way to fix this is to build digit by digit in the opposite direction after counting digits. This is what the constant mantissaOverflowLowerBound and parseIntOverflow in JSGlobalObjectFunctions.h are written to deal with. We might want to take a similar approach for float. I'm happy enough to implement code for SVGParserUtilities that is equivalent to what's in JSGlobalObjectFunctions if that's what you and Simon feel is best. Just let me know if this is what you prefer. I'm curious ... what would happen if 'Inf'/'-Inf' float values started propagating into WebKit?
Comment on attachment 51467 [details] Patch to use floating point in SVG number parser Clearing Simon Fraser's R+ from this patch because it appears to cause a test failure. Please upload a single patch that includes all the changes that need to landed at once so that we don't break anything.
Created attachment 57913 [details] Revised patch to fix SVG number parsing of large numbers Revised patch. This patch updates two expected results in the layout tests. I have modified the number parsing to parse the integer portion from right-to-left, allowing an as-accurate-as-possible representation using FloatType instead of double. This should pass the layout tests for Mac. The Windows/Linux chromium layout tests may break until an upstream Skia patch (changelist submitted) is committed.
Hi W. James, can you mark your patch for review and set the "Patch" flag, otherwhise I can't officially review it. Please obsolete the old patches, it's hard to find out which one is valid :-) Cheers, Niko
Attachment 57913 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/svg/SVGParserUtilities.cpp:83: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] WebCore/svg/SVGParserUtilities.cpp:85: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] WebCore/svg/SVGParserUtilities.cpp:88: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] WebCore/svg/SVGParserUtilities.cpp:89: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] WebCore/svg/SVGParserUtilities.cpp:92: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] WebCore/platform/graphics/FloatRect.cpp:116: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] WebCore/platform/graphics/FloatRect.cpp:118: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] WebCore/platform/graphics/FloatRect.cpp:126: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] WebCore/platform/graphics/FloatRect.cpp:127: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Total errors found: 9 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Done ... I've asked David to mark his patch as obsolete. Cheers, James (In reply to comment #31) > Hi W. James, > > can you mark your patch for review and set the "Patch" flag, otherwhise I can't officially review it. Please obsolete the old patches, it's hard to find out which one is valid :-) > > Cheers, > Niko
Comment on attachment 57913 [details] Revised patch to fix SVG number parsing of large numbers Hi W. James, almost r+. Your patch is missing a proper ChangeLog (use prepare-Changelog --bug=25645, to generate a template to be filled out). The style bot also reported issues regarding trailing spaces that need to be fixed.Some more comments: WebCore/platform/graphics/FloatRect.cpp:124 + float lf = floorf(rect.x()); Please use descriptive variable names like "left" "right" "top" "bottom" here. WebCore/platform/graphics/FloatRect.cpp:126 + float wf = ceilf(rect.right()) - lf ; Why do we need to use right - left here to figure out the width? Why is using width() not sufficient? WebCore/svg/SVGParserUtilities.cpp:83 + const UChar *ptr2 = ptr ; Wrong style: you need to use "const UChar*" (star next to type). Also "ptr2" is not a good name, please use longer descriptive names. Waiting your new patch :-) I guess you tested running all webkit tests - enclosingIntRect() is used in lots of places - and I am afraid of side-effects in tests. Please rule that out.
(In reply to comment #34) > (From update of attachment 57913 [details]) > > WebCore/platform/graphics/FloatRect.cpp:124 > + float lf = floorf(rect.x()); > Please use descriptive variable names like "left" "right" "top" "bottom" here. I will improve these names ... I just re-used the existing names. > WebCore/platform/graphics/FloatRect.cpp:126 > + float wf = ceilf(rect.right()) - lf ; > Why do we need to use right - left here to figure out the width? Why is using width() not sufficient? > I think we need 'right - left' since we want an enclosing rect, and ceil(right) - floor(left) is not going to equal round/ceil/floor (right - left) in general (where width = right - left). > Waiting your new patch :-) > I guess you tested running all webkit tests - enclosingIntRect() is used in lots of places - and I am afraid of side-effects in tests. Please rule that out. I believe I am running the full set of tests, but will verify. Cheers, James
Created attachment 58066 [details] Revised (v3) patch to fix SVG number parsing of large numbers Revised as per Nikolas' suggestions, although kept "right-left" as opposed to "width" as per my comment #35. Re-added David's test cases.
Comment on attachment 58066 [details] Revised (v3) patch to fix SVG number parsing of large numbers Almost there, sorry for forcing you to another iteration: LayoutTests is also missing a ChangeLog. A last suggestion: Can you just name the variables "left" / "top" / etc. in enclosingIntRect(), we don't use type prefixes in variable names like "int iFoo" / "float fFoo" in WebKit. Can you land on your own, or do you need cq+ after I set r+ on the new patch?
Comment on attachment 58066 [details] Revised (v3) patch to fix SVG number parsing of large numbers Oops, hit Submit too fast, some more comments: Please don't change the order in the ChangeLog, the "Bug description" and bug number fields should stay in their current location. Your text needs to go below them. So: Reviewed by NOBODY (OOPS!). SVG - numeric overflow https://... Revised to improve... Another style issue (no idea why the style bot did not complain): WebCore/svg/SVGParserUtilities.cpp:83 + const UChar *ptrStartIntPart = ptr; "const UChar* ptrStartIntPart = ptr";
Comment on attachment 58066 [details] Revised (v3) patch to fix SVG number parsing of large numbers Oops, hit Submit too fast, some more comments: Please don't change the order in the ChangeLog, the "Bug description" and bug number fields should stay in their current location. Your text needs to go below them. So: Reviewed by NOBODY (OOPS!). SVG - numeric overflow https://... Revised to improve... Another style issue (no idea why the style bot did not complain): WebCore/svg/SVGParserUtilities.cpp:83 + const UChar *ptrStartIntPart = ptr; "const UChar* ptrStartIntPart = ptr"; WebCore/svg/SVGParserUtilities.cpp:88 + const UChar *ptrScanIntPart = ptr - 1; const UChar* ptrScanIntPart = ptr - 1;
Created attachment 58072 [details] Revised (v4) patch to fix SVG number parsing of large numbers - fixed var names in enclosingIntRect() - fixed 'UChar*' style issue - ChangeLog files fixed (for some reason my git commit comments are ending up ahead of bug number, etc., but I fixed this manually) "or do you need cq+ after I set r+ on the new patch?" I assume I'll need 'cq+' - I'm not sure of the exact steps to finish landing the patch ...
Comment on attachment 58072 [details] Revised (v4) patch to fix SVG number parsing of large numbers Hi W. James, almost there! Your WebCore/ChangeLog contains unrelated entries though, you need to remove them, before I can set r+ and cq+. Setting cq+ will auto-land the patch for you. I don't think you need to mention twice that you've revised the variable naming. Just make it one statement. Thanks for your patience, Niko
Created attachment 58167 [details] Revised (v5) patch to fix SVG number parsing of large numbers Fixed change log issues.
Comment on attachment 58167 [details] Revised (v5) patch to fix SVG number parsing of large numbers Looks good, r=me. Thanks for fixing this bug!
Comment on attachment 58167 [details] Revised (v5) patch to fix SVG number parsing of large numbers Rejecting patch 58167 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Last 500 characters of output: e Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found Testing 19073 test cases. svg/custom/mask-excessive-malloc.svg -> failed Exiting early after 1 failures. 16523 tests run. 316.95s total testing time 16522 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 15 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/3176104
Created attachment 58370 [details] Revised (v6) patch to fix SVG number parsing of large numbers Previous patch forgot to change two test outputs that were previously discussed (Comment #22) but I missed putting into the patch submission (sorry!)
Comment on attachment 58370 [details] Revised (v6) patch to fix SVG number parsing of large numbers Patch looks good. I made up my mind regarding safeFloatToInt, I find it rather dangerous to hardcode these values. How about using: static inline int safeFloatToInt(float x) { static const int s_intMax = std::numeric_limit<int>::max(); static const int s_intMax = std::numeric_limit<int>::min(); if (x >= float(intMax) + 1.0f) return intMax; if (x < float(intMin) return intMin; // NOTE: You've returned intMin + 1, I don't know why intMin, is not allowed. But if you see errors with intMin, just return intMin+1 return static_cast<int>(x); } Sorry for bringing up this so late :(
Created attachment 58470 [details] Revised (v7) patch to fix SVG number parsing of large numbers Agreed about using the values from numeric_limits ... I should have done this in the first place :-)
Attachment 58470 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/graphics/FloatRect.cpp:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 58472 [details] Revised (v8) patch to fix SVG number parsing of large numbers Alphabetised includes in FloatRect.cpp.
Comment on attachment 58472 [details] Revised (v8) patch to fix SVG number parsing of large numbers Hi W. James, still r-, sorry: ChangeLog order is wrong again. Your comments have to go below the bug number & description, just have a look at WebCore/ChangeLog to see how it works. Also the ChangeLog entries are confusing. You've basically added a line for every new patch you've uploaded here, instead you should only the describe the "final patch" > + Revised safeIntToFloat() to use values from <limits>. > + > + Fixed two test outputs involving large coordinate values. > + > + Two of the expected test outputs were incorrect now that parsing of large values > + is handled correctly. > + > + Revised to improve variable names in enclosingIntRect(), _parseNumber(), and fix style issues. > + > + - Revised FloatRect to remove bad float-to-int conversions in enclosingIntRect() > + - Revised _parseNumber to do right-to-left float-based parsing of input value Just use: > + Two of the expected test outputs were incorrect now that parsing of large values > + is handled correctly. > + - Revised FloatRect to remove bad float-to-int conversions in enclosingIntRect() > + - Revised _parseNumber to do right-to-left float-based parsing of input value > +static inline int safeFloatToInt(float x) > +{ > + static int intMax = std::numeric_limits<int>::max(); > + static int intMin = std::numeric_limits<int>::min(); Recently the naming of these statics has been discussed on webkit-dev: use s_intMax, s_intMin here. Other than those last two issues, it's r+ :-) You must hate me by now, sorry for the numerous of reiterations ;-)
Created attachment 58476 [details] Revised (v9) patch to fix SVG number parsing of large numbers Fixed changelogs, change static var names.
Comment on attachment 58476 [details] Revised (v9) patch to fix SVG number parsing of large numbers Looks great, r=me! Thanks for taking the time to fix it properly.
Comment on attachment 58476 [details] Revised (v9) patch to fix SVG number parsing of large numbers Rejecting patch 58476 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Last 500 characters of output: e Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found Testing 19084 test cases. svg/custom/mask-excessive-malloc.svg -> failed Exiting early after 1 failures. 16534 tests run. 335.48s total testing time 16533 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 15 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/3220222
(In reply to comment #53) > (From update of attachment 58476 [details]) > Rejecting patch 58476 from commit-queue. > > svg/custom/mask-excessive-malloc.svg -> failed > > Exiting early after 1 failures. 16534 tests run. > 335.48s total testing time Is there any way I can see the actual output that failed? I've tested this case myself, and it passed, so I need more info to see what's going on. > Full output: http://webkit-commit-queue.appspot.com/results/3220222 This link has no more info than the comment I'm replying to :-(
(In reply to comment #54) > (In reply to comment #53) > > (From update of attachment 58476 [details] [details]) > > Rejecting patch 58476 from commit-queue. > > > > svg/custom/mask-excessive-malloc.svg -> failed > > > > Exiting early after 1 failures. 16534 tests run. > > 335.48s total testing time > > Is there any way I can see the actual output that failed? I've tested this case myself, and it passed, so I need more info to see what's going on. > > > Full output: http://webkit-commit-queue.appspot.com/results/3220222 > > This link has no more info than the comment I'm replying to :-( Hm, can you find someone @ Google with access to Safari to run the test? I probably can't test it for another two days... if you still need someone, drop me a line then. Cheers, Niko
Comment on attachment 58167 [details] Revised (v5) patch to fix SVG number parsing of large numbers Cleared Nikolas Zimmermann's review+ from obsolete attachment 58167 [details] so that this bug does not appear in http://webkit.org/pending-commit.
I'd like to re-submit this patch, but I'm a bit confused about expected output files. It seems the expected output from some tests is a bit different on mac than other platforms: do I then need to provide expected output for *all* other platforms? (gtk, chromium-linux, ... there are 19 platforms other than 'mac' listed in the Layout/platform directory). What's the best way to handle this?
(In reply to comment #57) > I'd like to re-submit this patch, but I'm a bit confused about expected output files. It seems the expected output from some tests is a bit different on mac than other platforms: do I then need to provide expected output for *all* other platforms? No. You can provide cross-platform expected results, then override those with platform-specific results when necessary (e.g. in LayoutTests/platform/mac/...).
Created attachment 62859 [details] Revised (vA) patch to fix SVG number parsing of large numbers Revised to update GTK and QT expected outputs to match those for Chromium.
Created attachment 62866 [details] Revised (vB) patch to fix SVG number parsing of large numbers Previous patch missing binary info.
Attachment 62866 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/platform/graphics/FloatRect.cpp:128: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebCore/platform/graphics/FloatRect.cpp:129: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebCore/platform/graphics/FloatRect.cpp:130: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebCore/platform/graphics/FloatRect.cpp:131: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 62869 [details] Revised (vC) patch to fix SVG number parsing of large numbers Fixed spaces in FloatRect.cpp.
Comment on attachment 62869 [details] Revised (vC) patch to fix SVG number parsing of large numbers > +static inline int safeFloatToInt(float x) > +{ > + static int s_intMax = std::numeric_limits<int>::max(); > + static int s_intMin = std::numeric_limits<int>::min(); Is caching in global data really worth it? std::numeric_limits<int>::max() may just 'return MAX_INT' which is much faster than accessing global data.
(In reply to comment #63) > (From update of attachment 62869 [details]) > > +static inline int safeFloatToInt(float x) > > +{ > > + static int s_intMax = std::numeric_limits<int>::max(); > > + static int s_intMin = std::numeric_limits<int>::min(); > > Is caching in global data really worth it? std::numeric_limits<int>::max() may just 'return MAX_INT' which is much faster than accessing global data. Should just be static const int, and then it will not involve any global data at all in the compilers we use.
Created attachment 62940 [details] Revised (vD) patch to fix SVG number parsing of large numbers Made int vars 'const' in safeFloatToInt().
Comment on attachment 62940 [details] Revised (vD) patch to fix SVG number parsing of large numbers They are still static, so I think this is still a global memory access. Maybe you can disassemble to find out.
(In reply to comment #66) > They are still static, so I think this is still a global memory access. Maybe you can disassemble to find out. I can disassemble and did, and it is not a global memory access.
Comment on attachment 62940 [details] Revised (vD) patch to fix SVG number parsing of large numbers r-, ChangeLogs are missing. And some additional issues: WebCore/svg/SVGParserUtilities.cpp:70 + ++ptr; // Advance to first non-digit. Four-spaces indention please. WebCore/svg/SVGParserUtilities.cpp:73 + const UChar* ptrScanIntPart = ptr - 1; Ditto. WebCore/svg/SVGParserUtilities.cpp:76 + integer = integer + multiplier * static_cast<FloatType>(*(ptrScanIntPart--) - '0'); Ditto. And you can use integer += multipler * ... here.
Created attachment 63063 [details] Revised (vE) patch to fix SVG number parsing of large numbers Fixed spacing, re-added CL entries
Comment on attachment 63063 [details] Revised (vE) patch to fix SVG number parsing of large numbers Looks good to me! Please watch the bots if anything breaks, maybe some other platforms need specific results.
Comment on attachment 63063 [details] Revised (vE) patch to fix SVG number parsing of large numbers Rejecting patch 63063 from commit-queue. Unexpected failure when processing patch! Please file a bug against webkit-patch. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 63063, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1 Logging in as eseidel@chromium.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=63063&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=25645&ctype=xml Processing 1 patch from 1 bug. Cleaning working directory Processing patch 63063 from bug 25645. ERROR: /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Created attachment 63096 [details] Revised (vF) patch to fix SVG number parsing of large numbers Added reviewer line to LayoutTests/Changelog (sigh).
Comment on attachment 63096 [details] Revised (vF) patch to fix SVG number parsing of large numbers 2nd try :-)
Comment on attachment 63096 [details] Revised (vF) patch to fix SVG number parsing of large numbers Clearing flags on attachment: 63096 Committed r64379: <http://trac.webkit.org/changeset/64379>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/64379 might have broken GTK Linux 32-bit Release, GTK Linux 64-bit Debug, and Qt Linux Release
(In reply to comment #70) > (From update of attachment 63063 [details]) > Looks good to me! Please watch the bots if anything breaks, maybe some other platforms need specific results. James, please have a look at: http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r64408%20(8613)/svg/custom/massive-coordinates-pretty-diff.html http://build.webkit.org/results/Qt%20Linux%20Release/r64410%20(16443)/svg/custom/massive-coordinates-pretty-diff.html Both Qt/Gtk have problems, bots are red.
(In reply to comment #77) > (In reply to comment #70) > > (From update of attachment 63063 [details] [details]) > > Looks good to me! Please watch the bots if anything breaks, maybe some other platforms need specific results. > > James, please have a look at: > http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r64408%20(8613)/svg/custom/massive-coordinates-pretty-diff.html > http://build.webkit.org/results/Qt%20Linux%20Release/r64410%20(16443)/svg/custom/massive-coordinates-pretty-diff.html > > Both Qt/Gtk have problems, bots are red. Same with the windows bot: http://build.webkit.org/results/Windows%20Release%20(Tests)/r64419%20(2158)/svg/custom/massive-coordinates-pretty-diff.html Please look into the problem ASAP.
This looks to be just a re-baselining issue for Qt/GTK/Windows ... I'll re-submit with updated -expected.txt files for this test on Tuesday morning when I'm back in the office. I'm assuming these are the only broken tests? (In reply to comment #78) > (In reply to comment #77) > > (In reply to comment #70) > > > (From update of attachment 63063 [details] [details] [details]) > > > Looks good to me! Please watch the bots if anything breaks, maybe some other platforms need specific results. > > > > James, please have a look at: > > http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r64408%20(8613)/svg/custom/massive-coordinates-pretty-diff.html > > http://build.webkit.org/results/Qt%20Linux%20Release/r64410%20(16443)/svg/custom/massive-coordinates-pretty-diff.html > > > > Both Qt/Gtk have problems, bots are red. > > Same with the windows bot: > http://build.webkit.org/results/Windows%20Release%20(Tests)/r64419%20(2158)/svg/custom/massive-coordinates-pretty-diff.html > > Please look into the problem ASAP.
I think it would be better to find a way to make the test insensitive to platform rounding/precision issues. Printing a float with lots of significant digits in tests in never a good idea.
(In reply to comment #80) > I think it would be better to find a way to make the test insensitive to platform rounding/precision issues. Printing a float with lots of significant digits in tests in never a good idea. Brings us to platform independent LayoutTests.. Something we could do, once SVGPath uses SVGPathByteStream.
I'm happy enough to revise the tests, but I'm not sure how we would go about making it insensitive to rounding/precision errors. Isn't that sort of dictated by using DumpRenderTree? If there's a way to devise a different style of test that doesn't use DumpRenderTree and someone can aim me at an example, I can look after it. Otherwise, I'll proceed with re-baselining the tests according to the output from the Qt/GTK/Win bots. (In reply to comment #80) > I think it would be better to find a way to make the test insensitive to platform rounding/precision issues. Printing a float with lots of significant digits in tests in never a good idea.
Created attachment 63464 [details] Re-baseline massive-coordinates.svg test expectations for platform/win. This patch re-baselines the expected output for LayoutTests/svg/custom/massive-coordinates.svg for platform/win. The change appears to just be a numeric precision/round-off issue. This patch also removes the expected output for LayoutTests/svg/custom/pattern-excessive-malloc.svg for the GTK platform, as the current expected output is *very broken*. This test, along with that for LayoutTests/svg/custom/massive-coordinates.svg needs to be re-baselined for GTK, but I think it is likely better not to test against a known bad -expected.txt file in the meantime. Since all other platforms seem to be in close agreement, I suspect this is a debugging-output issue in GTK. Both tests expectations for Qt were re-baselined in https://bugs.webkit.org/show_bug.cgi?id=43361 (thanks Martin Robinson!).
Created attachment 63584 [details] Re-baseline massive-coordinates.svg test expectations for platform/win, skip on GTK. Revised to skip pattern-excessive-malloc.svg test on GTK.
Comment on attachment 63584 [details] Re-baseline massive-coordinates.svg test expectations for platform/win, skip on GTK. r-, you need to modify platform/gtk/Skipped, not test_expectations.txt
Created attachment 63587 [details] Re-baseline massive-coordinates.svg test expectations for platform/win, skip on GTK. Moved 'skip' request to platform/gtk/Skipped.
Comment on attachment 63587 [details] Re-baseline massive-coordinates.svg test expectations for platform/win, skip on GTK. r=me.
Comment on attachment 58476 [details] Revised (v9) patch to fix SVG number parsing of large numbers Cleared Nikolas Zimmermann's review+ from obsolete attachment 58476 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 63063 [details] Revised (vE) patch to fix SVG number parsing of large numbers Cleared Nikolas Zimmermann's review+ from obsolete attachment 63063 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 63587 [details] Re-baseline massive-coordinates.svg test expectations for platform/win, skip on GTK. Clearing flags on attachment: 63587 Committed r64815: <http://trac.webkit.org/changeset/64815>
<rdar://problem/8777860>
This was assigned CVE-2011-0147.