Last Comment Bug 711404 - PRUint64 means PRInt64 in xpconnect
: PRUint64 means PRInt64 in xpconnect
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Makoto Kato [:m_kato]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 769938 771401
  Show dependency treegraph
 
Reported: 2011-12-16 06:26 PST by Mano (::mano, needinfo? for any questions; not reading general bugmail)
Modified: 2012-07-05 18:28 PDT (History)
4 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (6.63 KB, patch)
2011-12-18 23:28 PST, Makoto Kato [:m_kato]
bobbyholley: review+
Details | Diff | Splinter Review
Part b: Remove xpc_qsDoubleToUint64 (2.50 KB, patch)
2011-12-21 06:53 PST, :Ms2ger (⌚ UTC+1/+2)
bobbyholley: review+
Details | Diff | Splinter Review

Description Mano (::mano, needinfo? for any questions; not reading general bugmail) 2011-12-16 06:26:00 PST
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) (busy with Stylo) 2011-12-16 11:51:24 PST
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] 2011-12-16 17:58:07 PST
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] 2011-12-18 23:28:50 PST
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.
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2011-12-19 12:14:21 PST
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] 2011-12-20 02:05:32 PST
(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) (busy with Stylo) 2011-12-20 10:19:54 PST
Mano, can you elaborate on how this will break things?
Comment 7 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2011-12-20 14:02:49 PST
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) (busy with Stylo) 2011-12-20 14:35:39 PST
(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 (⌚ UTC+1/+2) 2011-12-21 06:53:09 PST
Created attachment 583482 [details] [diff] [review]
Part b: Remove xpc_qsDoubleToUint64

Might as well do this here too.
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2011-12-21 10:47:00 PST
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 (⌚ UTC+1/+2) 2011-12-21 11:09:53 PST
I don't think it makes a lot of sense to move to stdint on a per-file basis.
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2011-12-22 03:29:38 PST
Makoto, I need to land some stuff tomorrow; if you don't mind, I'll land your patch as well.
Comment 14 Eric Shepherd [:sheppy] 2012-04-19 12:09:52 PDT
Documentation updated:

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

Mentioned on Firefox 12 for developers.

Note You need to log in before you can comment on or make changes to this bug.