327719 - Passing a big double serialize a rounded double

Status: RESOLVED FIXED

(Core Graveyard :: Web Services, defect)

Description

[Ariel Fatecha]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; es-ES; rv:1.8.0.1) Gecko/20060124 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; es-ES; rv:1.8.0.1) Gecko/20060124 Firefox/1.5.0.1

Taking an operation in a wsdl with a parameter xs:double if you pass a big double (like 1000006) the created value is rounded (ej: 1000010)
Using tcpdump I could get (using 1000006):

<env:Envelope xmlns:env="http://schemas.xmlsoap.org/soap/envelope/" xmlns:enc="http://schemas.xmlsoap.org/soap/encoding/" env:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/" xmlns:xs="http://www.w3.org/1999/XMLSchema" xmlns:xsi="http://www.w3.org/1999/XMLSchema-instance"><env:Header/><env:Body><doubleTest xmlns="http://test.impl.mirilla.forja.tdi.py"><val xsi:type="xs:double">1000010</val></doubleTest></env:Body></env:Envelope> 

this error doesn't happen in mozilla 1.7 but does so in xulrunner 1.8.0.1 (linux  and windows) and firefox 1.5.0.1

Reproducible: Always

Steps to Reproduce:
1. Get a service with a xs:double as parameter
2. pass a big double (like 1000006)
3. see your passed value (using tcpdump o logging in the server)

Actual Results:  
using 1000006 your serialized value is 1000010

Expected Results:  
1000006

Comment 1

[Philip Withnall (unavailable)]

Could you provide a link to a page demonstrating this, please?

Comment 2

[Ariel Fatecha]

Attachment: (testcase) xul page to use from chrome

This file use the service "http://forjademo.idea.com.py/jboss-net/services/SrvPrimitiveTypes?wsdl"

Comment 3

[Ariel Fatecha]

Sorry, this service: 
http://forjademo.idea.com.py/jboss-net/services/SrvTestPrimitiveTypes?wsdl

Comment 4

[Ariel Fatecha]

I was looking into this a little...
File extension/webservices/soap/src/nsDefaultSOAPEncoder.cpp:1637
use value.AppendFloat(f)
As the comment says, AppendFloat takes a double but nsStringObsolete.AppendFloat calls:
Modified_cnvtf(buf, sizeof(buf), 6, aFloat);
where 6 means the precision value.
So, if I'm right, the method appends a float not a double, even when it takes a double.

Comment 5

[Ariel Fatecha]

Using gbd I could confirm what I'm thinking about nsStringObsolete.AppendFloat (see comment above)

gdb:

Breakpoint 4, nsDoubleEncoder::Encode (this=0x86b9c30, aEncoding=0x863a948, aSource=0x86648c8, aNamespaceURI=@0xbfffbb44, aName=@0xbfffbbe4, aSchemaType=0x861f450, aAttachments=0x0, aDestination=0x84342ac, aReturnValue=0xbfffc234) at /music/mozilla/extensions/webservices/soap/src/nsDefaultSOAPEncoder.cpp:1637
1637      value.AppendFloat(f);
(gdb) print f
$93 = 1000006
(gdb) print value
$94 = {<nsFixedString> = {<nsString> = {<nsSubstring> = {<nsAString_internal> = {mVTable = 0x413c5c08, mData = 0xbfffb32c, mLength = 0, mFlags = 65553}, <No data fields>}, <No data fields>}, mFixedCapacity = 63, mFixedBuf = 0xbfffb32c}, mStorage = {0, 49151, 14599, 17295, 39992, 2155, 3, 0, 61215, 17300, 5724, 16509, 29416, 2053, 44952, 16697, 45932, 49151, 60772, 16508, 46572, 49151, 17, 1, 45948, 49151, 33962, 16507, 46548, 49151, 44952, 16697, 45948, 49151, 44952, 16697, 62548, 2145, 62544, 2145, 45980, 49151, 8018, 16500, 29416, 2053, 62544, 2145, 4, 0, 53700, 17300, 26856, 17301, 26856, 17301, 46028, 49151, 18495, 17293, 62548, 2145, 4, 0}}
(gdb) next
1638      return EncodeSimpleValue(aEncoding, value,
(gdb) print value
$95 = {<nsFixedString> = {<nsString> = {<nsSubstring> = {<nsAString_internal> = {mVTable = 0x413c5c08, mData = 0xbfffb32c, mLength = 7, mFlags = 65553}, <No data fields>}, <No data fields>}, mFixedCapacity = 63, mFixedBuf = 0xbfffb32c}, mStorage = {49, 48, 48, 48, 48, 49, 48, 0, 61215, 17300, 5724, 16509, 29416, 2053, 44952, 16697, 45932, 49151, 60772, 16508, 46572, 49151, 17, 1, 45948, 49151, 33962, 16507, 46548, 49151, 44952, 16697, 45948, 49151, 44952, 16697, 62548, 2145, 62544, 2145, 45980, 49151, 8018, 16500, 29416, 2053, 62544, 2145, 4, 0, 53700, 17300, 26856, 17301, 26856, 17301, 46028, 49151, 18495, 17293, 62548, 2145, 4, 0}}

where: "mStorage = {49, 48, 48, 48, 48, 49, 48, " means 1000010 in ascii code

I break the point nsStringObsolete:1312 where it calls Modified_cnvtf(...6...)
and I call it using 20 as precision value and produce a correct value.
There are 2 posible ways to solve this.
1. Revert code inside nsDefaultSOAPEncoder.cpp (nsDoubleEncoder::Encode) to the previous implementation that was using lower level code instead AppendFloat.
2. Patch AppendFloat to use bigger precision than 6 (float).

I think that AppendFloat taking a double (and append a float really) causes some confussion, but that problem is about strings not soap.
imho, option 1 is better and safer than option 2. About option 2 a string coder must decide if it is necessary open a bug.

I will create an attachment reverting the code. Who should review it?

Comment 6

[Ariel Fatecha]

Attachment: Patch to revert nsDoubleEncoder::Encode

Comment 7

[Philip Withnall (unavailable)]

I'm not the person to review this; you'll be wanting one of the web services guys to review this.

Comment 8

[Ariel Fatecha]

reviewer list (http://www.mozilla.org/hacking/reviewers.html) has no webservices item...

Comment 9

[Ariel Fatecha]

Comment on attachment 212922 [details] [diff] [review]
Patch to revert nsDoubleEncoder::Encode

QA contact for review

Comment 10

[Doron Rosenberg (IBM)]

Comment on attachment 212922 [details] [diff] [review]
Patch to revert nsDoubleEncoder::Encode

>Index: extensions/webservices/soap/src/nsDefaultSOAPEncoder.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/webservices/soap/src/nsDefaultSOAPEncoder.cpp,v
>retrieving revision 1.93.6.1
>diff -u -1 -0 -r1.93.6.1 nsDefaultSOAPEncoder.cpp
>--- extensions/webservices/soap/src/nsDefaultSOAPEncoder.cpp	24 Aug 2005 19:21:19 -0000	1.93.6.1
>+++ extensions/webservices/soap/src/nsDefaultSOAPEncoder.cpp	23 Feb 2006 18:47:13 -0000
>@@ -1623,25 +1623,28 @@
>                         nsIDOMElement * aDestination,
>                         nsIDOMElement * *aReturnValue)
> {
>   NS_ENSURE_ARG_POINTER(aEncoding);
>   NS_ENSURE_ARG_POINTER(aDestination);
>   NS_ENSURE_ARG_POINTER(aReturnValue);
>   *aReturnValue = nsnull;
>   nsresult rc;
>   double f;
>   rc = aSource->GetAsDouble(&f);        //  Check that double works.
>-  NS_ENSURE_SUCCESS(rc, rc);
>-
>+  if (NS_FAILED(rc))
>+    return rc;

what is rc in this case that warrants removing the ENSURE_SUCCESS?

Comment 11

[Ariel Fatecha]

Comment on attachment 212922 [details] [diff] [review]
Patch to revert nsDoubleEncoder::Encode

rc is "return check" (just for control the double assignment from nsIVariant), in mozilla 1.7 was the conditional instead the NS_ENSURE, but I will keep NS_ENSURE.

Comment 12

[Ariel Fatecha]

Attachment: Patch keeping NS_ENSURE because comment #10

Comment 13

[Ariel Fatecha]

The code in nsDefaultSOAPEncoder.cpp was changed because of bug 282245:
"SOAPCall encodes SOAPParameter with floating point number value with comma (,) instead of point (.)"
attachment:
https://bugzilla.mozilla.org/attachment.cgi?id=174586
I will cc some people of that bug

Comment 14

[Boris Zbarsky [:bzbarsky]]

Right.  You can't use locale-aware functions when implementing any W3C spec.

The right fix here is probably to either fix the double version of AppendFloat or to add an nsString::AppendDouble....

Comment 15

[Ariel Fatecha]

Attachment: Patch for nsStringObsolete.cpp (easy way)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 213084 [details] [diff] [review]
Patch for nsStringObsolete.cpp (easy way)

I'm not a module owner for this code.

Comment 17

[Ariel Fatecha]

Comment on attachment 213084 [details] [diff] [review]
Patch for nsStringObsolete.cpp (easy way)

Ok. I'll try with [e-mail]

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #17)
> (From update of attachment 213084 [details] [diff] [review] [edit])
> Ok. I'll try with [e-mail]

[e-mail] is not actively reviewing mozilla code. See http://www.mozilla.org/owners.html#string . darin@meer is probably your best bet.

Comment 19

[Darin Fisher]

Comment on attachment 213084 [details] [diff] [review]
Patch for nsStringObsolete.cpp (easy way)

Why 20?  Also, could you please add a unit test to xpcom/tests/TestStrings.cpp?

Comment 20

[Darin Fisher]

Comment on attachment 213084 [details] [diff] [review]
Patch for nsStringObsolete.cpp (easy way)

minusing patch to get attention.  please see my last comment.  please re-request review when you have answers ;-)

Comment 21

[Ariel Fatecha]

ok, I'm sorry for that. :(
why 20? because 2^64 has 20 characters but It's too big apparently...
This is not working very good using 9223372036854775807 (max int) has a result: "9223372036854775808"
I'll add a test for this tomorrow night.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

a double can't represent 2^64 exactly. and what's the significance of that value anyway?

Comment 23

[Ariel Fatecha]

Nothing, I was wrong!
I need to know the max number of characters that can exist into a double type

Comment 24

[Ariel Fatecha]

Attachment: Patch for nsStringObsolete.cpp with 15 as precision

15 (not 20) because :
"The double type contains 64 bits: 1 for sign, 11 for the exponent, and 52 for the mantissa. Its range is +/1.7E308 with at least 15 digits of precision."
from:
http://msdn.microsoft.com/library/en-us/vccelng/htm/decla_37.asp?frame=true

about test in TestString.cpp: next attachment (in 20 minutes)

Comment 25

[Ariel Fatecha]

Attachment: Patch for TestString.cpp

A very simple test case for this bug.

Comment 26

[Darin Fisher]

Comment on attachment 213800 [details] [diff] [review]
Patch for TestString.cpp

Thank you for including this test case!

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm somewhat concerned that there may be some callers that depend on appending not that many characters, e.g., computed style.  Should we overload this for float and double, and append fewer characters for floats?  It seems like this could easily append a large number of totally meaningless digits for floats.  Or am I on the wrong track?

Comment 28

[Ariel Fatecha]

I have tested str.AppendFloat( 0.1f* 0.1f ) and the result was "0.0100000007078052".
I'll add a new method AppendFloat( float ) and a new testcase (for both methods)

Comment 29

[Ariel Fatecha]

Attachment: Patch nsStringObsolete.cpp and nsTString.h

Add AppendFloat( float ).

If we promote float to double by using the modified AppendFloat(double) with a float, it propagates error into the double due to its inferior precision, and since we are serializing to 15 digits, we output noise. This method avoids that and serializes only to the limit of float's precision.

Comment 30

[Ariel Fatecha]

Attachment: New test for TestStrings.cpp

Test for AppendFloat( double ) and AppendFloat( float )

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 213891 [details] [diff] [review]
New test for TestStrings.cpp

>+    double aDouble = 11223344556.66;

aDouble is our standard convention for function arguments, so could you use some other name not beginning with a followed by a capital letter?

Comment 32

[Ariel Fatecha]

Attachment: test for TestString.cpp

I changed the variable name "aDouble" to "bigdouble"

Comment 33

[Ariel Fatecha]

I don't have cvs access. Can somebody commit this?
QA maybe...

Comment 34

[Darin Fisher]

fixed-on-trunk, thanks for the patch!