| Summary: | ASSERTION FAILED: std::isfinite(num) in WebCore::CSSPrimitiveValue::CSSPrimitiveValue | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Renata Hodovan <rhodovan.u-szeged> | ||||||||||||||||||||||||||||||
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | benjamin, buildbot, commit-queue, darin, esprehn+autocc, glenn, gyuyoung.kim, kling, koivisto, macpherson, menard, mhodovan.u-szeged, rniwa, zoltan | ||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||||||
| OS: | Linux | ||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||
|
Description
Renata Hodovan
2014-01-21 11:49:48 PST
Created attachment 221772 [details]
Proposed patch
I am not completely sure that the value check is at the right place, so if anyone has a better idea please let me know.
Comment on attachment 221772 [details]
Proposed patch
I’m not sure this is the right fix and a helpful change. Turning the colossally large number into 0 just to avoid the assertion doesn’t necessarily help improve the behavior of the engine, other than sidestepping an assertion that may not be correct. It might be more appropriate to turn such values into parse failures. Not sure it matters either way. Loosening the assertion also might be sufficient. I especially regret adding more parsing special cases here since it might prevent us from doing clean-up refactoring in the future and even has a small performance cost.
Created attachment 223339 [details]
Proposed patch
The previous patch got updated according to Darin's review. (parsing failure) (In reply to comment #4) > The previous patch got updated according to Darin's review. (parsing failure) The patch you uploaded contains 2 changelog modifications. Please pay attention to upload the diff to the original repository. Created attachment 223372 [details]
Proposed patch
Sorry about this mistake, I attached the patch again.
Comment on attachment 223372 [details] Proposed patch Attachment 223372 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4653103882174464 New failing tests: fast/css/parsing-infinite-floating-value.html Created attachment 223375 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 223372 [details] Proposed patch Attachment 223372 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5610099905134592 New failing tests: fast/css/parsing-infinite-floating-value.html Created attachment 223381 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 223372 [details] Proposed patch Attachment 223372 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6622795748343808 New failing tests: fast/css/parsing-infinite-floating-value.html Created attachment 223392 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 223372 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=223372&action=review Looks good but. The test fails. > LayoutTests/fast/css/parsing-infinite-floating-value.html:16 > + <head> > + <script> > + if (window.testRunner) > + testRunner.dumpAsText(); > + </script> > + </head> > + > + <body> > + <p>This test passes if it does not crash.</p> > + > + <svg xmlns="http://www.w3.org/2000/svg" > > + <polyline font-size="8E+617%"></polyline> > + </svg> > + > + </body> Let's fix the indent here. Can you please extend the test to non SVG values? It would be useful to test some HTML. I have run the following test on Chrome, Firefox, Opera and all of them produced the same output: "aaa" remains small, "bbb" grows big. For some reason, normalized scientific notation can not be used to represent font-size values outside of SVG blocks. (Not sure if it is a bug or not.)
<html>
<body>
<div style="font-size:8E+2%">aaa</div>
<svg xmlns="http://www.w3.org/2000/svg">
<text y="150" font-size="8E+2%">bbb</text>
</svg>
</body>
</html>
I could only extend the previous test to non SVG values by using standard decimal notation.
<html>
<body>
<div style="font-size: 900000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000%;">aaa </div>
<svg xmlns="http://www.w3.org/2000/svg">
<text y="150" font-size="8E+2%">bbb</text>
</svg>
</body>
</html>
Is the test OK like this? (In reply to comment #15) > Is the test OK like this? Yep, that looks ok. :) Created attachment 224422 [details]
Proposed patch
There was an extra new line character at the end of the expected file, that's why the test failed on Mac. I also fixed the indentation.
Comment on attachment 224422 [details] Proposed patch Attachment 224422 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5792670509170688 New failing tests: fast/css/parsing-infinite-floating-value.html Created attachment 224429 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 224422 [details] Proposed patch Attachment 224422 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5247733648588800 New failing tests: fast/css/parsing-infinite-floating-value.html Created attachment 224436 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 224422 [details] Proposed patch Attachment 224422 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5057601754628096 New failing tests: fast/css/parsing-infinite-floating-value.html Created attachment 224440 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 224638 [details]
Proposed patch
Created attachment 225072 [details]
Proposed patch
I have relocated the infinity check to CSSValuePool::createValue(), I think this is the function where it really belongs to. This way we dont have to handle the SVG and non-SVG cases separately.
Could someone take a look at this? Comment on attachment 225072 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=225072&action=review > Source/WebCore/css/CSSValuePool.cpp:93 > + if (!std::isfinite(value)) > + return createIdentifierValue(CSSValueID::CSSValueInfinite); It’s fine to change infinity to infinite, but not good to change NaN to infinite. I suggest using std::isinf here instead of std::isfinite. Also, is it OK to change negative infinity to positive infinity? Comment on attachment 225072 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=225072&action=review >> Source/WebCore/css/CSSValuePool.cpp:93 >> + return createIdentifierValue(CSSValueID::CSSValueInfinite); > > It’s fine to change infinity to infinite, but not good to change NaN to infinite. I suggest using std::isinf here instead of std::isfinite. Also, is it OK to change negative infinity to positive infinity? Testing ought to cover more than just parsing. Which properties support "infinite" as a value? Is it correct for these large numbers to act the same as "infinite" for those? What about properties that don’t support "infinite"? Is it OK for the large number to act as if it was an illegal value? I believe that no infinite values should be supported in CSS at all. Properties that are expecting numeric values (most of them are listed in CSSParser::isSimpleLengthPropertyID like FontSize, (Min/Max of) Height/Width, Bottom/Left/Right/Top-Margin/Padding, etc.) do not make any sense in infinite cases, regardless it is a positive or negative value, and since there is no sensible way to present them, I think we should just ignore them all. Maybe I should use CSSValueID::CSSValueInvalid instead of CSSValueID::CSSValueInfinite. (?) The standard only says "UAs should support reasonably useful ranges and precisions." I am a little confused, how do you mean that testing ought to cover more than just parsing? I was using the parsed values as font-size and line-height property values. Am I wrong about it? (In reply to comment #29) > I believe that no infinite values should be supported in CSS at all. Makes sense to me. > Maybe I should use CSSValueID::CSSValueInvalid instead of CSSValueID::CSSValueInfinite. Why do we even have CSSValueInfinite? Created attachment 227043 [details]
Proposed patch
(In reply to comment #31) I replaced !std::isfinite() with std::isinf() and CSSValueID::CSSValueInfinite with CSSValueID::CSSValueInvalid. I also changed the name of the test case, because it really was a little misleading (this patch is not parsing-related). > Why do we even have CSSValueInfinite? CSSValueInfinite is used by the -webkit-marquee-repetition property, when we want to set the number of repeats to infinite explicitly, like "-webkit-marquee-repetition: infinite;". Comment on attachment 227043 [details] Proposed patch Rejecting attachment 227043 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 227043, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.appspot.com/results/6731491404939264 Comment on attachment 227043 [details] Proposed patch Clearing flags on attachment: 227043 Committed r166114: <http://trac.webkit.org/changeset/166114> All reviewed patches have been landed. Closing bug. |