Bug 16652 – Firefox and JavaScriptCore differ in Number.toString(integer)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Firefox and JavaScriptCore differ in Number.toString(integer)

See:
http://trac.webkit.org/projects/webkit/browser/trunk/LayoutTests/fast/js/number-toString-expected.txt?rev=29020

Opera agrees with us.  We'll need to check IE.  If IE agrees with us, then we should check in passing results and file a bug with Mozilla.

[Eric Seidel (no email)]

Unfortunately the spec isn't very helpful here:
http://bclary.com/2004/11/07/#a-15.7.4.2

So we simply chose to match Firefox (or IE if we determine their behavior to be different).

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> So we simply chose to match Firefox (or IE if we determine their behavior to be
> different).

Results from running MSIE 7 on Windows XP SP2 in Virtual PC 7.0.3 on Mac OS X 10.4.11 (8S165):

PASS (0.0).toString(4) is "0"
PASS (-0.0).toString(4) is "0"
PASS (0.0).toString() is "0"
PASS (-0.0).toString() is "0"
PASS (1234.567).toString() is "1234.567"
PASS (1234.567).toString(0) threw exception [object Error].
PASS (1234.567).toString(null) threw exception [object Error].
PASS (1234.567).toString(false) threw exception [object Error].
PASS (1234.567).toString('foo') threw exception [object Error].
PASS (1234.567).toString(nan) threw exception [object Error].
PASS (1234.567).toString(1) threw exception [object Error].
PASS (1234.567).toString(true) threw exception [object Error].
PASS (1234.567).toString('1') threw exception [object Error].
FAIL (1234.567).toString(3) should be 1200201.120022100021001021021002202. Was 1200201.120022100021001021021002201.
PASS (1234.567).toString(4) is "103102.21010212322113203111"
FAIL (1234.567).toString(5) should be 14414.240414141414141414. Was 14414.2404141414141414142.
FAIL (1234.567).toString(6) should be 5414.32224554134430233. Was 5414.322245541344302332.
FAIL (1234.567).toString(7) should be 3412.365323661111653. Was 3412.3653236611116525.
PASS (1234.567).toString(8) is "2322.44223351361524"
PASS (1234.567).toString(9) is "1621.50830703723265"
PASS (1234.567).toString(10) is "1234.567"
FAIL (1234.567).toString(11) should be a22.62674a0a5885. Was a22.62674a0a588491.
FAIL (1234.567).toString(12) should be 86a.697938b17701. Was 86a.697938b177012.
FAIL (1234.567).toString(13) should be 73c.74a91191a65. Was 73c.74a91191a64cb.
PASS (1234.567).toString(14) is "642.7d1bc2caa757"
FAIL (1234.567).toString(15) should be 574.87895959596. Was 574.87895959595a.
PASS (1234.567).toString(16) is "4d2.9126e978d5"
PASS (1234.567).toString(17) is "44a.9aeb6faa0da"
FAIL (1234.567).toString(18) should be 3ea.a3cd7102ac. Was 3ea.a3cd7102abf.
FAIL (1234.567).toString(19) should be 37i.aed102a04d. Was 37i.aed102a04d4.
FAIL (1234.567).toString(20) should be 31e.b6g. Was 31e.b6g00000001.
PASS (1234.567).toString(21) is "2gg.bj0kf5cfe9"
FAIL (1234.567).toString(22) should be 2c2.ca9937cak. Was 2c2.ca9937cak1.
PASS (1234.567).toString(23) is "27f.d0lfjb1a7c"
FAIL (1234.567).toString(24) should be 23a.dee4nj99j. Was 23a.dee4nj99j0.
FAIL (1234.567).toString(25) should be 1o9.e49999999. Was 1o9.e49999999a.
FAIL (1234.567).toString(26) should be 1lc.ej7fa4pkf. Was 1lc.ej7fa4pkfb.
FAIL (1234.567).toString(27) should be 1ij.f8971772k. Was 1ij.f8971772j.
PASS (1234.567).toString(28) is "1g2.foelqia8e"
FAIL (1234.567).toString(29) should be 1dg.gcog9e05q. Was 1dg.gcog9e05p.
FAIL (1234.567).toString(30) should be 1b4.h09. Was 1b4.h09000000.
FAIL (1234.567).toString(31) should be 18p.hhrfcj3t. Was 18p.hhrfcj3t1.
PASS (1234.567).toString(32) is "16i.i4jeiu6l"
FAIL (1234.567).toString(33) should be 14d.inf96rdvm. Was 14d.inf96rdvl.
FAIL (1234.567).toString(34) should be 12a.j9fchdtm. Was 12a.j9fchdtm1.
FAIL (1234.567).toString(35) should be 109.jtk4d4d4e. Was 109.jtk4d4d4d.
FAIL (1234.567).toString(36) should be ya.kety9sifl. Was ya.kety9sifkl.
PASS (1234.567).toString(37) threw exception [object Error].
PASS (1234.567).toString(-1) threw exception [object Error].
PASS (1234.567).toString(posInf) threw exception [object Error].
PASS (1234.567).toString(negInf) threw exception [object Error].
PASS posInf.toString() is "Infinity"
PASS negInf.toString() is "-Infinity"
PASS nan.toString() is "NaN"
PASS successfullyParsed is true

TEST COMPLETE

[Eric Seidel (no email)]

Since the checked in results match Firefox, it looks like IE and Firefox are nearly identical.  So we're in the wrong here.  I guess we'll eventually fix our Number.toString implementation to match IE & FF.

[Eric Seidel (no email)]

I fixed the throwing exceptions issue as part of bug 16672.

[Gavin Barraclough]

Created attachment 99429 [details]
Preliminary patch, requires testing & new layout test results.

[Gavin Barraclough]

Created attachment 99490 [details]
The patch

[WebKit Review Bot]

Attachment 99490 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/JavaScriptCore/runtime/NumberPrototype.cpp:118:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/NumberPrototype.cpp:320:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/NumberPrototype.cpp:370:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/NumberPrototype.cpp:526:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/NumberPrototype.cpp:636:  One space before end of line comments  [whitespace/comments] [5]
Source/JavaScriptCore/runtime/NumberPrototype.cpp:636:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 99490 [details]
The patch

Attachment 99490 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8978036

[Gavin Barraclough]

Created attachment 99494 [details]
The patch, style fixes

[Gavin Barraclough]

Created attachment 99495 [details]
Fix Qt bot's style quibbles.

[Sam Weinig]

Comment on attachment 99495 [details]
Fix Qt bot's style quibbles.

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

Overall, this looks really good.  I think we should put the two classes in their own headers in WTF and even write some unit tests for them in TestWebKitAPI.  You should probably also move decomposeDouble to MathExtras.h and again, consider unit tests.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:130
> +// A value 1 greater than max uint16_t.
> +#define Uint16Infinity 0x10000

Please put a comment about why this is necessary to work around a compiler issue.  I also find the name a little confusing.  Perhaps be explicit and call it something like oneGreaterThanMaxUInt16.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:351
> +    bool isNormalized() const

Normalized is a bit odd here, since there are so many different concepts of normal in double-ish math.  Perhaps something lengthy including the word valid would help.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:494
> +    double fractionPart = number - floor(number);

The floor(number) is redundant, you can just use integerPart.

[Gavin Barraclough]

Sam,

I've landed this in r90347 on the basis of your r+ & the fact that the code is covered by tests in fast/js, without unit tests.  The unit testing framework is new to me, and given this code has test coverage it makes sense to me to land this, and follow up with a patch to add additional test cases once I have a chance to sit down with you & learn how to write unit tests.  Hope this sounds right to you.

cheers,
G.

[Kent Tamura]

r90347 caused build erros on Leopard, WinCariro, WinCE, and Chromium-win.

http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Build%29/builds/36837/steps/compile-webkit/logs/stdio

/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/Source/JavaScriptCore/runtime/Uint16WithFraction.h: In member function 'JSC::Uint16WithFraction& JSC::Uint16WithFraction::operator*=(uint16_t)':
/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/Source/JavaScriptCore/runtime/Uint16WithFraction.h:125: warning: implicit conversion shortens 64-bit value into a 32-bit value


http://build.webkit.org/builders/WinCairo%20Debug%20%28Build%29/builds/8133/steps/compile-webkit/logs/stdio

10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(266) : error C2061: syntax error : identifier 'int32_t'
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(272) : error C2065: 'uint64_t' : undeclared identifier
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(272) : error C2146: syntax error : missing ';' before identifier 'bits'
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(272) : error C2065: 'bits' : undeclared identifier
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(272) : error C2783: 'TO WTF::bitwise_cast(FROM)' : could not deduce template argument for 'TO'
10>        D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/StdLibExtras.h(94) : see declaration of 'WTF::bitwise_cast'
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(273) : error C2065: 'exponent' : undeclared identifier
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(273) : error C2061: syntax error : identifier 'int32_t'
10>D:\Projects\BuildSlave\win-cairo-debug\build\WebKitBuild\Debug_Cairo_CFLite\Include\private\JavaScriptCore/MathExtras.h(274) : error C2065: 'mantissa' : undeclared identifier
10>D:\Projects\BuildSlave\win-cairo-debug\build\Source\JavaScriptCore\os-win32\stdint.h(42) : error C2378: 'uint64_t' : redefinition; symbol cannot be overloaded with a typedef

[Kent Tamura]

Rolled out as r90349.

[Gavin Barraclough]

Created attachment 99666 [details]
Patch with build fixes

[Gavin Barraclough]

Oh.  The last patch didn't cause errors on the EWS bots anyway, so watching them won't help. :-(  LAnding patch & watching the buildbots.

[Gavin Barraclough]

Re-landed in r90383 with build fixes; watching the build bots.