The default bug view has changed. See this FAQ.

PRUint64 means PRInt64 in xpconnect

RESOLVED FIXED in mozilla12

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mano, Assigned: m_kato)

Tracking

({dev-doc-complete})

Trunk
mozilla12
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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

Comment 2

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

Comment 3

5 years ago
Created attachment 582755 [details] [diff] [review]
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.
Assignee: nobody → m_kato
(Assignee)

Updated

5 years ago
Attachment #582755 - Flags: review?(bobbyholley+bmo)
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.
Attachment #582755 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 5

5 years ago
(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.)
Mano, can you elaborate on how this will break things?
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.
(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!
Created attachment 583482 [details] [diff] [review]
Part b: Remove xpc_qsDoubleToUint64

Might as well do this here too.
Attachment #583482 - Flags: review?(bobbyholley+bmo)
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+
Attachment #583482 - Flags: review?(bobbyholley+bmo) → review+
I don't think it makes a lot of sense to move to stdint on a per-file basis.
Makoto, I need to land some stuff tomorrow; if you don't mind, I'll land your patch as well.
https://hg.mozilla.org/mozilla-central/rev/b59760807d25
https://hg.mozilla.org/mozilla-central/rev/f6d0b8d7ab3e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Keywords: dev-doc-needed
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Version: unspecified → Trunk
Documentation updated:

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

Mentioned on Firefox 12 for developers.
Keywords: dev-doc-needed → dev-doc-complete

Updated

5 years ago
Blocks: 769938
Blocks: 771401
You need to log in before you can comment on or make changes to this bug.