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

RESOLVED FIXED in Q1 12 - Brannan

Status

P3
major
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: brbaker, Assigned: lhansen)

Tracking

unspecified
Q1 12 - Brannan
All
Windows XP
Dependency tree / graph
Bug Flags:
in-testsuite ?
flashplayer-qrb +
flashplayer-bug +
flashplayer-triage +
flashplayer-needsversioning +

Details

(Whiteboard: has-patch)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 397069 [details]
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.
Flags: flashplayer-triage+
Flags: flashplayer-qrb?
(Reporter)

Comment 1

9 years ago
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.
(Reporter)

Comment 2

9 years ago
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"

Updated

9 years ago
Assignee: nobody → rreitmai
Status: NEW → ASSIGNED
Flags: flashplayer-qrb? → flashplayer-qrb+
Priority: -- → P2
Target Milestone: --- → flash10.1
(Reporter)

Updated

9 years ago
Blocks: 479769
(Reporter)

Updated

9 years ago
Flags: in-testsuite?
(Assignee)

Updated

9 years ago
Blocks: 521196
(Assignee)

Comment 3

9 years ago
Poaching.
Assignee: rreitmai → lhansen
(Assignee)

Updated

9 years ago
Duplicate of this bug: 442293
(Assignee)

Updated

9 years ago
Duplicate of this bug: 478796
(Assignee)

Updated

9 years ago
Duplicate of this bug: 513019
(Assignee)

Comment 7

9 years ago
TC pushed: redux changeset:   2760:2e75d5d3010b
(Assignee)

Updated

9 years ago
Depends on: 521772
(Assignee)

Comment 8

9 years ago
Created attachment 405840 [details] [diff] [review]
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.
Attachment #405840 - Flags: review?(stejohns)

Updated

9 years ago
Attachment #405840 - Flags: review?(stejohns) → review+
(Assignee)

Updated

9 years ago
Whiteboard: Has patch
(Assignee)

Updated

9 years ago
Severity: normal → major
Priority: P2 → P3

Comment 9

9 years ago
Any fix for this bug will likely need version-checking for backwards compatibility.
(Assignee)

Updated

9 years ago
Whiteboard: Has patch → Has patch; versioning

Updated

9 years ago
Flags: flashplayer-needsversioning+
(Assignee)

Updated

9 years ago
Whiteboard: Has patch; versioning → Has patch

Updated

9 years ago
Target Milestone: flash10.1 → Future
(Assignee)

Updated

9 years ago
Assignee: lhansen → nobody
Priority: P3 → --

Updated

9 years ago
Duplicate of this bug: 530345
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.
Status: ASSIGNED → NEW
(Assignee)

Updated

9 years ago
Priority: -- → P3
Whiteboard: Has patch → has-patch
Target Milestone: Future → flash10.2
(Assignee)

Updated

8 years ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED

Updated

8 years ago
Flags: flashplayer-bug+
Whiteboard: has-patch → has-patch, must–fix-candidate

Updated

8 years ago
Whiteboard: has-patch, must–fix-candidate → has-patch, must-fix-candidate

Updated

8 years ago
Target Milestone: Q3 11 - Serrano → Q1 12 - Brannan
(Assignee)

Comment 12

7 years ago
Created attachment 542117 [details] [diff] [review]
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).
Attachment #542117 - Flags: review?(stejohns)
(Assignee)

Updated

7 years ago
Whiteboard: has-patch, must-fix-candidate → has-patch
(Assignee)

Comment 13

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

Incorrect use of bug compatibility, new patch coming up.
Attachment #542117 - Flags: review?(stejohns)
(Assignee)

Comment 14

7 years ago
Created attachment 542123 [details] [diff] [review]
Patch, v4

Now also with test cases.
Attachment #542117 - Attachment is obsolete: true
Attachment #542123 - Flags: review?(stejohns)

Comment 15

7 years ago
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.
Attachment #542123 - Flags: review?(stejohns) → review+
(Assignee)

Comment 16

7 years ago
> 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.
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 17

7 years ago
changeset: 6432:22702a6d14ba
user:      Lars T Hansen <lhansen@adobe.com>
summary:   Fix 513039 - Number.toFixed(0) returns incorrect numbers, rounding issues (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/22702a6d14ba
You need to log in before you can comment on or make changes to this bug.