513039 - Number.toFixed(0) returns incorrect numbers, rounding issues

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Brent Baker]

Attachment: test

I noticed that there are other Number.toFixed() bug reports. Before marking this as a duplicate. Please read this carefully. I'm not sure the other reports are reporting the same thing. This problem I found is very simple. Bascially, if you call Number.tofFixed(0) with a zero at the precision, it will return the wrong VALUE (I don't care about the format), for certain numbers.

I wrote a test program to demonstrate this. Specifically, please observe the return values for numbers like 0.05, 0.06, 0.005, 0.006. These are returning "1" when they should be returning "0". This is bad. It seems as though any number between -.5 and .5 having a 5,6,7,8,9 in it as it's first significant digit will return 1. It's as if it's ignoring the place of the number.

Here is the output from my program.
v=-0.1 v.toFixed(0)=-0.
v=-0.09 v.toFixed(0)=-1.
v=-0.08 v.toFixed(0)=-1.
v=-0.07 v.toFixed(0)=-1.
v=-0.06 v.toFixed(0)=-1.
v=-0.05 v.toFixed(0)=-1.
v=-0.04 v.toFixed(0)=-0.
v=-0.03 v.toFixed(0)=-0.
v=-0.02 v.toFixed(0)=-0.
v=-0.01 v.toFixed(0)=-0.
v=-0.009 v.toFixed(0)=-1.
v=-0.008 v.toFixed(0)=-1.
v=-0.007 v.toFixed(0)=-1.
v=-0.006 v.toFixed(0)=-1.
v=-0.005 v.toFixed(0)=-1.
v=-0.004 v.toFixed(0)=-0.
v=0 v.toFixed(0)=0.
v=0.004 v.toFixed(0)=0.
v=0.005 v.toFixed(0)=1.
v=0.006 v.toFixed(0)=1.
v=0.007 v.toFixed(0)=1.
v=0.008 v.toFixed(0)=1.
v=0.009 v.toFixed(0)=1.
v=0.01 v.toFixed(0)=0.
v=0.02 v.toFixed(0)=0.
v=0.03 v.toFixed(0)=0.
v=0.04 v.toFixed(0)=0.
v=0.05 v.toFixed(0)=1.
v=0.06 v.toFixed(0)=1.
v=0.07 v.toFixed(0)=1.
v=0.08 v.toFixed(0)=1.
v=0.09 v.toFixed(0)=1.
v=0.1 v.toFixed(0)=0.

I'll attach my test program. It's just a simple Mxml written in flex builder.

The workaround I've come up with it to create my own wrapper for Number.toFixed(). In the wrapper I have an "if" on precision == 0. If it is 0, I don't call Number.toFixed, but instead call Math.round(). It seems to work ok.

Comment 1

[Brent Baker]

Transferred from JIRA ASC-3826 http://bugs.adobe.com/jira/browse/ASC-3826
Transferred comments:

Mark DeMichele - [09/18/08 09:03 AM ]
This is worse than I thought. I could handle the fact that it was messed up when passing a 0 as the precision. In that case I wrote a wrapper for it and when the precision was 0, I would call Number.round instead. However, I just came accross another bug when passing a 2 or more for the precision. It seems that any number where the first significant >=5 (i.e., 0.0005, 0.0006,0.000000008, etc.) will return 0.01 (0.001, etc). This is really bad when you're depending on this thing to work.

Look at some of these new results. Most of these should be returning 0. The way this is. This method it totally useless since it leads to what I call timebomb bugs. Someone should really address this in Flash 10. Though it's moot since I can't depend on my users having it anyway. I guess it's time to write own "toFixed".

v=0.00005 v.toFixed(2)=0.01
v=0.00007 v.toFixed(2)=0.01
v=0.00009 v.toFixed(2)=0.01
v=5e-7 v.toFixed(2)=0.00
v=7e-7 v.toFixed(2)=0.01
v=9e-7 v.toFixed(2)=0.01
v=0.00005 v.toFixed(3)=0.001
v=0.00007 v.toFixed(3)=0.001
v=0.00009 v.toFixed(3)=0.001
v=5e-7 v.toFixed(3)=0.000
v=7e-7 v.toFixed(3)=0.001
v=9e-7 v.toFixed(3)=0.001
v=-0.1 v.toFixed(0)=-0.

Comment 2

[Brent Baker]

Comment on attachment 397069 [details]
test

Shows that there are weird rounding issues happening with the toFixed()


Example:
0.09->toFixed(0) SHOULD be "0" but tamarin returns "1."

Same rounding issues can be seen with 0.00005->toFixed(2), should be "0.00" but tamarin returns "0.01"

Comment 3

[Lars T Hansen]

Poaching.

Comment 7

[Lars T Hansen]

TC pushed: redux changeset:   2760:2e75d5d3010b

Comment 8

[Lars T Hansen]

Attachment: Patch

A couple of different fixes here.  First, the case of precision==0 must be handled specially because we don't want to emit the '.'.  Second, rounding must only be performed if we emitted as many digits as called for by the precision without filling in any zeroes.

Comment 9

[Steven Johnson]

Any fix for this bug will likely need version-checking for backwards compatibility.

Comment 11

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 12

[Lars T Hansen]

Attachment: Patch, v2

This is the same patch but rebased to current TR and with proper version checking added.  An extremely minor tweak to the version system is included (check that version name table and enum are in sync).

Comment 13

[Lars T Hansen]

Comment on attachment 542117 [details] [diff] [review]
Patch, v2

Incorrect use of bug compatibility, new patch coming up.

Comment 14

[Lars T Hansen]

Attachment: Patch, v4

Now also with test cases.

Comment 15

[Steven Johnson]

Comment on attachment 542123 [details] [diff] [review]
Patch, v4

Review of attachment 542123 [details] [diff] [review]:
-----------------------------------------------------------------

::: core/AvmCore.cpp
@@ +5028,5 @@
>      {
> +#ifdef DEBUG
> +        // Prevent the name table from getting out of sync with the enum.
> +
> +        for ( int i=0 ; i < VersionCount ; i++)

rogue extra space needs to die.

::: core/MathUtils.cpp
@@ +872,5 @@
>          char *s = buffer;
>          bool negative = false;
> +        bool round = true;
> +        bool noFraction = (precision == 0);
> +        bool bugzilla513039 = core->currentBugCompatibility()->bugzilla513039;  // Number.toFixed(0) returns incorrect numbers, rounding issues 

We should do some performance reality checks here; currentBugCompatibility() is cheap but not free, and while I suspect it's just noise compared to the rest of the work done in this function, would be good to do a quick verification of that.

Comment 16

[Lars T Hansen]

> rogue extra space needs to die.

OK.

> We should do some performance reality checks here; currentBugCompatibility()
> is cheap but not free, and while I suspect it's just noise compared to the
> rest of the work done in this function, would be good to do a quick
> verification of that.

If currentBugCompatibility() is that expensive then it is fundamentally unusable.  Too bad - we will be using it all over the floating-point parsing and formatting code to fix the huge number of bugs in that code.

Comment 17

[Tamarin Bot]

changeset: 6432:22702a6d14ba
user:      Lars T Hansen <[e-mail]>
summary:   Fix 513039 - Number.toFixed(0) returns incorrect numbers, rounding issues (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/22702a6d14ba