Last Comment Bug 387502 - (tb-4gb) Mailboxes are allowed to grow larger than 4gb in size
(tb-4gb)
: Mailboxes are allowed to grow larger than 4gb in size
Status: RESOLVED FIXED
[repair info comment 107] [see Bug 59...
: dataloss, fixed1.8.1.24
Product: MailNews Core
Classification: Components
Component: Database (show other bugs)
: unspecified
: All All
: -- critical with 3 votes (vote)
: Thunderbird 3.0b4
Assigned To: ISHIKAWA, Chiaki
:
Mentors:
http://kb.mozillazine.org/Limits_-_Th...
: 477430 497554 529567 530400 556036 562535 (view as bug list)
Depends on: 598104
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-09 17:29 PDT by BJ Quinn
Modified: 2011-10-28 11:42 PDT (History)
21 users (show)
mkmelin+mozilla: blocking‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
A patch to nsLocalMailFolder.cpp to check for 4GB file size and dump values for debugging. (4.18 KB, patch)
2009-06-19 03:49 PDT, ISHIKAWA, Chiaki
no flags Details | Diff | Splinter Review
The dump message containing reported file sizes during buggy copying (3.05 KB, application/octet-stream)
2009-06-19 03:57 PDT, ISHIKAWA, Chiaki
no flags Details
system call captured by strace under linux while the copying was performed. (255.54 KB, application/x-gzip)
2009-06-19 04:00 PDT, ISHIKAWA, Chiaki
no flags Details
patch for nsLocalFileUnix.cpp (5.35 KB, patch)
2009-06-23 08:05 PDT, ISHIKAWA, Chiaki
no flags Details | Diff | Splinter Review
Patch for nsLocalMailFolder.cpp (6.94 KB, patch)
2009-06-23 08:11 PDT, ISHIKAWA, Chiaki
no flags Details | Diff | Splinter Review
Patches to TB3(comm-central) nsLocalMailFolder.cpp (8.24 KB, patch)
2009-07-03 04:06 PDT, ISHIKAWA, Chiaki
no flags Details | Diff | Splinter Review
Patch to TB3(comm-central) ns (7.92 KB, patch)
2009-07-03 05:53 PDT, ISHIKAWA, Chiaki
no flags Details | Diff | Splinter Review
patch to TB3(comm-central 1.9.1) nsLocalFileUnix.cpp for stat64 cleanliness (7.51 KB, patch)
2009-07-07 00:18 PDT, ISHIKAWA, Chiaki
benjamin: review-
Details | Diff | Splinter Review
Patch to TB3(comm-central) nsLocalMailFolder.cpp to fix 4GB+ file size problem. (6.80 KB, patch)
2009-07-07 00:28 PDT, ISHIKAWA, Chiaki
mozilla: review-
Details | Diff | Splinter Review
Patch to TB3(comm-central) nsLocalMailFolder.cpp to fix 4GB+ file size problem. (8.98 KB, patch)
2009-07-16 22:41 PDT, ISHIKAWA, Chiaki
mozilla: review-
Details | Diff | Splinter Review
A patch to messenger.properties to include a new message. (1.51 KB, patch)
2009-07-17 09:32 PDT, ISHIKAWA, Chiaki
no flags Details | Diff | Splinter Review
A patch to messenger.properties to include a new message. (for TWO files) (3.01 KB, patch)
2009-07-18 10:52 PDT, ISHIKAWA, Chiaki
no flags Details | Diff | Splinter Review
Screendump to show how the error is caught with the proposed patches. (238.60 KB, image/png)
2009-07-23 11:48 PDT, ISHIKAWA, Chiaki
no flags Details
Patch to TB3 (comm-central) nsLocalMailFolder.cpp to fix 4GB+ file size problem also with a set of string changes. (11.10 KB, patch)
2009-08-02 04:19 PDT, ISHIKAWA, Chiaki
timeless: review-
Details | Diff | Splinter Review
Patch to TB3 (comm-central) nsLocalMailFolder.cpp to fix 4GB+ file size problem also with a set of string changes. (12.07 KB, patch)
2009-08-04 10:17 PDT, ISHIKAWA, Chiaki
no flags Details | Diff | Splinter 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. (9.94 KB, patch)
2009-08-04 10:23 PDT, ISHIKAWA, Chiaki
no flags Details | Diff | Splinter Review
tweaks to previous patch (6.42 KB, patch)
2009-08-05 20:29 PDT, David :Bienvenu
neil: superreview-
Details | Diff | Splinter Review
fix addressing Neil's comments (3.68 KB, patch)
2009-08-06 10:38 PDT, David :Bienvenu
mozilla: review+
neil: superreview+
Details | Diff | Splinter Review
Patch for TB 2.0.0.23 of this bug. (4.02 KB, patch)
2009-11-22 19:53 PST, ISHIKAWA, Chiaki
mozilla: review+
dveditz: approval1.8.1.next+
Details | Diff | Splinter Review

Description BJ Quinn 2007-07-09 17:29:50 PDT
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.
Comment 1 David :Bienvenu 2007-07-09 19:54:41 PDT
we do try to warn users about this - not sure why you didn't get the warning.
Comment 2 timeless 2007-07-10 07:33:27 PDT
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
Comment 3 BJ Quinn 2007-07-10 10:08:55 PDT
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.

Comment 4 David :Bienvenu 2007-07-10 10:50:44 PDT
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.
Comment 5 BJ Quinn 2007-07-10 11:21:49 PDT
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?
Comment 6 David :Bienvenu 2007-07-10 11:24:11 PDT
4GB really is the limit - I'm surprised you can read the messages that are after the 4GB limit.
Comment 7 BJ Quinn 2007-07-10 11:27:28 PDT
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?
Comment 8 BJ Quinn 2007-07-10 12:41:26 PDT
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?
Comment 9 Joshua Cranmer [:jcranmer] 2009-06-11 14:47:06 PDT
*** Bug 497554 has been marked as a duplicate of this bug. ***
Comment 10 WADA 2009-06-12 01:59:59 PDT
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.
Comment 11 WADA 2009-06-12 02:38:05 PDT
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.
Comment 12 ISHIKAWA, Chiaki 2009-06-12 02:55:41 PDT
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
Comment 13 Joshua Cranmer [:jcranmer] 2009-06-12 13:12:00 PDT
(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.
Comment 14 WADA 2009-06-12 21:21:32 PDT
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?
Comment 15 ISHIKAWA, Chiaki 2009-06-14 19:09:01 PDT
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.
Comment 16 Joshua Cranmer [:jcranmer] 2009-06-14 19:48:14 PDT
(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.
Comment 17 ISHIKAWA, Chiaki 2009-06-15 23:57:54 PDT
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.
Comment 18 ISHIKAWA, Chiaki 2009-06-16 00:02:08 PDT
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.
Comment 19 ISHIKAWA, Chiaki 2009-06-16 00:16:40 PDT
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
Comment 20 WADA 2009-06-16 00:37:04 PDT
(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)
Comment 21 ISHIKAWA, Chiaki 2009-06-16 02:06:08 PDT
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.
Comment 22 ISHIKAWA, Chiaki 2009-06-19 03:47:03 PDT
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
Comment 23 ISHIKAWA, Chiaki 2009-06-19 03:49:21 PDT
Created attachment 384078 [details] [diff] [review]
A patch to nsLocalMailFolder.cpp to check for 4GB file size and dump values for debugging.
Comment 24 ISHIKAWA, Chiaki 2009-06-19 03:57:27 PDT
Created attachment 384079 [details]
The dump message containing reported file sizes during buggy copying

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.
Comment 25 ISHIKAWA, Chiaki 2009-06-19 04:00:51 PDT
Created attachment 384080 [details]
system call captured by strace under linux while the copying was performed.

gzipped.
Captured the system calls invoked during the copying operation.

Cursory check doensn't reveal strange stat64 calls.
Comment 26 ISHIKAWA, Chiaki 2009-06-19 04:18:19 PDT
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
Comment 27 WADA 2009-06-20 00:16:04 PDT
DUP'ed bug 497554 is for Linux. Changing to "All".
Comment 28 ISHIKAWA, Chiaki 2009-06-22 02:50:54 PDT
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...)
Comment 29 ISHIKAWA, Chiaki 2009-06-22 05:44:19 PDT
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...
Comment 30 ISHIKAWA, Chiaki 2009-06-23 08:00:58 PDT
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
Comment 31 ISHIKAWA, Chiaki 2009-06-23 08:05:41 PDT
Created attachment 384624 [details] [diff] [review]
patch for nsLocalFileUnix.cpp

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.
Comment 32 ISHIKAWA, Chiaki 2009-06-23 08:11:08 PDT
Created attachment 384626 [details] [diff] [review]
Patch for nsLocalMailFolder.cpp

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.
Comment 33 ISHIKAWA, Chiaki 2009-06-23 08:18:59 PDT
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...
Comment 34 Magnus Melin 2009-06-23 12:33:37 PDT
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
Comment 35 ISHIKAWA, Chiaki 2009-06-23 23:11:24 PDT
>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.
Comment 36 Magnus Melin 2009-06-24 10:47:58 PDT
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 ;)
Comment 37 ISHIKAWA, Chiaki 2009-06-25 07:39:42 PDT
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.
Comment 38 ISHIKAWA, Chiaki 2009-07-03 04:01:55 PDT
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.
Comment 39 ISHIKAWA, Chiaki 2009-07-03 04:06:50 PDT
Created attachment 386708 [details] [diff] [review]
Patches to TB3(comm-central) nsLocalMailFolder.cpp

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.
Comment 40 ISHIKAWA, Chiaki 2009-07-03 05:53:36 PDT
Created attachment 386715 [details] [diff] [review]
Patch to TB3(comm-central) ns

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.
Comment 41 ISHIKAWA, Chiaki 2009-07-03 05:55:29 PDT
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 ... ".
Comment 42 ISHIKAWA, Chiaki 2009-07-03 09:54:42 PDT
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
Comment 43 Magnus Melin 2009-07-05 11:52:16 PDT
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.
Comment 44 ISHIKAWA, Chiaki 2009-07-05 18:20:09 PDT
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
Comment 45 Magnus Melin 2009-07-06 10:34:45 PDT
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
Comment 46 ISHIKAWA, Chiaki 2009-07-07 00:03:30 PDT
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.)
Comment 47 ISHIKAWA, Chiaki 2009-07-07 00:18:20 PDT
Created attachment 387143 [details] [diff] [review]
patch to TB3(comm-central 1.9.1) nsLocalFileUnix.cpp for stat64 cleanliness

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.
Comment 48 ISHIKAWA, Chiaki 2009-07-07 00:28:15 PDT
Created attachment 387146 [details] [diff] [review]
Patch to TB3(comm-central) nsLocalMailFolder.cpp to fix 4GB+ file size problem.

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 49 David :Bienvenu 2009-07-08 08:04:49 PDT
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
Comment 50 ISHIKAWA, Chiaki 2009-07-09 11:58:15 PDT
>(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.
Comment 51 David :Bienvenu 2009-07-09 12:04:37 PDT
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.
Comment 52 Dan Mosedale (:dmose) 2009-07-09 12:24:43 PDT
(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
Comment 53 Joshua Cranmer [:jcranmer] 2009-07-09 14:25:52 PDT
(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
Comment 54 ISHIKAWA, Chiaki 2009-07-10 21:32:48 PDT
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
Comment 55 Magnus Melin 2009-07-11 00:16:13 PDT
Re the localization, look at the other uses of stringbundle in nsLocalMailFolder.cpp and just get the localized strings the same way.
Comment 56 Benjamin Smedberg [:bsmedberg] 2009-07-16 12:50:36 PDT
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
Comment 57 ISHIKAWA, Chiaki 2009-07-16 22:39:01 PDT
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.
Comment 58 ISHIKAWA, Chiaki 2009-07-16 22:41:13 PDT
Created attachment 389083 [details] [diff] [review]
Patch to TB3(comm-central) nsLocalMailFolder.cpp to fix 4GB+ file size problem.

(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.
Comment 59 ISHIKAWA, Chiaki 2009-07-16 22:51:15 PDT
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.
Comment 60 Magnus Melin 2009-07-17 03:26:28 PDT
(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.
Comment 61 ISHIKAWA, Chiaki 2009-07-17 04:13:08 PDT
(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.
Comment 62 ISHIKAWA, Chiaki 2009-07-17 09:32:49 PDT
Created attachment 389134 [details] [diff] [review]
A patch to messenger.properties to include a new message.

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
Comment 63 Joshua Cranmer [:jcranmer] 2009-07-18 07:31:01 PDT
(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.
Comment 64 ISHIKAWA, Chiaki 2009-07-18 10:52:15 PDT
Created attachment 389308 [details] [diff] [review]
A patch to messenger.properties to include a new message.  (for TWO files)

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
Comment 65 ISHIKAWA, Chiaki 2009-07-20 12:18:04 PDT
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.]
Comment 66 Magnus Melin 2009-07-20 22:27:49 PDT
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.
Comment 67 ISHIKAWA, Chiaki 2009-07-21 09:26:48 PDT
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
Comment 68 Magnus Melin 2009-07-21 09:45:24 PDT
You can use the "Clone This Bug" link and make the component the same as the original, or just do it manually.
Comment 69 ISHIKAWA, Chiaki 2009-07-21 23:53:06 PDT
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 :-( ]
Comment 70 ISHIKAWA, Chiaki 2009-07-23 11:48:17 PDT
Created attachment 390279 [details]
Screendump to show how the error is caught with the proposed patches.

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 71 David :Bienvenu 2009-07-29 17:54:11 PDT
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.
Comment 72 ISHIKAWA, Chiaki 2009-07-30 22:15:19 PDT
Thank you for your comment.

I will prepare a patch that incorporates  the new strings as well and post it over the weekend.

TIA
Comment 73 ISHIKAWA, Chiaki 2009-08-02 04:19:38 PDT
Created 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.

Cleaned up patch based on comment # 71.
This also includes the string patches to a couple of messanger.properties files.
Comment 74 timeless 2009-08-03 05:59:18 PDT
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"
Comment 75 ISHIKAWA, Chiaki 2009-08-04 10:13:42 PDT
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.
Comment 76 ISHIKAWA, Chiaki 2009-08-04 10:17:31 PDT
Created attachment 392522 [details] [diff] [review]
Patch to TB3 (comm-central) nsLocalMailFolder.cpp to fix 4GB+ file size problem also with a set of string changes.

The improved patch explained in comment # 75 (incorporated the requested changes
in the comment # 74).
Comment 77 ISHIKAWA, Chiaki 2009-08-04 10:23:58 PDT
Created 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.

Sorry, unintended local patch crept into the previously uploaded patch.
Please use this one.
Comment 78 Mark Banner (:standard8) 2009-08-04 11:08:11 PDT
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.
Comment 79 Joshua Cranmer [:jcranmer] 2009-08-04 13:50:31 PDT
(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).
Comment 80 ISHIKAWA, Chiaki 2009-08-04 18:20:43 PDT
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.
Comment 81 David :Bienvenu 2009-08-05 20:29:54 PDT
Created attachment 392865 [details] [diff] [review]
tweaks to previous patch

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.
Comment 82 ISHIKAWA, Chiaki 2009-08-05 21:51:08 PDT
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 83 neil@parkwaycc.co.uk 2009-08-06 03:13:40 PDT
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.
Comment 84 ISHIKAWA, Chiaki 2009-08-06 08:25:34 PDT
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.)
Comment 85 David :Bienvenu 2009-08-06 09:38:20 PDT
yes, I intend to address your and Neil's comments today.
Comment 86 David :Bienvenu 2009-08-06 09:57:57 PDT
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.
Comment 87 David :Bienvenu 2009-08-06 10:38:10 PDT
Created attachment 392974 [details] [diff] [review]
fix addressing Neil's comments

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 88 David :Bienvenu 2009-08-06 10:53:42 PDT
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...
Comment 89 neil@parkwaycc.co.uk 2009-08-06 11:54:13 PDT
(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 90 neil@parkwaycc.co.uk 2009-08-06 12:50:54 PDT
Comment on attachment 392974 [details] [diff] [review]
fix addressing Neil's comments

Interdiff looks good to me.
Comment 91 David :Bienvenu 2009-08-06 14:41:05 PDT
fix checked in, thx for the patch, Chiaki.
Comment 92 David :Bienvenu 2009-08-06 14:41:57 PDT
Comment on attachment 392974 [details] [diff] [review]
fix addressing Neil's comments

marking r=me.
Comment 93 ISHIKAWA, Chiaki 2009-08-06 19:24:22 PDT
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.
Comment 94 Mark Banner (:standard8) 2009-11-22 05:08:29 PST
*** Bug 530400 has been marked as a duplicate of this bug. ***
Comment 95 ISHIKAWA, Chiaki 2009-11-22 19:53:36 PST
Created attachment 413983 [details] [diff] [review]
Patch for TB 2.0.0.23 of this bug. 

Backported patch of this bug for TB 2.0.0.2x series.
Comment 96 ISHIKAWA, Chiaki 2009-11-22 20:05:59 PST
Comment on attachment 413983 [details] [diff] [review]
Patch for TB 2.0.0.23 of this bug. 

review requested.
Comment 97 ISHIKAWA, Chiaki 2009-11-22 20:08:38 PST
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
Comment 98 Mark Banner (:standard8) 2009-11-23 02:41:13 PST
(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.
Comment 99 ISHIKAWA, Chiaki 2009-11-23 06:37:38 PST
(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
Comment 100 David :Bienvenu 2009-11-23 06:43:03 PST
TB 3 is a few days from release, not a few months.
Comment 101 Ted Mielczarek [:ted.mielczarek] 2009-11-23 06:44:28 PST
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.
Comment 102 ISHIKAWA, Chiaki 2009-11-23 07:17:49 PST
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
Comment 103 Mark Banner (:standard8) 2009-11-23 07:49:12 PST
(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.
Comment 104 ISHIKAWA, Chiaki 2009-11-23 15:55:00 PST
(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.
Comment 105 ISHIKAWA, Chiaki 2009-11-23 19:24:31 PST
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.
Comment 106 Ludovic Hirlimann [:Usul] 2009-11-24 04:25:53 PST
*** Bug 529567 has been marked as a duplicate of this bug. ***
Comment 108 David :Bienvenu 2009-12-01 11:33:12 PST
Comment on attachment 413983 [details] [diff] [review]
Patch for TB 2.0.0.23 of this bug. 

looks reasonable, thx.
Comment 109 ISHIKAWA, Chiaki 2009-12-02 09:09:05 PST
(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 110 Daniel Veditz [:dveditz] 2010-01-19 13:04:31 PST
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
Comment 111 Charles 2010-02-10 04:46:28 PST
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...
Comment 112 David :Bienvenu 2010-02-10 06:25:41 PST
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.
Comment 113 Charles 2010-02-10 06:50:47 PST
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?
Comment 114 David :Bienvenu 2010-02-10 07:09:14 PST
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.
Comment 115 Charles 2010-02-10 07:40:57 PST
(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? ;)
Comment 116 David :Bienvenu 2010-02-10 07:46:37 PST
yes, we're talking 3.2, roughly.
Comment 117 Charles 2010-02-10 07:49:12 PST
Awesome... thanks David. :)
Comment 118 WADA 2010-05-08 00:03:25 PDT
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.
Comment 119 [:Aureliano Buendía] 2010-08-23 04:22:55 PDT
*** Bug 556036 has been marked as a duplicate of this bug. ***
Comment 120 ISHIKAWA, Chiaki 2010-08-23 07:14:30 PDT
(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. ]
Comment 121 David :Bienvenu 2010-08-23 07:59:24 PDT
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.
Comment 122 Charles 2010-08-23 09:34:43 PDT
(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?
Comment 123 WADA 2010-09-29 03:05:33 PDT
*** Bug 477430 has been marked as a duplicate of this bug. ***
Comment 124 Michael Adams 2010-10-12 13:28:09 PDT
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.
Comment 125 David :Bienvenu 2010-10-12 16:53:50 PDT
(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
Comment 126 WADA 2010-10-16 16:05:01 PDT
Local "Sent" folder case was special and was not resolved by this bug.
Setting dependency to Bug 598104 for ease of tracking.
Comment 127 Wayne Mery (:wsmwk, NI for questions) 2011-10-28 11:42:23 PDT
*** Bug 562535 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.