Bug 56367

Summary: REGRESSION(r80096): Number type input unexpectedly rounds fractional values
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, hamaji, inferno, jschuh
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
URL: data:text/html,<input type=number value=0.55555>
Attachments:
Description Flags
Patch
none
Patch 2
none
Patch 3
none
Patch 4 none

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.