Bug 56367 - REGRESSION(r80096): Number type input unexpectedly rounds fractional values
Summary: REGRESSION(r80096): Number type input unexpectedly rounds fractional values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P1 Normal
Assignee: Kent Tamura
URL: data:text/html,<input type=number val...
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-15 01:48 PDT by Kent Tamura
Modified: 2011-03-22 00:43 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.37 KB, patch)
2011-03-15 01:54 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (7.01 KB, patch)
2011-03-15 02:12 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (7.01 KB, patch)
2011-03-21 20:22 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 4 (8.51 KB, patch)
2011-03-21 23:33 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2011-03-15 01:48:08 PDT
http://code.google.com/p/chromium/issues/detail?id=76134

Because the default value of the maximum fractional digits of NSNumberFormatter and ICU NumberFormat is 3, the value 0.55555 is rounded to 0.556 in a localized representation.
Comment 1 Kent Tamura 2011-03-15 01:54:42 PDT
Created attachment 85786 [details]
Patch
Comment 2 Collabora GTK+ EWS bot 2011-03-15 02:03:43 PDT
Attachment 85786 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8175537
Comment 3 Early Warning System Bot 2011-03-15 02:05:16 PDT
Attachment 85786 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8179460
Comment 4 Kent Tamura 2011-03-15 02:12:53 PDT
Created attachment 85788 [details]
Patch 2
Comment 5 Build Bot 2011-03-15 02:15:19 PDT
Attachment 85786 [details] did not build on win:
Build output: http://queues.webkit.org/results/8178487
Comment 6 Eric Seidel (no email) 2011-03-18 15:02:22 PDT
Comment on attachment 85788 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=85788&action=review

> Source/WebCore/platform/text/LocalizedNumberICU.cpp:83
> +    int32_t digits = static_cast<int32_t>(fractionDigits);
> +    ASSERT(digits >= 0);

Is overflow OK in release builds?  Do we need to early-return here or use min() to make sure it's >= 0?
Comment 7 Kent Tamura 2011-03-21 20:22:59 PDT
Created attachment 86412 [details]
Patch 3
Comment 8 Kent Tamura 2011-03-21 20:25:18 PDT
Comment on attachment 85788 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=85788&action=review

Thank you for reviewing.

>> Source/WebCore/platform/text/LocalizedNumberICU.cpp:83
>> +    ASSERT(digits >= 0);
> 
> Is overflow OK in release builds?  Do we need to early-return here or use min() to make sure it's >= 0?

It's not ok and the 'digits' value is clipped at the next line.
I have updated the patch so that it uses std::min().
Comment 9 Eric Seidel (no email) 2011-03-21 22:30:46 PDT
Comment on attachment 86412 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=86412&action=review

> Source/WebCore/platform/text/LocalizedNumberICU.cpp:84
> +    int32_t digits = static_cast<int32_t>(fractionDigits);
> +    ASSERT(digits >= 0);
> +    formatter->setMaximumFractionDigits(max(digits, 0));

It seems we should have some sort of fractionDigits < INT_MAX ASSERT at the start of all of these implementations, instead of this one here.

Then this could be just:

statit_cast<int_32t(fractionDigits) or:

int32_t digits =max( static_cast<int32_t>(fractionDigits), 0); if you really want.  As written it seems like only the ICU port cares that this is < INT_MAX which seems wrong (or at least error-prone for ICU vs. non ICU ports.

Can we write a test which uses INT_MAX + 1 fraction digits?  Seems such could be generated from JS... although it would require a 2gb allocation... which seems like it should fail before we get here.
Comment 10 Eric Seidel (no email) 2011-03-21 22:31:28 PDT
Should we just add a toInt32() function to MathExtras.h which does this cast and calls CRASH() if it fails?
Comment 11 Eric Seidel (no email) 2011-03-21 22:32:21 PDT
It seems it would be useful to have a safe, overflow-proof, casting convention to use througout webkit.  Maybe the security gents have a suggestion.
Comment 12 Kent Tamura 2011-03-21 23:29:59 PDT
(In reply to comment #9)
> (From update of attachment 86412 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86412&action=review
> 
> > Source/WebCore/platform/text/LocalizedNumberICU.cpp:84
> > +    int32_t digits = static_cast<int32_t>(fractionDigits);
> > +    ASSERT(digits >= 0);
> > +    formatter->setMaximumFractionDigits(max(digits, 0));
> 
> It seems we should have some sort of fractionDigits < INT_MAX ASSERT at the start of all of these implementations, instead of this one here.
> 
> Then this could be just:
> 
> statit_cast<int_32t(fractionDigits) or:
> 
> int32_t digits =max( static_cast<int32_t>(fractionDigits), 0); if you really want.  As written it seems like only the ICU port cares that this is < INT_MAX which seems wrong (or at least error-prone for ICU vs. non ICU ports.
> 
> Can we write a test which uses INT_MAX + 1 fraction digits?  Seems such could be generated from JS... although it would require a 2gb allocation... which seems like it should fail before we get here.

We need to set 2G+ string to HTMLInputElement::value to test this code.  I'm afraid it won't work on 32 bit builds.

I reconsider what we should do for fractionalDigits>INT_MAX, and my conclusion is that we had better pass INT_MAX to setMaximumFractionDigits() rather than 0 and we don't need ASSERT().
I'll update the patch.
Comment 13 Kent Tamura 2011-03-21 23:33:34 PDT
Created attachment 86425 [details]
Patch 4
Comment 14 Eric Seidel (no email) 2011-03-22 00:39:33 PDT
Comment on attachment 86425 [details]
Patch 4

This feels much nicer.  Thank you.
Comment 15 Kent Tamura 2011-03-22 00:43:32 PDT
Comment on attachment 86425 [details]
Patch 4

Clearing flags on attachment: 86425

Committed r81649: <http://trac.webkit.org/changeset/81649>
Comment 16 Kent Tamura 2011-03-22 00:43:40 PDT
All reviewed patches have been landed.  Closing bug.