Closed Bug 640371 Opened 13 years ago Closed 11 years ago

Local Inbox folder allowed to grow past 4GB by mail download

Categories

(MailNews Core :: Networking: POP, defect)

Product:
MailNews Core
Component:
Networking: POP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 23.0

People

(Reporter: World, Assigned: aceman)

References

Details

(Keywords: dataloss)

Attachments

(3 files, 1 obsolete file)

Small extension to show messageKey, messageOffsert, messageSize
61.18 KB, application/octet-stream
Details
With patch, messageOffset, messageKey, messageSize, after download over 4B and Repair Folder (Sorted by messageOffset)
308.93 KB, text/plain
Details
patch v2 + test
22.38 KB, patch
Irving
: review+
Details | Diff | Splinter Review 
Local Inbox folder allowed to grow past 4GB by mail download.
This is Inbox version of bug 598104 on local Sent folder.

[Build ID]
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.15) Gecko/20110303 Thunderbird/3.1.9

Next phenomenon was observed during test for bug 634544 and fix verification test for bug 608449.

1. Inbox file size = 4GB - 6MB (255*16MB mails + 1*10MB mail)
　 Copy of 1MB mail was rejected by "Inbox full". As designed/expected?
2. 20*700KB mail exist in mbox of POP3 server.
   mail-1 to mail-20. (mail size is same, because crafted mail for test)
3. Get Mail.
   Activity manager display : 20 messages downloaded
   Thread pane display :
     mail-1 to mail-9 is shown. 
       offset of mail-8 = 4293694497, Size=700KB
       offset of mail-9 = 4294411338, Size=700KB
       (delta = 716848 == mail size of each 700KB mail)
     For mail-9 data.
       4GB=4294967296, 4294411338 - 4294411338 = 555958.
       So, latter part of mail-9 is placed over 4GB.
       But whole mail data of mail-9 is normally shown by View/Message Source,
       because Size is corectly written in .msf.
       This is true even after Compact Folder.
       If .msf is manually deleted and rebuild-index is executed before
       Compact, mail data of mail-9 over 4GB is lost.        
       This is current restriction due to file size limitation.
     mail-10 to mail-20 is not shown at thread pane.
     (current restriction due to file size limitation of 4GB) 
   Inbox file size = 4303014860 bytes
                   = 4GB + 8047564 = 4GB + 7MB + 690KB + 972bytes
     All 20*700KB mails are appended to Inbox, even after file size > 4GB.

(Problems observed in test)
(Problem-1)
If free space in Inbox is smaller(e.g. less than 1MB), "Get Mail" is rejected with "Ibox full", and PO3 server connection is not requested.
Same criteria of "Inbox full" as "Copy/Move" is better used for download of mail too. 
(Problem-2)
All 20*700KB mails are appended to Inbox, even after file size > 4GB.

Note: 
Qurantine option was enabled(mailnews.downloadToTempFile=true) when tested, because used Tb's profile was one for test of mailnews.downloadToTempFile=true relevant problems on POP3.
I don't know whether it's relevant to this bug or not.
sounds like effective dataloss (if one is not able to access the new messages)
Keywords: dataloss
Observation in comment #0 was not accurate partially, so checked again.
Checked with 16MB mails and 7MB mails and simpler procedure.
Wrap around of offset was observed, with both mailnews.downloadToTempFile = true and false.

(1) Inbox : 255*16MB mails, mail-16MB-001 to mail-16MB-255, file size=4GB-16MB
(2) Mbox of POP3 server : 10*7MB mails, mail-7MB-01 to mail-7MB-10

(3) Download mails from POP3 server
> -------------  ------------------------  ----   
> Subject        Offset                    Size
> -------------  ------------------------  ----   
> (at bottom of thread pane)
> mail-16MB-254  4GB-(16MB*3)              16MB
> mail-16MB-255  4GB-(16MB*2)              16MB
>  mail-7MB-01   4GB-16MB                   7MB
>  mail-7MB-02   4GB-16MB+(1*7MB)=4GB-9MB   7MB
>  mail-7MB-03   4GB-16MB+(2*7MB)=4GB-2MB   7MB
> (mail-7MB-04   4GB-16MB+(3*7MB)=4GB+5MB => offset is wrapped)
> (at top of thread pane)
> mail-16MB-001               0            16MB
>  mail-7MB-04   5MB                        7MB => wrapped(part of mail-16MB-001)
>  mail-7MB-05   5MB+(7MB*1)=12MB           7MB => wrapped(part of mail-16MB-001 and mail-16MB-002)
> mail-16MB-002              16MB          16MB
>  mail-7MB-06   5MB+(7MB*2)=19MB           7MB => wrapped(part of mail-16MB-002)
>  mail-7MB-07   5MB+(7MB*3)=26MB           7MB => wrapped(part of mail-16MB-002 and mail-16MB-003)
> mail-16MB-003  16MB*2     =32MB          16MB
>  mail-7MB-08   5MB+(7MB*4)=33MB           7MB => wrapped(part of mail-16MB-003)
>  mail-7MB-09   5MB+(7MB*5)=40MB           7MB => wrapped(part of mail-16MB-003)
>  mail-7MB-10   5MB+(7MB*6)=47MB           7MB => wrapped(part of mail-16MB-003 and mail-16MB-004)
> mail-16MB-004  16MB*3     =48MB          16MB
> mail-16MB-005  16MB*4     =64MB          16MB
> mail-16MB-006  16MB*5     =80MB          16MB

At this step, next manual "Get Mail" is rejected due to "Inbox full".
I don't know automatic new mail download case.

(4) Delete some mails, and Compact Folder.
    As total mail data length = (4GB-16MB) + 7MB*10 = 4GB+54MB,
    at least 54MB is needed to be deleted before Compact.
    Unless Compact silently fails because file size after Compact is still >4GB
(4-1) Delete mail-16MB-001 to mail-16MB-004, Compact Folder.
Mail data over 4GB(mail-7MB-04 to mail-7MB-10) is lost.
Mail data over 4GB of mail-7MB-03 is recovered, because Size is written correctly.
> -------------  ------------  ----   
> Subject        Offset        Size
> -------------  ------------  ----   
> (at top of thread pane)
>  mail-7MB-04           0      7MB (part of mail-16MB-001)
>  mail-7MB-05    7MB*1= 7MB    7MB (part of mail-16MB-001,mail-16MB-002, X-Mozilla-Status: 0009)
>  mail-7MB-06    7MB*2=14MB    7MB (part of mail-16MB-002)
>  mail-7MB-07    7MB*3=21MB    7MB (part of mail-16MB-002,mail-16MB-003, X-Mozilla-Status: 0009)
>  mail-7MB-08    7MB*4=28MB    7MB (part of mail-16MB-003)
>  mail-7MB-09    7MB*5=35MB    7MB (part of mail-16MB-003)
>  mail-7MB-10    7MB*6=42MB    7MB (part of mail-16MB-003,mail-16MB-004, X-Mozilla-Status: 0009)
> mail-16MB-005   7MB*7=49MB   16MB
> mail-16MB-006         65MB   16MB

(5) Rebuild-Index => Garbage by wrap around is hidden.
    Data at offset=0 is ignored, because of no "From -" line.
    Other data is hidden because of "From -" line and "X-Mozilla-Status: 0009".

(6) Compact Folder again => Garbage by wrap around is removed.
i.e. data of mail-7MB-04 to mail-7MB-10 is lost by above operations.
So, manual recovery for mail data over 4GB is needed.
> -------------  ------  ----   
> Subject        Offset  Size
> -------------  ------  ----   
> (at top of thread pane)
> mail-16MB-005   0      16MB
> mail-16MB-006  16MB    16MB
If correct mails at wrapped position is not deleted bofore Compact folder, corrupted mail data is generated after Compact and Rebuild-Index.

Delete mails at bottom of Thread pane, and Compact Folder.
Compact normally ends, because file size after compact is less than 4GB.
> -------------  ------ ----   
> Subject        Offset Size
> -------------  ------ ----   
> (at top of thread pane)
> mail-16MB-001    0    16MB
>  mail-7MB-04    16MB   7MB => wrapped(part of mail-16MB-001)
>  mail-7MB-05    23MB   7MB => wrapped(part of mail-16MB-001, part of mail-16MB-002)
> mail-16MB-002   30MB  16MB
>  mail-7MB-06    46MB   7MB => wrapped(part of mail-16MB-002)
>  mail-7MB-07    53MB   7MB => wrapped(part of mail-16MB-002, part of mail-16MB-003)
> mail-16MB-003   60MB  16MB
>  mail-7MB-08    76MB   7MB => wrapped(part of mail-16MB-003)
>  mail-7MB-09    83MB   7MB => wrapped(part of mail-16MB-003)
>  mail-7MB-10    90MB   7MB => wrapped(part of mail-16MB-003, part of mail-16MB-004)
> mail-16MB-004   97MB  16MB ( From -, X-Mozilla-Status: 0000)
> mail-16MB-005  103MB  16MB
> mail-16MB-006  119MB  16MB

After Rebuil-Index, corrupted mails are generated at top of mail folder.
> -------------  ------ -------- ---------  
> Subject        Offset Size     Content
> -------------  ------ -------- ---------  
> (at top of thread pane)
> mail-16MB-001    0    27647KB  Corrupted
> mail-16MB-002   27MB  32768KB  Corrupted
> mail-16MB-003   59MB  32768KB  Corrupted
> mail-16MB-004   91MB  22529KB  Corrupted
> mail-16MB-005  113MB  16384KB  Not affected
> mail-16MB-006  No problem
Unless Rebuild-Index is executed, mail data at 4GB boundary(largest Order Received value) can be accessed by Tb normally because Size of mail is kept correctly. So, all mail data can be recovered by next procedure when "Inbox Full" due to over 4GB Inbox.

(1) Sort by "Order Received" column. Disable auto-compact.
(2) Create a folder(call Inbox-X), move all mails over 2GB to Inbox-X.
    Mail at 4GB boundary is normally moved.
(3) Keep backup of data over 4GB.
    By script, open Inbox with read mode, seek to 4GB,
    read data till EOF and write data to Inbox-Y.
(4) Rebuild-Index(Repair Folder) of Inbox(active mail data is less than 2GB).
    Entries for wrapped mails is removed from .msf.
(5) Compact Folder of Inbox => File size is now less than 2GB
(6) Copy Inbox-Y(created at step 3) to Tb's mail directory, restart Tb,
    Rebuild-Index(Repair Folder) of Inbox-Y, and Compact Folder of Inbox-Y.
(7) Now, mails are in next folders;
    - Inbox   : mail data at 0GB to 2GB, without garbage
    - Inbox-X : mail data at 2GB to 4GB, including mail at 4GB boundary
    - Inbox-Y : mail data over 4GB
WADA, can you still see this?
This may be what Ishikawa Chiaki is seeing in bug 789679.

(In reply to ISHIKAWA, chiaki from comment #58)
> This is the dialog I got when the mail folder size goes over 4GB.
> 
> The folder Inbox is full, and can't hold any more messages. To make
> room for more messages, delete any old or unwanted mail and compact
> the folder.
> 
> Then the extra column extension shows Inbox as having 8298 e-mails
> (8284 unread) and size is "382KB" while actually it has the size shown
> in the following listing.
> 
> total 4203388
> -rw-rwx--- 1 mtest2 ishikawa 4295358606 Mar 17 10:23 Inbox
> -rw-rwxr-- 1 mtest2 ishikawa    3634842 Mar 17 10:26 Inbox.msf
> -rw-rwx--- 1 mtest2 ishikawa    5247690 Mar 17 01:59 Sent
> -rw-rwxr-- 1 mtest2 ishikawa       3170 Mar 17 02:07 Sent.msf
> -rw-rwx--- 1 mtest2 ishikawa          0 Mar 17 01:38 Trash
> -rw-rwxr-- 1 mtest2 ishikawa       1320 Mar 17 02:00 Trash.msf
> -rw-rwxr-- 1 mtest2 ishikawa         25 Mar 17 01:38 msgFilterRules.dat
> -rw-rwx--- 1 mtest2 ishikawa         61 Mar 17 10:23 popstate.dat
> 
> Funny, I thought there was this advance check for 10MB room, but
> obviously it did not kick in. (Either the check is not there, or
> put in an incorrect place, or is now removed altogether?.)

Check for 0xFFFFF (1MB?) free space is still there at http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Service.cpp#98 . However I am not sure if that is actually used and when. Is it for each new message or all of them? In any way that looks wrong as it is not dependent on any message size coming in. I.e. it checks if 1MB message could be stored and then writes anything that comes in (be it even 100MB) ? The message download and storage jumps through many functions so we'd need some help here. Neil?
Depends on: 789679
Obviously, we seem to have a few issues when mail folder size goes over 4GB.
I: compaction failure,
   - Bug 794303 - Compact folder fails if local mail folder size is near 4GB(less than 4GB) or over 4GB, even though bug 462665 was changed to FIXED after patch for bug 402392 was landed on Tb 12.0. 

II: Wrong article is deleted if we try to delete an article at the end of such folder, etc.) (See comment in
https://bugzilla.mozilla.org/show_bug.cgi?id=789679#c62
(and the comment that contains the observation which triggered Neil's comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=789679#c58 )

So we should NOT make the mail folder go over 4GB for now.

But somehow POP3 allows downloading until the Inbox goes over 4GB.

The check mentioned in comment 6
> Check for 0xFFFFF (1MB?) free space is still there at http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Service.cpp#98 . 

This check is about 
(1)  - Is there enough space left in the file system to grow the file any more?
       [Free space low condition, or if the file system itself can handle 4GB, say.]
    
and NOT ABOUT

(2) - Is the file (mail folder: I a concerned about Inbox for now)
      still below  this 4GB limit (or 4GB - 10MB, say)?

I think POP3 does not have this second check at all, then.

(Hmm, just had a crash of TB, but unrelated to this...)
(In reply to ISHIKAWA, chiaki from comment #7)
> But somehow POP3 allows downloading until the Inbox goes over 4GB.

I looks so.

nsPop3Service::GetMail is main entry of POP3 download(new mail check).
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Service.cpp#77
> 77 nsresult nsPop3Service::GetMail(bool downloadNewMail,
Before connection to server, calls WarnIfLocalFileTooBig.
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Service.cpp#98
> 98     destLocalFolder->WarnIfLocalFileTooBig(aMsgWindow, &destFolderTooBig);

WarnIfLocalFileTooBig calls HasSpaceAvailable.
This check is equivallent to "file size + 1MB" < "4GB - 4MB", i.e. 1MB room check in Berkley Mbox file.
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#3579
> 3579 nsMsgLocalMailFolder::WarnIfLocalFileTooBig(nsIMsgWindow *aWindow, bool *aTooBig)
> 3587   // check if we have a reasonable amount of space left
> 3588   rv = msgStore->HasSpaceAvailable(this, 0xFFFFF, &spaceAvailable);
> 3589   if (!spaceAvailable)
> 3590     {
> 3591       ThrowAlertMsg("mailboxTooLarge", aWindow);
> 3592       *aTooBig = true;
> 3593     }

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

So, when "current file size" < "4GB - 4MB -1MB", POP3 download is initiated.

I think HasSpaceAvailable or similar logic is called upon "append of mail data to Inbox" for each downloaded mail, but I'm not sure.
(In reply to WADA from comment #8)
> So, when "current file size" < "4GB - 4MB -1MB", POP3 download is initiated.
Yes.

> I think HasSpaceAvailable or similar logic is called upon "append of mail
> data to Inbox" for each downloaded mail, but I'm not sure.
I'll check this out via printfs in the hasSpaceAvailable function. But it seems for me it is not called for each message.
Quarantine option(mailnews.downloadToTempFile=true/false) is processed in IncorporateBegin.
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Sink.cpp#454
> 454 nsPop3Sink::IncorporateBegin(const char* uidlString,
Rough logic.
  if (m_downloadingToTempFile)
    MsgGetFileStream(m_tmpDownloadFile,getter_AddRefs(m_outFileStream))
  else m_msgStore->GetNewMsgOutputStream(m_folder, 
       getter_AddRefs(newHdr),&reusable,getter_AddRefs(m_outFileStream))
After it, at least when m_downloadingToTempFile==true, required header such as X-Mozilla-Staus: and mail data downloaded to temp file is appended to Inbox without any mail size check, without any file size check.

So, if file size/mail size is checked for each mail after invoking new mail check process, it should be done beore call of nsPop3Sink::IncorporateBegin.
As I wrote in comment #0, comment #0 is test result with Qurantine option=enabled(mailnews.downloadToTempFile=true). Qurantine option may be relivant to problem.
It looks like somewhere here http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Protocol.cpp#3267 we have the incoming message size available and can check hasSpaceAvailable.
If it fails and we returns POP3_ERROR_DONE we may be able to abort the download here: http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Protocol.cpp#4017 . If we set the proper state, we may abort the whole download that could solve bug 634544.
Or maybe we should use POP3_MESSAGE_WRITE_ERROR that is already emitted by nsPop3Protocol::RetrResponse.
OS: Windows XP → All
Hardware: x86 → All
Version: 1.9.2 Branch → unspecified
(In reply to :aceman from comment #13)
> Or maybe we should use POP3_MESSAGE_WRITE_ERROR that is already emitted by nsPop3Protocol::RetrResponse.

I also think nsPop3Protocol::RetrResponse and/or m_nsIPop3Sink->IncorporateBegin is good place for "per mail free disk space size check", and "per mail file size check of '16 Exa bytes - 4MB'" if needed ;-)
  nsPop3Protocol::RetrResponse
    Puts RETR(or TOP) response lines to buffers
    At 3280, call IncorporateBegin
       rv = m_nsIPop3Sink->IncorporateBegin( ... )
       IncorporateBegin writes data to Inbox, or temp if Qurantine=On
    Writes "recv:" log
    After it, executes action of "fetch body from server" by filter or
    "delete from POP3 server" held in popstate.dat.

Another place for "per mail free disk space check".
  Just before request RETR or TOP.
Because message_no(mandatory in POP3)/size(mandatory)/UIDL(optional) is already obtained by LIST/UIDL, required space estimation is possible at there.
  - If RETR, Max( (size * 100+α %), 1MB )
  - If TOP,
    (80 * lines by size limitation) + ( 80 * average header lines ),
    or approximately, 1MB
Because "truncate" is issued for Inbox after filter move if Berkley Mbox any append of new mail data is done to end of file, "free disk space check by estimation" won't produce big issue.
I did same test as comment #0 again - when 4GB-7MB < Inbox file size < 4GB-4MB-1MB, download 10*7MB mails.
(0) Tb 17.0.3 on Win XP.
    Quarantine option is disabled in this test.
    mailnews.downloadToTempFile is reset state, not defined (== false)
(1) Initial inbox size :
    4288430080 bytes == 3 GB + 1017 MB + 784 KB + 0 Bytes
(2) As 4GB-7MB < Inbox file size < 4GB-4MB-1MB, download is initiated.
(3) After POP3 download of (10 * 7MB mail).
    ( 7340184 bytes == 7 MB + 0 KB + 152 Bytes ) 
> Mail #       :  Property value of msgDBHder of each downloaded mail in Inbox
> 7MB-mail-#01 :  messageKey = 4288430080, messageOffset = 4288430080, messageSize = 7340184
> 7MB-mail-#02 :  messageKey = 4288430081, messageOffset = 4295770264, messageSize = 7340184
> 7MB-mail-#03 :  messageKey = 4288430082, messageOffset = 4303110448, messageSize = 7340184
> 7MB-mail-#04 :  messageKey = 4288430083, messageOffset = 4310450632, messageSize = 7340184
> 7MB-mail-#05 :  messageKey = 4288430084, messageOffset = 4317790816, messageSize = 7340184
> 7MB-mail-#06 :  messageKey = 4288430085, messageOffset = 4325131000, messageSize = 7340184
> 7MB-mail-#07 :  messageKey = 4288430086, messageOffset = 4332471184, messageSize = 7340184
> 7MB-mail-#08 :  messageKey = 4288430087, messageOffset = 4339811368, messageSize = 7340184
> 7MB-mail-#09 :  messageKey = 4288430088, messageOffset = 4347151552, messageSize = 7340184
> 7MB-mail-#10 :  messageKey = 4288430089, messageOffset = 4354491736, messageSize = 7340184

"MsgKey == MsgKey of previous mail + 1" was generated upon POP3 mail download, instead of by "filter move of conntinuous mails upon download".
(i) Because file size=4288430080, messageKey=4288430080 is assigned to first mail.
(ii) messageKey is incremented by 1 for each second to last mail.

There is no problem due to "file size grater than 4GB" in POP3 download, except problem due to "Disk full".
I can't imagine reason why problem in "delete mail when 4GB<size" occurs.
If something wrong will happen, I guess it's after Compact with one liner patch for bug 794303, because "wrap of messageOffset" may be produced by Compact.
Problem in "Extra Folder Columns" : 
  Inbox file size : 4361831920 bytes == 4 GB + 63 MB + 785 KB + 496 Bytes
  Size field display by "Extra Folder Columns" = 64MB
"Extra Folder Columns" uses 32bits unsigned integer for filesize.
This test extension named WinBackMyTrash is created for testing of Trash related bugs.
How to use in "4GB-4MB" test.
(1) Extension adds three ToolBar buttons to Customize panel.
    Add button_2("i" icon) to MenuBar or ToolBar.
(2) Open Error Console
(3) Select a mail folder at folder pane, select a mail at thread pane, change View if required(reduce listed mails), sort mails by appropriate column if required, 
(4) Select "Selected Folder Info"->"All Mail(View)" via button menu,
(5) At Error Console, select shown log, Copy
(6) At Text Editor, Paste

This test extension doesn't always remove reference to XPCOM object etc., and object destroy is done only on variable by let/var by JavaScript. So, memory leak may happen. And, this test extension dumps large volume of data to single string variable and put it in Error Console, so large virtual memory may be consumed if msgDBHdr data of many mails are dumped at once by the extension.
Don't install in daily use profile, Be careful if you try to list data of many mails at once, Terminate Tb as soon as possible after test, Remove extension as soon as possible after test, please.
(In reply to WADA from comment #16)
> Problem in "Extra Folder Columns" : 
>   Inbox file size : 4361831920 bytes == 4 GB + 63 MB + 785 KB + 496 Bytes
>   Size field display by "Extra Folder Columns" = 64MB
> "Extra Folder Columns" uses 32bits unsigned integer for filesize.

Extra Folder Columns is a pure javascript extension, it does not use C integers. It uses JS variables that do not have this limit (probably some higher one). Have you tested this on my patched build from bug 789679? Ishikawa reported that with it the size is reported correctly.
(In reply to :aceman from comment #18)
> Have you tested this on my patched build from bug 789679?

No. When I checked, no win32.zip build was available. 

> Extra Folder Columns is a pure javascript extension, (snip)
> Ishikawa reported that with it the size is reported correctly.

Dump of nsIMsgFolder object property of Inbox in my test.
> msgFolder : [xpconnect wrapped (nsISupports, nsIMsgFolder, nsIRDFResource)]
>   msgFolder / Property
>     prettiestName = Inbox
>     sizeOnDisk = 66864624
Your patch has next change.
  -  attribute unsigned long sizeOnDisk;
  +  attribute unsigned long long sizeOnDisk;
Extra Folder Columns paerhas looks nsIMsgFolder.sizeOnDisk.
(In reply to WADA from comment #19)
> Dump of nsIMsgFolder object property of Inbox in my test.
> > msgFolder : [xpconnect wrapped (nsISupports, nsIMsgFolder, nsIRDFResource)]
> >   msgFolder / Property
> >     prettiestName = Inbox
> >     sizeOnDisk = 66864624
> Your patch has next change.
>   -  attribute unsigned long sizeOnDisk;
>   +  attribute unsigned long long sizeOnDisk;
> Extra Folder Columns paerhas looks nsIMsgFolder.sizeOnDisk.
Yes, it does. So it uses whatever number the TB core sends it. In original TB the value is internally clamped to 32 bit so 66864624. In the patched version it should be sent properly with full value of >4GB.

The builds that Aryx produced in bug 789679 comment 56 should be here:
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/archaeopteryx@coole-files.de-868900d3da0b/ . It should contain both patches. You can test with these.
Attached patch patch (obsolete) — — Splinter Review 
Ok, I found another spot at nsPop3Protocol::GetMsg() where we already check if there is enough disk space for all messages to be downloaded, but not for the 4GB limit. So I added the proper hasSpaceAvailable check here. This prevents the whole batch (not yet downloaded messages) to not be downloaded and left on the server with the proper "folder full" warning.
This would actually fix bug 634544 too the download does not need to be aborted.
This still does not catch the problem that the download starts (with enough space) but the disk gets filled with another application while downloading.
If we want this granularity I'll try to look at that in the next iteration of the patch.

Chiaki, can you try this out (with all the other patches from bug 789679)?
For me this prevented the inbox to grow above 4GB.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #726953 - Flags: feedback?(ishikawa)
Component: Backend → Networking: POP
Version: unspecified → Trunk
"MsgKey=Previous message's MsgKey+1" was done at here.
( 0xFFFFFF00 = 4GB - 256 )
> http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgFolderCompactor.cpp#1116
> 1116 nsFolderCompactState::EndCopy(nsISupports *url, nsresult aStatus)
> 1133     // if mbox is close to 4GB, auto-assign the msg key.
> 1134     nsMsgKey key = m_startOfNewMsg > 0xFFFFFF00 ? nsMsgKey_None : (nsMsgKey) m_startOfNewMsg;
> 1135     m_db->CopyHdrFromExistingHdr(key, m_curSrcHdr, true, getter_AddRefs(newMsgHdr));
> http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#3445
> 3445 NS_IMETHODIMP nsMsgDatabase::CopyHdrFromExistingHdr(nsMsgKey key, nsIMsgDBHdr *existingHdr, bool addHdrToDB, nsIMsgDBHdr **newHdr)
> 3453     CreateNewHdr(key, (nsIMsgDBHdr **) &destMsgHdr);
> http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#3351
> 3351 NS_IMETHODIMP nsMsgDatabase::CreateNewHdr(nsMsgKey key, nsIMsgDBHdr **pnewHdr)
> 3371     // Mork will assign an ID to the new row, generally the next available ID.
> 3372     err  = m_mdbStore->NewRow(GetEnv(), m_hdrRowScopeToken, &hdrRow);

Because writing to "4GB-4MB-1MB" is currently prohibited except for "POP3 download when file size < 4GB-4MB-1MB", phenomenon of "MsgKey=Previous message's MsgKey+1" can be observed only when POP3 download to Inbox of "file size is near 4GB".
FYI.
(A) Writing to near-4GB Mbox was rejected by Tb 17 in following cases.
    Draft save when Drafts folder is near 4GB
    Send Later when Outbox folder is near 4GB(Outbox of Local Folders)
Because Draft save etc. shares code with "Sent mail copy", I think near-4GB problem around Sent mail copy" is still surely avoided by bug 598104.
(B) Filter Copy/Move (Message filter, Junk filter)
Because "Filter copy/move" is similar to ordinal "mail copy/move", "filter copy/move" is perhaps rejected by local-to-local-copy module. However, "Filter copy/move" usually silently fails when error occurs while copy/move action is executed and nothing about error is written to filter log.
In this case, "Folder full" message may not be shown.
It may be same in "Junk move by Junk filter".
(In reply to :aceman from comment #21)
> Created attachment 726953 [details] [diff] [review]
> patch
> 
> Ok, I found another spot at nsPop3Protocol::GetMsg() where we already check
> if there is enough disk space for all messages to be downloaded, but not for
> the 4GB limit. So I added the proper hasSpaceAvailable check here. This
> prevents the whole batch (not yet downloaded messages) to not be downloaded
> and left on the server with the proper "folder full" warning.
> This would actually fix bug 634544 too the download does not need to be
> aborted.
> This still does not catch the problem that the download starts (with enough
> space) but the disk gets filled with another application while downloading.
> If we want this granularity I'll try to look at that in the next iteration
> of the patch.
> 
> Chiaki, can you try this out (with all the other patches from bug 789679)?
> For me this prevented the inbox to grow above 4GB.

I am sure this is the bugzilla entry where I should report this.

Thank you :aceman, the patch seems to work as you expected.

What I did.:

I tried to download more e-mails to almost filled up Inbox and see if
the proper warning comes up.


[Preparation step]

Before staring, I already had a filled up Inbox from a previous test run.

Extra column showed:
unread total size
7895 7900 4150MB

I got "full" warning on startup.

So deleteed  about 300 articles for compacting.

After deleteion, TB automatically offerred the following. Very nice.

Do you wish to compact all
local and offline folders to save disk space
[ ] Do you wish to compact all local and offline folders to save disk space
[YES] [CANCEL]


After compaction, I got
8095 8100 4001MB

Now I just had a hunch and made sure that it is possible to copya big
article from Sent to Inbox to check that the size is small enough for
accommodating newer articles there.
I could.

Now extra column showd the following: (the copied article has an attachment of big library
from /usr/lib )

8095 8101  4003MB

[Real Test Step] Downloading many e-mails to fill up the Inbox folder.

I sent e-mails from another console.
While downloading the e-mails injected, TB complained as follows.

There is not enough disk space to download new messages. Try deleting
old mail, emptying the Trash folder, and compacting your mail folders,
and then try again.

At that moment, 
Extra columns showed:
8176 8182 4043MB

The directory listing from within Emacs:
  /TEST-MAIL-DIR/Mail/127.0.0.1:
  合計 4250260
  drwxrwxr-x 2 mtest2 ishikawa       4096  3月 21 12:51 .
  drwxrwx--- 4 mtest2 ishikawa       4096  3月 17 01:38 ..
  -rw------- 1 mtest2 mtest2   4239318137  3月 21 13:10 Inbox
  -rw-r--r-- 1 mtest2 mtest2      4496540  3月 21 13:10 Inbox.msf
  -rw------- 1 mtest2 mtest2      4722922  3月 18 22:10 Sent
  -rw-r--r-- 1 mtest2 mtest2         3854  3月 21 13:07 Sent.msf
  -rw------- 1 mtest2 mtest2    103599600  3月 21 12:51 Trash
  -rw-r--r-- 1 mtest2 mtest2        96743  3月 21 13:07 Trash.msf
  -rw-rwxr-- 1 mtest2 ishikawa         25  3月 17 11:29 msgFilterRules.dat
  -rw-rwx--- 1 mtest2 ishikawa         61  3月 21 13:11 popstate.dat

Inbox size is less than 2^32.

I am not sure if the size of Inbox is checked at each e-mail or BEFORE
the downloading begins, but AT LEAST WE ARE SAFER now (!).

[Another TEST]

I tried downloading of another couple of batches of e-mails after
deleting 300 e-mails and compacting, now I got:

Extra Columns showed:
8265 8272 4087MB

The size:
  -rw------- 1 mtest2 mtest2   4285941037  3月 21 13:16 Inbox
  -rw-r--r-- 1 mtest2 mtest2      4547345  3月 21 13:16 Inbox.msf
  -rw------- 1 mtest2 mtest2      4722922  3月 18 22:10 


Note that the Inbox is tad larger than the last time I started., but it is still
less than 2^32 octets.

On the next "Get Mail" attmpted, TB complained about the Inbox size
and not started the download at all.

So all in all, the check now seems to work so that the Inbox won't go
over 2^32 octets during (or before?) POP3 downloading.  (And the test
seems to take care of the accumulated size of e-mails to be
downloaded?)

Great! Thank you for the patches!
Now I am more comfortable starting large download.
[Splitting large folder files into manageable pieces offline was never
a pleasant excersise.]

(But I think we need to make sure the following case is handled too:
Downloading to Inbox, then filtering tries to move the article to a
different, almost full folder. In this case, POP3 downloading works,
but the problem was that I could not get to stop downloading since the
modal dialog about the full folder continued popping up and did not
give me the chance to stop downloading (!?) at all. If pop3 download continues with hundreds of e-mails which are then being fed to the full folder due to filtering
then we may still have a problem. But changing the status of download may be a way to go as mentioned somewhere.
 Gee, I can not find the bugzilla entry handy, but I reported this problem before.)


As for Win build, I can't get it work on TryServer. Do the files for windows build 
need to have CRLF ending as opposed to LF?
(Or is it possible that the patch in question has unix-like directory separator "/" instead of
DOS style separator (\)?)
I am stumped for windows build. (Or do we need to add more patches to client.py, etc?)

TryServer Submission
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=d6fe1500ab71

See the brief log of Win build 
https://tbpl.mozilla.org/php/getParsedLog.php?id=20897348&tree=Thunderbird-Try

It is possible that I might have contaminated my local tree for
tryserver build.  (I must have done something stupid since now and
then my mercurial command has included some patchtes from tryserver
unintentionally like the following one : "8a50d60dd7a0 Sebastian
Hengst – Bug 852690 - Remaining conversion to mailServices.js: instant
messaging" )
Even then TB for linux32 and linux64 built fine.

Thank you again for improving the robustness of TB!
Looking at the patch
--- /dev/null
     1.3 +++ b/mozilla-prtime-patch-0312.patch
     1.4 @@ -0,0 +1,1184 @@
     1.5 +diff --git a/nsprpub/pr/src/misc/prtime.c b/nsprpub/pr/src/misc/prtime.c

... looks suspicious since prtime.c already exists!
Thanks for the test!

(In reply to ISHIKAWA, chiaki from comment #24)
> I am not sure if the size of Inbox is checked at each e-mail or BEFORE
> the downloading begins, but AT LEAST WE ARE SAFER now (!).
>
> So all in all, the check now seems to work so that the Inbox won't go
> over 2^32 octets during (or before?) POP3 downloading.  (And the test
> seems to take care of the accumulated size of e-mails to be
> downloaded?)
As I understand the code this patch only checks before download of all messages. It does not recheck before each message. But I'd be glad if it did. I may look into that more.

> Great! Thank you for the patches!
> Now I am more comfortable starting large download.
> [Splitting large folder files into manageable pieces offline was never
> a pleasant excersise.]
> 
> (But I think we need to make sure the following case is handled too:
> Downloading to Inbox, then filtering tries to move the article to a
> different, almost full folder. In this case, POP3 downloading works,
> but the problem was that I could not get to stop downloading since the
> modal dialog about the full folder continued popping up and did not
> give me the chance to stop downloading (!?) at all. If pop3 download
> continues with hundreds of e-mails which are then being fed to the full
> folder due to filtering
> then we may still have a problem. But changing the status of download may be
> a way to go as mentioned somewhere.
>  Gee, I can not find the bugzilla entry handy, but I reported this problem
> before.)
This should be bug 634544. But did you experience this problem with this patch?

> As for Win build, I can't get it work on TryServer. Do the files for windows
> build 
> need to have CRLF ending as opposed to LF?
> (Or is it possible that the patch in question has unix-like directory
> separator "/" instead of
> DOS style separator (\)?)
> I am stumped for windows build. (Or do we need to add more patches to
> client.py, etc?)
> 
> TryServer Submission
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=d6fe1500ab71
> 
> See the brief log of Win build 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=20897348&tree=Thunderbird-
> Try

I have never tried the try-server but as I understand it, you should upload the patch in the format you use for check-in, that means unix line endings. The tryserver should check-in the patch into its copy of the tree and produce the builds for all platforms from this one tree.
Comment on attachment 726953 [details] [diff] [review]
patch

I've got confirmation from irving that he will look at the patches we created.
Attachment #726953 - Flags: feedback?(ishikawa) → review?(irving)
Please can we have a test for this before we land it. We've already got one that has some tests for over 4GB mailboxes, so we should extend that or base a new test on it:

http://mxr.mozilla.org/comm-central/source/mailnews/local/test/unit/test_over4GBMailboxes.js
(In reply to Magnus Melin from comment #25)
> Looking at the patch
> --- /dev/null
>      1.3 +++ b/mozilla-prtime-patch-0312.patch
>      1.4 @@ -0,0 +1,1184 @@
>      1.5 +diff --git a/nsprpub/pr/src/misc/prtime.c
> b/nsprpub/pr/src/misc/prtime.c
> 
> ... looks suspicious since prtime.c already exists!

Maybe my understanding of leaving the patch to m-c portion (i.e., below ./mozilla
directory of comm-central) might have been screwed.
But if so, why didn't the linux32, linux64 build complain?
That is my point. :-)
Come to think of it, this patch can be left out for
testing 4GB issue. Maybe I will submit a job WITHOUT this patch and see
how OTHER patches are handled.

TIA
(In reply to Mark Banner (:standard8) from comment #28)
> Please can we have a test for this before we land it. We've already got one
> that has some tests for over 4GB mailboxes, so we should extend that or base
> a new test on it:
> 
> http://mxr.mozilla.org/comm-central/source/mailnews/local/test/unit/
> test_over4GBMailboxes.js

I think that test "appends" a file using
"copyFileMessageInLocalFolder" from the comment. (Very quick reading.)
But we need to create a real POP3 download, I think.

Can we use a local test SMTP server and POP3 server on the TryServer,
or maybe is there a known-to-exist (and not-likely-to-disappear-very-soon) mail server host for testing purposes within mozilla infrastructure?
[It needs to handle maybe a 10MB of e-mails accumulated after a large enough folder is created by appending locally. But the final check must be done
by receiving e-mails via POP3 (and presumably sending them from the test locally to this SMTP and POP3 server combination: and AFTER the account for this POP3 is created so that we can access and download the server.)
A test for this would be strange. yes we could maybe make one to test if it does not allow us to go over 4GB. But our intention is to remove the limit in hasSpaceAvailable (or make it 2^64). Do we then want to test with a 2^64 bytes long file? :) Or we just remove the test.

It makes sense to build a test that tests if we can handle (copy, delete, compact) a >4GB mbox file as that can be broken anytime in the future (by some patch introducing new 32bit variables by mistake). But to test the patch here (pop3 download going over limit) we'd need a way to temporarily lower the limit in a test so that we can test against a sane boundary (e.g. 2^10), not 2^64.

Maybe there is a pop3 server in the testing infrastructure.
(In reply to :aceman from comment #31)
> A test for this would be strange. yes we could maybe make one to test if it
> does not allow us to go over 4GB. But our intention is to remove the limit
> in hasSpaceAvailable (or make it 2^64). Do we then want to test with a 2^64
> bytes long file? :) Or we just remove the test.

I didn't think that was what you were doing (especially as I've seen no responses about my comments elsewhere about David saying it is not compatible just to update the pointer into the file).

> It makes sense to build a test that tests if we can handle (copy, delete,
> compact) a >4GB mbox file as that can be broken anytime in the future (by
> some patch introducing new 32bit variables by mistake). But to test the
> patch here (pop3 download going over limit) we'd need a way to temporarily
> lower the limit in a test so that we can test against a sane boundary (e.g.
> 2^10), not 2^64.

Yes, we should definitely have a test (or mutate this one) for putting emails in > 4GB mbox. I wouldn't worry about the limit.

> Maybe there is a pop3 server in the testing infrastructure.

Yes, xpcshell tests have a pop3 fake server, there are plenty of examples of its use.
(In reply to Mark Banner (:standard8) from comment #32)
> (In reply to :aceman from comment #31)
> > A test for this would be strange. yes we could maybe make one to test if it
> > does not allow us to go over 4GB. But our intention is to remove the limit
> > in hasSpaceAvailable (or make it 2^64). Do we then want to test with a 2^64
> > bytes long file? :) Or we just remove the test.
> 
> I didn't think that was what you were doing (especially as I've seen no
> responses about my comments elsewhere about David saying it is not
> compatible just to update the pointer into the file).
That is a different bug 793865 where you put that comment and for which I agree we should have a test as I write later in the comment.

The bug here is to make TB obey any limit we set in hasSpaceAvailable which it does not do at POP3 download. But hopefully the limit can be increased in the future.

> > It makes sense to build a test that tests if we can handle (copy, delete,
> > compact) a >4GB mbox file as that can be broken anytime in the future (by
> > some patch introducing new 32bit variables by mistake). But to test the
> > patch here (pop3 download going over limit) we'd need a way to temporarily
> > lower the limit in a test so that we can test against a sane boundary (e.g.
> > 2^10), not 2^64.
> 
> Yes, we should definitely have a test (or mutate this one) for putting
> emails in > 4GB mbox. I wouldn't worry about the limit.
(In reply to :aceman from comment #26)
> Thanks for the test!
>
   [...]

> > (But I think we need to make sure the following case is handled too:
> > Downloading to Inbox, then filtering tries to move the article to a
> > different, almost full folder. In this case, POP3 downloading works,
> > but the problem was that I could not get to stop downloading since the
> > modal dialog about the full folder continued popping up and did not
> > give me the chance to stop downloading (!?) at all. If pop3 download
> > continues with hundreds of e-mails which are then being fed to the full
> > folder due to filtering
> > then we may still have a problem. But changing the status of download may be
> > a way to go as mentioned somewhere.
> >  Gee, I can not find the bugzilla entry handy, but I reported this problem
> > before.)
> This should be bug 634544. But did you experience this problem with this
> patch?

   [...]

Well, the new behavior in 22.0a1 (2013-03-21) is a mixed blessing.

I tested as follows.

I copied almost full Inbox to a differently named folder "FilteredFolder", and
created a mail filter to move e-mail article that has "FILTER to other folder" in its subject line. And, I injected e-mails with such subject every two e-mails and see how TB handles the situation.

STEP-1. (I forgot to re-create the .msf file for FilteredFolder folder by simply renaming Inbox to this name.)

Empty Inbox received the incoming e-mails, but received e-mails are not
filtered to the intended destination. It seems that if the .msf file is not
there, the filter doesn't work. 

I see something like the following in the console where thunderbird
was started.

I think the message is stored into Inbox, but then
somehow when the filter was run, destMailDB (i.e., .msf file?) was
not accessible and thus the filtering SILENTLY failed!

Incorporate message begin:  	      <--- this is not filtered.
uidl string: n'$#!0HO!!8+#"!QbI"!
Incorporate message complete.
Incorporate message begin:            <--- every 2nd message is filtered.
uidl string: W~\!!Y~d!!d9'#!I~k!!
GetDiskSpaceAvailable returned: 8276869120 bytes
WARNING: failed to open mail db parsing folder: 'destMailDB && NS_SUCCEEDED(rv)', file /COMM-CENTRAL/comm-central/mailnews/local/src/nsParseMailbox.cpp, line 2540
Incorporate message complete.         <--- It seems filtering was ignored.
Incorporate message begin:                 But there was no visible feedback.
uidl string: m2F!!$#m!!ZYQ"!YB#"!
Incorporate message complete.
Incorporate message begin:
uidl string: A*\!!/j~!!QQj"!2c;!!
GetDiskSpaceAvailable returned: 8275832832 bytes
WARNING: failed to open mail db parsing folder: 'destMailDB && NS_SUCCEEDED(rv)', file /COMM-CENTRAL/comm-central/mailnews/local/src/nsParseMailbox.cpp, line 2540
Incorporate message complete.
Incorporate message begin:
uidl string: UAm!!mTk!!Xlp!!o(D"!
Incorporate message complete.

I am afraid that this is a buggy behavior. We should be warned (ONCE at least).


STEP-2: I now re-created .msf file and
deleted enough e-mails to carry out more testing.

OK, what happened is this.

Now filtering works up to the point where "FilteredFolder" reaches
the size limit.

After that, again, filtering SILENTLY FAILS and
the message is left in the Inbox WITHOUT ANY VISIBLE FEEDBACK to the user.
(and for that matter no discernible console message either on the 
original terminal console.)
I am sure this will confuse the users to no end eventually.
I don't want the error dialog to pop up for EVERY download, but
at least this should be warned JUST ONCE during the whole POP3 download
session to tell the user
  - the target folder of a filtering is full and e-mail can not be
    moved/copied.
    So making space in the folder by moving e-mails to somewhere else
    or delete) and compact it, AND THEN
* - RE-RUN FILTER on Inbox so that the intended filtering takes place then!

The second point marked (*) is important.

And showing this JUST ONCE, and not for every single failure of a
filter to this destination, is enough. Otherwise the user would get
panicky like I did. (Hmm, maybe the POP3 download session needs
to keep track of a destination folder where move/copy to it fails.)

But I will move this discussion to Bug 634544

TIA
Blocks: 789679
No longer depends on: 789679
Blocks: 239455
Ｃｈｅｃｋｅｄ Compact with Tb 17.0.3 on MS Win:
  - "Over 4GB" by POP3 download.
  - Increment of messageKey by 1, because 0xFFFFFF00 < messageOffset.

(1) Initial.
    1022 x just 4MB mail is held (Inbox size == 4GB - 8MB)

Bottom message before download.
> ------------------------------------------------------------------------------------------
> 4MB-mail-#1022: messageKey = 4282384384, messageOffset = 4282384384, messageSize = 4194304
> ------------------------------------------------------------------------------------------

(2) 10 x 7MB mails, 1 x 10KB mail, 512 x 1KB mails, are downloaded.

> ------------------------------------------------------------------------------------------
> 7MB-mail-#01  : messageKey = 4286578688, messageOffset = 4286578688, messageSize = 7340184
> 7MB-mail-#02  : messageKey = 4293918872, messageOffset = 4293918872, messageSize = 7340184
>                 -------------------------- 4GB boundary( 4294967296 ) is here ------------
> ------------------------------------------------------------------------------------------
> 7MB-mail-#03  : Mail is Lost. Not shown at thread pane (Folder View=All, View=All)
>       If shown, messageKey = 4293918873, messageOffset = 4293918872 + 7340184 == 4301259056, messageSize = 7340184
> ------------------------------------------------------------------------------------------
> 7MB-mail-#04  : messageKey = 4293918874, messageOffset = 4308599240, messageSize = 7340184
> 7MB-mail-#05  : messageKey = 4293918875, messageOffset = 4315939424, messageSize = 7340184
> 7MB-mail-#06  : messageKey = 4293918876, messageOffset = 4323279608, messageSize = 7340184
> 7MB-mail-#07  : messageKey = 4293918877, messageOffset = 4330619792, messageSize = 7340184
> 7MB-mail-#08  : messageKey = 4293918878, messageOffset = 4337959976, messageSize = 7340184
> 7MB-mail-#09  : messageKey = 4293918879, messageOffset = 4345300160, messageSize = 7340184
> 7MB-mail-#10  : messageKey = 4293918880, messageOffset = 4352640344, messageSize = 7340184
> ---------------------------------------------------------------------------------------------------------------------------------
> 10KB-mail-#1  : messageKey = 4293918880 + 1,       messageOffset = 4352640344 + 7340184,                    messageSize = 1048030
> ---------------------------------------------------------------------------------------------------------------------------------
> 1KB-mail-#001 : messageKey = 4293918880 + 1 + 001, messageOffset = 4352640344 + 7340184 + 1048030 + 000 KB, messageSize =  1 KB
>             |                                   |                                                     |
> 1KB-mail-#512 : messageKey = 4293918880 + 1 + 512, messageOffset = 4352640344 + 7340184 + 1048030 + 511 KB, messageSize =  1 KB
> ------------------------------------------------------------------------------------------

(3) At this stage, any mail except lost 7MB-mail-#03 can be viewed, because messageOffset and messageSize is correct.

(4) After Repair Folder.
    (A) all mails of "messageKey != messageOffset" is lost by Repair Folder
    (B) mail at 4MB boundary(7MB-mail-#2) is broken.
        Top part is correct because messageOffset is correct,
        but message data is cut at mid of mail,
        because messageSize is cut.
          
> ------------------------------------------------------------------------------------------
> 4MB-mail-#1020: messageKey = 4273995776, messageOffset = 4273995776, messageSize = 4194304
> 4MB-mail-#1021: messageKey = 4278190080, messageOffset = 4278190080, messageSize = 4194304
> 4MB-mail-#1022: messageKey = 4282384384, messageOffset = 4282384384, messageSize = 4194304
> 7MB-mail-#01  : messageKey = 4286578688, messageOffset = 4286578688, messageSize = 7340184
> 7MB-mail-#02  : messageKey = 4293918872, messageOffset = 4293918872, messageSize = 1048422 <= mail size is broken
> ------------------------------------------------------------------------------------------
Sorry, typo. 
  Ｃｈｅｃｋｅｄ "Over 4GB by download" with Tb 17.0.3 on MS Win:
Sorry, another typo. 10KB-mail-#1 => 1MB-mail-#1
(In reply to WADA from comment #35)
> (4) After Repair Folder.
>     (A) all mails of "messageKey != messageOffset" is lost by Repair Folder
>     (B) mail at 4MB boundary(7MB-mail-#2) is broken.
>         Top part is correct because messageOffset is correct,
>         but message data is cut at mid of mail,
>         because messageSize is cut.
>           
> > ------------------------------------------------------------------------------------------
> > 4MB-mail-#1020: messageKey = 4273995776, messageOffset = 4273995776, messageSize = 4194304
> > 4MB-mail-#1021: messageKey = 4278190080, messageOffset = 4278190080, messageSize = 4194304
> > 4MB-mail-#1022: messageKey = 4282384384, messageOffset = 4282384384, messageSize = 4194304
> > 7MB-mail-#01  : messageKey = 4286578688, messageOffset = 4286578688, messageSize = 7340184
> > 7MB-mail-#02  : messageKey = 4293918872, messageOffset = 4293918872, messageSize = 1048422 <= mail size is broken
> > ------------------------------------------------------------------------------------------

Can you now try with the build with patch from bug 793865, here:
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/archaeopteryx@coole-files.de-1359bd2452f9/
Highgest message key in above test:
  4293918880 +1 512 = 3 GB + 1023 MB + 0 KB + 673
So, messageKey overflow is not checked yet.
> Can you now try with the build with patch from bug 793865, here:
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/
> archaeopteryx@coole-files.de-1359bd2452f9/

Does win32.zip of the build run normally?
> Does win32.zip of the build run normally?

I don't know, I only ran linux build. The win build compiles, it just fails tests/crashes when filters are run. But I think you can try it on a test profile for some manual message moves.
(In reply to :aceman from comment #38)
> Can you now try with the build with patch from bug 793865, here:
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/
> archaeopteryx@coole-files.de-1359bd2452f9/

Attached is messageKey, messageOffset, message size of mails after;
(Sorted by messageOffset which corresponds to order of download)
(1) Inbox state is after test of Comment #38.
    Because noCompact is executed, data in Berkley Mbox file is not altered.
(2) With patch, Download 10 x 7MB mail, 1 x 1 MB mail, 512 x 1KB mail.
    No problem.
(3) Repair Folder.
    All disappeared mails in test of Comment #38 without patch, was recovered,
    except following lost mail. 
    Truncated mail size(7MB-mail-#2 at 4GB boundary) was also recovered.
    messageKey assign algorythm seems;
      If offset is near 2GB, increase by 1.
> ------------------------------------------------------------------------------------------
> 7MB-mail-#03  : Mail is Lost. Not shown at thread pane (Folder View=All, View=All)
>       If shown, messageKey = 4293918873, messageOffset = 4293918872 + 7340184 == 4301259056, messageSize = 7340184
> ------------------------------------------------------------------------------------------
Attachment #728649 - Attachment description: With patch, messageOffset, messageKey, messageSize, after download over 4B and Repair Folder (In reply to :aceman from comment #38) > Can you now try with the build with patch from bug 793865, here: > → With patch, messageOffset, messageKey, messageSize, after download over 4B and Repair Folder (Sorted by messageOffset)
FYI.
"Repair Folder" in test of comment #42 took longer(I felt approximately doubled) than "Repair Folder" with "4GB Inbox filled by 4MB x 1024 mails + outdated .msf or no .msf" by Tb 17.0.3.
Checked Compact with try server build(win32.zip).
(1) Inbox : after test of comment #42
(2) Delete one mail at offset=0 (4194304 bytes == just 4 MB)
    File size after Compact > 4GB
(3) Do Compact
    => All mails including offset>4GB mail are normally shifted
       4194304 bytes.
    sizeOnDisk = 4420954336
    4420954336 = 4 GB + 120 MB + 154 KB + 224 Bytes

Position switch from "messageKey = messageOffset" to "messageKey = highest messageKey + 1" :
Last message of "messageKey = messageOffset"
> messageKey = 2143289344, messageOffset = 2143289344, messageSize = 4194304
> 2143289344 = 1 Giga + 1020 Mega + 0 Kilo + 0 / 2143289344 = 1 GB + 1020 MB + 0 KB + 0 Bytes
Message where starts "messageKey = highest messageKey + 1"
> messageKey = 2143289345, messageOffset = 2147483648, messageSize = 4194304
> 2143289345 = 1 Giga + 1020 Mega + 0 Kilo + 1 / 2147483648 = 2 GB +    0 MB + 0 KB + 0 Bytes
Logic looks;
  messageOffset < 2GB   => messageKey = messageOffset
  2GB <= messageOffset  => messageKey = highest messageKey + 1
(In reply to WADA from comment #44)
> Logic looks;
>   messageOffset < 2GB   => messageKey = messageOffset
>   2GB <= messageOffset  => messageKey = highest messageKey + 1

Yes, that is as intended, as we talked in another bug.
The problem is, I can't promise this logic for all messages, e.g. when copying across folders I get "highest messageKey + 1" also when below 2GB, because I do not know the offset. I don't know if that is a problem. But the original code did that too. In that case I don't know why we need 'messageKey = messageOffset' for any emails and really can't just make messageKey increment by 1 starting at 0 for all messages.
Flags: needinfo?(mozilla)
(In reply to :aceman from comment #45)
> (In reply to WADA from comment #44)
> > Logic looks;
> >   messageOffset < 2GB   => messageKey = messageOffset
> >   2GB <= messageOffset  => messageKey = highest messageKey + 1
> original code did that too. In that case I don't know why we need
> 'messageKey = messageOffset' for any emails (snip)

I guess that;
Because messageKey==messageOffset==envelopePos continued for long time,
many codes written during "less than 2GB" era don't distinguish messageKey, messageOffset, envelopePos, so developer(David or MScott perhaps) kept messageKey==messageOffset for less than 2GB.
Another reason is perhaps "compatibility of .msf file" with older Tb/Mozilla Mail&News to avoid rebuild-index by fall back to older version.

Because messageKey and messageOffset are cleanly separated by David(64bits offset for IMAP offline-store and pluggable message store), and because envelopePos is also isolated by you, I don't think there is no need to keep messageKey==messageOffset any place, as far as "incompatibility of .msf file with older Tb" is clearly specified in Release Notes and is clearly written in announcement news or letter by Mozilla Messaging Company.

A way to force rebuild-index in older Tb upon fall back.
- in Tb 17.0.x, add code to detect incompatibility and do rebuild index.
- by Tb 17.0.y(y>x) or Tb XX(XX>17), do spec change of messageKey.
This is ordinal method when enhancement or new feature will be released by next version.
Comment on attachment 726953 [details] [diff] [review]
patch

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

Please don't put "r=irving" into the patch comment until after I've given the r+ - usually it's the last step before pushing the patch.

Aside from that, the patch looks good. I agree with Standard8 that it would be good to have a test case for this, but it's going to be hard to make a test that stays meaningful when we remove the 4gb limit. I wonder if it's possible to stub in a message store that lets our test control when to return false from HasSpaceAvailable()?

Giving feedback+, but I'd like to settle the test question before we go ahead.
Attachment #726953 - Flags: review?(irving) → feedback+
(In reply to :aceman from comment #21)
> This still does not catch the problem that the download starts (with enough
> space) but the disk gets filled with another application while downloading.
> If we want this granularity I'll try to look at that in the next iteration
> of the patch.

This is awesome work!

We should definitely file and fix that type of "bug", here, and everywhere in Thunderbird, given the potential for problems.  In general we should be more conservative / less skimpy about what we consider to be a safe amount of space available. In other words, it is not our job to help users keep their disks as full as possible. :)
Wayne, we try to consistently keep 4MB free on the disk before growing a folder. Before that, there were inconsistently applied limits like 3MB and 1MB. What limit do you propose?

(In reply to Wayne Mery (:wsmwk) from comment #48)
> (In reply to :aceman from comment #21)
> > This still does not catch the problem that the download starts (with enough
> > space) but the disk gets filled with another application while downloading.
> > If we want this granularity I'll try to look at that in the next iteration
> > of the patch.

I'll check that out in the next patch in this bug as maybe in that case we just interrupt the download and do not inform the user verbosely (not sure how hard that will be). But on the next download attempt we will inform him thanks to this already existing patch.
(In reply to :Irving Reid from comment #47)
> Please don't put "r=irving" into the patch comment until after I've given
> the r+ - usually it's the last step before pushing the patch.
But I am never the one pushing it, so I prepare it for the pusher. But no problem, I can leave to to him to add it.

> Aside from that, the patch looks good. I agree with Standard8 that it would
> be good to have a test case for this, but it's going to be hard to make a
> test that stays meaningful when we remove the 4gb limit. I wonder if it's
> possible to stub in a message store that lets our test control when to
> return false from HasSpaceAvailable()?
What about a hidden pref?

> Giving feedback+, but I'd like to settle the test question before we go
> ahead.
Ok, I'll need to check out the pop3 test server. For the existing limit of 4GB we should be able to make a test if the download fails at the limit.
Flags: in-testsuite?
Attached patch patch v2 + test — — Splinter Review 
Added the requested test.
Attachment #726953 - Attachment is obsolete: true
Attachment #742703 - Flags: review?(irving)
Blocks: 794303
Blocks: 616559
Attachment #742703 - Flags: review?(irving) → review+
https://hg.mozilla.org/comm-central/rev/cc6213af0ce8

(also clearing request for David, as I suspect that's no longer needed)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: needinfo?(mozilla)
Flags: in-testsuite?
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
Depends on: 872357
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: