User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4 Build Identifier: 20070604 If you copy a large batch of messages (say 1GB worth) into a folder that already contains nearly 4GB of mail (say, 3.5GB) then the messages are copied into the folder. The folder continues to function. I have a 4.45 GB folder. Once you do this mass copy that breaks the limit, any additional copies are disallowed ("the folder is full and cannot hold any more messages"). Two things are odd here. First of all, I imagine it would be appropriate to warn or stop copying at the message that would cross the 4GB barrier. Secondly, the mailbox continues to function quite well with 4.45 GB. Is there any real reason why we still have the 4GB barrier? Maybe we should get rid of it? Reproducible: Always Steps to Reproduce: 1. Take a folder with a lot of messages. 2. Copy those messages to a temporary folder. Keep recopying the same messages to the temporary folder until the folder in question is about to pass the 4GB barrier. 3. Do another copy and cross the 4GB barrier. If copying in large enough chunks, the 4GB barrier can be crossed by a significant margin and no warning is issued. Next attempt to add messages to this folder results in a warning/error message as expected. Actual Results: User can cross the magical 4GB barrier. Expected Results: User cannot cross the magical 4GB barrier, or perhaps the 4GB barrier is not needed. New versions of M$ Outlook allow for 30GB .PST files, so it's not an OS limitation. Perhaps it's a limitation of the mbox format, but my 4.45 GB mbox works quite well, thank you.

we do try to warn users about this - not sure why you didn't get the warning.

reporter: could you please indicate what mail user agent (MUA) you're using, (SeaMonkey, Thunderbird, Penelope, ...) and what version you have? not that it's particularly relevant, but are you using FAT16/FAT32/NTFS/network storage, and is this a local folder, a local store for a pop folder, or an imap store? (the string appears in both thunderbird and seamonkey, so I can't really guess which you're using, yes, the codebase is shared, but it's still nice to have complete bug reports) /mail/locales/en-US/chrome/messenger/messenger.properties, * line 116 -- 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. /suite/locales/en-US/chrome/mailnews/messenger.properties, * line 118 -- 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. (note that exact strings are appreciated, as it makes searching a lot easier) http://mxr-test.landfill.bugzilla.org/seamonkey/search?string=mailboxTooLarge&find=mail The code is in: nsMsgLocalMailFolder::CopyMessages david: I think the code's doing the wrong math. AFAICT it checks whether the mailbox is already too big, instead of "will the mailbox become too big once I add the new messages" which matches perfectly with the description of the reporter. http://bonsai.mozilla.org/cvsblame.cgi?file=%2Fmozilla%2Fmailnews%2Flocal%2Fsrc%2FnsLocalMailFolder.cpp&rev=1.560&mark=1569,1588,3667,3675,3680,1593,1631#1560

Using Thunderbird version 2.0.0.4 (20070604). Using NTFS on local drive Using a local mail folder (there's not really a POP account attached to it or anything, it's just a local mail folder). Exact error string - The folder TEST4GB2 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. timeless: It looks like it is in fact checking to see if the mailbox is ALREADY too full rather than whether it will BECOME too full by the requested operation. Your description makes sense. Does anyone know how big of a problem this actually is? What's this 4GB limit all about anyway? My 4.5 GB folder is displaying and deleting messages and compacting quite fine.

We start warning if the file is close to 4GB, not over it, i.e., if the folder size is greater than 0xFFF00000, in other words, within 0x10000 of 4GB. Does it not look like that's what the code is doing? That doesn't catch the case where you try to copy a large amount of mail into a folder that's more than 0x10000 bytes from being full, but that should just affect that one copy operation.

That seems like what's going on. I've just been dealing with chunks much larger than 0x10000, so I never noticed the "pre-warning". My worry is that I thought the mbox format had a hard 4GB limit. Obviously it doesn't. At what point would the mailbox cease to function? Slightly related is my curiosity over the fact that my 4.45 GB mailbox works fine. What actually IS the real limit and what would happen if you reached it?

4GB really is the limit - I'm surprised you can read the messages that are after the 4GB limit.

Is there anything you want me to test specifically? Deleting, reading any message, compacting, etc. all seems to work fine. Again only slightly related - should 3GB+ mailboxes be expected to work relatively well, or has that been tested much?

Ok your comment about not being able to read messages "after the 4GB limit" caused me to try something else. I've got my nice new 5.27GB mailbox. The way I created it was by taking the same 100MB or so of messages and copying them in over and over again. That means if I take any one message I can have a representative sample of various portions of the mbox file itself. Well, it turns out that of my 55 or so copies of any given email, easily 10-15 of them have a subject line and a from line but no message body. This means, of course, that I'm not actually able to view the messages past the 4GB limit (just like you were saying) even though I was able to put them in there (and can still now view the other messages, delete, and compact). This means that if you warn at > 0xFFF00000, then an email with a size of 1MB (0xFFFFF) could break the limit and be lost, does it not?

I could observe phenomenon by test procedure I wrote in Bug 497554 Comment #8, with Tb 2.0.0.21 on MS Win-XP SP3. Confirming.

Once "mail folder file larger than 4GB" was generated by Tb, next phenomena was observed(checked with Tb trunk 2009/6/08 build on MS Win-XP SP3). (1) Rebuild-index generated MailDB data(thread pane display) only for mail data placed at less than 4GB => mails after 4GB is lost. Mail body data of last mail (highest offset, less than 4GB) was cut-off at 4GB boundary => mail body data after 4GB is lost. (2) Even though mails at less than 4GB are accessed normally after rebuild-index, rebuild-index was invoked upon click of the folder after each restart of Tb.

Coming in from bug 497554: >> Reading the code, we will throw a >> warning if there is less than 1 MB of free room left. > >There must be a subtle bug in there. (comparison of signed and unsigned, >going over 2^32, etc..) I am curious at the checking code. Is there a source code browser for TB similar to the Debian GNU/Linux source browser? http://walrus.rave.org/source/ --- I am curious as to the next point. Doesn't TB check whether the folder WILL grow over {2|4}GB if a message is inserted BEFORE the insertion is done? Checking whether "1 MB of free room left" is useless considering that we have incoming messages with more than 1MB of attachement (PDF, PPT, etc.) these days and suddenly "Sent" folder is larger than the TB-imposed limit, for example. We NEED to check BEFORE the insertion, don't we? I think the checking ought to be done - both BEFORE the insertion is done (very logical, isn't it?), and - at strategic places to see if a folder has already become larger than the {2|4} GB limit on platforms where such big files are allowed AFTER the mistake(s) are made. We must detect that a folder becomes too large and warn the user to take remedial action. (This is the so-called defensive programming. We need these kludges now that there are mail folders which goes slightly over the TB-imposed limit already(!).) BTW, the colleague of mine who got bitten by this bug (on Win XP, 32bit) yesterday mentioned that he never seen the warning, and that he has been using Thunderbird happily until the problem yesterday. "Sent" was approximately 4GB when I checked, and so presumably his "Sent" folder went over 2G limit some weeks ago, and he has been happily using it until yesterday. So Wada-san's mention of 32bit signed summary index issue may have been already solved in the recent build? Otherwise, he would have come to me much earlier. Or MAYBE no arithmetic is done on the seek position value AT ALL (which makes sense!), and thus the position value is actually simply copied from ftell() and friends and then passed back to lseek() and friends unmodified. This explains the behavior observed by my colleague. (Of course, we want to use the 64 bit versions ftell, lseek, etc. if applicable...) TIA

(In reply to comment #12) > Is there a source code browser for TB similar to the > Debian GNU/Linux source browser? http://mxr.mozilla.org/comm-central.

nsMsgLocalMailFolder::WarnIfLocalFileTooBig is defined at: > http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#3766 WarnIfLocalFileTooBig is called at: (in nsMsgLocalMailFolder::CopyMessages) > http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#1662 WarnIfLocalFileTooBig is called for first mail only if bulk copy(or move)? Or called upon each copy of a mail, but file size used to check TooBig is not updated by previous copy?

Thank you for the pointer to comm-central! (#13 above) I see a definite readability problem in the following code. Should not - (a) "nsInt64" on line 3777 and 3778 both read nsUint64 (unsigned, that is), - and (b) 0xFFF00000 on line 3777 read 0xEFF00000 ? http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#3766 3764 3765 NS_IMETHODIMP 3766 nsMsgLocalMailFolder::WarnIfLocalFileTooBig(nsIMsgWindow *aWindow, PRBool *aTooBig) 3767 { 3768 NS_ENSURE_ARG_POINTER(aTooBig); 3769 *aTooBig = PR_FALSE; 3770 PRInt64 sizeOnDisk; 3771 nsCOMPtr <nsILocalFile> filePath; 3772 nsresult rv = GetFilePath(getter_AddRefs(filePath)); 3773 NS_ENSURE_SUCCESS(rv, rv); 3774 rv = filePath->GetFileSize(&sizeOnDisk); 3775 if (NS_SUCCEEDED(rv)) 3776 { 3777 const nsInt64 kMaxFolderSize = 0xFFF00000; 3778 nsInt64 folderSize(sizeOnDisk); 3779 if (folderSize > kMaxFolderSize) 3780 { 3781 ThrowAlertMsg("mailboxTooLarge", aWindow); 3782 *aTooBig = PR_TRUE; 3783 } 3784 } 3785 return NS_OK; 3786 } My thinking: If, indeed, 2GB limit was the original intention, then 0xFFF0000 ought to read 0xEFF00000; (my point (b)). The checking on 3779 ought to be done using unsigned arithmetic, thus my point (a) above. Actually, through the sign-extension, etc., the above code *may* be right (it may indeed be, but then in that case, the comparison on 3779 would be a signed comparison of negative values which are not quite straight-forward), We ought to strive for clarity and conciseness for long-term maintenance. So I would suggest rewriting the above into /** * checking 2GB limit */ 3777 const nsUint64 kMaxFolderSize = 0xEFF00000; 3778 nsUint64 folderSize(sizeOnDisk); I am not sure whether we should change 3770 accordingly. Still investigating the following: In any case, I still think that we need the advance growth check (i.e., the advance checking based on the future size when a copy of a message will have been done). Still investigating where the checking is done and if the checking is appropriate. A kludgy idea to give a bandaid to the current state of affairs. In the meantime, I think a fast bandaid is as follows. After looking at the code, I noticed GetSizeOnDisk() (that returns the filesize modulo 2^32) is called by RefereshSizeOnDisk(). I feel that we should call WarnIfLocalFileTooBig() immediately after each call of RefreshSizeOnDisk() [which is called in a layer closer to UI, it seems to me] so that TB and users can take remedial action before the damage is done due to OUT-OF-BOUND filesize. (Then again, we need to handle the case of receiving message into Inbox and sending message into Sent folder so that we don't lose the temporary message in flight. And this is done behind the user's back and users can't prevent it once the action is in motion.) (To comment #14) Probably we need to call RefreshSizeOnDisk() before WarnIfLocalFileTooBig() to get the updated size after a previous copy. Just a wild guess.

(In reply to comment #15) > Should not > - (a) "nsInt64" on line 3777 and 3778 both read nsUint64 > (unsigned, that is), No. The file returns a signed 64-bit, so it should be nsInt64. > - and (b) 0xFFF00000 on line 3777 read 0xEFF00000 ? OxFFF00000UL maybe, but my specification seems to state that the compiler should be choosing the proper type of an unsigned 32-bit integer. > If, indeed, 2GB limit was the original intention, then > 0xFFF0000 ought to read 0xEFF00000; (my point (b)). I believe the intention is to use 4GB--the maximum of PRUint32, which is essentially what message keys are. > The checking on 3779 ought to be done using unsigned arithmetic, > thus my point (a) above. These are 64-bit integers, whereas the values in the case concerned about are clearly in the unsigned 32-bit range.

Correction: Sorry, I got confused with another function that returns file size modulo 2^32, GetSizeOnDisk() (also set mFolderSize to modulo 2^32) vs GetFileSize() (uses stat64, say under posix, if available) http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#1406 1406 NS_IMETHODIMP nsMsgLocalMailFolder::GetSizeOnDisk(PRUint32* aSize) # mailnews/base/util/nsMsgDBFolder.h (View Hg log or Hg annotations) * line 224 -- PRUint32 mFolderSize; Yes, you are correct that the compiler ought to pick up the correct sign extension since hexadecimal constant is treated as unsigned data type by the compiler (I just checked.) That said, after a quick investigation, I think the problem is essentially pointed out by #2 and #14. TB seems to check that - if the folder is close to the limit (2GB, er, 4GB) only once by checking if there is more than 1MB space left in advance. (THIS CHECKING IS ALREADY BOGUS when we can simply copy a message more than 1MB easily these days.) - TB doesn't seem to check the successive adding of messages will go over the limit (2GB/4GB) at all thereafter. I think we really ought to check this BEFORE the copying happens. (This is for copying only. I don't know whether copying caused by sending a message or receiving a message also fits this case yet.) So I am posting an idea (not the tested code) for a possible check along the line outlined above. in the next message. BTW, is there an agreement to support mail folder size up to 4GB (> 2GB) as opposed to 2GB? This is a moot question now, but I just wanted to be sure.

I would suggest that we add a quasi-code in the following. (I am not yet familiar with the TB internals, so please bear with my misundestandings of the data types, etc..) This quasi-code code tries to take care of the checking -(a) whether the foder actually already went over the supposed limit. -(b) whehter the copying of the messages make the folder grow over the supposed limit. I am assuming that we prefer NOT to copy a single message if a copying of a large list of messages will grow the folder size beyond limit. I.e., I assume atomicity of the whole operation is preferred. We want to be warned before the copying proceeds. (OLD) 1660 PRBool mailboxTooLarge; 1661 1662 (void) WarnIfLocalFileTooBig(msgWindow, &mailboxTooLarge); 1663 if (mailboxTooLarge) 1664 { 1665 if (isMove) 1666 srcFolder->NotifyFolderEvent(mDeleteOrMoveMsgFailedAtom); 1667 return OnCopyCompleted(srcSupport, PR_FALSE); 1668 } (As it turns out that this old check will see if the file is not too large [ (a) above ] if the past discussion is correct. So we leave it in there.) (NEW Addition.) After line 1710 (we need to finish the checking the offline status of message first) add the following check. Idea: we check if the adding of messages will grow the file folder size beyond correct 32 bit arithmetic. Loop over the message list and add the size of each message. And see if the size reaches the limit or CLOSE TO it at each step.. TODO/FIXME: It seems that TB may want to add its own special headers within the folder and I am not sure if the GetMessageSize() accounts for it. If not, either the arbitraily subtraction from the file size limit, or an arbitrarily constant addition to each message size should be used to to account for this size overhead per message. I add 200 bytes for each message below. In the quasi-code below, I do away with the 64 bit arithmetic with the trick I mentioned in message # because I read in a post of settting up compiler flags properly to obtain correct 64 bit arithmetic. After line 1710 , add a check like the following.: /** will set mFolderSize to the current size. * I don't know if this is the correct method, but it seems like * workable. */ PRUint32 oldmfsize; (void) RefreshSizeOnDisk(); oldmfsize = mFolderSize; /* * The next loop is modeled after the loop at line 1717 */ PRUint32 numMsgs2 = 0; PRUint32 newmfsize = oldmfsize; PRBool sizecheck = PR_FALSE; messages->GetLength(&numMsgs2); for(PRUInt32 i = 0; i < numMsgs2; i++) { nsCOMPtr<nsIMsgDBHdr> aMessage = do_QueryElementAt(messages, i, &rv); if(NS_SUCCEEDED(rv) && aMessage) { /* Obtain the size of each message */ PRUint32 size; PRUint32 temp; aMessage->GetMessageSize(&size); temp = newmfsize + size + 200; /* 200 is the overhead mentioned */ /* Check for overflow (2^32) without using 64 bit arithmetic.*/ if( (temp <= newmfsize ) || (temp <= size ) || (temp <= 200)) { /* ERROR. Break out ! */ sizecheck = PR_TRUE; break; } else newmfsize = temp; } } /* if we reach here, either we broke out the loop due to size check failure or at least the 2^32 wrapping didn't occur. If wrapping didin't occur, newmfsize will be the approximate size of folder. We may want to check again if the newmfsize is not too large enough before creating a problematic folder size. */ if( sizecheck || newmfsize > 0xFFF00000 /* or whatever */ ) { /* Warn as in WarnIfLocalFileTooBig() .... */ /* "Copying messages will create a folder close to the file size limit or above, aborting ...". TODO/FIXME: do we have to to create a new keymessage? */ ThrowAlertMsg("mailboxWillBecomeTooLarge", aWindow); return OnCopyCompleted(srcSupport, PR_FALSE); } Just an idea. I wonder if someone can incorporate the above check in the source and see if the procedure mentioned in comment #14 will now be properly checked and error is generated before a too large folder is generated.

Two typos. 1. >In the quasi-code below, I do away with the 64 >bit arithmetic with the trick I mentioned in message # >because I read in a post of settting up compiler flags properly of "difficulty" of seeting up compiler flags properly >to obtain correct 64 bit arithmetic. 2. >see if the procedure mentioned in comment #14 will now be comment #10 was intended. TIA

(Off-Topic for this bug's problem) (In reply to comment #17) > BTW, is there an agreement to support mail folder size up to 4GB (> 2GB) > as opposed to 2GB? This is a moot question now, but I just wanted to be sure. Bug 362244 is support request of file_size>2GB on Linux/Mac OS X (NSPR). Bug 343907 is a report of problem with mail_folder_file_size>2GB on Mac OS X. Bug 428147 is a report of special case of mail_folder_file_size>2GB on Linux. 4GB>=file_size>2GB is created by MS Win and is mounted on Linux. Main reason of "up to 2GB on Linux/Mac OS X" is still-not-fixed Bug 362244. Bug 462665 is support request of mail_folder_file_size>4GB. (64bit integer use at any where)

BTW, this is being marked as "normal" urgency, but given the problems, I think this warrants the "CRITICAL" urgency mark. People can LOSE their mail articles because if a folder goes over a certain size limit, we can't add (or remove). If the folders are "Sent" or "Inbox", we lose the articles being sent (or being received!?) assuming these problems are caused by the same underlying issues. Losing of day's worth of mail messages he composed and sent is what happened to one of my colleagues, which in turn prompted me to write the bug report (in the duped bug 497554) in the first place. User mail agent should NEVER lose the user mail message, not a single one, IMHO. Just IMHO. People's milage may vary.

I am experimenting with creating a patch for 2.0.0.21 to fix the problem. Attached is the patch (in the next post). Under linux (32bit), Debian GNU/Linux lenny distribution. However, there is something fishy going on. The WarnIfLocalSizeTooBig() is FAILING to check a file size (>4GB). What happens is that WarnIfLocalSizeTooBig() is called, but the GetFileSize() called inside is failing to provide the correct filesize. That is the return value of GetFileSize is ERROR value and thus the checking is not done at all and the routine returns OK value silently. This is a very problematic behavior. Equally bad is that RefershSizeOnDisk() also fails and return 0 size value! (We get 0 size value and so the file size check succeeds incorrectly.) BTW, the patch in the work is a kludge in that I don't know the proper manner of dumping various values to console, but it helps me to identify the problem. If WarnIfLocalSizeTooBig() is failing for an already large folder, there is no way the current binary works as expected. When the copying is to a folder (much smaller than the limit), WarnIfLocalSizeTooBig() calles GetFileSize() which does't not fail and we get meaningful values. I am attaching the system trace dump during the operation(s). It seems that fstat64 is called and the big file size is reported correctly, but somehow it is not picked up. As far as system calls are concerned, the returned values look OK, so the low-level routines inside TB may not be picking up the value correctly, or in a certain case, stat64 is not called at all(?). Can anyone in the know identify why on earth the filesize is not returned correctly or rather GetFileSize fails on 4GB+ file? TIA

Message to the console where the bin was invoked during the copying of a message to a couple of folders were tried twice. Note the bogus 0 value reported due to the failure of GetFileSize() for a mysterious reason.

gzipped. Captured the system calls invoked during the copying operation. Cursory check doensn't reveal strange stat64 calls.

Maybe I was not clear enough in comment #22. TB's call to GetFileSize() failes on (comm-central's code) on line 3779. http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#3779 If rv is not a success value, then the whole {} clause is skipped(!). Thus there is no size check performed. (The same problem holds for GetSizeOnDisk () called from RefreshSizeOnDisk(). If GetFileSize fails, its failure is not flagged at all, and the bogus value 0 is used as the filesize and thus folder size 4GB check succeeds incorrectly. Why then GetFileSize() fails? (Although the trunk, TB3.x seems to use a slightly different code, the majority of users stick to 2.0.0.2y series for sometime now, and the code is basically the same here, I would like to have this problem fixed in 2.0.0.21 (and its follow-ups) and possibly 3.0. The problem has been around long enough and it bites very badly. Any insight into the cause of the problem id welcome. TIA

DUP'ed bug 497554 is for Linux. Changing to "All".

Oh, boy. After sprinkling fprintf statements, I found that stat() and lstat() failed for 4GB file sizes. YES, "stat" and "lstat" ! For reasons unknown to me, the UNIX port (posix/linux/solaris and what have you) is not using stat64 and lstat64 at all.xpcom/io/ I am trying to come up with a fix for this. But I will leave the checking of other ports such as win32 to others. (I could not make the build on win32 platform over the weekend.) IMHO, this problem merits a serious look. Yes, I noticed there are a few comments marked with "XXX" that states we should use stat64 and friends. But it seems they were never done. TIA. PS: After fixing this, I can possibly check the sanity of checking the folder size IN ADVANCE of copying multiple messages into that folder. (They seem to work now, but I am yet to go across the boundary of less than 4GB, and over 4GB during the size check. I think fixing the above is in order first to protect those users who already went over 4GB limit without knowing...)

Sorry, I forgot to mention that this was with TB 2.0.0.21 under linux. (3.x seems to check for stat64, lstat64 at least.) I am yet to check if fstat64 is called for etc. (It doesn't seem so.) More after tomorrow...

As I mentioned earlier, I want to have this bug fixed in the current release, namely TB 2.0 series first. (This is because official release of 3.0 version seems far into the future yet.) If there are more than a few million users, surely there are many who got bitten by this bug one way or the other over the course of months of usages. I want to save these users from the bug. Now, I have identified a few problems and am producing patches for review by the people in the know. Below is the problem identified for linux port. (Mainly in 2.0.0.21 version.) =============================================+========= TB 2_0_0_21_RELEASE | 3.0b2 ---------------------------------------------+--- Proper file size (>=4GB) No. (1) | Probably OK check using lstat64, stat64 | ---------------------------------------------+---- Failure to the pass the | 64 bit file size thus | obtainedto upper-level Yes.(2) | (No. It is fixed. Good!) routine due to incorrect cast | in GetFileSize. | (An argument to LL_UI2L macro) | ---------------------------------------------+---- Check whether the file | size goes over 4GB Not done.(3) | Not done (4) while copying | IN ADVANCE | ---------------------------------------------+---- Creating a proper warning message | for the case where the advance checking | prohibits copying messages. (5) | { (5)' } ---------------------------------------------+-------- Problem (1) and (2) together causes TB-2 to fail WarnIfFileTooBig check before the copying starts under linux. (So, a largish folder is handled without issues no matter what, and results in too large files which are no lonrer handled correctly.) Once such file size check is properly in place with patches for problems (1) and (2), we still have the logical flaw of checking the file size folder only before the copying and not checking the future file size that will be over 4GB. [Problem (3), and (4) mentioned above.] This problem (3) should be universal across all the platforms (other than linux also.) I am trying to fix (1) and (2) for linux/solaris, etc., and (3) in general. I think I already fixed (1) and (2) in my local source tree (but for debugging purposes, it is sprinkeled with fprintf().) Again, I wonder if someone can look intot he problem for (1) and (2) in the Win32 port, say. As for (4), Patch for (3) should be easy to port to 3.0b2. I differentiate (4) and (3) since the function names et al are different between 2.0.0.21 and 3.0b2 to warrant a separate patch. I am testing patch for (3) now, and it seems to work now. Since I am done with (3), I am going to post the following files. (a) a set of patches with fprintf(stderr, ...) still sprinkled in key places { with the output to show the problems } and how the would-be patches fix the problems. These patches are for mailnews/local/src/nsLocalMailFolder.cpp xpcom/io/src/nsLocalFileUnix.cpp I will post them following this comment. (b) After the initial review is finished, and the general direction and the fix is accepted, a set of patches with fprintf(stder, ...) removed. This will be later. Then I hope I can produce patch for (4) for 3.0b2. Now what is the proper procedure for asking for review of the proposed patches? Should I ask for such formal review at the step (a) or at the step (b)? TIA

Patch in the works for problem (1) and (2) identified in the previous comment. fprintf() inside #if/#endif will be removed once the general direction and fix is deemed OK.

Patch in the works for problem identified as (3) in my previous post. This adds the check for the future folder size by adding the message(s) size BEFORE actual copying is done. fprintf() inside #if/#endif will be removed once the general direction of the patch and fix is deemed OK.

Oh, the patch mentioned in comment #31 is uploaded as x-vd or x-dv and it is considered as video(?) content. It is a simple diff output like the patch that follows (explained in comment #30). I am not sure how to fix this. TODO/FIXME: Now, I am not sure how to create and register the proper message for (5) mentioned in the previous message. We need a message that tells the user that "Copying of specified message(s) will increase the target folder size to reach the TB's folder size limit or very close to it, and thus the copying is not attempted. Please break down the folder into smaller chunks or see if compatifying the folder helps." (or some such). Originally, I thought I would leave to people more familiar with the innards of TB the steps necessary for (5). However, as it turns out, ThrowAlertMsg() doesn't seem to produce ANYTHING AT ALL when it is called with an unknown key as in the following: ThrowAlertMsg("mailboxWillBecomeTooLargeWeHave4or2GBLimit", msgWindow); What happens now is that the checking of the would-be file size BEFORE copying now works, and the check is invoked and copying is avoided. However, no alert message is shown and TB-2 remains silent. (I can tell that patched TB-2 avoids to create too large a file by looking at the verbose print out from fprintf() in the current patch.) I was under the impression that functions like ThrowAlertMsg() would show the key string as the placeholder when no corresponding text resource data is found, but I was mistaken. Does anyone care to step in to produce such a message? As it is, TB-2's previous logic of checking that there is 1MB room before the limit is reached is very flawed considering that we can easily copy a message or messages of which total size exceeds 1 MB. So we need to introduce the new check [in the patch for (3)] and a new message to warn the user. Unless such a message is produced and shown in an alert message, the patch is not yet complete. BTW, the message shown when the check for file size larger than 4GB size is invoked seems to be the correct one. (This check is now FIXED by patch for (1) and (2). I used the Japanese port of 2.0.0.21 with my patch under linux, and the shown message is the proper one telling me either to break down the folder or compactify to see if it helps.) Another TODO/FIXME: I mentioned that file size check should be performed in several strategic places to catch errant files which grew too big due to previous bugs. I noticed a few such places. (1) We shoudld invoke WarnIfFileTooBig() before an index is re-created. ... I will find more such places where such check will be appropriate...

Thanks for working on this. Please note o fixes that aren't strictly branch only (tb2) must land on for tb3 first o core changes (nsLocalFileUnix.cpp etc) first have to land on mozilla-central, then mozilla-1.9.1 to get into tb3 o no string changes allowed for tb2

>Thanks for working on this. Please note Thank you for the explanation. I would like to make the TB and mozilla more robust and usable for more users. (Especialy people in the marketing and sales who routinely send and receive large ppt and pdf files regulary.) > o fixes that aren't strictly branch only (tb2) must land on for tb3 first All right, I will try to put the patch (3) and (4), i.e., the ADVANCE check for the would-be-filesize, into tb3 code first. > o core changes (nsLocalFileUnix.cpp etc) first have to land on > mozilla-central, then mozilla-1.9.1 to get into tb3 To be honest, I am a little lost here. Is TB2 code related to mozilla-central? Is there a geneology tree or something like this to learn the relationship of the various branches, etc.. tb2, mozilla-central, mozilla-1.9.1, and tb3? In any case, I will try to modify the mozilla-central code first to reflect the changes I made, namely to fix the problems (1) and (2) above, to nsLocalFileUnix.cpp in tb2. (But it may be that mozilla-central code is more up to date and these problems may not be there any more. Let me investigate and come up with a proper patch anyway.) > o no string changes allowed for tb2 Oh, tough nut to crack. I will see what we can do here. TIA.

ff trunk is mozilla-central - http://hg.mozilla.org/mozilla-central/ ff3.5 is the 1.9.1 branch - http://hg.mozilla.org/releases/mozilla-1.9.1/ tb2 is the 1.8.1 branch in cvs. (We've since moved to mercurial for source control.) tb3 nightlies is hg tip in comm-central - http://hg.mozilla.org/comm-central/ comm-central code includes a copy of the 1.9.1 branch (the pull script will add it for you) To complicate it, there's also a tb3.1 "trunk" (or whatever) nightlies, which are comm-central tip with mozilla-central tip pulled in. These are done just so we'll notice regressions in core code affecting thunderbird. Thunderbird3 will be released from some branching of the 1.9.1 branch. HTH ;)

Thank you. I am afraid it might take me a while to digest the mouthful :-) Anyway, I got the comm-central code using mercurial now and am trying to compile it. (I believe I pulled in 1.9.1 by "python client.py checkout". Once I have the patch completed for comm-central code (the problem (4) mentioned above), I will post it. Once it is accepted, I will prepare the patch for the problem (3) above. Core changes for (1) and (3) actually seem to be solved already in comm-central code pretty much and so I will check if the problem is still there in mozilla-central and 1.9.1. The variety of branches, and the relative difficulty to set up the build environment may explain that many reported bugs have never been worked out somehow. A novice to the TB code gets lost easily. Anyway, I will keep my progress posted. (My PC is old one with 1GHz CPU and 512KB. I will see if I can set up VM on a 2GB, 2GHz dual core note book to speed up the compilation.) I have found out that another colleague of mine also suffered from the bug reported here under windows last year just when he was busy preparing the exhibition (many pamphlets and other documents got exchanged as e-mail attachment in about a month.) So more incentive to fix this problem once for all. TIA.

I am going to upload a patch to nsLocalMailFolder.cpp (universal across platforms) and a patch to nsLocalFileUnix.cpp (specific to unix-like systems, linxu, solaris, macos x, you name it). It took me days to set up a build environment and successfully to compile TB-3 code from comm-central. What a pain (!). Anyway, I will add my comment after the upload later today.

Patch to nsLocalMailFodler.cpp The few line patches to nsMsgFilterService.cpp nsMsgDBFolder.cpp is to catch the non-registered string for the NEW ERROR(that the mail folder will become too big to handle if we copy messages into it, and so we are not copying at all...). Very similar to early patch to the same file in TB2.

Patch to nsLocalFileUnix.cpp The minor patch for freetype2.m4 is to get the compilation done under Debian GNU/Linux. (You can safely ignore it.) I thought the code fixed the problems for large files (32bit), but unfortunately, - it failed to use 64 bit versions in certain places, - it failed to check the return values in many places, - so I cleaned it up pretty much. With these changes to this file and nsLocalMailFolder.cpp (previous patch), I asceratined that TB3 no longer tries to create 4G+ message folder (i.e., a folderfile), and warns us. One caveat, we need to create a new message to show this in a dialog. The patch is raising ThrowAlertMsg("mailboxWillBecomeTooLargeWeHave4or2GBLimit", msgWindow); and so we need a proper message for that. More on other things later.

I meant to say nsLocalFileUnix.cpp (the header subject was cut short. grr), and that "I though *the current code in comm-central* fixed the problems for lage files (32bit+), but ... ".

So now, using the old figure to show the problems, the patches posted lately fix the problems in TB3 (comm-central) codes as follows. ============================================+========= TB 2_0_0_22_RELEASE | 3.0 (comm-central) ---------------------------------------------+--- Proper file size (>=4GB) No. (1) | insufficient, fixed in patch to check using lstat64, stat64 | nsLocalFileUnix.cpp (#40) ---------------------------------------------+---- Failure to the pass the | 64 bit file size thus | Cleaned up in patch (#40) obtainedto upper-level Yes.(2) | routine due to incorrect cast | in GetFileSize. | (An argument to LL_UI2L macro) | ---------------------------------------------+---- Check whether the file | size goes over 4GB Not done.(3) | Now fixed in patch (#39) while copying | nsLocalMailFolder.cpp IN ADVANCE | ---------------------------------------------+---- Creating a proper warning message | This requires attention. for the case where the advance checking | prohibits copying messages. (5) | { (5)' } ---------------------------------------------+-------- So a prompt review of these patches in TB3 (comm-central) and discussion to merge these critical patches to TB3 code will be appreciated. (I am not sure how problem (5)' is handled., by the way.) To be frank, we need to fix this CRITICAL bug in TB2, the current version used by millions(?) of users. But if TB3 needs to be fixed first, so be it. TIA

If the patches are ready for review, please ask reviews from appropriate people https://developer.mozilla.org/en/Getting_your_patch_in_the_tree#Getting_the_patch_reviewed People are likely to want to review patches without the printf and debug stuff though.

I see. I just wanted to show the initial idea (approach to fix the problem, the concrete code) before tidying up the patch. Do appropriate people, then, read these bug reports at the bugzilla. I am a little confused. Anyway, if you have some comments to the patches, I will appreciate to hear them. The patches are very straightforward. (#39) It simply checks the future size of the folder by adding the size of each message in advance, and decide whether the copying should be allowed or not. (#40) This patch just makes sure that 64 bit version of lstat, stat are used everywhere and their return code is checked always, too. Regarding the review process, I will see what I can do in the next couple of days. TIA

The appropriate people may or may not happen to read the reports. Usually you can look at the hg logs who's been reviewing the file lately (the r= and sr= marks) E.g. for nsLocalFileUnix.cpp bsmedberg seems like a good reviewer http://hg.mozilla.org/releases/mozilla-1.9.1/log/c21d4f5fb12a/xpcom/io/nsLocalFileUnix.cpp

Thank you, Magnus. Now I figured out how to find out the possible reviewer. I am going to post slightly cleaned up patches shortly for review. (I still have some debug code left, but I will remove it once the general direction of the patch was deemed OK.)

Clean up to call lstat64, stat64 in all the appropriate places. (Some places are missed in the current code). Check the return value of lstat{,64}, stat{,64} in all the places. Again no check is done in a few places in the current code. These changes are necessary to propery handle 64 bit file size (where appropriate). In the patch, the first section is for freetype2.m4 is a configuration issue for Debian GNU/Linux and please ignore it. For completelness sake, not to miss any diff, I simply gathered all the output of "hg diff" under mozilla directory of comm-central code.

Check for the would-be 4GB+ file size BEFORE copying of message(s) into a mail folder is performed. If the resulting size is >4GB, then the copying is not performed and error is raised. Pretty straightforward check.

Comment on attachment 387146 [details] [diff] [review] Patch to TB3(comm-central) nsLocalMailFolder.cpp to fix 4GB+ file size problem. thx for working onthis. + + if(!NS_SUCCEEDED(rv)) + { + NS_ASSERTION(PR_FALSE, "GetSizeOnDisk: call to GetFilSize failed"); + abort(); + } + we don't abort() the app like this - instead we would return an error and stop the process of adding messages to the folder... why don't you want to use 64 bit math? That would make the code easier to read. This error message doesn't exist: mailboxWillBecomeTooLargeWeHave4or2GBLimit Depending on where you need the function to add up the sizes of an array of messages, it could either be a function in nsMsDBFolder.cpp, or a helper routine in base/util/nsMsgUtils.cpp

>(From update of attachment 387146 [details] [diff] [review]) >thx for working onthis. You are welcome. (a) >+ >+ if(!NS_SUCCEEDED(rv)) >+ { >+ NS_ASSERTION(PR_FALSE, "GetSizeOnDisk: call to GetFilSize failed"); >+ abort(); >+ } >+ > >we don't abort() the app like this - instead we would return an error and stop >the process of adding messages to the folder... --> OK, I was trying to catch problematic case where internally "SHOULD NOT FAIL syscalls somehow fail", but I can live with removing abort() here for now. [However, some day into the future, I will post my rant about the shoddy code practice of not checking the return values of low-level routines, or if the value is checked, no remedial action or emergency bail out action is taken. These cases are rampant in TB2 and TB3 code where I checked. This 4GB size file problem would have been caught much earlier, maybe in 2007 or earlier if the low-level code such as nsLocalFileUnix.cpp is written with such better code practice in mind. Where should I post such a rant. Is bugzilla one of the venues? ] (b) >why don't you want to use 64 bit math? That would make the code easier to read. I agree, but I didn't do this because in TB2 code (and TB3 code to some extent), there are cases where explicit 64 bit arithmetic is avoided. I think this may be dated back the day when some platforms have compilers that didn't support 64 bit integer (a la long long) data types. Since the code is written in that way (please note that nsLocalFileUnix.cpp is one such file. In one place, the large data size returned by stat64() is not copied diretly, but a macro LL_UI2L is used to copy the data (at least the lower 32 bits portion) to 32-bit entity.) Historical background: When large file support came into POSIX (system call side), since POSIX API was offered as C-binding, there were discussions about how to represent large (>32 bits) file sizes and offsets, etc. when the native compiler on that system doesn't support large scalar. The agreed approach was to define an opaque data type that can store such largish size and define appropriate macros, etc. which can work on these data. I think the use of LL_UI2L macro in nsLocalFileUnix.cpp was based on a similar approach. Also, in one place I found that explicit multiplication to produce 64 bit entity from two 32 bit entities were avoided and instead a special macro is used. (This is done to return a time interval in milliseconds unit instead of second unit.) So judging from reading the pieces of code like these above, I decided TB{2,3} codes try to avoid explicit 64 bit arithmetic (add/subtract/multiplication, etc.) [a simple assignment is OK, though. Even in the old POSIX days, opaque data types can be assigned, passed to functions, etc. without the support of large scalar on the compiler side.] That is one reason why I decided to avoid 64 bit arithmetic. Another reason is that the code here needs to work eve when lstat64, stat64, and long long are unavailable. We should be able to check that copying message will fail due to 4GB limit IN ADVANCE under such restricted environment also. Thus pure 32 bit arithmetic is preferred. The above is the reason for somewhat contorted code. But the clarifying comment should avoid any misunderstanding and the code in question is very localized. (c) >This error message doesn't exist: >mailboxWillBecomeTooLargeWeHave4or2GBLimit Yes, I know. Previously, the message was like the file folder grew too large to handle (essential meaning.) But the new check is that the file folder grows too big if we copy messages into it, and so copying is not done. We need a different message here. (I read in a previous comment, the message catalogue is fixed for TB2. Tough luck. But for TB3, we can do better, can't we?) This is touched as follows in my earlier comment. TB2 TB3 >---------------------------------------------+---- >Creating a proper warning message | This requires attention. >for the case where the advance checking | >prohibits copying messages. (5) | { (5)' } >---------------------------------------------+-------- I need someone's guidance regarding what to do here to put such a elucidating message into the new error message catalogue. (d) >Depending on where you need the function to add up the sizes of an array of >messages, it could either be a function in nsMsDBFolder.cpp, or a helper >routine in base/util/nsMsgUtils.cpp To be honest, I doubt if summing up the sizes like this is often required, and so I feel it is OK to leave the added code in the current place. The ONLY piece of code which I felt might be better off packaged in a function is the block to obtain the file size in advance. (line 1734-1767) But this could also be left there... (For some reason, calling a function RerfeshSizeOnDisk() and using mFolderSize didn't seem to work when I tried to produce patch for TB2. I still don't know the details of TB{2,3} code structure well. I will modify my patch(es) after these issues are discussed and some consensus emerge. TIA.

the reasons for avoiding 64 bit math are long gone; the fact that there's still legacy code that does so doesn't mean new code should do the same. I agree about error checking, but abort() the process is not a nice way of handling errors.

(In reply to comment #50) > [However, some day into the future, I will post my rant about the > shoddy code practice of not checking the return values of > low-level routines, or if the value is checked, no remedial action > or emergency bail out action is taken. These cases are rampant in TB2 and TB3 > code where I checked. Indeed, there many unfortunate cases of this in the tree. > This 4GB size file problem would have been caught much earlier, maybe > in 2007 or earlier if the low-level code such as nsLocalFileUnix.cpp > is written with such better code practice in mind. Where should > I post such a rant. Is bugzilla one of the venues? ] A blog is probably a better place for such a rant. > Another reason is that the code here needs > to work eve when lstat64, stat64, and long long are unavailable. Which of the environments that Gecko supports meet this criteria? Generally speaking, I would much prefer to trade support for ancient systems away in exchange for more readable (and thus more maintainable and less buggy) code. > (I read in a previous comment, the message catalogue is fixed for TB2. > Tough luck. But for TB3, we can do better, can't we?) Yes, definitely. > This is touched as follows in my earlier comment. > TB2 TB3 > >---------------------------------------------+---- > >Creating a proper warning message | This requires attention. > >for the case where the advance checking | > >prohibits copying messages. (5) | { (5)' } > >---------------------------------------------+-------- > > I need someone's guidance regarding what to do here to put such a > elucidating message into the new error message catalogue. Depending on how this message gets surfaced through the UI, one of the following pages is likely to be helpful: https://developer.mozilla.org/en/XUL_Tutorial/Localization https://developer.mozilla.org/en/XUL_Tutorial/Property_Files

(In reply to comment #50) > --> OK, I was trying to catch problematic case where internally "SHOULD NOT > FAIL syscalls somehow fail", but I can live with removing abort() here for now. The standard NS_ENSURE_SUCCESS macro tends to ensure that the operation fails instead of brutally crashing the entire browser. > Where should I post such a rant. Is bugzilla one of the venues? No. > Since the code is written in that way (please note that nsLocalFileUnix.cpp > is one such file. In one place, the large data size returned by > stat64() is not copied diretly, but a macro LL_UI2L is used > to copy the data (at least the lower 32 bits portion) to 32-bit entity.) I have been told that the LL_* macros are now deprecated and should be stripped out if you're touching the code. > Another reason is that the code here needs > to work eve when lstat64, stat64, and long long are unavailable. A cursory examination suggests to me that this code might fail on big-endian architectures, which is probably more common than 64-bit-less architectures. > To be honest, I doubt if summing up the sizes like this > is often required, and so I feel it is OK to leave the added code in > the current place. Well, you said it should be fixed in another case. > I will modify my patch(es) after these issues are discussed and > some consensus emerge. I don't think bsmedberg knows about the patch yet, so I'll CC him

Thank you all for various comments. (a) I will remove abort() from this part of the code. [I will pick up one of the mozilla newsgroup to post my rant about the code quality/bad practice, and then decide whether I will host a blog on this topic depending on the response. A mailer needs better code IMHO.] (b) 64bit cleanliness. Nobody seems to defend the old stance to do with 32-bit arithmetic as much as possible, and so I will simply rewrite this part. Does anyone know any written page about the demise of LL_* macros? I am not sure what to do with the multiplication(?) macro [well, I assumed it was a multiplication macro], but will simply remove the use of it in one place where {l,}stat64 is used in nsLocalFileUnix.cpp. Re endian issue, the patch prints for debugging purposes a long long entity assuming little-endian layout. (A good catch.) But aside from the debug output done thusly, the code should work on big endian and 32bit only machine as such. Only for the purpose of printing Long Long entity for debugging, I assumed little-endianness. [I don't know if I will keep this debug output portion in the final patch.] (c) Missing message. I will follow the advice here. >Depending on how this message gets surfaced through the UI, one of the >following pages is likely to be helpful: > >https://developer.mozilla.org/en/XUL_Tutorial/Localization >https://developer.mozilla.org/en/XUL_Tutorial/Property_Files But more hurdles to clear. It will be another couple of weeks to figure this out. I wonder if mozilla's choice of letting the maintainer ONLY REVIEW the patch, and not write the patches themselves is a good idea after all. But I digress. (d) Creating a small helper function. I am not sure if I will do this or not in the end. We will see as we polish the patches. I will re-create the patch in a day or two. So any more comments will be appreciated. TIA

Re the localization, look at the other uses of stringbundle in nsLocalMailFolder.cpp and just get the localized strings the same way.

Comment on attachment 387143 [details] [diff] [review] patch to TB3(comm-central 1.9.1) nsLocalFileUnix.cpp for stat64 cleanliness The nsLocalFileUnix changes are bug 389087

nsLocalMailFolder.cpp - Calls to abort() removed. (Does NS_ASSERTION() abort, BTW?) 64bit arithmetic is used for readability. Didn't create a helper function. (I deemed it to be an overkill.) TODO/FIXME: Missing message. Still trying to figure out what to do. (How do we tell other L10N people about the new message? Yes, the question is more like the work flow rather than preparing the initial English-only fix.) TIA.

(Sorry that you see this comment twice. Failed to attach the patch properly.) nsLocalMailFolder.cpp - Calls to abort() removed. (Does NS_ASSERTION() abort, BTW?) 64bit arithmetic is used for readability. Didn't create a helper function. (I deemed it to be an overkill.) TODO/FIXME: Missing message. Still trying to figure out what to do. (How do we tell other L10N people about the new message? Yes, the question is more like the work flow rather than preparing the initial English-only fix.) TIA.

To comment # 56, I will investigate the patch for nsLocalFileUnix.cpp. The use of the macros is very similar to what I did to propose a change for the file in TB-2. The patch in >------- Comment #31 From ishikawa, chiaki 2009-06-23 08:05:41 PDT ------- > >Created an attachment (id=384624) [details] >patch for nsLocalFileUnix.cpp I would suggest using a prefixed macro names and use a different macro name for struct and function, but if it works, don't break it :-) I see a place (or two?) where return values are not checked where I think checking is required. (For example, near the line 1463). I wonder why it was missed. (Don't tell me CHECK_mpath() has done that already, using, gasp, lstat{,64}, stat{,64} and friends(!) [During testing the patch, I noticed FillStatCache is called too many times to my taste. Waste of system calls. Maybe this is one reason?] Anyway, I will get back with a little more detail.

(In reply to comment #58) > TODO/FIXME: Missing message. Still trying to figure out what to do. > (How do we tell other L10N people about the new message? > Yes, the question is more like the work flow rather than preparing > the initial English-only fix.) You only add the english values. Localizers have ways to notice (they check e.g. http://tinderbox.mozilla.org/showbuilds.cgi?tree=Mozilla-l10n-ja, there's some scripts to check all is ok etc), and will add the values to their own localization when they do.

(In reply to comment #60) Thank you. I see that I only need to add the English message. I will fix this over the weekend, then. TIA.

This adds the error message we pass to ThrowAlertMsg() when the folder woud become too large, and copying is not performed. Exactly the same patch applies both to - mail/locales/en-US/chrome/messenger/messenger.properties - suite/locales/en-US/chrome/mailnews/messenger.properties (I don't know which is used or whey there are two similar [same] files.) So I am only attaching the patch file for mail/locales/en-US/chrome/messenger/messenger.properties Please study the appropriateness of the new message. I will modify the grammer, etc. and create a new patch if better message is proposed. TIA

(In reply to comment #62) > Exactly the same patch applies both to > - mail/locales/en-US/chrome/messenger/messenger.properties > - suite/locales/en-US/chrome/mailnews/messenger.properties > > (I don't know which is used or whey there are two similar [same] files.) One is for Thunderbird, the other is for SeaMonkey. > So I am only attaching the patch file for > mail/locales/en-US/chrome/messenger/messenger.properties For ease of checkin purposes, it would probably be better to include both.

Per suggestion in commet #63, I am attaching a new patch that fixes the two files. ---- (In reply to comment #62) > Exactly the same patch applies both to > - mail/locales/en-US/chrome/messenger/messenger.properties > - suite/locales/en-US/chrome/mailnews/messenger.properties > > (I don't know which is used or whey there are two similar [same] files.) One is for Thunderbird, the other is for SeaMonkey. > So I am only attaching the patch file for > mail/locales/en-US/chrome/messenger/messenger.properties For ease of checkin purposes, it would probably be better to include both. One is for Thunderbird, the other is for SeaMonkey. ---- TIA

To comment #56 and comment #59 I posted a comment to Bug 389087 (actually comment #18 and #19 there) The proposed patch in 389087 missed checks for lstat{,lstat64} in two places. I wonder how this could be put into comm-central code [the bug is closed there.]

Best to create a new bug for that (you can clone bug 389087). It will be used in comm-central after the 1.9.1 branch patch gets approved and is checked in.

Thank you for the comment. I am ready to "clone" the bug. Is there a specific procedure? From what I found out, I think I basically "file a new bug", and just mention that the bug fix is a continuation of bug 389087? (But I am not sure if I can fill in all the details as the mirror image of bug 389087, say, for example, the affected component, etc. Maybe I was not paying enough attention when I filed a few new bugs before.) If there is no special method/manner to "clone" a bug, I will simply file a bug. TIA

You can use the "Clone This Bug" link and make the component the same as the original, or just do it manually.

Additional fix for nsLocalFileUnix.cpp has been re-filed as bug 505517 "nsLocalFileUnix.cpp fails to check the return value of lstat{,64} in a couple of places.", so that will be the place where nsLocalFileUnix.cpp patch will be discussed in the future, hopefully. nsLocalMailFolder.cpp continues to be discussed in bug 387502 (here). [PS: somehow "clone this bug" didn't work as I had expected and so a few entries are messed up :-( ]

Just to show the people concerned with this bug that TB3 with the patch(es) can check whether the folder will become too large when message(s) are inserted, and if so, doesn't copy at all. The attach screendump shows the pop-up created by the addition of new error message thrown under this error condition. On the left, the debug messages shows that in one instance (above), the copying was done, while another copy was aborted due to the size check.

Comment on attachment 389083 [details] [diff] [review] Patch to TB3(comm-central) nsLocalMailFolder.cpp to fix 4GB+ file size problem. thx for the patch. Please remove all the #if debug code space between if and ( here: + if(!NS_SUCCEEDED(rv)) + { + use NS_ERROR here: NS_ASSERTION(PR_FALSE, "GetSizeOnDisk: call to GetFilSize failed"); lose this comment - we're not going to abort. + //We really should die here, but we forsake abort() fow now. + } you don't need to cite bug #'s - we can get that from our web tools: + // To try to warn the 4GB size limit BEFORE copying takes place. + // bug # 387052 typo - Refresh: + // cf. RerfeshSizeOnDisk() and using mFolderSize doesn't seem to work. Please remove this comment: + // Following block COULD BE a function, but I am not familiar + // with TB's coding style and organization to decide + // where to put such a function. use NS_ERROR and remove comment here: + NS_ASSERTION(PR_FALSE, "CopyMessages: call to GetFilSize failed. This is FATAL."); + // We reall should die here, but we forsake abort() for now. please remove this comment: + // The next loop is modeled after the loop in comm-central code . + // We can use pure 32 bit arithmetic for overflow check + // by declaring temp, newmfsize to be PRUint32 instead of PRUint64. + // The overflow occurs if ((temp <= newmfsize ) || (temp <= size ) || (temp <= 200)) + // But we use LONG LONG arithmetic in 2009. please remove this comment: + // member function : GetLength for comm-central code (TB3) + // Count for TB2 code. space between for and (, and fix indentation of { here: + for(PRUint32 i = 0; i < numMsgs2; i++) + { this comment is not so useful - maybe just say /* 200 is a per-message overhead to account for any extra data added */ : + /* 200 is the overhead mentioned in bugzila report.*/ please add the strings you added in the next patch, so I can apply a single patch to try.

Thank you for your comment. I will prepare a patch that incorporates the new strings as well and post it over the weekend. TIA

Cleaned up patch based on comment # 71. This also includes the string patches to a couple of messanger.properties files.

Comment on attachment 392140 [details] [diff] [review] Patch to TB3 (comm-central) nsLocalMailFolder.cpp to fix 4GB+ file size problem also with a set of string changes. >@@ -105,16 +105,17 @@ > #include "nsNetUtil.h" > #include "nsIMsgFolderNotificationService.h" > #include "nsReadLine.h" > #include "nsLocalStrings.h" > #include "nsArrayUtils.h" > #include "nsIMsgTraitService.h" > #include "nsIStringEnumerator.h" > >+ please don't add this random blank line >@@ -1412,18 +1413,25 @@ > nsresult rv = NS_OK; > if (!mFolderSize) > { > nsCOMPtr <nsILocalFile> file; > rv = GetFilePath(getter_AddRefs(file)); > NS_ENSURE_SUCCESS(rv, rv); > PRInt64 folderSize; > rv = file->GetFileSize(&folderSize); >+ >+ if(!NS_SUCCEEDED(rv)) NS_FAILED >+ { please don't use this indentation style, see the style used at the beginning of this block ('{' aligns with 'if' from previous line, and there's a space after 'if') >+ NS_ERROR("GetSizeOnDisk: call to GetFilSize failed"); Please spell 'GetFileSize' correctly >@@ -1703,16 +1711,101 @@ >+ if( temp >= 0x100000000ULL ) no spaces on the inside of ()s, but one space after 'if' >+ { >+ sizecheck = PR_TRUE; >+ break; please omit the 'else' after this 'break'. ('else after return', 'else after break', ...) >+ } >+ else >+ newmfsize = temp; >+ } >+ please omit trailing spaces from lines >+ // end of 4GB size check we don't need that comment, or this extra blank line: >+ > > // don't update the counts in the dest folder until it is all over >+mailboxWillBecomeTooLargeWeHave4or2GBLimit=Copying message(s) will make the destination folder too large to handle. Copying is not performed. Please divide it into small folders and/or compactify it. "compact"

Thanks for the comment. I am going to post a modified patch shortly. A few comments. - I removed the random blank lines. Sorry, I was in a rush and didn't think much of them. - Re "if" indentation style or rather the "{" that follows "if" or "for". I use Emacs for editing. Does any Emacs hacker among the readers can suggest a proper way of indenting "{", "}" in mozilla source files? I noticed that there is a mode line like this at the beginning of the source files: /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ But this is not enough for the proper mozilla indentation of "{" and "}" in vanilla emacs and C-mode. I think we need to add "(list (cons 'block-open 0) (cons 'block-close 0))" at the beginning of the variable c-offsets-alist. I just checked and it works. However, I am not sure of how to write it in the mode line. Writing c-offsets-alist: (append (list (cons 'block-open 0) (cons 'block-close 0)) c-offsets-alist) in the mode line seems a little too overkill. (And this might even alert a user of a function being invoked.) Any suggestions or tips? I don't want to make this style universal since I occasionally fix linux drivers and often report patches to programs from GNU project where different indentation is used. Any tips are appreciated. - Space between "If" and "(". I fixed the spacing in my patch, but I noticed many places where "if(" appears without a space in the file. I left them as is. - Fixed spelling. - Not sure where extra trailing spaces were. I hope the new patch fixed it anyway. - "compact" vs "compactify". Yes, I agree "compact" is it. I am afraid that I am too much influenced by early LISP literature such as "Multiprocessing compactifying garbage collection", Volume 18 , Issue 9 (September 1975). The word "compactifying" is used often inside memory allocator source code, but I digress. Anyway an improved patch follows.

The improved patch explained in comment # 75 (incorporated the requested changes in the comment # 74).

Sorry, unintended local patch crept into the previously uploaded patch. Please use this one.

Comment on attachment 392526 [details] [diff] [review] Patch to TB3 (comm-central) nsLocalMailFolder.cpp to fix 4GB+ file size problem Patch to TB3 (comm-central) nsLocalMailFolder.cpp to fix 4GB+ file size problem also with a set of string changes. Switching to bienvenu - I don't know who fxm is.

(In reply to comment #75) > I noticed that there is a mode line like this at the beginning of the source > files: > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > > But this is not enough for the proper mozilla indentation of "{" and "}" in > vanilla emacs and C-mode. That is the modeline requested by the coding standard (see https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide). > I don't want to make this style universal since I occasionally fix linux > drivers and often report > patches to programs from GNU project where different indentation is used. > Any tips are appreciated. While I don't use emacs myself, I'm sure there is some option that allows you to conditionally set styles on a per-directory basis--this is how my editor setup works (everything within /src/trunk/ gets my mozilla settings).

To comment # 78, thank you for fixing the mistake. I am not sure what happened. I thought I set the reviewer address to bienvenu, but I somehow screwed up obviously. To comment # 79, thank you for the idea of per-directory setting. I will go and ask around Emacs newsgroup.

I took chiaki's patch (thx!) and tweaked it a bit - changed the error message text, and moved the total size calculation into the existing loop over the messages to be copied.

Thank you for the improved patch, David. I just noticed a couple of things. (1) typo. + // check if the folder size + the size of the messages will be > 4MB or so. 4MB should read 4GB (and actually is less about 50MB from 4GB to account for any problems. 0xFFC00000). (2) re message (Having the name of the folder in the message is a good idea indeed, but I was not familiar how to do it. thanks for fixing this. During testing, I was puzzled to see moving was performed when I deleted a few messages. They were actually copied to Trash folder.) By having combined the size calculation, we will copy messages until the size check interrupts (correct me if I am wrong). So this means that there are situations when some messages are copied and the remaining ones are not copied. Previously in my suggested patch, the size check is done wholesale in advance, and thus copy/moving is either all go or no go (no copy/move was done at all). In this sense, maybe the text in the improved message "... The move/copy was not performed. ..." should read "... The move/copy was interrupted. ..." ? (Or there may be a better English phrasing.) Just my two cents worth. TIA

Comment on attachment 392865 [details] [diff] [review] tweaks to previous patch > NS_ENSURE_SUCCESS(rv, rv); > PRInt64 folderSize; > rv = file->GetFileSize(&folderSize); >+ if (!NS_SUCCEEDED(rv)) >+ { >+ NS_ERROR("GetSizeOnDisk: call to GetFileSize failed"); >+ } > mFolderSize = (PRUint32) folderSize; Surely NS_ENSURE_SUCCESS(rv, rv); would be safer - if you think the call may fail, then no point using the bogus value. >@@ -1678,23 +1682,47 @@ nsMsgLocalMailFolder::CopyMessages(nsIMs You can't see it in context, but we've just done the basic 0xFFC00000 size check. We're then going to do the check again, but adding on the estimated message size. This means that the first check is now redundant. >+ if (NS_FAILED(rv)) >+ { >+ OnCopyCompleted(srcSupport, PR_FALSE); >+ return rv; >+ } Need to also call HandleDeleteOrMoveMsgFailed in the move case. >+ nsCOMPtr<nsIMsgDBHdr> message; >+ messages->QueryElementAt(i, NS_GET_IID(nsIMsgDBHdr),(void **)getter_AddRefs(message)); Nit: could fix this to nsCOMPtr<nsIMsgDBHdr> message(do_QueryElementAt(messages, i, &rv)) >+ if(NS_SUCCEEDED(rv) && message) Assuming you want the rv ;-) >+ // check if the folder size + the size of the messages will be > 4MB or so. >+ // If so, warn, and return an error. >+ if (sizeOnDisk + totalMsgSize > 0xFFC00000) >+ { >+ ThrowAlertMsg("mailboxWillBecomeTooLarge", msgWindow); >+ return OnCopyCompleted(srcSupport, PR_FALSE); HandleDeleteOrMoveMsgFailed again. You'd probably be better off computing the additional size first, then trying to get the current size, and then doing if (NS_FAILED(rv) || sizeOnDisk + totalMsgSize > 0xFFC00000) > // don't update the counts in the dest folder until it is all over >@@ -3839,6 +3875,11 @@ nsMsgLocalMailFolder::WarnIfLocalFileToo See above about this method now becoming redundant. You could keep the string though; I think it reads better than the new one here.

To comment # 79, For users of emacs version 23.1 (the latest as of now), putting in a file named ".dir-locals.el" at the top of local TB3 source tree with contents like the following will do the trick of setting the desired indentation behavior (coupled with the properly placed mode line.) ((c-mode . ((c-file-style . "BSD"))) (c++-mode . ((c-file-style . "BSD"))) (java-mode . ((c-file-style . "BSD")))) (See Info (emacs) Directory Variables ) I think I will ask someone to edit the the coding standard page to mention this. https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide). Re comment # 83, will David handle the additional fix? (Some of the comments are beyond my immediate comprehension. I need to study the code more.)

yes, I intend to address your and Neil's comments today.

grumble, I forgot to qrefresh before posting that last patch, so I'd already fixed some of those issues. I'll post a new patch shortly.

I got rid of the call to WarnIfLocalFileTooBig but there are other callers so I can't remove the method. I did re-use its string and removed the new strings from the patch. To answer chiaki's question, this code is run before we start move/copying any messages, so we won't be move/copying any messages if the resulting folder would be too big. We won't copy some of the messages, in other words.

Comment on attachment 392974 [details] [diff] [review] fix addressing Neil's comments I've fixed the line ending issue on the do_QueryElementAt line in my tree...

(In reply to comment #87) > I got rid of the call to WarnIfLocalFileTooBig but there are other callers so I > can't remove the method. Sorry, I didn't notice that it was in the .idl too.

Comment on attachment 392974 [details] [diff] [review] fix addressing Neil's comments Interdiff looks good to me.

fix checked in, thx for the patch, Chiaki.

Comment on attachment 392974 [details] [diff] [review] fix addressing Neil's comments marking r=me.

Thank you, David. I was not sure how java code and this C++ code interacts. So my take is the next version of TB3 will be free of this problem. Great! I guess I will file a bugzilla entry to retrofit this and other change (lstat64 and friends handling) to TB2.0.0.2y series since at least three of colleagues who got bitten with this problem in the last 12 months are not going to switch over to beta version any time soon. Thank you again everybody for comments and helpful tips.

Backported patch of this bug for TB 2.0.0.2x series.

Comment on attachment 413983 [details] [diff] [review] Patch for TB 2.0.0.23 of this bug. review requested.

Hi, I uploaded a backported patch to nsLocalMailFolder.cpp for TB 2.0.0.23 As was pointed earlier, comment 34, > o fixes that aren't strictly branch only (tb2) must land on for tb3 first > o core changes (nsLocalFileUnix.cpp etc) first have to land on > mozilla-central, then mozilla-1.9.1 to get into tb3 > o no string changes allowed for tb2 This fix has already landed in tb3 (part of tb3b4 now), and the accompanying fix for nsLocalFileUnix.{cpp,h} have also made it into tb3. Luckily, contrary to my earlier misunderstanding, no string changes are necessary (!). Hence, it is about time to put this bug to rest in TB 2.0.0.2x series. I created the uploaded patch by comparing the latest file in TB3 with the earlier TB2 modification proposed and reflect the patch done by David into 2.0.0.23. It works. I have checked the alert that is generated when a folder is about to go over 4GB limit. (good!) PS: I initially createad a separate entry for this new patch ( Bug 530400 ),but I was advised to attach this patch to the existing entry. PPS: TB3 is about a few months away for public release, and not everybody switches to .0 version public release right away, and so I think it is prudent to fix this data-loss bug for 2.0.0.23 (and later versions). TIA

(In reply to comment #97) > Hence, it is about time to put this bug to rest in TB 2.0.0.2x series. This bug still stays fixed as it is fixed in the latest code.

(In reply to comment #98) > (In reply to comment #97) > > Hence, it is about time to put this bug to rest in TB 2.0.0.2x series. > > This bug still stays fixed as it is fixed in the latest code. I am afraid that Mark and I may be referring to different code bases. I am referring to the released 2.0.0.23 source code and its binary. Is there a newer still to be released 2.0.0.24 code? At least, the bug as I see it is not fixed in 2.0.0.23 binaries. (At least on both linux and win win/xp 32 version.) They both happily created mail folders larger than 4GB. [CAUTION: You ought to copy a group of messages that amounts to more than a few megabytes at a time to create such a folder. The current incorrect code checks that there is a room of about a few meg bytes in the destination folder before copying takes place, and never bothers to check that total amount messages copied into it is more than such remaining space.] (But that doesn't mean they can handle such large folders later on. Well, at least not on unix-based code.) So if there is a newer TB2 source code from which a binary will be released, fine. But I just wanted to be sure that we are looking at different code base or not. I am talking about TB*2(TWO)* here and the public release version 2.0.0.23. TIA

TB 3 is a few days from release, not a few months.

In Mozilla projects, we mark bugs as 'FIXED' when they have been checked into the latest development repository. In this case, that is probably comm-central: http://hg.mozilla.org/comm-central Thunderbird 3 development branched from comm-central recently, I'm not sure if your patch made it in before that or not. Thunderbird 2 development is still in CVS on an older branch. Your patch can certainly land there (with the proper approvals), but that will be tracked in bugzilla with separate keywords. We leave bugs as "RESOLVED FIXED" in this case even though they are not fixed on branch releases.

In reply to Comment 100 Wow, that is good news. (Funny I must have missed the RC1, etc. I have been busy with other things for the last few weeks.) In reply to Comment 101 Thunderbird 3 has the patch in there all right if I am not mistaken. Thunderbird 2 needs this patch. OK, I am fine with "RESOLVED FIXED" keyword here now. But then how can I push this patch to tb2 branch. I understand that TB3 now requires utmost attention, but I also have to remind the maijtainers that there will be many users of TB2 who don't switch to major revision especially .0 revision right away, and these are the folks who may get affected by this bug in TB2. (Hmm, unless I was told that this patch needs to land in TB3 branch first, I may not have begun using TB3 on one of my machines, and all my PCs would have run all TB2.0.0.23. There are conservative users out there to be sure.) Anyway, thank you for the clarification. I would like to thank David Bienvenu who already has given thumb up for the patch(es) by reviewing. (I just noticed it as I am writing this message. THX.) Now, I will wait for the other maintainers to take a look and merge it for the next release of TB2.0.0.24. My colleagues who have been bitten with this bug over the last couple of years will be happy to see either TB2.0.0.24 with the fix or new TB3 soon. TIA

(In reply to comment #102) > OK, I am fine with "RESOLVED FIXED" keyword here now. > > But then how can I push this patch to tb2 branch. As I've said to you before, once the patch has appropriate reviews, you need to go onto the patch and set 'approval1.8.1.next' to ?. That will then alert branch drivers to consider the patch for the security & stability releases of Thunderbird 2.

(In reply to comment #103) > As I've said to you before, once the patch has appropriate reviews, you need to > go onto the patch and set 'approval1.8.1.next' to ?. That will then alert > branch drivers to consider the patch for the security & stability releases of > Thunderbird 2. Thank you for the pointer. I will wait for a couple of weeks so that reviewers have time to look at the patch. When I get the appropriate reviews, I will set approval1.8.1next as you say. (It is hard for Sunday programmers to figure out these details for thunderbird development, so to speak. We probably need a Wiki-like information page which gets updated now and then by wider community: they may not be up-to-date all the time or correct 100% of the times, but from there, one could produce a useful guidance page every quarter or so. But, I digress.) TIA.

A brown bag Correction. (In reply to comment #102) > I would like to thank David Bienvenu who already has given thumb up > for the patch(es) by reviewing. (I just noticed it as I am writing this > message. THX.) Sorry, I got confused. It was not David. He still has yet to approve or disapprove of the proposed patch. I should have stated that it was Benjamin Smedberg who gave a thumb up by giving the approval for the OTHER patch uploaded at the same time as this patch, for Bug 389087. That patch is needed on top of this patch for POSIX-like systems (linux, solaris, et al.) Again, sorry for the confusion. The mail notice from bugzilla came in and I got confused and thought it was for this bug.

see also http://kb.mozillazine.org/Edit_large_mbox_files http://kb.mozillazine.org/Limits_-_Thunderbird#Folders_and_messages

Comment on attachment 413983 [details] [diff] [review] Patch for TB 2.0.0.23 of this bug. looks reasonable, thx.

(In reply to comment #108) > (From update of attachment 413983 [details] [diff] [review]) > looks reasonable, thx. Since David gave the approval, I took the liberty of setting approval.1.8.1.next to '?' as was suggested in Comment 103. TIA

Comment on attachment 413983 [details] [diff] [review] Patch for TB 2.0.0.23 of this bug. Approved for 1.8.1.24, a=dveditz for release-drivers

I wonder if it would have been simpler to just allow TB to automatically create new secondary mbox files for any mailbox greater than a given size... For example, when an mbox file goes over a pre-defined size (configurable with max?) - it could simply create a 'child' - say, Mbox.1 - and carry on as usual. Then all that would be needed is code to manage things when there is more than one - ie, 'combine' the parent/child mbox files into a coherent view (e.g., Inbox+Inbox.1+Inbox.2 would all be seen in the folder pane as just 'Inbox'), manage what happens when user adds/deletes a lot of messages (maybe some could be recombined, indexes rebuilt, etc)... Seems like this would be better in the long run anyway...

I thought about that, and it would have been quite a bit more complicated. The "all that would be needed" part touches a lot of code.

Yeah, I figured as much (but didn't see it ever specifically mentioned, so...) I know enough about programming to know that some things that can seem really easy/simple, can actually be quite complex to code properly - and vice-versa... Follow up question if you don't mind... Does TB3 suffer from the same limitation even on 64 bit systems? Ie, is this an OS level problem, or a TB code problem? Specifically, will a 64 bit version of Windows have this same limitation?

TB uses a 32 bit value as the offset into the berkeley mailbox of a message, so, yes, a 64 bit version of Windows will have this same limitation. I plan on switching to a 64 bit int for the offset at some point, but supporting formats like maildir is also a priority, since giant berkeley mailboxes don't perform well for things like compaction.

(In reply to comment #114) > I plan on switching to a 64 bit int for the offset at some point, but > supporting formats like maildir is also a priority, since giant berkeley > mailboxes don't perform well for things like compaction. !!! this is really good news... I would *love* for TB to support maildir directly... Do you have even a very rough guesstimate as to a timeline? Are we talking 3.2? Or 5.0? ;)

yes, we're talking 3.2, roughly.

Awesome... thanks David. :)

A way to recover mail data is "split file to f-1 and f-2". f-1 : data from offset=0 to 2GB-1 (2GB data) + 0x0D0A([CRLF]) if "From " is splitted, change size. Restart Tb, wait for rebuild-index of f-1. Show "Order Received" column, sort in ascending order. XXX : "Order Received" column value of bottom most mail(XXX is largest). f-2 : data from offset=XXX to end of file larger than 4GB. Restart Tb, wait for rebuild-index of f-2. Last mail in f-1 can be partial mail of first mail in f-2. Remove it if last mail in f-1 is really partial mail of first mail in f-2.

(In reply to comment #119) > *** Bug 556036 has been marked as a duplicate of this bug. *** Is the reporter of Bug 556036 using TB 3.x series or TB 2.x series? I thought TB3.x series (reasonably new one) would warn when the folder goes over certain limit. [But I am familiar with ONLY WITH POP3 server operation and it is possible IMAP may have a different code path??? But it is unlikely. ]

IMAP offline store has a completely different code path, but 3.1 essentially has no limit on the size of the IMAP offline store, unlike the pop3 store, which still has a 4GB limit.

(In reply to comment #121) > IMAP offline store has a completely different code path, but 3.1 essentially > has no limit on the size of the IMAP offline store, unlike the pop3 store, > which still has a 4GB limit. Interesting... I thought this was an issue with respect to mbox file sizes, meaning, not specific to POP3 or IMAP at all... but you're saying I can have a local fully sync'd mbox file for an IMAP account greater than 4GB?

Is there any interest at all in allowing more than 4GB for a POP store? 3.1.2 x64-build ran into it: moving messages into new folder & deleting the original Inbox allowed for a new one to receive new messages.

(In reply to comment #124) > Is there any interest at all in allowing more than 4GB for a POP store? 3.1.2 > x64-build ran into it: moving messages into new folder & deleting the original > Inbox allowed for a new one to receive new messages. Yes, I'm working on it - see bug 402392

Local "Sent" folder case was special and was not resolved by this bug. Setting dependency to Bug 598104 for ease of tracking.