Bug 20674 - Very large and small numbers fail to round-trip through CSS
: Very large and small numbers fail to round-trip through CSS
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To: Simon Fraser (smfr)
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-05 14:54 PDT by Simon Fraser (smfr)
Modified: 2010-10-18 14:48 PDT (History)
11 users (show)

See Also:


Attachments
Testcase (1.14 KB, text/html)
2008-09-05 14:55 PDT, Simon Fraser (smfr)
no flags Details
smfr's layout test with a failing baseline (2.72 KB, patch)
2009-10-06 19:33 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
smfr's layout test with a failing baseline (2.73 KB, patch)
2009-10-06 19:34 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
smfr's layout test with a failing baseline (2.79 KB, patch)
2009-10-07 10:49 PDT, Evan Martin
no flags Details | Formatted Diff | Diff
Patch (20.15 KB, patch)
2010-10-16 13:44 PDT, Simon Fraser (smfr)
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2008-09-05 14:54:38 PDT
When converting very large or small numbers back to strings, exponential notation is used, so a value like 90010000px is returned from getComputedStyle() as 9.001e+07px.

9.001e+07px does not parse, so such a value cannot be round-tripped.

This is a particular problem with tranformation matrices, which can often contain very large or small numbers.
Comment 1 Simon Fraser (smfr) 2008-09-05 14:55:15 PDT
Created attachment 23202 [details]
Testcase
Comment 2 Simon Fraser (smfr) 2008-09-08 14:21:26 PDT
See thread starting here:
<http://lists.w3.org/Archives/Public/www-style/2008Sep/0049.html>
Comment 3 Simon Fraser (smfr) 2009-01-16 10:50:36 PST
Parsing of pixel values like "-131.33376515546504px" seems broken too.
Comment 4 Evan Martin 2009-10-06 17:51:31 PDT
In fixing bug 18994 (https://bugs.webkit.org/show_bug.cgi?id=18994) I'm looking at reimplementing the way we generate floats in CSS.  My fix for that bug would fix this as well.
Comment 5 Evan Martin 2009-10-06 17:52:20 PDT
Can you check in your test?  Or would you mind if I checked it in with appropriate baselines?
Comment 6 Simon Fraser (smfr) 2009-10-06 19:23:26 PDT
Please go ahead and check it in. We should also add some tests which read/write style.webkitTransform, since this often contains floating point numbers with lots of digits.
Comment 7 Evan Martin 2009-10-06 19:33:10 PDT
Created attachment 40757 [details]
smfr's layout test with a failing baseline
Comment 8 Evan Martin 2009-10-06 19:34:02 PDT
Created attachment 40758 [details]
smfr's layout test with a failing baseline
Comment 9 WebKit Commit Bot 2009-10-06 20:47:16 PDT
Comment on attachment 40758 [details]
smfr's layout test with a failing baseline

Rejecting patch 40758 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
ayoutTests/fast/css/large-number-round-trip.html
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following files contain tab characters:

        trunk/LayoutTests/ChangeLog

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/libexec/git-core//git-svn line 469
Comment 10 Evan Martin 2009-10-07 10:49:04 PDT
Created attachment 40804 [details]
smfr's layout test with a failing baseline
Comment 11 Evan Martin 2009-10-07 10:49:26 PDT
Comment on attachment 40804 [details]
smfr's layout test with a failing baseline

now without tabs.

FFFFFUUUUUUUUU
Comment 12 WebKit Commit Bot 2009-10-07 11:10:33 PDT
Comment on attachment 40804 [details]
smfr's layout test with a failing baseline

Clearing flags on attachment: 40804

Committed r49254: <http://trac.webkit.org/changeset/49254>
Comment 13 WebKit Commit Bot 2009-10-07 11:10:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Evan Martin 2009-10-07 11:14:02 PDT
Patch was only a test; not fixed yet.
Comment 15 Evan Martin 2009-10-14 14:55:16 PDT
fixed in
http://trac.webkit.org/changeset/49585
darin++
Comment 16 Simon Fraser (smfr) 2009-12-13 16:10:50 PST
Reopening; the change was reverted. See bug 18994.
Comment 17 Simon Fraser (smfr) 2010-10-16 13:41:01 PDT
I plan to fix CSSPrimitiveValue.cssText() via bug 20674.
Comment 18 Simon Fraser (smfr) 2010-10-16 13:44:14 PDT
Created attachment 70962 [details]
Patch
Comment 19 Nikolas Zimmermann 2010-10-17 02:21:58 PDT
Comment on attachment 70962 [details]
Patch

This is correct, and I'm tempted to r+. Though I was planning to solve this in a more generic fashion, by moving this code (formatNumber) into WTFString, as default String::number() implementation for double.
We can still move this to String::number() though as a later point, it's good to see less String::format() usage anyway.
If you prefer to land it as is, I'll r+ it later - waiting for your comment first.
Comment 20 Simon Fraser (smfr) 2010-10-17 09:42:15 PDT
I'd like to get this in, and we can do string cleanup later.
Comment 21 Nikolas Zimmermann 2010-10-17 12:39:37 PDT
Comment on attachment 70962 [details]
Patch

Looks great, r=me. I'll take your formatNumber code as starting point for rewriting String::number(double).
Comment 22 Simon Fraser (smfr) 2010-10-17 13:06:48 PDT
http://trac.webkit.org/changeset/69928
Comment 23 Simon Fraser (smfr) 2010-10-18 14:48:38 PDT
There's a possible buffer overflow here: bug 47851.