711404 - PRUint64 means PRInt64 in xpconnect

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Asaf Romano (gone)]

Paste this code in your Error Console:
var p = Components.classes["@mozilla.org/hash-property-bag;1"].createInstance(Components.interfaces.nsIWritablePropertyBag2); p.setPropertyAsInt64 ("a", -4000);  p.getPropertyAsUint64("a")

Note that fixing this will break using nsIBinaryStream's read64 for signed numbers (that is, it will break my code).

Comment 1

[Bobby Holley (:bholley)]

So the culprit is here:

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#163

I'd be kind of curious as to why MSVC can't handle this, and what it does instead. Also, it's possible that the compiler might provide some macro we could use.

Alternatively, if we care less about correctness in the top half of the range and more about protecting callers from unexpected negative numbers, we could just take the absolute value at some point.

This isn't something I'm likely to work on, but I'm happy to help out anyone who needs to fix it for something.

Comment 2

[Makoto Kato [:m_kato]]

Interesting.  As long as CVS log, this code is from 1999!, so this comment is for VC6.  We should test on VS2005 and VS2010.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: fix

This workaround is for VC6 and this comment issue doesn't occurs on VC2005 or later.
Also, m-c doesn't support non-HAVE_LONG_LONG platform.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 582755 [details] [diff] [review]
fix

This seems reasonable. However, comment 0 indicates that it will break other things. Can you make sure those concerns are addressed before landing? r=bholley otherwise.

Comment 5

[Makoto Kato [:m_kato]]

(In reply to Bobby Holley (:bholley) from comment #4)
> Comment on attachment 582755 [details] [diff] [review]
> fix
> 
> This seems reasonable. However, comment 0 indicates that it will break other
> things. Can you make sure those concerns are addressed before landing?
> r=bholley otherwise.

Although several IDLs use "unsigned long long" now, test is all passed.  https://tbpl.mozilla.org/?tree=Try&rev=9434cd36016b

If comment #0 occurs on some interfaces, IDL should use long long instead of unsigned long long, or we should remove PRUint64 support on xpidl.  (PRUint64 becomes PRInt64 on current code.)

Comment 6

[Bobby Holley (:bholley)]

Mano, can you elaborate on how this will break things?

Comment 7

[Asaf Romano (gone)]

Well, people *could* rely on the current behavior (I /almost/ planned to do so with nsIBinaryInputStream). I cannot tell you of particular in-tree examples, nor about extensions.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Mano from comment #7)
> Well, people *could* rely on the current behavior (I /almost/ planned to do
> so with nsIBinaryInputStream). I cannot tell you of particular in-tree
> examples, nor about extensions.

I just checked the addons MXR. There are 3 uses of nsIBinaryInputStream.read64, and all 3 use them along with read8, read16, and read32. These 3 methods _do_ return unsigned numbers, so it seems unlikely that anyone was relying on signed numbers here.

Long story short - I think this can land as long as our test suite goes green, which is the case according to Makoto. Let's do it!

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Remove xpc_qsDoubleToUint64

Might as well do this here too.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 583482 [details] [diff] [review]
Part b: Remove xpc_qsDoubleToUint64

>diff --git a/dom/workers/File.cpp b/dom/workers/File.cpp

There are only a few instances of PR* integer types in this file. So if you feel like switching the whole file over to <mozilla/Stdint.h> while you're at it, feel free. But you don't have to if you don't want to.

r+

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I don't think it makes a lot of sense to move to stdint on a per-file basis.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Makoto, I need to land some stuff tomorrow; if you don't mind, I'll land your patch as well.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/b59760807d25
https://hg.mozilla.org/mozilla-central/rev/f6d0b8d7ab3e

Comment 14

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/PRUint64
https://developer.mozilla.org/en/DOM/Blob#Gecko_notes

Mentioned on Firefox 12 for developers.