556860 - Fix unsigned/signed warnings in nsGeolocation.cpp

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Daniel Holbert [:dholbert]]

I get these warnings when building nsGeolocation.cpp:
../../../../mozilla/dom/src/geolocation/nsGeolocation.cpp: In member function 'virtual nsresult nsGeolocationRequest::Allow()':
../../../../mozilla/dom/src/geolocation/nsGeolocation.cpp:276: warning: comparison between signed and unsigned integer expressions
../../../../mozilla/dom/src/geolocation/nsGeolocation.cpp: In member function 'virtual nsresult nsGeolocation::ClearWatch(PRInt32)':
../../../../mozilla/dom/src/geolocation/nsGeolocation.cpp:925: warning: comparison between signed and unsigned integer expressions

Filing this bug on fixing 'em.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

Here's a fix.

The first chunk is for a comparison between a PRTime (signed 64-bit val) and a DOMTimeStamp (unsigned 64-bit val).  Patch casts the PRTime to be a DOMTimeStamp, so they're both unsigned. (This is what already happens under-the-hood anyway)

The second chunk is a comparison between PRUint32 and PRInt32, and when we hit it, we've already checked that the PRInt32 is >=0. (earlier in the "if" statement).  So it's safe to cast that value to a PRUint32 for the later comparison.

Comment 2

[Daniel Holbert [:dholbert]]

Not completely sure about the first chunk -- the "- maximumAge" might bring us below 0, which would make our cast-to-unsigned be a bit bogus.

Perhaps it'd be better to create a PRTime out of cachedPositionTime, so we'd be doing our comparison in (signed) PRTime space, instead of in (unsigned) DOMTimeStamp space?  This would almost certainly be safe -- it could only fail if cachedPositionTime were ridiculously large.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Seems safer to have the first chunk do its comparison in a signed number-space, given the subtraction involved. So, I changed the cast as described in comment 2.

Comment 4

[Doug Turner (:dougt)]

is a "unsigned long long" always a PRTime?

Comment 5

[Daniel Holbert [:dholbert]]

No -- PRTime is the one that's _signed_.  It's typedef'd to PRInt64.
http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prtime.h#80

It comes into play here due to the use of PR_Now() (which returns PRTime).

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 436759 [details] [diff] [review]
fix v2

thanks!

Comment 7

[Josh Matthews [:jdm]]

Marking this as blocking the e10s geolocation work, as there's a lot of code being moved around, and I'd prefer not to lose these changes when merging later.

Comment 8

[Daniel Holbert [:dholbert]]

Landed on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/7c568c6d18f6