Bug 56367 – REGRESSION(r80096): Number type input unexpectedly rounds fractional values

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 85786 [details]
Patch

[Collabora GTK+ EWS bot]

Attachment 85786 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8175537

[Early Warning System Bot]

Attachment 85786 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8179460

[Kent Tamura]

Created attachment 85788 [details]
Patch 2

[Build Bot]

Attachment 85786 [details] did not build on win:
Build output: http://queues.webkit.org/results/8178487

[Eric Seidel (no email)]

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?

[Kent Tamura]

Created attachment 86412 [details]
Patch 3

[Kent Tamura]

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().

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Should we just add a toInt32() function to MathExtras.h which does this cast and calls CRASH() if it fails?

[Eric Seidel (no email)]

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.

[Kent Tamura]

(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.

[Kent Tamura]

Created attachment 86425 [details]
Patch 4

[Eric Seidel (no email)]

Comment on attachment 86425 [details]
Patch 4

This feels much nicer.  Thank you.

[Kent Tamura]

Comment on attachment 86425 [details]
Patch 4

Clearing flags on attachment: 86425

Committed r81649: <http://trac.webkit.org/changeset/81649>

[Kent Tamura]

All reviewed patches have been landed.  Closing bug.