123402 - PRTime abuse

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[timeless]

/home/timeless/mozilla/mailnews/base/util/nsMsgDBFolder.cpp:1398: warning: decimal integer constant is so large that it is unsigned

http://lxr.mozilla.org/mozilla/source/mailnews/base/util/nsMsgDBFolder.cpp
  65 #define oneHour 3600000000
1394    PRTime timeOfLastPurgeCheck = gtimeOfLastPurgeCheck;
1398    timeOfLastPurgeCheck += oneHour;   

http://lxr.mozilla.org/nspr/source/nsprpub/pr/include/prtime.h#77
  77 typedef PRInt64 PRTime;

http://lxr.mozilla.org/nspr/ident?i=PRInt64
Defined as a struct type in:
  *	nsprpub/pr/include/prtypes.h, line 340 
[this only happens when !HAVE_LONG_LONG, but this is possible]

$ cat /tmp/abcd.cpp
struct a{short b; short c;};

the current code as seen by a 
int main(){
a d;
d=201;
return 0;
$ g++ /tmp/abcd.cpp
/tmp/abcd.cpp: In function `int main()':
/tmp/abcd.cpp:5: no match for `a & = int'
/tmp/abcd.cpp:1: candidates are: struct a & a::operator =(const a &)

What should the code do?

use LL_ADD and LL_INIT, write oneHour as 0xD693A400

Comment 1

[David :Bienvenu]

correcting component - I don't think writing the constant in equivalent hex is
going to make any difference, cc'ing Navin, who I think wrote this code. There's
a way to indicate that you want a 64 bit constant long, but I don't remember off
the top of my head what it is.

Comment 2

[Navin Gupta]

taking, my code,  i will have to find out more..

Comment 3

[timeless]

indeed switching to hex isn't as important as using the LL_ macros.  The point 
of this bug is that you aren't guaranteed that PRTime is a 64bit number.

I think you can use PR_UINT32(36...) if you prefer that over hex.

Comment 4

[Navin Gupta]

Attachment: proposed fix

Sean was here this morning and he was prompted to auto-compact twice in a row
within seconds interval. That should not have happened. He lost two folders!!

Basically nsMsgDBFolder::AutoCompact() throws that prompt. looks like
gTimeOfLastPurgeCheck may not have been set correctly. So I changed it to set
gTimeOfLastPurgeCheck much earlier and use those macros in case there could
have been an overflow. I am still looking into nsMsgFolderCompactor code.

Comment 5

[Navin Gupta]

cavin, david. Can I get reviews ? thx

Comment 6

[timeless]

Comment on attachment 94417 [details] [diff] [review]
proposed fix

>-   PRTime timeNow = PR_Now();  //time in microsec
this was correct unless you're intentionally truncating
>+   PRTime timeNow;
>+   LL_I2L(timeNow,PR_Now());  //time in microseconds
this is wrong unless you're trying to do something strange

http://lxr.mozilla.org/nspr/source/nsprpub/pr/include/prtime.h#177
177 NSPR_API(PRTime) 
178 #endif
179 PR_Now(void);

http://lxr.mozilla.org/nspr/source/nsprpub/pr/include/prlong.h#166
166 **	LL_I2L		  Convert signed to 64 bit

Comment 7

[David :Bienvenu]

I believe timeless is correct.

Comment 8

[Navin Gupta]

casting PRInt64 to PRInt64 is harmless as far as I can see (though it may not be
needed)

Comment 9

[Navin Gupta]

Attachment: patch

ok made that change.

Comment 10

[timeless]

Comment on attachment 94423 [details] [diff] [review]
patch

>+   PRTime timeNow = PR_Now();   //time in microseconds
>+   PRTime timeAfterOneHourOfLastPurgeCheck;
...
>+   if (timeAfterOneHourOfLastPurgeCheck < timeNow && prompt)

i suck
please use LL_CMP
http://lxr.mozilla.org/nspr/ident?i=LL_CMP
so
if (LL_CMP(timeAfterOneHourOfLastPurgeCheck, <, timeNow) && prompt)

I believe that's everything. I'm very sorry that I missed this.

Comment 11

[Navin Gupta]

I changed it to LL_CMP in my tree. 

Comment 12

[Navin Gupta]

Attachment: patch with LL_CMP

Comment 13

[David :Bienvenu]

Comment on attachment 94518 [details] [diff] [review]
patch with LL_CMP

sr=bienvenu

Comment 14

[Navin Gupta]

fixed

Comment 15

[[e-mail] (gone - use [e-mail] instead)]

Verified FIXED using my local source tree.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 94518 [details] [diff] [review]
patch with LL_CMP

a=[e-mail] for 1.0 branch.  Please change mozilla1.0.2+ to fixed1.0.2
when checked in

Comment 17

[Navin Gupta]

fixed on branch

Comment 18

[[e-mail] (gone - use [e-mail] instead)]

Verified on the branch: 

http://lxr.mozilla.org/mozilla1.0/source/mailnews/base/util/nsMsgDBFolder.cpp