789679 - Remove 4GB of folder size warning (mailboxTooLarge="The folder %S is full") after 4GB backend work is complete [4GB backend began in TB 12.0 by bug 462665]

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Camaleon]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0.1
Build ID: 20120905151427

Steps to reproduce:

Moving a ~12 MiB message into a folder sized 4 GiB.


Actual results:

A pop-up windows appears instructing about the issue ("the folder cannot contain more messages, either compact or delete..."). The message cannot be moved to that folder.


Expected results:

I think nothing, given the new limits for the folder size should have been fixed. I'm running Thunderbird 15 on Debian Lenny (64-bits) and the file system where Thunderbird profile and messages are stored is formatted with ReiserFS. The folder was manually created (i.e., no special folder such Sent, Inbox, etc...) and this happens for a POP3 account (thus messages are stored under Local Folders). Compacting (which I usually run for the whole profile) did not help.

Comment 1

[:aceman]

Should the warning be removed on all (file)systems?

Comment 2

[Camaleon]

From Comment #1, should I take it as this warning is expected to be "normal"? 

I thought the 4 GiB folder size limit was already something of the past (at least that's what can be read from the Limits page). In the end, is not the message what matters but the folder size limitation itself, should still exists.

Comment 3

[Camaleon]

Sorry for the noise, not a bug. This feature has not been fixed yet (work in progress) and the limits for 4 GiB folder size still apply for the usual mbox storage format in local accounts.

Comment 4

[WADA:World Anti-bad-Duping Agency]

Because bug 462665 was changed to FIXED after enhancement by bug 402392(internal offset value was perhaps changed from 32bits integer to signed 64bits integer),
> Bug 462665 - Remove the 4GB local mailbox file size limit
even if "removal of old size limit at any place" is still "work in progress", Tb should correctly support "2**63 bytes" or "file size limit by OS or File System including NFS or memory card" as "new file size limit" at any place.

As for "warning about 4GB(Win) or 2GB(Linux,Mac)", why it's mandatory is;
  Once mail folder file size exceeds max value of 32bits unsigned/signed integer,
  Tb can't process mail data above 4GB/2GB, and there is no way to recover from
  mail folder file larger than 4GB/2GB.
After 64bits integer use as offset by Tb, "max file size supported by Tb" is far larger than "max file size by OS/File System". So, as far as Repair Folder(rebuild-index)/Compact/Move mail etc. works well, there is no need of "warning about exceeding file size".
Required new behaviour and new warning is;
  If append of mail data fails due to file size limit of file system,
  undo append(truncate at previous file size),
  and warn user about copy/move/archive failure, download failure etc. due to
  file size limit.
   
I believe this bug can't be INVALID.

Comment 5

[Camaleon]

@WADA

While I agree that Thunderbird should provide support for larger folder sizes (>4 GiB) this is a feature which has not been implemented yet thus this bug is, of course, invalid (please, note that despite the bug subject, the "warning" message is not the main issue here).

Feel free to open a new bug report if you think there's a different problem you estimate that needs to be reviewed or solved.

Comment 6

[Wayne Mery (:wsmwk)]

Camaleon, a screen shot would be very helpful to us please.

http://forums.mozillazine.org/viewtopic.php?f=39&t=2537603 and http://forums.mozillazine.org/viewtopic.php?p=12147347#p12147347 also claim to encounter 4GB limit.  The former is a linux user, that later is XP.

Either there is a false warning, a correct warning and 4gb isn't working, or the warning isn't coming from Thunderbird.  If either of the first two, then we have a bug.

Comment 7

[Camaleon]

I just have read the forum posts, thanks for the pointer. It looks like more people is experiencing this behaviour. 

By reading from Mozilla bug reports, I finally found this feature is not implemented and local "mbox" folders are still limited to 4 GiB of size, is this right? What's the current status about this? Can you please clarify?

Anyway, I will send a snapshot of the warning as soon as I move the messages back to the same folder, no problem, just let me some time.

Comment 8

[Wayne Mery (:wsmwk)]

(In reply to Camaleon from comment #7)
> By reading from Mozilla bug reports, I finally found this feature is not
> implemented and local "mbox" folders are still limited to 4 GiB of size, is
> this right? What's the current status about this? Can you please clarify?

I am not convinced yet that 4GB isn't correctly implemented. 3-4 reports is great, but isn't compelling, given we have millions of users. In short, we need to dig deeper.

Comment 9

[Camaleon]

(In reply to Wayne Mery (:wsmwk) from comment #8)

> I am not convinced yet that 4GB isn't correctly implemented. 3-4 reports is
> great, but isn't compelling, given we have millions of users. 

I'd say more than three or four: 

https://getsatisfaction.com/mozilla_messaging/tags/bug_462665?sort=recently_created&style=topics

> In short, we need to dig deeper.

Well, after reading bug report #462665, I concluded this is not solved (as I understand, the limitation will be removed as soon as new mail storage alternatives are in play). I can be wrong, of course, that's why this point needs to be firmly confirmed by you, guys.

Comment 10

[Wayne Mery (:wsmwk)]

(In reply to Camaleon from comment #9)
> (In reply to Wayne Mery (:wsmwk) from comment #8)
> 
> > I am not convinced yet that 4GB isn't correctly implemented. 3-4 reports is
> > great, but isn't compelling, given we have millions of users. 
> 
> I'd say more than three or four: 
> 
> https://getsatisfaction.com/mozilla_messaging/tags/
> bug_462665?sort=recently_created&style=topics
> 
> > In short, we need to dig deeper.
> 
> Well, after reading bug report #462665, I concluded this is not solved (as I
> understand, the limitation will be removed as soon as new mail storage
> alternatives are in play). I can be wrong, of course, that's why this point
> needs to be firmly confirmed by you, guys.

4GB support wasn't fully complete until TB12, which came out in April 2012. Although there were bits fixed in the year preceding it.  But all the topics in https://getsatisfaction.com/mozilla_messaging/tags/bug_462665 are a year or more old.

Comment 11

[Camaleon]

(In reply to Wayne Mery (:wsmwk) from comment #10)

> 4GB support wasn't fully complete until TB12, which came out in April 2012.
> Although there were bits fixed in the year preceding it.  

Can you please ellaborate that point? From the mentioned bug report #462665 I can't read the problem was fixed. According to Comment #57, the issue is still there.

Comment 12

[WADA:World Anti-bad-Duping Agency]

FYI.
Bug 462665 was changed to FIXED because enhancement by bug 402392 was made.
Bug 402392 has "Target Milestone: Thunderbird 12.0" flag, and has no "Tracking Flags:" - i.e. Patch for Bug 402392 was landed on Tb 12.0.

Comment 13

[Camaleon]

(In reply to WADA from comment #12)
> FYI.
> Bug 462665 was changed to FIXED because enhancement by bug 402392 was made.

Can you please point to the exact patch that solved this? Bug 402392 is about discussing a new mailbox storage options.

Comment 14

[Camaleon]

Attachment: Warning when I try to move a message on a folder that reached 4 GiB

This is what I get when I try to move a ~500 MiB message into a folder that curretnly is sized at 3,7 GiB:

-rw------- 1 sm01 sm01 3,7G sep 14 15:54 2012
-rw-r--r-- 1 sm01 sm01 1,1M sep 14 16:02 2012.msf

The warning is in Spanish but is the usual message: "The folder 2012 is full, and can't hold anymore messages. To make room for more messages, delete any old or unwanted mail and compact folder."

Comment 15

[WADA:World Anti-bad-Duping Agency]

(In reply to Camaleon from comment #13)
> (In reply to WADA from comment #12)
> > FYI.
> > Bug 462665 was changed to FIXED because enhancement by bug 402392 was made.
> Can you please point to the exact patch that solved this? Bug 402392 is
> about discussing a new mailbox storage options.

You haven't read bug 462665 comment #57 by who created patch for bug 402392?
By the patch, in change of /mailnews/local/src/nsMovemailService.cpp, "PRUint64 messageOffset;" was introduced.
I don't understand reason why following is seen in the patch for Imap and Nntp module.
> -        PRUint64 messageOffset;
> +        PRInt64 messageOffset;

Comment 16

[Wayne Mery (:wsmwk)]

Is the message merely warning, and the copy or move actually works?  I.e. you can access the messages added after the folder exceeds 4GB?

Comment 17

[Camaleon]

(In reply to WADA from comment #15)

> You haven't read bug 462665 comment #57 by who created patch for bug 402392?

No, sorry. I see the comment but I can't see the patch, could you point me exactly to the source?

Comment 18

[Camaleon]

(In reply to Wayne Mery (:wsmwk) from comment #16)
> Is the message merely warning, and the copy or move actually works?  I.e.
> you can access the messages added after the folder exceeds 4GB?

As mentioned at Comment #0, messages cannot be moved.

Comment 19

[Eric Moore]

I can confirm the problem with both Thunderbird 15.0.1 and 12.0.1. I used the same profile with each version. I tested using a drafts folder for a POP account with a global inbox, with Windows 7/NTFS. I verified using a text editor that the almost empty mbox file was good and then saved drafts of plain text messages with large attachments rather than moving or copying messages. 

I got the mbox file up to "3.99 GB (4,290,211,840 bytes)" according to "Size on disk" in the files properties in windows explorer. After that it started to become a problem finding files small enough to attach that wouldn't cause the error. When the error occurs nothing is saved to the mbox file.

Comment 20

[Wayne Mery (:wsmwk)]

Eric, thanks for testing this.

I don't know whether the previously "fixed" bugs were verified fixed, but it might be worth a look. Also, perhaps the tests to ensure correct functionality (I would guess/hope those fixes had tests) aren't working, or don't exist.

Comment 21

[WADA:World Anti-bad-Duping Agency]

(In reply to Camaleon from comment #17)

> but I can't see the patch, could you point me　exactly to the source?

Patch was proposed at bug 402392. Have you looked into the bug where patch was proposed?

Comment 22

[Joe Sabash [:JoeS1]]

Hmm..I'm pretty sure the fixes applied did not take into account the file system in use. I still have a few system set up for Fat32, and according to Wicap.. 4GB isa design restraint for Fat32
"The maximum possible size for a file on a FAT32 volume is 4 GB minus 1 byte or 4,294,967,295 (232−1) bytes. This limit is a consequence of the file length entry in the directory table and would also affect huge FAT16 partitions with a sufficient sector size.[1] Video applications, large databases, and some other software easily exceed this limit."

Comment 23

[J2m06]

(translation with google ;-( )
hello


you just describe my personal experience
1/il long ago I warn users about the thunderbird folder size. I even made ​​a tutorial in this direction http://j2m-06.pagesperso-orange.fr/faq_tb-P_T.html # recup_boite_volum
2/lors help remote I had three times the case.
Once the file 3/chaque inbox was unrecoverable file (zero filled).
4/les users tried compaction as indicated by thunderbird.
5/je not think thunderbird is involved.
6/Thunderbird should refuse to work with large files!.
Especially not run compaction if the file is too large.
J2m

***************
bonjour

juste pour vous décrire mon experience  personnelle
1/il y a longtemps que je mets en garde les utilisateurs de thunderbird sur la taille des dossiers. J'ai meme fait un tutoriel dans ce sens http://j2m-06.pagesperso-orange.fr/faq_tb-P_T.html#recup_boite_volum
2/lors de l'aide à distance j'ai eu  trois fois le cas.
3/chaque fois le fichier inbox a été irrécupérable (fichier rempli de zero).
4/les utilisateurs avaient tenté un compactage comme indiqué par thunderbird.
5/je ne pense pas que thunderbird soit en cause .
6/Thunderbird devrait refuser de travailler avec des fichiers trop volumineux!.
Surtout ne pas lancer de compactage si le fichier est trop volumineux .

Comment 24

[Camaleon]

(In reply to WADA from comment #21)

> Patch was proposed at bug 402392. 

It was proposed and accepted?

> Have you looked into the bug where patch was proposed?

It's a 99-comments bug report with 3 attachments. The attachments filenames don't suggest a fix for this. Sorry, I don't want to sound rude, it's simply that can't locate the corresponding fix.

Comment 25

[WADA:World Anti-bad-Duping Agency]

(In reply to Camaleon from comment #24)
> it's simply that can't locate the corresponding fix.

If you want to know actal(and entire) change by that bug, see changeset pointed by bug 402392 comment #87, please.
> http://hg.mozilla.org/comm-central/rev/097bc36f180d
nsMsgBrkMBoxStore.cpp is a module relevant to the change, and chekins on it are;
> http://hg.mozilla.org/comm-central/log/36e05656d225/mailnews/local/src/nsMsgBrkMBoxStore.cpp
Module source which contains messageOffset are;
> http://mxr.mozilla.org/comm-central/search?string=messageoffset
String of "messageOffset" in nsMsgBrkMBoxStore.cpp of latest trunk is;
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMsgBrkMBoxStore.cpp#972

Comment 26

[Camaleon]

(In reply to WADA from comment #25)
 
> If you want to know actal(and entire) change by that bug, see changeset
> pointed by bug 402392 comment #87, please.
> > http://hg.mozilla.org/comm-central/rev/097bc36f180d
> nsMsgBrkMBoxStore.cpp is a module relevant to the change, and chekins on it
> are;
> > http://hg.mozilla.org/comm-central/log/36e05656d225/mailnews/local/src/nsMsgBrkMBoxStore.cpp
> Module source which contains messageOffset are;
> > http://mxr.mozilla.org/comm-central/search?string=messageoffset
> String of "messageOffset" in nsMsgBrkMBoxStore.cpp of latest trunk is;
> > http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMsgBrkMBoxStore.cpp#972

Thanks.

According to the comments for the mentioned code block (starting at line 951) I still fail to see a direct relation between the two things ("messageOffset" seems to be related for tracking/index the messages on a folder). Anyway, in the same file there's an interesting comment (line 196) which makes me think some work for overcoming the 4 GiB limit was done but the author is even doubtful about it.

Comment 27

[WADA:World Anti-bad-Duping Agency]

FYI.

Warning about "exceeding 4GB limit" on local mail folder was done by bug 387502 and bug 598104. This warning is required because "Tb's supported offset by 32bits integer" << "supported file size by file system".
"Removal of 4GB limit of IMAP offline-store file" was done by bug 532323.

Even if "Tb's supported offset by 64bits integer" >> "supported file size by file system", similar warning about "exceeding supported offset by 64bits integer" will be required in case that OS will support file size larger than max of 64bits integer, if Unix Mbox format will be continuously used without enhancement like "multiple Unix Mbox format files per a local mail folder".

Appropriate warning or error message about "exceeding file size limit of file system"(2GB if nfs like SMB) is needed if "multiple mails in Unix Mbox format file" will be continuously used.

"Pluggable mail store system by bug 402392" is a prerequisite of enhancement to "one file per a mail like qmail or maildir" or "SQLDB as mail store" which is needed to get rid of many restrictions/issues due to "multiple mails in a Unix Mbox format file".
Note:
Apple used "multiple mails in a Unix Mbox file" by Apple Mail Ver 1. But Apple already changed to "emlx==single mail in a Unix Mbox format file==one mail per a file" by Apple Mail Ver 2.

Comment 28

[Camaleon]

(In reply to WADA from comment #27)
> FYI.
> 
> Warning about "exceeding 4GB limit" on local mail folder was done by bug
> 387502 and bug 598104. This warning is required because "Tb's supported
> offset by 32bits integer" << "supported file size by file system".
> "Removal of 4GB limit of IMAP offline-store file" was done by bug 532323.

Thanks for the note. The warning is not what worries me but the fact of a real limitation when a folder has reached > 4 GiB file size which seems to be the key issue here.

Comment 29

[Kent James (:rkent)]

So do I understand this bug correctly that, even though previous patches claimed to enable support for > 4GB messages stores, that in fact it does not work even though the file system supports it?

Comment 30

[WADA:World Anti-bad-Duping Agency]

(In reply to Kent James (:rkent) from comment #29)
You are right. 
4GB(2GB) limit was removed in back-end, but file size larger than 0xFFC00000 == 4GB - 4MB is still prohibited on local mail folder file(not IMAP offline-store) in code added by bug 387502 and bug 598104. 

If Bug 462665 were for "removing size limit in back-end", this bug would be merely a followup request. But that bug is request of "removing file size limit in Tb" and has status of FIXED, this bug is apparently for fault in fixing Bug 462665 :-)

Comment 31

[Kent James (:rkent)]

In doing some unrelated work, I came across important changes that were landed in Mozilla 17:

Bug 215450 - uploading files that are larger the 2GB fails

This extends the available function in nsIInputStream from 32 bit to 64 bit, and may have been needed to support 4 GB files.

Comment 32

[Emre]

With win7 x64 and Thunderbird 16.02, I was trying to move all my 2012 mail into one single folder (sent & received) and the size of these mails are 5GB, Inbox 3.5 and sent 1.5. Once I moved the inbox, I can't anymore move the Sent folder messages into that folder, Thunderbird claims folder is full.

So I guess I hit the 4gb local folders mbox file limit? Is this fixed now in TB 17?

Comment 33

[Wayne Mery (:wsmwk)]

perhaps https://getsatisfaction.com/mozilla_messaging/topics/how_to_compress_or_compact_large_folders_10gb is related

Comment 34

[Emre]

I think >4gb works for IMAP accounts (ie gmail) but not for local folders. That is our problem (ie my local work archives can easily go over 8gb per year)

Comment 35

[Matt]

To quote David Bienvenu who did most of the work on these bugs, in an email regarding Compaction he said the following 

"Other than the extra disk space used, and the potential privacy issues of thinking deleted messages are gone from disk, the other problem that compaction fixes is the fact that we used to only allow folders of less than 4GB on disk, and couldn't add new messages to folders > 4GB. In TB 12, we got rid of the 4GB limit, so the main issues are wasted disk space and privacy."

Based on that and the fact he closed the original bug, there is nop doubt in my mind that he felt the issue was resolved and closed.

Comment 36

[Emre]

Well, we definitely have the 4gb limit on "local folders" still in TB 16.0.2.

Comment 37

[Vincent (caméléon)]

I was thinking that Thunderbird now splits the mbox file in two if its size goes over 4gb. Am I wrong???

Comment 38

[:aceman]

No, I didn't hear anything like that. You'd then see it as 2 separate folders in the folder pane.

Comment 39

[Wayne Mery (:wsmwk)]

(In reply to Vincent (caméléon) from comment #37)
> I was thinking that Thunderbird now splits the mbox file in two if its size
> goes over 4gb. Am I wrong???

yes, you are wrong :)


regarding "the folder cannot contain more messages, either compact or delete...", why is this different from other warning like "The folder Inbox is full..."? Is it occurring in different places in the code?

Comment 40

[Vincent (caméléon)]

(In reply to Wayne Mery (:wsmwk) from comment #39)
> (In reply to Vincent (caméléon) from comment #37)
> > I was thinking that Thunderbird now splits the mbox file in two if its size
> > goes over 4gb. Am I wrong???
> 
> yes, you are wrong :)

So the note in this support article should be edited: https://support.mozillamessaging.com/en-US/kb/compacting-folders#w_why-is-compaction-required
As I don't feel very comfortable on this topic, can someone else do this?

Comment 41

[:aceman]

There are still places in the code where it looks like the folder size is used as uint32_t, e.g.:
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#1135
http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#1279
http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#1402
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#1126

Can't this be a problem?

Bug 387502 and bug 598104 (disable 4GB) are fixed before bug 462665 (allow 4GB). So it seems 4Gb should be allowed now.

irving, will you look at this or should I try to look at the places I pointed out and let somebody adventurous test a try build with that patch? :)

Comment 42

[Domel]

In the newest TB 17, as well as all previous versions, it's still not possible to feed a regular mbox folder above 4 GB limit.

Comment 43

[ISHIKAWA, Chiaki]

(In reply to Domel from comment #42)
> In the newest TB 17, as well as all previous versions, it's still not
> possible to feed a regular mbox folder above 4 GB limit.

I *think* I hit the same bug and got curious and searched,found this entry.

I am using POP3 if this is important for this bug.

I was downloading a large number of e-mails for an account, each e-mail tends to have 100KB-3MB attachements (PDF for proofreading material). Since they came in rapid succession,
the folders grew rapidly.
The messages were filed into different folders based on the sender and some strings in the subjects.

During the course of downloading, TB complained that as in comment 1: "A pop-up windows appears instructing about the issue ("the folder cannot contain more messages, either compact or delete..."). The message cannot be moved to that folder." 
[sorry I didn't record the exact wording myself, but I believe the message is the same.]

There are two things which surprised me when this happened.

1. I checked the folder size shown in the folder pane.
The particular folder to which the e-mail message was supposed to go into showed
a very small size of mere "254KB" (!?) 

I think there is a bug in the printing code of the size. 
(Printing only the lower 32bits of 64 bits offset maybe. This is a separate bug.)

2. So I quit TB and checked the file size of the folder in question.

ls -l showed:
  -rw-------  1 ishikawa users 4295227305 2013-01-10 09:44 000assistant
  -rw-r--r--  1 ishikawa users    4147344 2013-01-10 10:00 000assistant.msf

There were other folders which were more than 3gb already.
I was pleasantly surprised to see that the size of 2GB of folder is now supported comfortably 
on Linux. (After reading the comments above in this bugzilla entry, now I am not so sure, though).

Note the size. I used "bc" under linux to calculate some numbers.
$ bc
bc 1.06.95
Copyright 1991-1994, 1997, 1998, 2000, 2004, 2006 Free Software Foundation, Inc.
This is free software with ABSOLUTELY NO WARRANTY.
For details type `warranty'. 
2^32
4294967296
f=4295227305
f - 2^32
260009
(f-2^32)/1024
253
quit
$

So I conclude that somehow the problematic folder became LARGER than 2^32 and
only then TB complained after the fact. 

I thought we had put in some advance  check for
2GB limit BEFORE the folder is modified (during TB2->TB3 transition period)
that would have warned us BEFORE the folder reaches 2GB so that removing/moving
from the folder still works. I wonder if the same is not done for handling 4GB limit.
is there no advance check for the size limit  before the limit is reached?

One other thing is that the file size is larger than 2^32 by 253 KB and this matches my earlier
observation that maybe the printing of the size is flawed.

I was thrilled when I saw this error, but somehow after compacting, I could continue downloading.
*HOWEVER*, the message(s) that would have been put into the problematic folder were not
found there. I got panicky thinking that the e-mails were forever lost.
After searching, I found that they were sitting in the Inbox. It seems that TB first
downloads the message into Inbox safely, and then runs the filter and tries to move
the message as appropriately. Since the move fails, the original messages were still in Inbox. 
Until I realized this, I was a little worried about losing the messages. 
This is a usablity and HMI issue. But I am not sure how to tell the user where to expect "possibly unmoved messages may be still in Inbox".
This is because I am not entirely sure what happens *if Inbox overflows first.
Would I lose the e-mails or is TB clever enough to stop the deletion of message from POP3 account. I hope the latter is the case.

In any case, I am reporting
 - the warning does get issued for 4GB limit.
 - the printing of size is flawed.
 - TB should stop moving files BEFORE the limit is reached.
   We should allow some room (some arbitrary fixed size) before TB refuses downloading
   a message into folder (See comment 30) even for POP3 and other situations.
   (OK, make it downloading or moving/copying instead of simple "downloading".)

TIA

PS: This is the latest TB 17.0.2 on linux (TB was upgraded to this just before
the above mentioned , and it happened 
under 32bit Debian GNU/Linux. 
Sorry I forgot to check which file system (ext3, or ext4) it uses, but I can find out
if necessary.

PPS: I just realized that I posted two years ago a similar but slight different 
experience (where TB kept on downloading although a folder reached a size limit)
Bug 634544
During download, when a mail folder reaches size limit, there is no way to stop download, and the folder can't compacted...

But since my experience yesterday showed TB stopped downloading somehow (or more exactly speaking moving the articles to the overflowing folder) and yesterday's experience is closely related to this 4GB limit, I will follow up with my checking of the code in this bugzilla entry.

Writing this comment helps me realize the cause of the slightly different manifestation of the bugs: there are different code paths for downloading and moving/copying the articles. to enter a new article into a folder: both of these need to have the advance check of size limit to avoid the type of problem mentioned here (not being able to delete/move any more after the limit is hit).

TIA

Comment 44

[irneb]

Have same issue. Generally I manually (or through filters) ensure my folders never exceed 2GB. But unfortunately some of them might still go that way (e.g. the All Mail folder of an IMAP GMail account).

Anyhow, the "bug" runs deeper than simply a warning and stopping from adding to the folder. If such adding has already occured and the folder exceeds the 4GB limit, there are at least 2 extremely worrying symptoms:

(1) The folder size is listed as the overflow of trying to indicate a > 4GB size in a unsigned 32bit integer. I.e. the actual size minus 4096GB (2^32).

(2) *MOST WORRYING OF ALL* The message count is also incorrect. It seems as if TB reads only up to the 4GB limit, then stops. So the client effectively only displays the messages up to that limit - the rest just disappears.

Comment 45

[WADA:World Anti-bad-Duping Agency]

4GB warning in nsMsgLocalMailFolder::CheckIfSpaceForCopy, which was by bug 598104, is already removed from any of Releace, Aurora, Central.
> http://mxr.mozilla.org/comm-release/source/mailnews/local/src/nsLocalMailFolder.cpp#1406
> http://mxr.mozilla.org/comm-aurora/source/mailnews/local/src/nsLocalMailFolder.cpp#1401
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#1401

And, nsMsgMaildirStore::HasSpaceAvailable does do nothing and always returns true. (This may be reason of dataloss when disk full)
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMsgMaildirStore.cpp#237

However, nsMsgBrkMBoxStore::HasSpaceAvailable still had code for 4GB check.
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMsgBrkMBoxStore.cpp#184
> 196   // ### I think we're allowing mailboxes > 4GB, so we should be checking
> 197   // for disk space here, not total file size.
> 198   // 0xFFC00000 = 4 GiB - 4 MiB.
> 199   *aResult = ((fileSize + aSpaceRequested) < 0xFFC00000);
> 200   return NS_OK;

(1) 4GB check in nsMsgBrkMBoxStore::HasSpaceAvailable should be removed.
(2) nsMsgMaildirStore::HasSpaceAvailable should check available disk space correctly.

Comment 46

[WADA:World Anti-bad-Duping Agency]

For space check and space shortage while downloading.
IIRC, bug report for next issue exists.
  When Berkley Mbox size exceeds 4GB limit while new mail　downloading,
  warning of this bug is issued and download of mail fails,
  and the error due to 4GB check occurs on any mail after first error,
  and there is no way to stop the attempts of downloading mails.
So, as far as proper disk space check is effective, "data loss due to disk space issue while downloading" won't occur.
If 4GB size check in nsMsgBrkMBoxStore::HasSpaceAvailable will be removed without logic correction of nsMsgMaildirStore::HasSpaceAvailable, probability of "data loss due to disk full while downloading" will increase.

Comment 47

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #41)
> There are still places in the code where it looks like the folder size is
> used as uint32_t, e.g.: (snip)
FYI. Bug 793865, Bug 794303 are example of problem due to it.

Comment 48

[:aceman]

So what is the plan here?

I think we should leave the 4GB warning in place for now and first fix the hasAvailableSpace functions as WADA found and fix the handling of folder size as int32 (GetSizeOnDisk). That may fix the problems of wrong display size of folders.

I wonder if we can create a GetInt64Property as I do not see any yet.

It seems there is still test in http://mxr.mozilla.org/comm-central/source/mailnews/local/test/unit/test_over4GBMailboxes.js to check if we get the 4GB warning. It also includes ways to check the free disk space so that can probably be used to fix the hasAvailableSpace functions.

Comment 49

[:aceman]

Attachment: fix hasSpaceAvailable

So this adds disk space check to the hasSpaceAvailable functions.

Comment 50

[:aceman]

Attachment: make GetSizeOnDisk 64 bit

This could help the display of the folder size.
In my tests I could not go over 4GB, but I've only tried to copy messages. Maybe filters or POP3 download do not consult hasSpaceAvailable properly.
So I need anybody who has a folder larger than 4GB to try out this patch. Please backup the folder and also a repair may be necessary to see the folder size correctly. Chiaki and WADA, can you try it? Also, do you use the Extra folder columns extension so that you see the folder size? I will try to make a folder bigger than 4GB from outside TB but I also need somebody who managed to do it inside TB.

Comment 51

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #50)
> Created attachment 725187 [details] [diff] [review]
> make GetSizeOnDisk 64 bit
> 
> This could help the display of the folder size.
> In my tests I could not go over 4GB, but I've only tried to copy messages.
> Maybe filters or POP3 download do not consult hasSpaceAvailable properly.
> So I need anybody who has a folder larger than 4GB to try out this patch.
> Please backup the folder and also a repair may be necessary to see the
> folder size correctly. Chiaki and WADA, can you try it? Also, do you use the
> Extra folder columns extension so that you see the folder size? I will try
> to make a folder bigger than 4GB from outside TB but I also need somebody
> who managed to do it inside TB.

I will try this over the weekend. I have been able to
use a local POP3 server to feed e-mails and so I can re-create the
condition rather easily (I HOPE(.

TIA

Comment 52

[WADA:World Anti-bad-Duping Agency]

Comment on attachment 725164 [details] [diff] [review]
fix hasSpaceAvailable

Review of attachment 725164 [details] [diff] [review]:
-----------------------------------------------------------------

nsMsgBrkMBoxStore.cpp
> +  *aResult = ((fileSize + aSpaceRequested) < 0xFFC00000) &&
> +             ((aSpaceRequested + 0x100000) < diskFree);
If other changes in disk space checking etc. is applied at same time, I think "((fileSize + aSpaceRequested) < 0xFFC00000)" part can be removed at same time.

Comment 53

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #50)
sary to see the
> Chiaki and WADA, can you try it?

If win32.zip tryserver build will be provided, I can test with "Extra Folder Column". I have Rexx script to generate Mbox file filled by many crafted mails of any size, of very long thread, althought it'll eat up my valuable 10GB free space of my HDD and CPU 100% for 10-to-15 minutes to generate 4GB Mbox... :-)

Comment 54

[:aceman]

(In reply to WADA from comment #52)
> nsMsgBrkMBoxStore.cpp
> > +  *aResult = ((fileSize + aSpaceRequested) < 0xFFC00000) &&
> > +             ((aSpaceRequested + 0x100000) < diskFree);
> If other changes in disk space checking etc. is applied at same time, I
> think "((fileSize + aSpaceRequested) < 0xFFC00000)" part can be removed at
> same time.

I want to keep this check (limit mbox to 4G - 4M) until other 32bit bugs are fixed (like yours from comment 47).

Comment 55

[:aceman]

Aryx, could you make us a try-build with the 2 patches here? Thanks.

Comment 56

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed to Thunderbird-Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=868900d3da0b

Comment 57

[ISHIKAWA, Chiaki]

I have created a sub 4GB mail folder (Inbox) using a local POP3 server (downloading the e-mails from this server after sending a series of e-mails with attachment many times).

env LANG=C ls -l /COMM-CENTRAL/TEST-MAIL-DIR/Mail/127.0.0.1
total 4190676
-rw-rwx--- 1 mtest2 ishikawa 4282409531 Mar 17 07:15 Inbox
-rw-rwxr-- 1 mtest2 ishikawa    3567433 Mar 17 09:47 Inbox.msf
-rw-rwx--- 1 mtest2 ishikawa    5247690 Mar 17 01:59 Sent
-rw-rwxr-- 1 mtest2 ishikawa       3170 Mar 17 02:07 Sent.msf
-rw-rwx--- 1 mtest2 ishikawa          0 Mar 17 01:38 Trash
-rw-rwxr-- 1 mtest2 ishikawa       1320 Mar 17 02:00 Trash.msf
-rw-rwxr-- 1 mtest2 ishikawa         25 Mar 17 01:38 msgFilterRules.dat
-rw-rwx--- 1 mtest2 ishikawa         61 Mar 17 09:49 popstate.dat

The file system on which this Mail directory resides is ext4 under 32-bit Debian GNU/Linux.
(I am testing 32 bit version.)

Because my system-wide mail spool in this development linux image is not that large, I had to send many small e-mails at a time and download it so as not to
overflow the mail spool. It took me overnight to create this.
(One run adds about  103592600 bytes to the mail folder. [This consists of 200 e-mails.]. I had to add "sleep 2" between each e-mail, and "sleep 60" between
one batch run so that TB has chance to download the e-mails from POP3 spool area
before the incoming test e-mails would fill up system root partition (/var/mail, and /var/spool/pop3. Hmm, testing TB is not easy is it?)

I am saving this mail folder and .msf files  for later experiments.

So far, no discernable problems. (So 2G+ mail folder is certainly working more or less.)

BTW, I do use Extra Column extension so that I can see the folder size.
With the file size shown above, the extension shows 4084 MB.

(BTW, we need to check three passes in TB where the folder size (precisely
 speaking mail filder implemented as single file)  may cause problems. 
  POP3 -> Inbox [I am testing THIS.]
  IMAP -> Inbox 
  Inbox -> other folders (filtering or manual)
  If I am not mistaken, I think the sending of e-mail and saving the sent
  mail into Sent may also be a different path. 
  Tough luck. )

Stay tuned.

Comment 58

[ISHIKAWA, Chiaki]

Here is the continuation of test, still work-in-progress.

(Without your patch)

This is the dialog I got when the mail folder size goes over 4GB.

The folder Inbox is full, and can't hold any more messages. To make
room for more messages, delete any old or unwanted mail and compact
the folder.

Then the extra column extension shows Inbox as having 8298 e-mails
(8284 unread) and size is "382KB" while actually it has the size shown
in the following listing.

total 4203388
-rw-rwx--- 1 mtest2 ishikawa 4295358606 Mar 17 10:23 Inbox
-rw-rwxr-- 1 mtest2 ishikawa    3634842 Mar 17 10:26 Inbox.msf
-rw-rwx--- 1 mtest2 ishikawa    5247690 Mar 17 01:59 Sent
-rw-rwxr-- 1 mtest2 ishikawa       3170 Mar 17 02:07 Sent.msf
-rw-rwx--- 1 mtest2 ishikawa          0 Mar 17 01:38 Trash
-rw-rwxr-- 1 mtest2 ishikawa       1320 Mar 17 02:00 Trash.msf
-rw-rwxr-- 1 mtest2 ishikawa         25 Mar 17 01:38 msgFilterRules.dat
-rw-rwx--- 1 mtest2 ishikawa         61 Mar 17 10:23 popstate.dat

Funny, I thought there was this advance check for 10MB room, but
obviously it did not kick in. (Either the check is not there, or
put in an incorrect place, or is now removed altogether?.)

This was without your patch.

Now with your patch.

[I created the binary using TryServer. Thank you for the earlier tips
regarding TryServer. But due to my linux-centric development
environment, it seems that I can't create win binary, etc. My files
have LF as end of the line terminator and not CRLF and this seems
to interfere with Win build(?). Anyway, you can obtain a copy of linux binary from
> Results will be displayed on TBPL as they come in:
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=e06da28d79ad
>
> Once completed, builds and logs will be available at:
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-e06da28d79ad]

I reverted the Inbox and its .msf file to the
sub-4GB status and repeated
the final addition of [200 e-mails] to go across 4GB size boundary.

This is again the "before" state:

  drwxrwxr-x 2 mtest2 ishikawa       4096  Mar 17 01:52 .
  drwxrwx--- 4 mtest2 ishikawa       4096  Mar 17 01:38 ..
  -rw-rwx--- 1 mtest2 ishikawa 4282409531  Mar 17 11:29 Inbox
  -rw-rwxr-- 1 mtest2 ishikawa    3610876  Mar 17 11:29 Inbox.msf
  -rw-rwx--- 1 mtest2 ishikawa    5247690  Mar 17 11:29 Sent
  -rw-rwxr-- 1 mtest2 ishikawa       3170  Mar 17 11:29 Sent.msf
  -rw-rwx--- 1 mtest2 ishikawa          0  Mar 17 11:29 Trash
  -rw-rwxr-- 1 mtest2 ishikawa       1320  Mar 17 11:29 Trash.msf
  -rw-rwxr-- 1 mtest2 ishikawa         25  Mar 17 11:29 msgFilterRules.dat
  -rw-rwx--- 1 mtest2 ishikawa         61  Mar 17 11:29 popstate.dat

I am using linux 32 build with your patch:

For reasons I don't quite understand (maybe because of the copying
from the backup, Inbox had a newer timestamp than Inbox.msf?), the new
TB tried to re-create .msf and it took a long time for 4GB mail
folder.

Once it is done, it shows again 8259, 8273, 4084MB.

TEST: 
I injected e-mails to this account from a different window.

After downloading some e-mails, TB showed 4098MB
as Inbox size (so the display seems to be correct now.)

However, I didn't see the automatic downloading of the next minute and
got anxious when I see the following dialog.

"The folder Inbox is full, and can't hold any more messages. To make
room for more messages, delete any old or unwanted mail and compact
the folder." [OK]

(Well, I though ext4 supports file system size> 4GB,
but maybe the individual file can't go over this size?)

At this stage, the file size has become this way.:

  /COMM-CENTRAL/TEST-MAIL-DIR/Mail/127.0.0.1:
  合計 4204872
  drwxrwxr-x 2 mtest2 ishikawa       4096  Mar 17 11:33 .
  drwxrwx--- 4 mtest2 ishikawa       4096  Mar 17 01:38 ..
  -rw-rwx--- 1 mtest2 ishikawa 4296912495  Mar 17 11:41 Inbox
  -rw-r--r-- 1 mtest2 mtest2      3594866  Mar 17 11:45 Inbox.msf
  -rw-rwx--- 1 mtest2 ishikawa    5247690  Mar 17 11:29 Sent
  -rw-rwxr-- 1 mtest2 ishikawa       3170  Mar 17 11:29 Sent.msf
  -rw-rwx--- 1 mtest2 ishikawa          0  Mar 17 11:29 

According the caluculation of bc
m=2^32
4296912495 - m
1945199

So the Inbox folder file size is larger than 2^32 by 1945199 bytes.
(So ext4 does seem to support > 4GB size. Or does the kernel and driver need to be compiled with special flag? 
The following URL suggests that ext4 can support a file larger than 2Ti !
https://ext4.wiki.kernel.org/index.php/Frequently_Asked_Questions#History_of_ext2.2C_ext3.2C_and_ext4
)

Anyway, with this Inbox I did some tests:

Test 1: I tried to copy one e-mail from another folder
(Sent).

I get the same warning. Good.

"The folder Inbox is full, and can't hold any more messages. To make
room for more messages, delete any old or unwanted mail and compact
the folder." [OK]

So I cannot copy something to this full Inbox.
(At least, I will not go into more problems.)

Test 2: I tried to delete one e-mail from Inbox to Trash.
(I chose the last one)

*SOME FISHY behavior* : Instead of the last e-mail, I am afraid
an e-mail article near the beginning of the mail folder (at least the
subject line suggests so) is now moved to Trash(!?)
I think the article was somewhere near the 20th position of the Inbox.
(I injected some smallish test e-mails when I started testing, so
I can not co-relate the mail article position easily with the position of the
deleted last article. The later
articles have exactly the same size [modulo the difference of system
generated header lines, etc.] .)

Test 2-a: I tried to delete the now shown last e-mail article from
Inbox to Trash. 

This time, the deleted article seems to correctly show up in Trash.

Test 2-b: I tried to delete the now shown last e-mail article from
Inbox to Trash one more time. This time again, the deleted article seems to
correctly show up in Trash.

So now trash has three articles. One from near the beginning of Inbox
and two from the end... OK, where did the last one (I deleted first) go ?

I used some numbers (sometimes repeated for each batch) in the subject
line for easy identification. Currently the subject list shows 3521,
3522, 3533 in the inbox pane at the end.  Trash showed "test attachment", 3524,
3525 (Instead of "test attachment" article from near the beginning of
Inbox, I thought I would have 3526 in the Trash when I deleted the
"last" one for the first time. Now I will compactify Inbox and see what 
happens. I suspect article with subject 3526 may show up again Inbox !)
 
But I hit a snug.

I forgot that I don't have that much space left in the file system and
I got: 

"The folder 'Inbox' could not be compacted because writing to
folder failed. Verify that you have enough disk space, and that you
have write privileges to the file system, then try again.compact " 

Well, getting this warning is good. [Without proper checking I lost 
some mails, 5 or 6 years ago. :-( ]

Here is where I am now. I will create more disk space by attaching
virtual disk image (I am testing using a vm image) and 
I will check what happens after compaction, and then plan to test
another scenario:

Each e-mail with attachment is abut 600KB, and so I will delete the last
10 e-mails from Inbox and compactified to see what happens.

I may be able to finish this later today, but may not finish it over
the weekend after all.

TIA

Comment 59

[WADA:World Anti-bad-Duping Agency]

(In reply to ISHIKAWA, chiaki from comment #58)
> The folder Inbox is full, and can't hold any more messages. To make
> room for more messages, delete any old or unwanted mail and compact
> the folder.

I also could observe the error dialog in Tb 17.0.3 on MS Win.
1. Mbox file size shown by Extra Folder column = 4080MB
2. Copy [ 10000 * 1KB(1024bytes) mails == 10MB ] mails to Mbox
   ( actual size on HDD == 1,024,000 bytes)
   => Copy normally ends 
   => Mbox file size shown by Extra Folder column = 4090MB
      Actual : 4294279466 bytes == 3 GB + 1023 MB + 352 KB + 298 Bytes
3. Copy [ 10000 * 1KB(1024bytes) mails == 10MB ] mails to Mbox again
   => Above error message was shown

This message is;
> http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messenger.properties#81
> 81 mailboxTooLarge=The folder %S is full, and can't hold any more messages. To make room for more messages, delete any old or unwanted mail and compact the folder.

Comment 60

[WADA:World Anti-bad-Duping Agency]

(A) Change by  "fix bug 402392, add support for pluggable stores".

"Removal of 4GB warning from nsMsgLocalMailFolder::CheckIfSpaceForCopy" was done by "fix bug 402392, add support for pluggable stores".
> http://hg.mozilla.org/comm-central/rev/097bc36f180d
> http://hg.mozilla.org/comm-central/diff/097bc36f180d/mailnews/local/src/nsLocalMailFolder.cpp
> http://hg.mozilla.org/comm-central/diff/097bc36f180d/mailnews/local/src/nsLocalMailFolder.cpp

Change of nsMsgMaildirStore.cpp was "new file mode 100644".
> http://hg.mozilla.org/comm-central/diff/097bc36f180d/mailnews/local/src/nsMsgMaildirStore.cpp
So, I couldn't know "when following was done" in nsMsgMaildirStore.cpp.
> 237 NS_IMETHODIMP nsMsgMaildirStore::HasSpaceAvailable(nsIMsgFolder *aFolder,
> 241   // This should probably check disk space available, though the caller
> 242   // might be doing that as well.
> 243   NS_ENSURE_ARG_POINTER(aResult);
> 244   *aResult = true;
> 245   return NS_OK;


(B) Currnt error message.
This message is;
> http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messenger.properties#81
> 81 mailboxTooLarge=The folder %S is full, and can't hold any more messages. To make room for more messages, delete any old or unwanted mail and compact the folder.

Because HasSpaceAvailable is called via "msgStore->HasSpaceAvailable" by David in the patch, it may be that nsMsgBrkMBoxStore::HasSpaceAvailable in nsMsgBrkMBoxStore.cpp is not called.
Or, nsMsgBrkMBoxStore::HasSpaceAvailable is finally called by following code.

POP3:
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Service.cpp#98
>  77 nsresult nsPop3Service::GetMail(bool downloadNewMail,
>  98     destLocalFolder->WarnIfLocalFileTooBig(aMsgWindow, &destFolderTooBig);
>  99     if (destFolderTooBig)
> 100       return NS_MSG_ERROR_WRITING_MAIL_FOLDER;

Message Copy/Move
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#2493
> 2459 nsresult nsParseNewMailState::MoveIncorporatedMessage(nsIMsgDBHdr *mailHdr,
> 2493     destLocalFolder->WarnIfLocalFileTooBig(msgWindow, &destFolderTooBig);

> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#3579
> 3579 nsMsgLocalMailFolder::WarnIfLocalFileTooBig(nsIMsgWindow *aWindow, bool *aTooBig)
> 3580 {
> 3581 NS_ENSURE_ARG_POINTER(aTooBig);
> 3582 *aTooBig = false;
> 3583 nsCOMPtr<nsIMsgPluggableStore> msgStore;
> 3584 nsresult rv = GetMsgStore(getter_AddRefs(msgStore));
> 3585 NS_ENSURE_SUCCESS(rv, rv);
> 3586 bool spaceAvailable;
> 3587 // check if we have a reasonable amount of space left
> 3588 rv = msgStore->HasSpaceAvailable(this, 0xFFFFF, &spaceAvailable);
> 3589 if (!spaceAvailable)
> 3590 {
> 3591 ThrowAlertMsg("mailboxTooLarge", aWindow);
> 3592 *aTooBig = true;
> 3593 }
> 3594 return NS_OK;

Which HasSpaceAvailable is actually used?
nsMsgMaildirStore::HasSpaceAvailable in nsMsgMaildirStore.cpp?
nsMsgBrkMBoxStore::HasSpaceAvailable in nsMsgBrkMBoxStore.cpp?
Or different one?

Comment 61

[WADA:World Anti-bad-Duping Agency]

Aha!, msgStore was obtained by following code.
> 3583 nsCOMPtr<nsIMsgPluggableStore> msgStore;
> 3584 nsresult rv = GetMsgStore(getter_AddRefs(msgStore));
nsMsgBrkMBoxStore::HasSpaceAvailable is perhaps called when Berkley Mbox after PluggableStore support.

Comment 62

[[e-mail]]

I am concerned that the deletion of the wrong message behaviour is because the message keys used to identify messages are 32-bit and for local folders are the actual 32-bit position of the message within the mbox file. By comparison I believe the keys for IMAP are simply the server's UIDs although I am not certain as to how the offline store is maintained.

Comment 63

[:aceman]

Thanks for your experiments guys!
For reference, 4GB is 4096MB but the check on mbox size should only allow to grow up to 4092MB.

Chiaki, it seems you have low disk space in the VM, could you please also test the new feature of the "folder is full" dialog coming up when the Inbox fills your disk even before the Inbox size is 4GB large. The "inbox full" should show if 1) the new message would grow the inbox above 4GB-4M 2) or if the free disk space after the new message would become less than 1MB (we can discuss if this is not too low for modern OSes). I have checked this to work on Linux, but it would be good if you doublechecked it on Windows too.


(In reply to ISHIKAWA, chiaki from comment #58)
> For reasons I don't quite understand (maybe because of the copying
> from the backup, Inbox had a newer timestamp than Inbox.msf?), the new
> TB tried to re-create .msf and it took a long time for 4GB mail
> folder.
Yes, you need to take both Inbox and Inbox.msg from backup so they are in the same state and timestamp.

> After downloading some e-mails, TB showed 4098MB
> as Inbox size (so the display seems to be correct now.)
Great!

> However, I didn't see the automatic downloading of the next minute and
> got anxious when I see the following dialog.
> 
> "The folder Inbox is full, and can't hold any more messages. To make
> room for more messages, delete any old or unwanted mail and compact
> the folder." [OK]
> 
> (Well, I though ext4 supports file system size> 4GB,
> but maybe the individual file can't go over this size?)
I intentionally did not remove the limit of 4GB-4MB for mbox. So it seems you still managed to grow above it when downloading new msgs (not copying). We need to find why this is happening. Probably the download code does not call hasSpaceAvailable.

> Anyway, with this Inbox I did some tests:
> 
> Test 1: I tried to copy one e-mail from another folder
> (Sent).
> 
> I get the same warning. Good.
> 
> "The folder Inbox is full, and can't hold any more messages. To make
> room for more messages, delete any old or unwanted mail and compact
> the folder." [OK]
> So I cannot copy something to this full Inbox.
> (At least, I will not go into more problems.)
Good!

> 
> Test 2: I tried to delete one e-mail from Inbox to Trash.
> (I chose the last one)
> 
> *SOME FISHY behavior* : Instead of the last e-mail, I am afraid
> an e-mail article near the beginning of the mail folder (at least the
> subject line suggests so) is now moved to Trash(!?)
> I think the article was somewhere near the 20th position of the Inbox.
> (I injected some smallish test e-mails when I started testing, so
> I can not co-relate the mail article position easily with the position of the
> deleted last article. The later
> articles have exactly the same size [modulo the difference of system
> generated header lines, etc.] .)
May be bug 793865. That is why I do not lift the 4GB limit for now.

> Test 2-a: I tried to delete the now shown last e-mail article from
> Inbox to Trash. 
> 
> This time, the deleted article seems to correctly show up in Trash.
> 
> Test 2-b: I tried to delete the now shown last e-mail article from
> Inbox to Trash one more time. This time again, the deleted article seems to
> correctly show up in Trash.
Good.

(In reply to WADA from comment #61)
> Aha!, msgStore was obtained by following code.
> > 3583 nsCOMPtr<nsIMsgPluggableStore> msgStore;
> > 3584 nsresult rv = GetMsgStore(getter_AddRefs(msgStore));
> nsMsgBrkMBoxStore::HasSpaceAvailable is perhaps called when Berkley Mbox
> after PluggableStore support.
I think you all use nsMsgBrkMBoxStore::HasSpaceAvailable. You haven't mentioned anywhere that you had manually migrated your account to the maildir-lite message store.

(In reply to [e-mail] from comment #62)
> I am concerned that the deletion of the wrong message behaviour is because
> the message keys used to identify messages are 32-bit and for local folders
> are the actual 32-bit position of the message within the mbox file.
May be bug 793865, so I do not lift the 4GB limit. But it seems we do not check it consistently and sometimes grow above it.

Comment 64

[ISHIKAWA, Chiaki]

I think we are gathering a lot of useful information.

*Still todo: test the low disk condition. (Maybe this coming week.)
*      I think window version testing is done by WADA san, not me :-).

OK, I added a disk and set environment variable TMP, TMPDIR to a
larger file system that can hold 4+ GB temporary file.

To recap, my Inbox is 4GB+ and I could not download or copy articles to it.

Now on the next startup, TB (with your patch) displays:

"The folder Inbox is full, and can't hold any more messages. To make
room for more messages, delete any old or unwanted mail and compact
the folder."

The main inbox pane shows the subject numbers (of my choice)
3521
3522
3523

Sizes shown by extra columns are 8275, 8299 4098MB.

Trash shows, ?, it is empty?!

(Oh, I found the newly copied thunderbird in this test account sets
the new profile, and in it, the setting "Trash folder is emptied upon
exit" is checked. Hmm... I hate clever setting that gets in the way of
debugging.)

OK, let me compact Inbox anyway. After a couple of minutes.
Grrr... It does not change at all.
The numbers shown in the extra columns are:
8275, 8299, 4098MB.

Strange that the size didn't decrease. I tried to download more e-mails to see what happens. When I clicked "Get Mail", I got, again, 
"The folder Inbox is full, and can't hold any more messages. To make
room for more messages, delete any old or unwanted mail and compact
the folder."

So I am afraid that I got stuck in this state...

The size of files are: not different from before.

  -rw-rwx--- 1 mtest2 ishikawa 4296912495  Mar 17 11:55 Inbox
  -rw-r--r-- 1 mtest2 mtest2      3728094  Mar 17 21:39 Inbox.msf
  -rw-rwx--- 1 mtest2 ishikawa    5247690  Mar 17 11:51 Sent
  -rw-r--r-- 1 mtest2 mtest2         4964  Mar 17 20:56 Sent.msf
  -rw------- 1 mtest2 mtest2            0  Mar 17 12:02 Trash
  -rw-r--r-- 1 mtest2 mtest2         2649  Mar 17 20:56 Trash.msf
  -rw-rwxr-- 1 mtest2 ishikawa         25  Mar 17 11:29 msgFilterRules.dat
  -rw-rwx--- 1 mtest2 ishikawa         61  Mar 17 11:41 popstate.dat


So the COMPACTion failed! (!!)
OK, why the compact failed?

I saw the following messages dumped in the console terminal :

--DOMWINDOW == 33 (0xa89c420) [serial = 41] [outer = (nil)] [url = about:blank]
WARNING: temp file not of expected size in compact: 'tempFileRightSize', file ../../../../mailnews/base/src/nsMsgFolderCompactor.cpp, line 432
WARNING: compact failed: 'msfRenameSucceeded', file ../../../../mailnews/base/src/nsMsgFolderCompactor.cpp, line 488
++DOMWINDOW == 34 (0x96c18190) [serial = 44] [outer = 0xa90b890]

Line 432 of nsMsgFolderCompactor.cpp: 

  if (NS_SUCCEEDED(rv))
    rv = cloneFile->GetFileSize(&fileSize);
  bool tempFileRightSize = (fileSize == m_totalMsgSize);
* NS_WARN_IF_FALSE(tempFileRightSize, "temp file not of expected size in compact");

So (filesize != m_totalMsgSize) somehow. But why?

I looked for m_totalMsgSize:

grep -n  -e m_totalMsgSize *.[chp]* /dev/null
nsMsgFolderCompactor.cpp:272:  m_totalMsgSize = 0;
nsMsgFolderCompactor.cpp:431:  bool tempFileRightSize = (fileSize == m_totalMsgSize);
nsMsgFolderCompactor.cpp:1156:    m_totalMsgSize += msgSize;
nsMsgFolderCompactor.h:65:  uint32_t m_totalMsgSize;  <=== AGAH!!!

OK, it seems that we missed increasing the size of m_totalMsgSize to uint64_t?

That is all for today.

(PS: I am running a build with a fix for m_totalMsgSize now on tryserver. Not sure if the simple redeclaration in the nsMsgFolderCompactor.h is enough.)

Comment 65

[:aceman]

Chiaki, thanks for the find. Compaction problem is bug 794303. Would you like to take it?

Comment 66

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #65)
> Chiaki, thanks for the find. Compaction problem is bug 794303. Would you
> like to take it?

I will. Changed the assigned field of bug 794303.

TIA

Comment 67

[:aceman]

(In reply to ISHIKAWA, chiaki from comment #58)
> Funny, I thought there was this advance check for 10MB room, but
> obviously it did not kick in. (Either the check is not there, or
> put in an incorrect place, or is now removed altogether?.)

Seems this can be bug 640371 so we could move there to not clutter the bug here any more.

Comment 68

[WADA:World Anti-bad-Duping Agency]

FYI. Size check by POP3 or Copy/Move is done via WarnIfLocalFileTooBig.

(i) Upon both POP3 download and Copy/Move to Berkley Mbox, 
    nsMsgLocalMailFolder::WarnIfLocalFileTooBig is called,
    and WarnIfLocalFileTooBig requests HasSpaceAvailable.
> msgStore->HasSpaceAvailable(this, 0xFFFFF, &spaceAvailable);
> (FFFFF=1024**2=1MB)
(ii) nsMsgBrkMBoxStore::HasSpaceAvailable does following file size check only, without checking actual free space of HDD.   
> *aResult = ((fileSize + aSpaceRequested) < 0xFFC00000);
>    Checks ( Current file size + 1MB ) < (4GB -4MB)
(iii-A) If current file size < 4GB - 4MB - 1MB, true is returned.
        So, if Disk Full condision occurs in following Copy/Move,
        data loss may occur if POP3.
(iii-B) If current file size >= 4GB - 4MB - 1MB, false is returned,
        then "The folder %S is full" is shown.
        In this case, "mail dala loss in mail store due to disk full"
        won't occur, unless Compact will corrupt mail store.
> 81 mailboxTooLarge=The folder %S is full, and can't hold any more messages. To make room for more messages, delete any old or unwanted mail and compact the folder.

This in Berkley Mbox will be resolved by aceman's patch followed by removel of "4GB-4MB" check.

When non-Berkley(maildir), file size check is not needed, because 'one mail per a file' and mail size is limited by 32bits integer(2GB or 4GB).
However, even if non Berkley, check of 'free disk space size' is mandatory. nsMsgMaildirStore::HasSpaceAvailable in nsMsgMaildirStore.cpp should be re-coded correctly too.

Comment 69

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #63)
> I intentionally did not remove the limit of 4GB-4MB for mbox. So it seems
> you still managed to grow above it when downloading new msgs (not copying).
> We need to find why this is happening. Probably the download code does not
> call hasSpaceAvailable.

If current file size < 4GB - 4MB - 1MB, hasSpaceAvailable returns true, then POP3 download code appends mail data to Berkley mail store, thus file size will exceed 4GB if downloaded mail size is larger than 5MB.
However, offset(MsgKey) is kept within 32bits integer by "4GB - 4MB" check in hasSpaceAvailable, and, because any data can not be appended to mail store any more by "4GB - 4MB" check, critical problem like data loss won't occur. This is the reason why the "4GB - 4MB" check was introduced.
Even after the "4GB - 4MB" check, problem of "Sent larger than 4GB" occured, because "Copy to Sent after mail send" is different from Copy/Move and POP3. This problem was perhaps resolved by calling hasSpaceAvailable. So, "sent mail larger than 5MB" may produce "file size larger than 4GB" too.

Comment 70

[WADA:World Anti-bad-Duping Agency]

(In reply to [e-mail] from comment #62)
> I am concerned that the deletion of the wrong message behaviour is because
> the message keys used to identify messages are 32-bit and for local folders
> are the actual 32-bit position of the message within the mbox file.
> By comparison I believe the keys for IMAP are simply the server's UIDs although
> I am not certain as to how the offline store is maintained.

By change for Pluggable message store support, Offset in message tore==Berkley Mbox(64bits integer) and MsgKey(unique identifier of message, also 64bits integer) is cleanly separated.
This is similar to IMAP offline-store. In IMAP, MsgKey is UID of mail, and Offset is 64bits integer of offset of mail data in offline-store file.

In almost all places, MsgKey==Offset in Berkley Mbox file. However, MsgKey!=Offset occurs in followig situation.
  When  multiple mails are consecutively moved to a FolderX by message
  filer upon mail download,
    moved mail #1   : Offset #1 = File Size before append mail data
                      MsgKey #1 = File Size before append mail data 
    moved mail #2   : Offset #2 = Offset #1 + Mail size #1,
                      MsgKey #2 = MsgKey #1 + 1
                |
    moved mail #N   : Offset #N = Offset #(N-1) + Mail size #(N-1),
                      MsgKey #N = MsgKey #(N-1) + 1
  "Move to local" only phenomenon. If Filter Copy, MsgKey==Offset.
  Quarantine option may be relevant. 

Note-1:
Uniqueness of MsgKey is always kept, because Offset is always unieque.
In ordinal Copy/Move, download to Inbox without filter move to local mail folder, MsgKey==Offset is always kept.
Even when filter move upon download, if number of moved mail is one, MsgKey==Offset is kept.
When MsgKey!=Offset, it is change to MsgKey==Offset by Compact, Copy/Move.

Note-2:
Even when MsgKey!=Offset, message is accessed via the unieque MsgKey, and read/write of mail data is done via MsgKey=>Offset conversion, so, "wrong mail delete" can't occur, as far as correctness of MsgKey=>Offset conversion is kept and correctness of message size is kept, unless someone copies Offset value to 32bits integer and uses it when offset of a mail is greater than 4GB.

Comment 71

[WADA:World Anti-bad-Duping Agency]

Currently, following problems are known(set in Blocks: of this bug). 
  Bug 634544 : Unable to stop POP3 download when reached at "4GB-4MB"
  Bug 640371 : File size can exceed 4Gb by POP3 mail download,
               even when "4GB - 4MB - 1MB" warning is effective
  Bug 794303 : Compact fails, even when file size is less than 4GB
These may be simple "32bit integer" problem, but different problem may be involved. And, if POP3 download is relevant, required disk space calculation, filter move, quarantine option, etc. may be relevant. Further, care for "Sent mail copy" is also needed. 

To remove "4GB - 4MB" check in HasSpaceAvailable by this bug, resolving of all above problems is needed.

Comment 72

[:aceman]

Yes, good summary WADA.

What the patches here do for now is this:
- In addition to 4GB-4MB check, also check the free disk space in hasSpaceSvailable to prevent dataloss (saving incomplete message or something). The disk space check is also done for maildir.
- If it happens that folder grows above 4GB show the size properly (via Extra folder columns) and folder -> properties. To avoid user panic that the whole folder is lost as the size shrunk to some small size (size minus 4GB). This work is also needed for future removal of the 4GB limit.

Yes, none of the patches/bugs intend to remove the 4GB (32bit) limit on single message size. But that seems fine for the foreseeable future.

Comment 73

[:aceman]

Anybody wanting to test a build with the patches here can find them here:
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-868900d3da0b/

Those are the ones produced by Aryx in comment 56.

Comment 74

[:aceman]

Attachment: fix hasSpaceAvailable v2

Please see also patch in bug 640371 to see where these code comments come from.

Comment 75

[:aceman]

Comment on attachment 726902 [details] [diff] [review]
fix hasSpaceAvailable v2

Sorry, this is broken.

Comment 76

[:aceman]

Attachment: fix hasSpaceAvailable v3

Please see also patch in bug 640371 to see where these code comments come from.

Comment 77

[:aceman]

If the changes here are deemed OK, I can also convert expungedBytes to 64bit as somebody may need to clear whole >4GB folder and it may fail with 32bit expungedBytes variable.

WADA, nsMsgKey is still uint32_t (unsigned long) at http://mxr.mozilla.org/comm-central/source/mailnews/base/public/MailNewsTypes2.idl#6 . Where do you see it is 64bit?
It is just not used as the message position in the mbox file if the value is nearing 4GB.

Comment 78

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #77)

IIRC, I didn't say that messageKey was 64bits...
As seen in following(copy of bug 640371 comment #22), when messageOffset is near 4GB value(larger than 4GB-256), messageOffset is not used and next msgKey is assigned by Mork, and it was "highest msgKey(==highest offset < 4GB-256) + 1" in test of bug 640371.
> http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgFolderCompactor.cpp#1116
> 1116 nsFolderCompactState::EndCopy(nsISupports *url, nsresult aStatus)
> 1133     // if mbox is close to 4GB, auto-assign the msg key.
> 1134     nsMsgKey key = m_startOfNewMsg > 0xFFFFFF00 ? nsMsgKey_None : (nsMsgKey) m_startOfNewMsg;
> 1135     m_db->CopyHdrFromExistingHdr(key, m_curSrcHdr, true, getter_AddRefs(newMsgHdr));
> http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#3445
> 3445 NS_IMETHODIMP nsMsgDatabase::CopyHdrFromExistingHdr(nsMsgKey key, nsIMsgDBHdr *existingHdr, bool addHdrToDB, nsIMsgDBHdr **newHdr)
> 3453     CreateNewHdr(key, (nsIMsgDBHdr **) &destMsgHdr);
> http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#3351
> 3351 NS_IMETHODIMP nsMsgDatabase::CreateNewHdr(nsMsgKey key, nsIMsgDBHdr **pnewHdr)
> 3371     // Mork will assign an ID to the new row, generally the next available ID.
> 3372     err  = m_mdbStore->NewRow(GetEnv(), m_hdrRowScopeToken, &hdrRow);

I didn't test "256 or more 7MB mail download at once when 4GB-4MB-1MB <I nbox file size" case yet, but I think Mork will assign free/non-used-yet key.
Wraparoud in messageKey(but never duplicated, uniequeness is always kept) is not problem, except that it can't be called "Order Received" any more even when no message is copied to an local Berkley Mbox file.

Comment 79

[WADA:World Anti-bad-Duping Agency]

Following may be a practical mid range solution for removing 4GB-4MB-1MB size limit, because Mork looks to ordially assign highest+1.
> 1134     nsMsgKey key = m_startOfNewMsg > 0xFFFFFF00 ==> 0x8FFFFFFF

Comment 80

[:aceman]

(In reply to WADA from comment #70)
> By change for Pluggable message store support, Offset in message
> tore==Berkley Mbox(64bits integer) and MsgKey(unique identifier of message,
> also 64bits integer) is cleanly separated.

I was referring to this comment of yours.

(In reply to WADA from comment #79)
> Following may be a practical mid range solution for removing 4GB-4MB-1MB
> size limit, because Mork looks to ordially assign highest+1.
> > 1134     nsMsgKey key = m_startOfNewMsg > 0xFFFFFF00 ==> 0x8FFFFFFF

This could be good. Because what if there are only small messages that fill the mbox until 4GB - 256 file size. The next message added gets message key of 4GB - 256 + 1. That means there is only space for new 256 messages when that happens. And that is bad.

Comment 81

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #80)
> I was referring to this comment of yours.

Sorry, my misunderstood, mistake.

How about next?
> 1134     nsMsgKey key = m_startOfNewMsg >= 0x00000000
Because messageKey and messageOffset is already cleanly separated, I think there is no need to use Offset value as messageKey even in Berkley Mbox file.

Comment 82

[:aceman]

(In reply to WADA from comment #81)
> > 1134     nsMsgKey key = m_startOfNewMsg >= 0x00000000
> Because messageKey and messageOffset is already cleanly separated, I think
> there is no need to use Offset value as messageKey even in Berkley Mbox file.

So making message key just be an increasing counter even for low offsets?
That looks nice, but I am not sure that is possible to do (or for existing mboxes).

Comment 84

[WADA:World Anti-bad-Duping Agency]

Removing my small protest against "closing bug of 'Remove the 4GB(MS Win)/2GB(*nix inxluding Mac OS X) mailbox limit' as FIXED" from this bug's summary, accordig to bug summary change of bug 462665.
Sorry for bug summary change.

Comment 85

[WADA:World Anti-bad-Duping Agency]

FYI.
As known by Process Monitor log of Compact in bug 794303 comment #15, current Compact is continuous "read into 8KB buffer" + "write from 4KB buffer". If bigger Berkley Mbox file than current will be officially supported by resolvig this bug, issue of bug 558528 should be resolved.

Comment 86

[:aceman]

Attachment: make GetSizeOnDisk 64 bit v2

Fixes bitrot.

Comment 87

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 726938 [details] [diff] [review]
fix hasSpaceAvailable v3

Review of attachment 726938 [details] [diff] [review]:
-----------------------------------------------------------------

Can you fix the non-virtual destructor warning on nsMsgBrkMBoxStore and nsMsgMaildirStore while you're in there?

::: mailnews/local/src/nsMsgLocalStoreUtils.cpp
@@ +309,5 @@
> +#ifdef DEBUG
> +    printf("Call to GetDiskSpaceAvailable FAILED! \n");
> +#endif
> +    return ((aSpaceRequested + EXTRA_SAFETY_SPACE) < diskFree);
> +  }

Logic here doesn't look right - we're doing the comparison with diskFree on the NS_FAILED side of the if statement, but diskFree shouldn't be valid if GetDiskSpaceAvailable() failed.

::: mailnews/local/src/nsMsgLocalStoreUtils.h
@@ +41,5 @@
>    nsresult UpdateFolderFlag(nsIMsgDBHdr *mailHdr, bool bSet,
>                              nsMsgMessageFlagType flag,
>                              nsIOutputStream *fileStream);
> +  bool DiskSpaceAvailableInStore(nsIFile *aFile,
> +                                 int64_t aSpaceRequested);

I'd be inclined to use size_t (which is uint64_t) for the space requested, though that forces an IDL change for HasSpaceAvailable() to be consistent. It's not like we'll ever overflow a 64-bit signed field here, so it's OK to leave it as is.

Comment 88

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 731455 [details] [diff] [review]
make GetSizeOnDisk 64 bit v2

Review of attachment 731455 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. A lot of this is in RDF code, which really should be ripped out, but converting everything to uint64 is valuable. r? me again after you change GetFolderSizeNode().

::: mailnews/base/src/nsMsgFolderDataSource.cpp
@@ +1453,5 @@
>    bool isServer;
>    nsresult rv = folder->GetIsServer(&isServer);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  int64_t folderSize;

uint64_t, and then you won't need the cast on calling GetSizeOnDisk. You will need to convert GetFolderSizeNode() to uint64_t...

Comment 89

[:aceman]

Attachment: fix hasSpaceAvailable v4

OK, fixed the issues.

Comment 90

[:aceman]

Attachment: make GetSizeOnDisk 64 bit v3

Fixed nits. So I propose to change OnFolderSizePropertyChanged to 64bit and use OnItemLongPropertyChanged (create it or convert OnItemIntPropertyChanged to 64bit) in next patch. Something similar is also attempted in bug 813459.
But we actually do not enable 32bit+ values to flow across these functions (hasSpaceAvailable observes 4GB limit) so we could apply this patch as is.

Comment 91

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 732996 [details] [diff] [review]
fix hasSpaceAvailable v4

Review of attachment 732996 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/local/src/nsMsgBrkMBoxStore.cpp
@@ +198,5 @@
> +
> +  // TODO: We are working on allowing mboxes > 4GB, but it is not ready yet.
> +  // So allow the mbox to only reach 0xFFC00000 = 4 GiB - 4 MiB for now.
> +  *aResult = // ((fileSize + aSpaceRequested) < 0xFFC00000) &&
> +              DiskSpaceAvailableInStore(pathFile, aSpaceRequested);

Did you mean to leave the <4GB test commented out in this patch? If so, I'd prefer that you duplicate the whole statement and leave one version commented out, rather than having the comment start in the middle of the statement. That is,

// TODO: blah blah
// *aResult = test 1 &&
//            test 2;
*aResult = test 2;

and make sure that some bug (possibly this one) tracks the TODO so that it will be cleaned up.

Otherwise, remove both the TODO: comment and the commented out text.

Comment 92

[:aceman]

Attachment: fix hasSpaceAvailable v5 [checkin: comment 103]

Sorry, that comes from the fact I have 2 versions of this patch in my queue. This one should be right.

Comment 93

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 732999 [details] [diff] [review]
make GetSizeOnDisk 64 bit v3

Review of attachment 732999 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine aside from the NotifyIntPropertyChanged issue. Not sure what we want to do about that. Standard8? Should we just change that to "long long"? I'd hate to have to audit all the code that uses folder int property listeners to register specifically the folder size under a different listener.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +4820,1 @@
>      NotifyIntPropertyChanged(kFolderSizeAtom, oldFolderSize, mFolderSize);

NotifyIntPropertyChanged is still defined in IDL as having 'long' (== int32_t) parameters for the before and after integers.

Comment 94

[:aceman]

Oh, that would be bad, we need at least uint32 for the current 4GB-4MB.

Comment 95

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 735319 [details] [diff] [review]
fix hasSpaceAvailable v5 [checkin: comment 103]

Review of attachment 735319 [details] [diff] [review]:
-----------------------------------------------------------------

Yay!

Comment 96

[:aceman]

(In reply to :aceman from comment #94)
> Oh, that would be bad, we need at least uint32 for the current 4GB-4MB.

The problem may be if some folder properties need negative values. Then we can't change to uint32 but would need to go to int64. As we discuss in bug 813459. However, it is still not sure if we really have such a property.

Comment 97

[Camaleon]

Sorry for disturbing your work but I've noticed the title change of this bug report and now looks like since Thunderbird relase 12.0 there's support for folders over 4 GiB which I guess is not still the case (that's the reason why the original report was opened). 

Can someone explain -in plain words- what's the current status of the original bug report? Thanks.

Comment 98

[:aceman]

We believe over 4GB folders are only supported if you manually switch to the new maildir-lite message store, not the default mbox.

However, I am not sure even then the folder size would be reported correctly.

Comment 99

[Camaleon]

(In reply to :aceman from comment #98)

Thanks for your reply.

> We believe over 4GB folders are only supported if you manually switch to the
> new maildir-lite message store, not the default mbox.

Mmm... and how can that be done? I mean, is that new "maildir-lite" storage option available right now? If so, how users can choose it for new accounts or switch to it from a current profile? Where is it documented? This is completely new to me, I mean, I know there's some work on the way but still not stable/well tested enough to be used by now.

> However, I am not sure even then the folder size would be reported correctly.

May I ask what are you now trying to fix in this bug report? I'm completely lost :-)

Comment 100

[:aceman]

(In reply to Camaleon from comment #99)
> Mmm... and how can that be done? I mean, is that new "maildir-lite" storage
> option available right now? If so, how users can choose it for new accounts
> or switch to it from a current profile? Where is it documented? This is
> completely new to me, I mean, I know there's some work on the way but still
> not stable/well tested enough to be used by now.

See bug 58308 on how to enable it. But backup your profile as there seem to be some bugs, see bug 845952.

> > However, I am not sure even then the folder size would be reported correctly.
> 
> May I ask what are you now trying to fix in this bug report? I'm completely
> lost :-)

Exactly this. The folder size is not reported properly if a folder is over 4GB (regardless of mbox or maildir).

Also, we found some problems why mbox folder can't be allowed to grow above 4GB so we are fixing them here and in the dependent bugs.

So over 4GB mbox folders will only be allowed as the last patch in this bug.

Comment 101

[Camaleon]

(In reply to :aceman from comment #100)
 
> See bug 58308 on how to enable it. But backup your profile as there seem to
> be some bugs, see bug 845952.

So I guess this is not yet an option for plain users and the 4 GiB limit is still in place. Okay.

> Exactly this. The folder size is not reported properly if a folder is over
> 4GB (regardless of mbox or maildir).

But the warning is still useful so people can be noticed about the folder size limit. Why remove it?

> Also, we found some problems why mbox folder can't be allowed to grow above
> 4GB so we are fixing them here and in the dependent bugs.

Mmm, so are you guys following two different paths to solve this problem so users can decide which one to take?

1/ Allow >4 GiB folders over mbox
2/ Allow >4 GiB folders by using the new maildir-lite storage

That would be simply great :-)
 
> So over 4GB mbox folders will only be allowed as the last patch in this bug.

Which is not still added to the realease branch. Okay, and thanks for the info.

Comment 102

[:aceman]

(In reply to Camaleon from comment #101)
> > Exactly this. The folder size is not reported properly if a folder is over
> > 4GB (regardless of mbox or maildir).
> 
> But the warning is still useful so people can be noticed about the folder
> size limit. Why remove it?

We do not remove it, we actually make it more strict (the 'hasSpaceAvailable' patch) for now.

> > Also, we found some problems why mbox folder can't be allowed to grow above
> > 4GB so we are fixing them here and in the dependent bugs.
> 
> Mmm, so are you guys following two different paths to solve this problem so
> users can decide which one to take?
> 
> 1/ Allow >4 GiB folders over mbox
> 2/ Allow >4 GiB folders by using the new maildir-lite storage
> 
> That would be simply great :-)

Yes.

> > So over 4GB mbox folders will only be allowed as the last patch in this bug.
> 
> Which is not still added to the realease branch. Okay, and thanks for the
> info.

This patch does not even exist yet. We first need to fix the dependent bugs.

Comment 103

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/115c44b4353a

Comment 104

[ISHIKAWA, Chiaki]

Hi,

I could finally make WIN32 build work for my submission on Thunderbird TryServer.

In the patch above,
https://bugzilla.mozilla.org/attachment.cgi?id=732999
there is a replaced line:
-      m_totalFolderSize = (int32_t) atol(num);  //we always initialize m_totalFolderSize to 0
+      m_totalFolderSize = atoll(num);  //we always initialize m_totalFolderSize to 0
   }

As it turns out MS C++ compiler does not have atoll() and so the compilation 
fails for WIN32 on TB TryServer.
We could either
 - reverse atoll() to atol() assuming that no sane download contains more than 32-bit amount of data (as reported as the sum by POP3 protocol), or
 - we can use #if/#else/#endif as below.


diff --git a/mailnews/local/src/nsPop3Protocol.cpp b/mailnews/local/src/nsPop3Protocol.cpp
--- a/mailnews/local/src/nsPop3Protocol.cpp
+++ b/mailnews/local/src/nsPop3Protocol.cpp
@@ -2278,18 +2278,24 @@ nsPop3Protocol::GetStat()
   nsCString oldStr (m_commandResponse);
   char *newStr = oldStr.BeginWriting();
   char *num = NS_strtok(" ", &newStr);  // msg num
   if (num)
   {
     m_pop3ConData->number_of_messages = atol(num);  // bytes
     num = NS_strtok(" ", &newStr);
     m_commandResponse = newStr;
+#if _MSC_VER
+    if (num)                            // No atoll for MS VS compiler.
+      m_totalFolderSize = _atoi64(num);  //we always initialize m_totalFolderSize to 0
+#else
     if (num)
       m_totalFolderSize = atoll(num);  //we always initialize m_totalFolderSize to 0
+#endif
+
   }
   else
     m_pop3ConData->number_of_messages = 0;
 
   m_pop3ConData->really_new_messages = 0;
   m_pop3ConData->real_new_counter = 1;
 
   m_totalDownloadSize = -1; // Means we need to calculate it, later.


Hope this helps.

Comment 105

[:aceman]

(In reply to ISHIKAWA, chiaki from comment #104)
> As it turns out MS C++ compiler does not have atoll() and so the compilation 
> fails for WIN32 on TB TryServer.
> We could either
>  - reverse atoll() to atol() assuming that no sane download contains more
> than 32-bit amount of data (as reported as the sum by POP3 protocol), or

I am not sure we can assume this. If you have a >4GB gmail account and your local copy is lost, you may want to redownload it again.
I did the change purposefully so that any size we can store in local folder we must be able to download too.

Yeah, it seems atoll is part of C++11 so may not be in all compilers. I'll try to find another function.

Could you check if strtoll exists in MS C++ ?

Can anybody comment if conditional compilation is allowed in the tree?

Comment 106

[:Irving Reid (No longer working on Firefox)]

There is an implementation of atoll in nsCRT.{h,cpp} that is used several places in the tree; you can find this sort of thing by searching in the Mozilla source cross-reference at mxr.mozilla.org/comm-central/. There is also a newer fancier implementation for mozilla-central at dxr.mozilla.org, but this index does not currently include the Thunderbird comm-central sources.

http://dxr.mozilla.org/search?tree=mozilla-central&q=atoll&redirect=false

In general, yes, conditional compilation is allowed but we try to limit it as much as possible; in the case of missing library functions like atoll we try to keep the conditional compilation in a compatibility header file rather than spreading all through the code base.

Comment 107

[ISHIKAWA, Chiaki]

(In reply to :Irving Reid from comment #106)
> There is an implementation of atoll in nsCRT.{h,cpp} that is used several
> places in the tree; you can find this sort of thing by searching in the
> Mozilla source cross-reference at mxr.mozilla.org/comm-central/. There is
> also a newer fancier implementation for mozilla-central at dxr.mozilla.org,
> but this index does not currently include the Thunderbird comm-central
> sources.
> 
> http://dxr.mozilla.org/search?tree=mozilla-central&q=atoll&redirect=false
> 
> In general, yes, conditional compilation is allowed but we try to limit it
> as much as possible; in the case of missing library functions like atoll we
> try to keep the conditional compilation in a compatibility header file
> rather than spreading all through the code base.

It is great to learn strtoll() is in mozilla tree.
So we can simply change atoll() to strtoll() and all is fine. (Oh, we should
include nsCRT.h?)

MS C++ compiler does not still support it natively as of last summer.
http://connect.microsoft.com/VisualStudio/feedback/details/758053/missing-strtold-strtoll-strtoull-functions-from-stdlib-h

Comment 108

[:aceman]

I haven't seen strtoll, but I'll use nsCRT:atoll() .
Yes, it needs to include nsCRT.h . I'll attach an updated patch if you wish.

Comment 109

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #108)
> I haven't seen strtoll, but I'll use nsCRT:atoll() .
> Yes, it needs to include nsCRT.h . I'll attach an updated patch if you wish.

Sorry, I got a little confused. atoll() is it.
strtoll() is used in a few places in mozilla, but I found that they are
inside the #if/#else/#endif type constructs to handle the portability issues with
MS C++ compiler.

If you can create an updated patch, I will appreciate it. I have been compiling locally and submitting thunderbird tryserver jobs with your patches to make sure
everything compiles OK. (I have noticed a few GC-related errors, etc. on top of
the usability bugs I found and am trying to figure out if I can dig the cause of 
the problems.)

TIA

Comment 110

[:aceman]

Attachment: make GetSizeOnDisk 64 bit v4

OK, here is the updated patch, also extended to cover news folders now (as they just now got folder size display:))

Comment 111

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #110)
> Created attachment 738666 [details] [diff] [review]
> make GetSizeOnDisk 64 bit v4
> 
> OK, here is the updated patch, also extended to cover news folders now (as
> they just now got folder size display:))

Thank you I will replace the old patch with this one and test it out.
(Now news folder also has size display? How interesting. Compared with e-mail,
I don't think news folder god that big. Oh wait, for old-fashined alt.sources, etc., it can become quite large, I suppose.)

Comment 112

[:aceman]

Yes, news folder size display was added by me in bug 851275 :)
If you have news set for offline use, they can probably grow as large as a normal local email folder.

Comment 113

[ISHIKAWA, Chiaki]

Comment on attachment 738666 [details] [diff] [review]
make GetSizeOnDisk 64 bit v4

Review of attachment 738666 [details] [diff] [review]:
-----------------------------------------------------------------

I could compile the binary for win32 API (MS VS C+ compiler) with the modification of atoll() to use nsCRT version.
Sorry, it took a while since I had a disk issue in the middle of last week, and lost the HG MQ series locally, and took a while to
figure out how to recover. (Not sure if recovered completely or not. I messed up badly. But TryServer compilation and test for WIN32 and linux 64 worked without an error, and linux worked both locally and TryServer. Thank you.
[I have not used the test program you posted in bugzilla. Not sure where to add it. Maybe in the mozmill test suite directory and
mention the name in mozmilltestlist or some such file?]

Comment 114

[:aceman]

Test program? It was probably in other bug and you do not need to worry about it. It is a xpcshell test and it is installed just by applying the patch.

Comment 115

[Wayne Mery (:wsmwk)]

clarified summary and whiteboard.

plan is for this bug to only remove the warning.  and other work like convert expungedBytes to 64bit to be done in blocking bugs

Comment 116

[Wayne Mery (:wsmwk)]

aceman clarified online there are known bugs so [4GB] definitely not supported universally.  

http://kb.mozillazine.org/Limits_-_Thunderbird needs revision to say 4GB is not supported.  Vincent already updated the Sumo article.

Will need to update these when this bug is fixed:
* http://kb.mozillazine.org/Limits_-_Thunderbird
* https://support.mozillamessaging.com/en-US/kb/compacting-folders#w_why-is-compaction-required

Comment 117

[:aceman]

(In reply to :Irving Reid (On vacation, responding sporadically) from comment #93)
> Comment on attachment 732999 [details] [diff] [review]
> -----------------------------------------------------------------
> 
> Looks fine aside from the NotifyIntPropertyChanged issue. Not sure what we
> want to do about that. Standard8? Should we just change that to "long long"?
> I'd hate to have to audit all the code that uses folder int property
> listeners to register specifically the folder size under a different
> listener.

Standard8, can we get your opinion here so we can move forward?

Comment 118

[Mark Banner (:standard8)]

(In reply to :aceman from comment #117)
> (In reply to :Irving Reid (On vacation, responding sporadically) from
> comment #93)
> > Comment on attachment 732999 [details] [diff] [review]
> > -----------------------------------------------------------------
> > 
> > Looks fine aside from the NotifyIntPropertyChanged issue. Not sure what we
> > want to do about that. Standard8? Should we just change that to "long long"?
> > I'd hate to have to audit all the code that uses folder int property
> > listeners to register specifically the folder size under a different
> > listener.

I think it would be good to whip up a separate patch that changes that listener to long long. It shouldn't affect javascript uses, as they'll just absorb it, the c++ uses would be the ones I'm worried about.

If we didn't change it to long long, then I'd say we'd need to audit everything anyway to check the effects of the 64 -> 32 bit notification change.

Comment 119

[:aceman]

Would you be OK with keeping the name as NotifyIntPropertyChanged while changing to long long so as not to rewrite all callers?

Comment 120

[Mark Banner (:standard8)]

(In reply to :aceman from comment #119)
> Would you be OK with keeping the name as NotifyIntPropertyChanged while
> changing to long long so as not to rewrite all callers?

Yes, I think that's probably fine, although there's not that many callers. Note that we're going to need to change nsIFolderListener as well - but that name can stay the same as well (as long as we change the iid), as javascript doesn't really care.

Comment 121

[Neustradamus]

I think this ticket can be linked to https://bugzilla.mozilla.org/show_bug.cgi?id=980764.

It will be good to received emails...

Comment 122

[stevedonato]

In this day and age to have any Folder content limit is ridicules. Use Mysql if you can't figure it out. 

Just write the darn record and store a key for the index. GMAIL and Yahoo and every other provider has virtually unlimited space.    The client app. should do the same.  These developers are a Joke, if they can't figure out how to eliminate this "in box full"  condition.

The fix for this is a Trainee level programing task.  (this comment comes from a Software Developer 
with over 30 years experience) Just FYI.

Comment 123

[Matt]

(In reply to stevedonato from comment #122)
> In this day and age to have any Folder content limit is ridicules. Use Mysql
> if you can't figure it out. 
> 
> Just write the darn record and store a key for the index. GMAIL and Yahoo
> and every other provider has virtually unlimited space.    The client app.
> should do the same.  These developers are a Joke, if they can't figure out
> how to eliminate this "in box full"  condition.
> 
> The fix for this is a Trainee level programing task.  (this comment comes
> from a Software Developer 
> with over 30 years experience) Just FYI.
I look forward to seeing your patch for the issue Steve.

Comment 124

[:aceman]

Attachment: make GetSizeOnDisk 64 bit v5

This refreshes the GetSizeOnDisk patch, makes the mFolderSize int64_t per rkent's suggestion (in bug 813459) and makes -1 be the special value meaning size needs to be determined yet (instead of 0 which is a valid folder size). It also updates the 4GB test to see sizeOnDisk can now return >2^32 values up to JS callers.

Comment 125

[Kent James (:rkent)]

Comment on attachment 8516876 [details] [diff] [review]
make GetSizeOnDisk 64 bit v5

Review of attachment 8516876 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this is fine, but there are enough little issues that I should see it again. This patch, as you said, was applied after the patch in bug 813459. 

Do you understand why the test fails in Windows? I enabled it, bypassed the failing setSparse, but then the test hangs in POP3pump.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +136,5 @@
>    mInitializedFromCache(false),
>    mSemaphoreHolder(nullptr),
>    mNumPendingUnreadMessages(0),
>    mNumPendingTotalMessages(0),
> +  mFolderSize(-1),

Although I agree that this is the correct thing to do, this also needs supporting in the Extra Folder Columns extension. Is there a bug to incorporate that? If so, we should make a note there.

In general, we should avoid magic numbers that are used in multiple places and not clearly defined. So you need a line like:

static const int64_t kSizeUnknown = -1;
or
#define SIZE_UNKNOWN -1

and use those in place of -1.

Because those definitions may span multiple files, they should probably be in a public file like MailNewsTypes.h

@@ +4564,3 @@
>  {
>    NS_ENSURE_ARG_POINTER(size);
> +  // TODO: should this be 0 or -1 (uninitialized)?

This should be -1 (kSizeUnknown)

::: mailnews/base/util/nsMsgUtils.cpp
@@ +475,1 @@
>  {

Because you are making size == -1 be a flag for unknown, you need to capture that here so that there is no risk that unknown gets displayed as -1. It is sufficient to simply have:

float unitSize = size < 0 ? 0.0 : size;

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +202,5 @@
> +
> +  // TODO: This should be pushed into RebuildIndex so that the size is
> +  // recalculated as part of parsing the folder data (important for maildir),
> +  // once GetSizeOnDisk is pushed down to the msgStores (bug 1032360).
> +  return RefreshSizeOnDisk();

I don't understand why you want to add this. RebuildIndex is always async in spite of the comment above, so the failure is just a failure to start the async process. Why would you want to refresh the disk size on failure to index?

@@ +1108,5 @@
>  
>  NS_IMETHODIMP nsMsgLocalMailFolder::RefreshSizeOnDisk()
>  {
> +  int64_t oldFolderSize = mFolderSize;
> +  mFolderSize = -1; // we set this to -1 to force it to get recalculated from disk

This should be = kSizeUnknown

@@ +1119,4 @@
>  {
>    NS_ENSURE_ARG_POINTER(aSize);
>    nsresult rv = NS_OK;
> +  if (mFolderSize == -1)

again kSizeUnknown

::: mailnews/news/src/nsNewsFolder.cpp
@@ +72,5 @@
>  #define NEWSRC_FILE_BUFFER_SIZE 1024
>  
>  #define kNewsSortOffset 9000
>  
> +#define kSizeUnknown -1

You'll want to use whatever you use in the common file. This definition is fine, you could use it it all of the files if you like.

Comment 126

[:aceman]

(In reply to Kent James (:rkent) from comment #125)
> Comment on attachment 8516876 [details] [diff] [review]
> make GetSizeOnDisk 64 bit v5
> 
> Review of attachment 8516876 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall this is fine, but there are enough little issues that I should see
> it again. This patch, as you said, was applied after the patch in bug
> 813459. 
> 
> Do you understand why the test fails in Windows? I enabled it, bypassed the
> failing setSparse, but then the test hangs in POP3pump.
No I do not. If you just disable the sparsing of the file and POP3Pump still hangs, then I don't know. I suspect there is some lock held (unclosed file or so) so POP3pump can't proceed. Windows FS is stricter in this regard (like you can't delete an open file). We track this in bug 872357.

> 
> ::: mailnews/base/util/nsMsgDBFolder.cpp
> @@ +136,5 @@
> >    mInitializedFromCache(false),
> >    mSemaphoreHolder(nullptr),
> >    mNumPendingUnreadMessages(0),
> >    mNumPendingTotalMessages(0),
> > +  mFolderSize(-1),
> 
> Although I agree that this is the correct thing to do, this also needs
> supporting in the Extra Folder Columns extension. Is there a bug to
> incorporate that? If so, we should make a note there.
I am not sure an external caller can ever see the value of -1 (but maybe on an account type that does not override GetSizeOnDisk to return a proper value). The extension knows that it can get a value of -1 for number of messages and is prepared for that. I can check folder size too and add the fix to my already improved local copy of the extension that I plan to upload for bug 464973.

> In general, we should avoid magic numbers that are used in multiple places
> and not clearly defined. So you need a line like:
> 
> static const int64_t kSizeUnknown = -1;
> or
> #define SIZE_UNKNOWN -1
> 
> and use those in place of -1.
> 
> Because those definitions may span multiple files, they should probably be
> in a public file like MailNewsTypes.h
Of course I'd love to do a named constant, I just didn't know where I would be allowed to put it :)
Thanks for the hint, I'm on it.

 @@ +4564,3 @@
> >  {
> >    NS_ENSURE_ARG_POINTER(size);
> > +  // TODO: should this be 0 or -1 (uninitialized)?
> 
> This should be -1 (kSizeUnknown)
> 
> ::: mailnews/base/util/nsMsgUtils.cpp
> @@ +475,1 @@
> >  {
> 
> Because you are making size == -1 be a flag for unknown, you need to capture
> that here so that there is no risk that unknown gets displayed as -1. It is
> sufficient to simply have:
> 
> float unitSize = size < 0 ? 0.0 : size;
> 
> ::: mailnews/local/src/nsLocalMailFolder.cpp
> @@ +202,5 @@
> > +
> > +  // TODO: This should be pushed into RebuildIndex so that the size is
> > +  // recalculated as part of parsing the folder data (important for maildir),
> > +  // once GetSizeOnDisk is pushed down to the msgStores (bug 1032360).
> > +  return RefreshSizeOnDisk();
> 
> I don't understand why you want to add this. RebuildIndex is always async in
> spite of the comment above, so the failure is just a failure to start the
> async process. Why would you want to refresh the disk size on failure to
> index?
I only catched this because the test didn't work. When execution got to ParseListener_growOver4GiB() (which I hoped would be after the parsing is done), the .sizeOnDisk still did not contain the new (above 4GB) value. So I had to add this refresh here. Maybe there is a better place for it? Or there is a bug that the size is not properly updated insize RebuildIndex?
Or is OnStopRunningUrl in ParseListener_growOver4GiB not really run after everything is done?

Comment 127

[:aceman]

Attachment: make GetSizeOnDisk 64 bit v5.1

I found a better place for the RefreshSizeOnDisk after reparse of folder.

Comment 128

[Kent James (:rkent)]

Comment on attachment 8517705 [details] [diff] [review]
make GetSizeOnDisk 64 bit v5.1

Review of attachment 8517705 [details] [diff] [review]:
-----------------------------------------------------------------

OK this looks good. I spent quite a bit of time getting the 4GB unit test running in Windows. I'll post those changes separately. This patch was reviewed with the patch for bug 813459 applied.

Comment 129

[:aceman]

Attachment: make GetSizeOnDisk 64 bit v5.2

Needs to be applied on top of bug 813459. If you want to try out the test, it only works on Linux or Mac for now.

Comment 130

[:aceman]

Attachment: make GetSizeOnDisk 64 bit v6

This just adds tests whether the updated OnItemIntPropertyChanged from bug 813459 can return 64bit values now. I could not test it in that bug before there actually was a first 64bit property.

The only change in C++ code is the addition of "UpdateSummaryTotals(true);" in nsMsgLocalMailFolder::OnStopRunningUrl to the RefreshSizeOnDisk I already had to add before. Otherwise number of messages were not updated immediately when folder re-parsing finished.

I see the number of do_check_eq() calls gets quite big, but I want to be sure we do not miss anything in the 32/64bit internal conversions.

Comment 131

[Kent James (:rkent)]

Comment on attachment 8524993 [details] [diff] [review]
make GetSizeOnDisk 64 bit v6

Review of attachment 8524993 [details] [diff] [review]:
-----------------------------------------------------------------

As a general comment, it is time to quit tweaking this patch and just get it landed. I have already spent a lot of time looking at this patch and followup bugs. Some of the issues that you are tweaking will end up getting changed significantly when we fix some of this for maildir, so this is not time well spent by you or me. I was satisfied with the previous version, the only required change was moving some changes here from bug 813459 (which was also unnecessary make-work IMHO), and these delays are slowing down the maildir work.

Comment 132

[:aceman]

I don't think the main part of the change (the extended test) becomes irrelevant after maildir work.

Comment 133

[[e-mail]]

Comment on attachment 8524993 [details] [diff] [review]
make GetSizeOnDisk 64 bit v6

I didn't spot anything obviously wrong.

> 
>-#define kSizeUnknown 1
> 
Nit: need to delete one of these blank lines too.

>diff --git a/mailnews/news/src/nsNewsFolder.h b/mailnews/news/src/nsNewsFolder.h
>--- a/mailnews/news/src/nsNewsFolder.h
>+++ b/mailnews/news/src/nsNewsFolder.h
>@@ -14,16 +14,17 @@
> #include "nsMsgDBFolder.h"
> #include "nsIFile.h"
> #include "nsINntpIncomingServer.h" // need this for the IID
> #include "nsNewsUtils.h"
> #include "nsMsgKeySet.h"
> #include "nsIMsgNewsFolder.h"
> #include "nsCOMPtr.h"
> #include "nsIMsgFilterService.h"
>+#include "MailNewsTypes.h"
Why are you including this here when you're not using the constant in this file? r=me with this moved to the .cpp file.

Comment 134

[:aceman]

Attachment: make GetSizeOnDisk 64 bit v6.1 [checked in - comment 135]

Thanks.

Comment 135

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/a228fcd72731

This has [leave open after checkin] in the whiteboard, not sure if that still applies or not. aceman, can you please close this if it is complete. Thanks!

Comment 136

[:aceman]

Yes, that status is correct. The "feature" is not done yet until all the blocking bugs are fixed. The last patch to attach here is a simple one to remove the 4GB limit (like https://bug793865.bugzilla.mozilla.org/attachment.cgi?id=751871).

Unless we decide to close this with the existing check-ins and many comments and create a new meta bug.

Comment 138

[[e-mail]]

Comment on attachment 8526336 [details] [diff] [review]
make GetSizeOnDisk 64 bit v6.1 [checked in - comment 135]

>-  if (folderSize == kDisplayBlankCount || folderSize == 0)
>+  if (aFolderSize == kDisplayBlankCount64)
This caused bug 1136696.

Comment 140

[Kent James (:rkent)]

It's a pity that we never finished the few things remaining to do this. Could we start working on this again?

Comment 141

[:aceman]

Yes, that is the plan.

Comment 142

[:aceman]

(In reply to Kent James (:rkent) from comment #140)
> It's a pity that we never finished the few things remaining to do this.

Actually I do not remember anything that would be remaining here (the last was the incremental generation of keys).

I will now create some tests.

As I still think there may be unexpected surprises in the code yet, would you support the plan that in the first stage we make a pref (off by default) that would turn on the support of 4GB+ folders? I think the pref only needs to be checked in nsMsg*Store::HasSpaceAvailable() which decides if new msgs can be accepted into the message store. In that way adventurous nightly users could turn it on.

Comment 143

[Kent James (:rkent)]

I think that the correct path is to add the pref, but default it to enable support for >4GB in nightlies and betas. We need testing, and a default off pref is not going to help.

Yes this is critical code, but reaching the 4GB limit is also a critical issue that needs fixing. I would love to see us leave support enabled for TB 52.

Comment 144

[Matt]

I have seen inbox full messages reported in Support in recent months,  so I am fairly sure more surprises await.

Comment 145

[:aceman]

Attachment: allow crossing 4GB limit by default

Maildir already doesn't have the 4GB limit (I think it was removed due to spurious errors). So this removes it for mbox, based on a pref. I read the pref once per folder into a static variable. That is similar to e.g. gTimeStampLeeway but I didn't want to do the gGotGlobalPrefs dance (not sure why we can't read the leeway pref too in the constructor).

Comment 146

[:aceman]

Pinning the value of the pref is not that great. I'd like to toggle it on the fly in tests. That would allow to just add a few tests into test_over4GBMailboxes.js. I'm not sure we want to duplicate that file completely (it takes its time to run).

Would it be to expensive to check the pref at each adding of a new msg?

Comment 147

[Kent James (:rkent)]

Comment on attachment 8774067 [details] [diff] [review]
allow crossing 4GB limit by default

Review of attachment 8774067 [details] [diff] [review]:
-----------------------------------------------------------------

It looks to me like the reading of a pref is done using a C++ hashtable, and therefore should not be considered expensive to do for each message.

The other change I know I would like with a brief look at this is the naming of mailnews.folders.4GBlimit  First, there is nothing else in the branch mailnews.folders and no plans to add anything, so I don't see why we need that extra level. Second, I cannot tell the polarity of "4GBlimit" without searching the code.

So I suggest instead mailnews.limitMboxTo4GB for the current definition. But that is a negative definition, which makes one think hard when it is false. So I really prefer a positive definition like mailnews.allowMboxOver4GB

r- only because this is going to change, but overall I like what I see!

Comment 148

[:aceman]

Attachment: allow crossing 4GB limit by default v2

I think creating the pref service is more expensive. I'm not sure why there is no mozilla::services::GetPrefService() shortcut for it yet.

Comment 149

[Magnus Melin [:mkmelin]]

There's Preferences.h so Preferences::GetBool?

Comment 150

[:aceman]

Attachment: allow crossing 4GB limit by default v2.1 [checked in - comment 152]

Thanks, that seems to work. Looks like we could convert a lot of code to this :)

Comment 151

[Kent James (:rkent)]

Comment on attachment 8775803 [details] [diff] [review]
allow crossing 4GB limit by default v2.1 [checked in - comment 152]

Review of attachment 8775803 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 152

[:aceman]

https://hg.mozilla.org/comm-central/rev/75ff4740fc0f1b558871c7f20a0b804b4e1ff2ea

This is the last commit that removes the 4GB warning on mbox. For maildir, the limit was already removed in another bug.

If any issues are found in folders larger than 4GB, please file new bugs. Thanks to all involved!