538512 - local folder size limit to extend to 4GB on Mac OS X

Status: RESOLVED FIXED

Description

[Makoto Kato [:m_kato]]

CString::ToInteger return error in nsParseLocalMessageURI() when msgkey is over PR_INT32_MAX.  So we should use nsCRT::atoll() for nsMsgKey.

Also, atol(over PR_INT32_MAX string) returns 0x7fffffff on Mac OS X.  So we use atoll version for unsigned long (nsMsgKey).

Comment 1

[Makoto Kato [:m_kato]]

Attachment: patch v1

Comment 2

[David :Bienvenu]

Comment on attachment 420670 [details] [diff] [review]
patch v1

Thx for the patch. Did you want reviews on it?

Comment 3

[Makoto Kato [:m_kato]]

(In reply to comment #2)
> (From update of attachment 420670 [details] [diff] [review])
> Thx for the patch. Did you want reviews on it?

I have waited TryServer build for Linux. It is done, so could you review this?

Comment 4

[David :Bienvenu]

Comment on attachment 420670 [details] [diff] [review]
patch v1

Thx for the patch. It looks OK. Can you use ParseUint64Str instead of nsCRT::atoll? We're trying to avoid using nsCRT...

Comment 5

[Makoto Kato [:m_kato]]

(In reply to comment #4)
> (From update of attachment 420670 [details] [diff] [review])
> Thx for the patch. It looks OK. Can you use ParseUint64Str instead of
> nsCRT::atoll? We're trying to avoid using nsCRT...

OK.  Since there is ParseUint64Str() in nsImapUtils.h, should I move it to nsMsgUtils.h.

Comment 6

[Makoto Kato [:m_kato]]

Attachment: patch v2

Comment 7

[David :Bienvenu]

thx. We're going to need an xpcshell test for this, I think. I can write it if you aren't able to, but it will take a little time for me to get to it.  The test would be similar to the one I added for the 4GB offline store limit, here - https://bugzilla.mozilla.org/attachment.cgi?id=421461

except that it would be for local mail. I would imagine the test would look something like the following:

test for free disk space > 6GB, just to be safe.
extend the local Inbox to be 3GB using the same approach as I used in the above test (create an output stream, seek to 3GB, and then write some data, and close the file).
Then, set the summary file valid using the local inbox db (you'll need to do this because you've changed the file size, which usually invalidates the db)
Then, append a message to the inbox
Finally, verify that you can stream the message

Comment 8

[Makoto Kato [:m_kato]]

Attachment: patch v2 with unit test

Comment 9

[David :Bienvenu]

Comment on attachment 421769 [details] [diff] [review]
patch v2 with unit test

thx for doing the test. I'd like the test to actually try to stream the message that's past 2GB to make sure that code works - you can use streamMessage with a listener for the data, and just verify that it looks like the original message. I'd also like the test name to not specifically mention the bug #, but be more descriptive - e.g, test_2GBMailboxes.js, something like that. Thx!

Comment 10

[David :Bienvenu]

Comment on attachment 421769 [details] [diff] [review]
patch v2 with unit test

minusing based on request for a slightly better test.

Comment 11

[Makoto Kato [:m_kato]]

Attachment: patch v3

Comment 12

[David :Bienvenu]

Comment on attachment 425184 [details] [diff] [review]
patch v3

great, thx for the test; I'll try this out as soon as I can (today, I hope).

Comment 13

[David :Bienvenu]

Comment on attachment 425184 [details] [diff] [review]
patch v3

thx very much, that works. One nit - // get message that msgkey is over 2G.
+
would be better as "get message whose msg key is over 2GB"

Comment 14

[Makoto Kato [:m_kato]]

landed
http://hg.mozilla.org/comm-central/rev/dfdc5d1a4278

Comment 15

[Wayne Mery (:wsmwk)]

missing target milestone

Comment 16

[Wayne Mery (:wsmwk)]

I picked the wrong TM in comment 15.
standard8, pretty sure this landed in 3.1a2/which got renamed to 3.1b1.
bienvenu, what about 3.0.x?  
fixing up flags.