793865 - nsIMsgParseMailMsgState.envelopePos must be 64bits to support large mbox folders (eg. 4GB)

Status: RESOLVED FIXED

(MailNews Core :: Database, defect, P1)

Description

[Kent James (:rkent)]

This must be set in certain file to mbox operations, and they will fail for >4GB folders as currently implemented.

Comment 1

[:aceman]

Is this really relevant only for 64bit platforms? I think >4GB folders are supported on 32bit too? We just need to force the  variable to be 64bit long.

Comment 2

[Kent James (:rkent)]

(In reply to :aceman from comment #1)
> Is this really relevant only for 64bit platforms? I think >4GB folders are
> supported on 32bit too? We just need to force the  variable to be 64bit long.

Right, this is an issue with 64bit file systems (like most modern file systems), not 64bit platforms.

Comment 3

[:aceman]

I was reacting to Usul's setting of 64bit keyword. Thanks for confirmation.

Comment 4

[Wayne Mery (:wsmwk)]

adding bug 794303 - but if there's better bug(s), or more, please update

Comment 5

[Wayne Mery (:wsmwk)]

Does this (and it's friend bug 794303) not result in effective dataloss for user, i.e. user not able to read all or part of folder?

Comment 6

[Kent James (:rkent)]

I think that currently there are other places that prevent the folder from exceeding 32 bits in size, so this is not currently causing data loss. But that is only a guess.

Comment 7

[WADA:World Anti-bad-Duping Agency]

Comment(perhaps by David) existed in IMAP code.
> http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#3031
> 3031   m_msgParser->SetEnvelopePos(m_curMsgUid);
> 3032   // m_envelope_pos, for local folders,
> 3033   // is the msg key. Setting this will set the msg key for the new header.

position etc. is already defined as 64bits integer in header file.
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.h#77
> 77   int64_t              m_position;
> 78   uint64_t              m_envelope_pos;
> 79   uint64_t              m_headerstartpos;

SetEnvelopePos intentionally interprets passed value as 32bits integer, even when passed parameter is 64bits integer?
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#644
> 644 NS_IMETHODIMP nsParseMailMessageState::SetEnvelopePos(uint32_t aEnvelopePos)
> 646   m_envelope_pos = aEnvelopePos;
> 647   m_position = m_envelope_pos;
> 648   m_headerstartpos = m_position;

Why nsPop3Sink::IncorporateBegin pass 64bits file size data to SetEnvelopePos after conversion to 32bits integerve long the way?
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Sink.cpp#556
> 556     if (m_downloadingToTempFile)
> 558       // Tell the parser to use the offset that will be in the dest folder,
> 559       // not the temp folder, so that the msg hdr will start off with
> 560       // the correct mdb oid
> 561       int64_t fileSize;
> 562       path->GetFileSize(&fileSize);
> 563       m_newMailParser->SetEnvelopePos((uint32_t) fileSize);

Same "conversion from 64bits to 32bits" is seen in "Copy from IMAP Mbox to offline-store file of IMAP.
But this is usually not problem in IMAP, because MsgKey==UID in IMAP.  "Over flow of 32bits integer for UID" occurs when 4,294,967,296 th mail is added to the IMAP Mbox. 
> http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#8285
> 8285 nsImapMailFolder::CopyFileToOfflineStore(nsIFile *srcFile, nsMsgKey msgKey)
> 8331         uint64_t offset;
> 8332         fakeHdr->GetMessageOffset(&offset);
> 8333         // This will fail for > 4GB mbox folders, see bug 793865
> 8334         msgParser->SetEnvelopePos((uint32_t) offset);
Kent, when did you add the comment in the source?

Can this bug cause "delete of wrong mail(different offset)?

AFAIK, "mail poistion/size of mail to move/copy/delete" is determined via message's index(index in enumerator, getElementAt) or messageKey => messageOffset and messageSize, and "position of X-Mozilla-Status:" is obtained from statusOffset which is relative to messageOffset.

I think this bug can cause "wrong subject/From/Date at thread pane, usually blank or broken/corrupted", or can cause "false negative in message/junk filter" frequently due to parsing of data at wrong position, but I can't think this bug causes "false positive in message/junk filter thus wrong mail delete" so frequently...

Comment 8

[:aceman]

Well, we can try updating the int32_t envelope usages to 64bit and see what happends. Chiaki seems to have a good test environment and can see if proper messages are being deleted.

I've also found some suspicious spot at http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#1394 where envelope_pos (64bit already) is being sent as argument that should be nsKeyMsg (32bit for now). Aside from the size mismatch, if envelope_pos is being used as absolute offset into the mbox file (in other code spots) message key is something else (at least for keys nearing 2^32) so there may be a semantic mismatch and behavior failure.
Similarly for http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#1399 .

rkent, are you working on this, or can we try to make some patch that you then look over?

Comment 9

[Mark Banner (:standard8)]

I'm pretty sure David Bienvenu said this was hard to do, because of the way the current offsets are stored in the mbox files which would cause issues if you just moved straight to 64 bit. Hence at the time moving to maildir was the "easiest" way of fixing this.

Comment 10

[:aceman]

But is envelope_pos actually stored in the .msf database? Probably message key is and that one is hacked to still be under 32bit.
Looks like message offset may be in the database too but it already is 64bit: http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgHdr.cpp#595

Also, there is already code storing the 64bit m_envelope_pos into the db as the message offset.
http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#824 :
m_newMsgHdr->SetMessageOffset(m_envelope_pos);

In this bug we do not intend (at least not me) to suddenly make some variable 64bit. We just want to see why the existing 64bit variable is being clamped to 32bit in some places (as we see some corruption due to another message being operated on than the one selected in UI).

Does it make sense?

Comment 11

[Kent James (:rkent)]

No I am not working on this, sorry, so I removed my name for now.

Comment 12

[:aceman]

Attachment: patch for nsMsgKey

So this patch changes some of the spots I found suspicious and does not mix msgKey with msgOffset or envelope_pos in function calls.

With this patch I could finally successfully compact a folder >4GB that was not compactable with only the patch in bug 794303. And the folder still stayed above 4GB as I did not remove enough messages. But that is fine.

The patch also changes the message key generation to start incrementing the key by 1 instead of using file position starting at position 0x7FFFFFFF:
nsMsgKey key = filePos > 0x7FFFFFFF ? nsMsgKey_None : (nsMsgKey) filePos;

Previously it was only at 0xFFFFFF00 which allowed to only store 254 messages once the 4GB filesize was hit. I think I was hit by this in my tests that is why patch in bug 794303 was not enough for me. In my tests I did not use big messages as WADA and Chiaki tried out. I used small messages (2-200KB) so fill the folder. So I have 1 MILLION messages in the folder. TB still works fine (albeit many operations on msgs in the folder are quite slow). So to go over 4GB I copied thousands of messages so I hit the 254 limit easily.

Chiaki, can you please try this patch if you still get the problem of wrong messages being deleted in the folder? I could not observe such thing. For the patch to work apply all the other patches in the bugs where we work the 4GB limit.
The patches are these:
bug 794303
Do not apply patch 1 in bug 789679 (the hasSpaceAvailable fix).
bug 789679 (patch 2 for filesize)
bug 640371
bug 793865 patch for nsMsgKey
bug 793865 patch for hasSpaceAvailable

I may attempt to do some tests but only once irving (or anybody) looks at the patches and decides the direction is fine and we can continue polishing it (and find bugs via tests).

Comment 13

[:aceman]

Attachment: patch for hasSpaceAvailable to allow >4GB

This is a temporary patch to apply instead of patch for hasSpaceAvailable in bug 789679. It removes the 4GB limit in hasSpaceAvailable so TB allows you to cross that limit while doing any message operations. You can try copying messages into the folder, pop3 download, delete and compact and all should work above 4GB.

Comment 14

[:aceman]

Aryx, could you please create a try-build for us from these patches (apply in this order):
https://bugzilla.mozilla.org/attachment.cgi?id=726176
https://bugzilla.mozilla.org/attachment.cgi?id=728540
https://bugzilla.mozilla.org/attachment.cgi?id=725187
https://bugzilla.mozilla.org/attachment.cgi?id=726953
https://bugzilla.mozilla.org/attachment.cgi?id=728537

Comment 15

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #12)
> patch for nsMsgKey

Good catch!

(In reply to :aceman from comment #10)
> Also, there is already code storing the 64bit m_envelope_pos into the db as the message offset.
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#824 :
> m_newMsgHdr->SetMessageOffset(m_envelope_pos);

This is perhaps code written in era of "messageKey === messageOffset === messageEnvelopePos always at anywhere"(era of fileSize=32bits signed integer anywhere). There is no need to distinguish messageKey, messageOffset, and messageEnvelopePos.

"(uint32_t)" in code like next is perhaps done to expand limit from 2GB to 4GB(32bits signed integer fileSize is changed to unsigned => force 32bits unsigned integer for envelopePos). 
> NS_IMETHODIMP nsParseMailMessageState::SetEnvelopePos(uint32_t aEnvelopePos)
> m_newMailParser->SetEnvelopePos((uint32_t) fileSize);

Culprits looks these two. 
  nsPop3Sink    : m_newMailParser->SetEnvelopePos((uint32_t) fileSize);
  nsParseMailbox: m_newMsgHdr->SetMessageOffset(m_envelope_pos);

Because messageOffset is passed to SetEnvelopePos of ParseMailbox from caller when local mail folder, I think "envelope_pos == messageOffset" is better declared explicitly.
> NS_IMETHODIMP nsParseMailMessageState::SetEnvelopePos(uint64_t aEnvelopePos)
=> 
> NS_IMETHODIMP nsParseMailMessageState::SetEnvelopePos(messageOffset)
> uint64_t EnvelopePos = messageOffset;
If "messageOffset" is explicitly indicated by parameter name, mystery of following code will disappear.
> nsParseMailbox: m_newMsgHdr->SetMessageOffset(m_envelope_pos);

:aceman, what do you think? Wasting of code, memory, CPU power(and electric bil)?

Comment 16

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #12)
> Created attachment 728537 [details] [diff] [review]
> patch for nsMsgKey
> 
> So this patch changes some of the spots I found suspicious and does not mix
> 


> Chiaki, can you please try this patch if you still get the problem of wrong
> messages being deleted in the folder? I could not observe such thing. For
> the patch to work apply all the other patches in the bugs where we work the
> 4GB limit.
> The patches are these:
> bug 794303
> Do not apply patch 1 in bug 789679 (the hasSpaceAvailable fix).
> bug 789679 (patch 2 for filesize)
> bug 640371
> bug 793865 patch for nsMsgKey
> bug 793865 patch for hasSpaceAvailable
> 
> I may attempt to do some tests but only once irving (or anybody) looks at
> the patches and decides the direction is fine and we can continue polishing
> it (and find bugs via tests).

Comment 17

[ISHIKAWA, Chiaki]

Sorry, hit the wrong key before completing the message

I will investigate using your patches.
I could not cause the symptom to manifest again easily. I think in the transient
patched state, the message key for two articles had the same lower 32 bit values
or something, but that needs to be have some lucky coincidence :-(
But Murphy's law struck me back when I started testing.
So I may not be able to reproduce the condition...
(Only large number of users may tell.)


Great work for the type replacement for so many places in your latest patch.
I have a suggestion. Why not convert nsMsgKey to a struct that has the same size of the current scalar size (for testing purposes).
I.e., just declare the struct type that has a member that holds the
value of the current nsMsgKey inside.

All incorrect usages (type mismatches) will cause compiler errors (not simple warnings), and only when you put the value into an internal/external DB or get the value from such a database, you convert the struct <-> scalar value if necessary. Since the size is the same it is a matter of taking out the member value or setting the member value.

I am sure this simple type change  will help spotting some 
obscure unintended usages.(I now see a place where initialization needs "{", "}", etc. So a few tweaking is certainly necessary. )
But getting the compiler find out the
problematic spots for you is an attractive idea.

TIA

Comment 18

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

(In reply to :aceman from comment #14)
> Aryx, could you please create a try-build for us from these patches (apply
> in this order):
> https://bugzilla.mozilla.org/attachment.cgi?id=726176
> https://bugzilla.mozilla.org/attachment.cgi?id=728540
> https://bugzilla.mozilla.org/attachment.cgi?id=725187
> https://bugzilla.mozilla.org/attachment.cgi?id=726953
> https://bugzilla.mozilla.org/attachment.cgi?id=728537

https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=1359bd2452f9
There are at least failing XPCShell tests.

Comment 19

[:aceman]

(In reply to ISHIKAWA, chiaki from comment #17)
> Great work for the type replacement for so many places in your latest patch.
> I have a suggestion. Why not convert nsMsgKey to a struct that has the same
> size of the current scalar size (for testing purposes).
> I.e., just declare the struct type that has a member that holds the
> value of the current nsMsgKey inside.
> 
> All incorrect usages (type mismatches) will cause compiler errors (not
> simple warnings), and only when you put the value into an internal/external
> DB or get the value from such a database, you convert the struct <-> scalar
> value if necessary. Since the size is the same it is a matter of taking out
> the member value or setting the member value.

Yes, I agree in the patch I change some uint32_t to nsMsgKey but that has basically no effect, only it indicates to the developer visually, that the function expects a msg key, not offset. I want to make the compiler check this but I am not that good at C++ so I have not yet done it. Thanks for the hints, I'll try your approach.
I wonder if we can declare a struct in an idl file where nsMsgKey is defined...

Comment 20

[:aceman]

(In reply to WADA from comment #15)
> If "messageOffset" is explicitly indicated by parameter name, mystery of
> following code will disappear.
> > nsParseMailbox: m_newMsgHdr->SetMessageOffset(m_envelope_pos);
> 
> :aceman, what do you think? Wasting of code, memory, CPU power(and electric
> bil)?

Message key is not message offset in current code. But I could not yet find out if message offset is the same as envelope position. I just made sure both are used as 64bit. In some places offset is set to envelope_pos value. But I am not sure that remains true later throughout the life of the message (header).

Comment 21

[:aceman]

(In reply to Archaeopteryx [:aryx] from comment #18)
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=1359bd2452f9
> There are at least failing XPCShell tests.

Thanks, that is what I needed too. Seems the test_over2GBMailboxes.js and test_over4GBMailboxes.js fail now. It is quite possible that these 2 just expect different msg keys than I now produce.

home/cltbld/talos-slave/test/build/xpcshell/tests/mailnews/local/test/unit/test_over2GBMailboxes.js | Over 2 GiB msgkey was not found! 

Strangely these tests pass for me locally (on Linux).

And there are some filter move crashes. I can see those locally so will investigate.

Comment 22

[:aceman]

(In reply to ISHIKAWA, chiaki from comment #17)
> Great work for the type replacement for so many places in your latest patch.
> I have a suggestion. Why not convert nsMsgKey to a struct that has the same
> size of the current scalar size (for testing purposes).
> I.e., just declare the struct type that has a member that holds the
> value of the current nsMsgKey inside.

OK, so I temporarily defined nsMsgKey as a struct (after fighting with idl). Making it actually build and run would be a possible, but maybe not necessary yet. But I at least audited all the compiler errors and found some more spots where I could convert uint32_t to nsMsgKey. But I think I have not found any spots where msgkey was mixed with offset.

Comment 23

[:aceman]

Attachment: patch for hasSpaceAvailable to allow >4GB v2

Comment 24

[Jim Michaels]

is this the one you wanted me to test?

Comment 25

[Jim Michaels]

if it's any help, fsetpos and fgetpos can handle 64-bit positions. fseek and ftell are 32-bit.

Comment 26

[:aceman]

Attachment: patch for nsMsgKey v2

OK, I found the cause of the crashes in filter move tests. Fortunately it was no architectural problem preventing us from going above 4GB, but me forgetting to initialize one pointer in case we moved messages and created a key without knowing the offset.

Comment 27

[:aceman]

Aryx, could you please create a try-build for us from these patches (apply in this order):
https://bugzilla.mozilla.org/attachment.cgi?id=726176
https://bugzilla.mozilla.org/attachment.cgi?id=728664
https://bugzilla.mozilla.org/attachment.cgi?id=725187
https://bugzilla.mozilla.org/attachment.cgi?id=726953
https://bugzilla.mozilla.org/attachment.cgi?id=728713

Comment 28

[:aceman]

(In reply to Jim Michaels from comment #25)
> if it's any help, fsetpos and fgetpos can handle 64-bit positions. fseek and
> ftell are 32-bit.

Our internal file handling versions of these functions are 64 bit. See e.g.
http://mxr.mozilla.org/comm-central/ident?i=Tell&tree=comm-central&filter=

I'll tell you which build to download when it is built from comment 27.

Comment 29

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #12)
> Created attachment 728537 [details] [diff] [review]
> patch for nsMsgKey
> 
 [...]
> Chiaki, can you please try this patch if you still get the problem of wrong
> messages being deleted in the folder? I could not observe such thing. For
> the patch to work apply all the other patches in the bugs where we work the
> 4GB limit.
> The patches are these:
> bug 794303
> Do not apply patch 1 in bug 789679 (the hasSpaceAvailable fix).
> bug 789679 (patch 2 for filesize)
> bug 640371
> bug 793865 patch for nsMsgKey
> bug 793865 patch for hasSpaceAvailable
> 
> I may attempt to do some tests but only once irving (or anybody) looks at
> the patches and decides the direction is fine and we can continue polishing
> it (and find bugs via tests).

I did the very brief testing using the patches and
a few scenarios, but I could not reproduce the strange deletion of
the unintended message any more. Maybe the proper 64-bit handling of
various data fixed the issue?

But I think we should rely on some of the test data (various folders) which Wada has produced to check for obscure bugs that would have happened
in the old incorrect code.
(Not sure how to create exactly the same test image using
a few number of suggested javascript files. It may be easist to compress
samples from Wada's and use them as part of the test suite. If the message body
only has repeated strings such as "aaa...aaa", it would compress rather well.)

So yes, I did not see any more problems with my limited testing, but
it could be that Murphy did not strike me this weekend. A systematic
attack [using carefully laid out test messages with known keys, etc.,
and test suite is the way to go.] I suppose that the message key is now created to avoid the issues, and if so, then, the test suite helps us in catching future regressions for potential problems. Some of the know folder images that have caused problems and reported in the bugzilla can be used for testing such potential regression problems, I suppose.]

TIA

Comment 30

[WADA:World Anti-bad-Duping Agency]

(In reply to ISHIKAWA, chiaki from comment #29)
> It may be easist to compress samples from Wada's and use them as part of the test suite.

FYI. ZIP file size of my 4GB test mail folder(majority is just 4MB crafted mails) is 1.8MB. Do you need script to generate crafted mails? (Object REXX script)

Comment 31

[:aceman]

(In reply to WADA from comment #30)
> FYI. ZIP file size of my 4GB test mail folder(majority is just 4MB crafted
> mails) is 1.8MB. Do you need script to generate crafted mails? (Object REXX
> script)

The folder would have to be unzipped in the test. Not sure if this is acceptable in the test suite. The current tests produce a 4GB folder but they cheat a bit using sparse files on the filesystem.

Comment 32

[:aceman]

OK, Aryx has produces the new build for us, it is available here:
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-700fbe00bf99/

Jim, WADA, Chiaki you can test on this one. Also tests below 4GB are useful as the internal changes affect all message operations, not only above 4GB.

The test_over4GBMailboxes.js failure is on my radar, but I have fixed all the other test failures.

Comment 33

[:aceman]

Attachment: patch for nsMsgKey v3 - mark nsMsgKey arguments

The patch has grown a bit so I split it for easier review.
This one just changes uint32_t arguments where nsMsgKey is actually wanted. This does not change any behaviour or compiled code, it just shows the developer what is the intention of the arguments and so he will less likely send something else as the argument (like offset). Yes, the arguments are usually in the idl as nsMsgKey, but let's make it even more obvious.
This patch could go in even if the other changes are stopped or postponed as not deemed good.

Comment 34

[:aceman]

Attachment: better separate offset/envelope_pos and msgKey

So this patch has the actual changes in behavior.
The existing test_over4GBMailboxes.js fails, because it expects the "folder full" message to appear. And I have actually disabled that one (in the patch for hasSpaceAvailable in this bug). I'll try to see how this test can be adapted to test normal operations over 4GB.

Comment 35

[Jim Michaels]

folks, I am actually getting errors from the OS on XP with regards to making .cmd files where the current dir path + command length is exceeding some internal limit and causing breakage of the shell script. ms bug I am going to have to report.
the cmd shell actually is supposed to handle command lengths of 8192 characters. but not if the current dir is really deep apparently...

vices.com|>rem if outdir surrounded with double quotes, strip them off, will add them later
( was unexpected at this time.

Tue 03/26/2013 18:32:29.14||E:\Documents and Settings\Jim Michaels\Application Data\Thunderbird\Profiles\md4bzvt7.default\Mail\mail.renewalcomputerser
vices.com|>if @"@==@"@ (

Tue 03/26/2013 18:32:29.14||E:\Documents and Settings\Jim Michaels\Application Data\Thunderbird\Profiles\md4bzvt7.default\Mail\mail.renewalcomputerser
vices.com|>

windows is treating cmd shell like old command.com for some reason. bizarre bug. I think the limit is 127 characters.


I think it reacts adversely to this if statement I just put in to my doubler:
if @%outdir:~0,1%@ equ @"@ (
    if @%outdir:~-1%@ equ @"@ set outdir=%outdir:~1,-1%
)

Comment 36

[Jim Michaels]

'twas bug in my .cmd shell script, needed a ^ before the " character to escape it. so here's the fixed version of my inbox doubler.

-----makeinboxpowerof2.cmd
rem handle help switch
if /i @%1@ equ @@       goto help
if /i @%1@ equ @/help@  goto help
if /i @%1@ equ @/h@     goto help
if /i @%1@ equ @/?@     goto help
if /i @%1@ equ @-help@  goto help
if /i @%1@ equ @-h@     goto help
if /i @%1@ equ @-?@     goto help
if /i @%1@ equ @--help@ goto help
if /i @%1@ equ @--h@    goto help
if /i @%1@ equ @--?@    goto help
if @%2@==@@ goto err
if @%3@==@@ goto err
rem set targetexponent=12
set targetexponent=%3
rem set outdir=\t
set outdir=%2
rem if outdir surrounded with double quotes, strip them off, will add them later
if @^%outdir:~0,1%@ equ @^"@ (
    if @^%outdir:~-1%@ equ @^"@ set outdir=%outdir:~1,-1%
)
rem set indir=\Documents and Settings\Jim Michaels\Application Data\Thunderbird\Profiles\md4bzvt7.default\Mail\mail.renewalcomputerservices.com
set indir=%1
rem if indir surrounded with double quotes, strip them off, will add them later
if @^%indir:~0,1%@ equ @^"@ (
    if @^%indir:~-1%@ equ @^"@ set indir=%indir:~1,-1%
)
del "%outdir%\inboxBIG" "%outdir%\z" "%outdir%\z2"
copy /b "%indir%\inbox" /b + "%indir%\drafts" /b + "%indir%\trash" /b + "%indir%\sent" /b "%outdir%\z2" /b
copy /b "%outdir%\z2" /b "%outdir%\InboxBIG" /b
set /a count+=1
set count=0
:lupe
if "%count%"=="%targetexponent%" goto exitlupe
copy /b "%outdir%\InboxBIG"+"%outdir%\InboxBIG" /b "%outdir%\z" /b
move /y "%outdir%\z" "%outdir%\InboxBIG"
set /a count+=1
dir "%outdir%\InboxBIG"
goto lupe
:exitlupe
del "%outdir%\z"
dir "%outdir%\InboxBIG"
goto end
:err
@echo ERROR: there need to be 3 arguments. see --help
goto end
:help
@echo makeinboxpowerof2.cmd - take inbox and create outdir\InboxBIG the size of (the size of inbox)*2^^integerTargetExponent
@echo usage: makeinboxpowerof2.cmd "inboxdirNoEndingBackslash" "outdirNoEndingBackslash" integerTargetExponent
@echo suggested value for integerTargetExponent is between 10 to 12 for an inbox of size 7MB.
@echo do a dir of your inbox, and copy+paste into ttcalc and use ttcalc to calculate the resulting size like this: 7,607,491*2^^10
@echo it is highly suggested you use double quotes around inboxdirNoEndingBackslash and outdirNoEndingBackslash.
@echo
goto end
:end

Comment 37

[Jim Michaels]

fixed the base on that code.
-----makeinboxpowerof2.cmd
rem handle help switch
if /i @%1@ equ @@       goto help
if /i @%1@ equ @/help@  goto help
if /i @%1@ equ @/h@     goto help
if /i @%1@ equ @/?@     goto help
if /i @%1@ equ @-help@  goto help
if /i @%1@ equ @-h@     goto help
if /i @%1@ equ @-?@     goto help
if /i @%1@ equ @--help@ goto help
if /i @%1@ equ @--h@    goto help
if /i @%1@ equ @--?@    goto help
if @%2@==@@ goto err
if @%3@==@@ goto err
rem set targetexponent=12
set targetexponent=%3
rem set outdir=\t
set outdir=%2
rem if outdir surrounded with double quotes, strip them off, will add them later
if @^%outdir:~0,1%@ equ @^"@ (
    if @^%outdir:~-1%@ equ @^"@ set outdir=%outdir:~1,-1%
)
rem set indir=\Documents and Settings\Jim Michaels\Application Data\Thunderbird\Profiles\md4bzvt7.default\Mail\mail.renewalcomputerservices.com
set indir=%1
rem if indir surrounded with double quotes, strip them off, will add them later
if @^%indir:~0,1%@ equ @^"@ (
    if @^%indir:~-1%@ equ @^"@ set indir=%indir:~1,-1%
)
del "%outdir%\inboxBIG" "%outdir%\z" "%outdir%\z2"
copy /b "%indir%\Inbox" /b + "%indir%\Drafts" /b + "%indir%\Trash" /b + "%indir%\Sent" /b "%outdir%\z2" /b
copy /b "%outdir%\z2" /b "%outdir%\InboxBIG" /b
set /a count+=1
set count=0
:lupe
if "%count%"=="%targetexponent%" goto exitlupe
copy /b "%outdir%\InboxBIG"+"%outdir%\InboxBIG" /b "%outdir%\z" /b
move /y "%outdir%\z" "%outdir%\InboxBIG"
set /a count+=1
dir "%outdir%\InboxBIG"
goto lupe
:exitlupe
del "%outdir%\z"
dir "%outdir%\InboxBIG"
goto end
:err
@echo ERROR: there need to be 3 arguments. see --help
goto end
:help
@echo makeinboxpowerof2.cmd - take inbox and create outdir\InboxBIG the size of (the size of inbox)*2^^integerTargetExponent
@echo usage: makeinboxpowerof2.cmd "inboxdirNoEndingBackslash" "outdirNoEndingBackslash" integerTargetExponent
@echo suggested value for integerTargetExponent is between 10 to 12 for an inbox of size 7MB.
@echo do a dir of your inbox, and copy+paste into ttcalc and use ttcalc to calculate the resulting size like this: 7,607,491*2^^10
@echo it is highly suggested you use double quotes around inboxdirNoEndingBackslash and outdirNoEndingBackslash.
@echo
goto end
:end

Comment 38

[Jim Michaels]

I ran debug version and it said MSVCR100d.dll was not found.

Comment 39

[Jim Michaels]

it seems to work just fine with 13GiB of emails on daily (that's all I tested it with, should I test under something else?). and it's very fast, faster than my utility by about 2x. I clock abourt 7 minutes to process a new 13GiB inbox. I also tested with one in the local folders. it loaded on demand and worked similarly.

when I had a 13GiB Inbox and InboxBIG in the main mailbox folder, the InboxBIG showed as a folder. 

scrollbar is a bit to low-res, kind of chunky. reduces scrollability to major chunks when you have 33k emails. it looks like you have a scroll resolution of about 100 I would guess. and the thumb is large too. sometimes those can be sized according to the number of emails I think by changing the max value of the vscrollbar. this would be a VERY good idea.

you MIGHT be able to set it to the number of emails (as one idea), but if you check the documentation and google user info on the scrollbar, you  may find that the email count is too large for the scrollbar so you may want to min() the email count with the maximum number the scrollbar can handle.  there may also be a data type difference....

Comment 40

[Jim Michaels]

there were 557056 emails in InboxBIG. it was originally an Inbox file of size 1MB ran through makeinboxpowerof2.cmd using a power of 13.

the equation you can use to size the inbox approximately is power=-((ln(inboxsize)+ln(desiredsize))/log(2))
you can calculate this in ttcalc or a decent scientific calculator.

Comment 41

[Jim Michaels]

there is a problem with the green progress bar indicator at bottom. it shows 100% in 5 seconds. I suspect it's still 32-bit OR there is just something very wrong with the way it's calculated. mind you, it takes minutes to finish the new inbox processing, and this process is disk-bound (I see the hard disk light staying on nearly 100% of time).

the spinner progress indicator at the top works just fine.

viewing emails works just fine and is snappy.

it takes 3 seconds to switch from inbox to local folders. during that time, the application shows as (Not Responding). I suppose this means that it is not processing window messages, which would usually cause something like that.

actually, it takes *2* clicks to switch. 1st click is ignored.

you can use 7-zip on these multiplied inboxes and they zip up very well. but I don't think anyone wants to share their data - I don't. mine bacsuee of a large compressed file 7z's up to about 5MB.

Comment 42

[Jim Michaels]

the email count in tb is correct. I am going to see if I can use a very small email and get that to a very small email and get the numbers up to something really big.

the smallest plain text email "test" in MBOX format would be 597 bytes, 601 bytes if you included the mail separator. so 601*2^32/2^30=2,404GiB to cross the 2^32 emails barrier. at that point,the program would become unbearably slow with current hard disk technology.
I don't think I will be able to test something like this with my current setup (2TB hard drives). it would take minimum 8 hours or 2 days (I forget which) just to process the emails. after that time I would need to reboot due to OS stability problems.
so I am testing with 2^25=32Mi emails using a simple 597-byte "test" email I sent myself. 
I made a new Daily profile to do this and removed the Inbox I got from POP3 "get emails" - I seem to be getting the same 3 "new" emails every time I start a new profile.

If you don't delete the .msf file, tb will NOT see your "replacement inbox" emails. I think the tb folks already know that.

tried to perform a compact. this did nothing, it did not even peg the disks. the folder is already compacted though. so maybe that's a moot point.

.msf file is 1% of inbox size. so if inbox is 20GB, msf file will be 200MB on average.
worse case for 2^25 qty of 601-byte emails,  msf file is then %? of Inbox size.
20,166,213,628 InboxBIG


I am seeing what is happing now, the green scrolling/moving progress bar is not a progress bar but is just a status indicator. and it pretty much halts during the "processing a new inbox" thing. also, the spinner stopped too. my platform is 32-bit windows xp pro with 3GB RAM (may have run out of memory - tb is using 1,714,752K right now and climbing). this is with the 20GB inbox with 2^25 emails.
I am pretty sure that with .msf files, IF all of that is held in memory before writing to disk, then there is an upper limit on the number of emails that you can calculate based on your RAM size.
well, I have given tb 33 minutes to process and it's at 50% cpu. I think it is hung in some sort of loop allocating more and more memory slowly in an out of memory state. ff has a similar creeping memory leak.
I am going to see where the limit is for 32-bit boxes.

I had violated the basic MBOX format by not separating with dashes with my batch file makeinboxpowerof2.cmd, so here is upgrade that fixes this. 
This gives a proper email count. and fixes the separation problem between inboxes in the batch file I made.
usage example: if batch file is in PATH and you are running from your inbox dir, and you have 1 test email,
makeinboxpowerof2 "." "." 24

-----makeinboxpowerof2.cmd
rem handle help switch
if /i @%1@ equ @@       goto help
if /i @%1@ equ @/help@  goto help
if /i @%1@ equ @/h@     goto help
if /i @%1@ equ @/?@     goto help
if /i @%1@ equ @-help@  goto help
if /i @%1@ equ @-h@     goto help
if /i @%1@ equ @-?@     goto help
if /i @%1@ equ @--help@ goto help
if /i @%1@ equ @--h@    goto help
if /i @%1@ equ @--?@    goto help
if @%2@==@@ goto err
if @%3@==@@ goto err
rem set targetexponent=12
set targetexponent=%3
rem set outdir=\t
set outdir=%2
rem if outdir surrounded with double quotes, strip them off, will add them later
if @^%outdir:~0,1%@ equ @^"@ (
    if @^%outdir:~-1%@ equ @^"@ set outdir=%outdir:~1,-1%
)
rem set indir=\Documents and Settings\Jim Michaels\Application Data\Thunderbird\Profiles\md4bzvt7.default\Mail\mail.renewalcomputerservices.com
set indir=%1
rem if indir surrounded with double quotes, strip them off, will add them later
if @^%indir:~0,1%@ equ @^"@ (
    if @^%indir:~-1%@ equ @^"@ set indir=%indir:~1,-1%
)
del "%outdir%\inboxBIG" "%outdir%\z" "%outdir%\z2"
echo -->"%outdir%\doubledash"
copy /b "%indir%\Inbox" /b "%outdir%\z2" /b
copy /b "%outdir%\z2" /b "%outdir%\InboxBIG" /b
set /a count+=1
set count=0
:lupe
if "%count%"=="%targetexponent%" goto exitlupe
copy /b "%outdir%\InboxBIG" /b + "%outdir%\doubledash" /b + "%outdir%\InboxBIG" /b "%outdir%\z" /b
move /y "%outdir%\z" "%outdir%\InboxBIG"
set /a count+=1
dir "%outdir%\InboxBIG"
goto lupe
:exitlupe
del "%outdir%\z"
dir "%outdir%\InboxBIG"
goto end
:err
@echo ERROR: there need to be 3 arguments. see --help
goto end
:help
@echo makeinboxpowerof2.cmd - take inbox and create outdir\InboxBIG the size of (the size of inbox)*2^^integerTargetExponent
@echo usage: makeinboxpowerof2.cmd "inboxdirNoEndingBackslash" "outdirNoEndingBackslash" integerTargetExponent
@echo suggested value for integerTargetExponent is between 10 to 12 for an inbox of size 7MB.
@echo do a dir of your inbox, and copy+paste into ttcalc and use ttcalc to calculate the resulting size like this: 7,607,491*2^^10
@echo it is highly suggested you use double quotes around inboxdirNoEndingBackslash and outdirNoEndingBackslash.
@echo it takes a few minutes because hard disk speed is 0.014GB/s with the copy command and usually you are doing multiple GB.
@echo
goto end
:end

Comment 43

[Jim Michaels]

on makeinboxpowerof2.cmd after :exitlupe change 
del "%outdir%\z"
to
del "%outdir%\z" "%outdir%\z2" "%outdir%\doubledash"

Comment 44

[[e-mail]]

(In reply to Jim Michaels from comment #38)
> I ran debug version and it said MSVCR100d.dll was not found.

To be able to run a debug build you need to install either VC Express 2010 or the appropriate parts of the Windows 7.1 SDK.

Comment 45

[:aceman]

Jim, please try to report if you see message corruption, TB operating on wrong message if you do some action (move/delete), compact failing and improper display of message count and folder size. That is what we try to fix here.
Please do not report slowness or UI weirdness, I can't help with that in this bug. Thanks.
The number of messages is limited to 2^31=2billion messages. The folder size of above 4GB can be displayed wrongly, if you do not use the build from comment 32 (I am not sure which dlls you need, see what Neil says). Testing on Daily will probably show you problems that we already know about.

Comment 46

[Jim Michaels]

I have been working off of comment 32's build.
10GB inbox of at 2^24 emails of size 597 bytes Virtual Memory keeps cycling resetting to 1702432K or 1702408K from 1749584K or 1765xxxK
it got to the point of displaying the standard windows titlebar with the icon in upper left hand corner and the title "Inbox - Daily", but it has not progressed beyond this.
it has progressed from norton reporting "High memory usage" to "high cpu usage", and it's at 50% so I think it's hung now. or, it just takes a REALLY LONG TIME to finish up with 16Mi emails.
at this point,it has created a 0-length .msf file, which hopefully will be filled in at some point. I am going to let this run overnight and turn off my internet to prevent new messages from coming in and see if this actually comes up.
8 hours ought to do something maybe if it's going to do anything.

on a 32-bit machine with 3GB RAM, and a boatload of 597-byte emails, the sheer quantity of emails only allows between 2^20-2^21 (1048576-2097152) emails, otherwise, you get the above behavior or locking up due to lack of memory.
responsiveness is good, usually within 17-minutes for larger files within 10GB for avg-sized emails with some including attachments.
see below:

03/27/2013  11:50 AM    <DIR>          .
03/27/2013  11:50 AM    <DIR>          ..
03/27/2013  11:30 AM       630,194,172 Inbox
03/27/2013  11:52 AM       356,604,262 Inbox.msf
03/26/2013  10:58 PM               597 InboxBIG
03/26/2013  10:28 PM                27 msgFilterRules.dat
03/26/2013  10:28 PM               246 popstate.dat
03/26/2013  10:27 PM               597 Sent
03/26/2013  10:27 PM                 0 Trash
03/27/2013  11:31 AM                 0 Trash.msf
               8 File(s)    986,799,901 bytes
			   
so in this instance with really small emails (inboxBIG was the original email) the .msf file size per email is about 356,604,262/630,194,172*597=337.8 bytes. this would be worst-case.

I also tried to select 196608/4=49,152 emails and I got a crash. this might again be due to memory limitations. there were 196608 emails in InboxBIG in this instance and this was the layout:
03/27/2013  02:01 PM    <DIR>          .
03/27/2013  02:01 PM    <DIR>          ..
03/27/2013  02:13 PM           150,184 Inbox
03/27/2013  02:14 PM             5,754 Inbox.msf
03/27/2013  12:50 PM     9,769,058,300 InboxBIG
03/27/2013  02:15 PM        53,443,935 InboxBIG.msf
03/26/2013  05:20 PM                27 msgFilterRules.dat
03/27/2013  02:10 PM               246 popstate.dat
03/23/2013  05:54 PM                 0 Trash
03/26/2013  09:33 PM                 0 Trash.msf
               8 File(s)  9,822,658,446 bytes
			   
I WAS going to delete those messages. so far, I have had no problems deleting messages, nor corruption. 
in fact,trying to delete about 300 emails can cause a crash. all i did was shift-pagedn about 15-25 times and then hit shift-delete and then it did some processing and bang.
I think I need to try a smaller mailbox size. so I tried half at 4*2^15 emails. this 32-bit platform just doesn't seem to be able to take large stuff.
before it crashes, I hear my machine physically make a downward sweeping quiet tweeting sound (not through the speaker!), and wattage goes up a little. 
it also does this after continuing the following dialog... 
I get a dialog box "Warning:Unresponsive script:chrome://messenger/content/selectionsummaries.js:218", so I continue 3 times.
and still it crashes selecting 2^15=32768 emails.
03/27/2013  02:39 PM           165,231 Inbox
03/27/2013  02:41 PM             6,163 Inbox.msf
03/27/2013  02:40 PM     4,921,360,380 InboxBIG
03/27/2013  02:42 PM        54,404,962 InboxBIG.msf
03/26/2013  05:20 PM                27 msgFilterRules.dat
03/27/2013  02:41 PM               215 popstate.dat
03/23/2013  05:54 PM                 0 Trash
               7 File(s)  4,975,936,978 bytes

these emails I used at the last were 4 HTML emails mostly in Inbox.
I found no corruption that I could tell (majority of the emails in my methods are duplicates). compact seems to work fine IF I have deleted or changed something. message count is accurate, even in folders.

Comment 47

[:aceman]

Thanks for the test Jim. There should be no crashes even on 32bit system. Can you elaborate more on that one? Maybe Tb got killed because it took too much memory?

To eliminate "Unresponsive script:chrome://messenger/content/selectionsummaries.js:218" try to hide the message pane (F8).

Comment 48

[:aceman]

Irving, this is the current order of patches to apply:
https://bugzilla.mozilla.org/attachment.cgi?id=726176
https://bugzilla.mozilla.org/attachment.cgi?id=728664
https://bugzilla.mozilla.org/attachment.cgi?id=725187
https://bugzilla.mozilla.org/attachment.cgi?id=726953
https://bugzilla.mozilla.org/attachment.cgi?id=728799
https://bugzilla.mozilla.org/attachment.cgi?id=728805

Comment 49

[Jim Michaels]

I am pretty sure that's precisely what happened - tb took too much memory, so it crashed. probably the way memory allocation is being handled when it can't get any is being handled wrong. pointer bug?

windows and background apps uses uses up about 1.3GB I would guess, leaving about 1.7GB for tb. somehow, even though an app says it uses 1.7GB it seems to actually use more RAM that it says it does in task manager...

I have very few metrics to go by, and perf counters are hard to set up, becausae they are slow to set up (takes up to 15 minutes, and you have to nail the process, so I just use XP's task manager).

right now my system says 1.4GB RAM available.

Comment 50

[:aceman]

I have also seen TB use much memory ~2GB when using these large >4GB folders. We can look at it in other bugs if this >4GB support is landed.

Comment 51

[:aceman]

Attachment: patch for nsMsgKey - mark nsMsgKey arguments v4

Fixes bitrot.

Comment 52

[:aceman]

Irving, this is the current order of patches to apply, after unbitrotting:
https://bugzilla.mozilla.org/attachment.cgi?id=726176
https://bugzilla.mozilla.org/attachment.cgi?id=728664
https://bugzilla.mozilla.org/attachment.cgi?id=731455
https://bugzilla.mozilla.org/attachment.cgi?id=726953
https://bugzilla.mozilla.org/attachment.cgi?id=731457
https://bugzilla.mozilla.org/attachment.cgi?id=728805

Comment 53

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

Comment on attachment 731457 [details] [diff] [review]
patch for nsMsgKey - mark nsMsgKey arguments v4

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

Thanks for doing this cleanup. I'd like to move the places where you change the default for the key from 0 to nsMsgKey_None out of this patch (and maybe clean up the error handling logic at the same time, wherever those changes end up).

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +2823,5 @@
>        return rv;
>      }
>      //Get key from row
>      mdbOid outOid;
> +    nsMsgKey key = nsMsgKey_None;

I'm concerned that changing the initializer from 0 to nsMsgKey_None changes semantics rather than just type declarations; that said, here and in the following examples I don't think the existing error handling is correct (we keep going and use key, even if the GetOid() call fails)

@@ +4378,4 @@
>        if (NS_SUCCEEDED(threadRow->GetOid(GetEnv(), &outOid)))
>          key = outOid.mOid_Id;
>        // find thread header for header whose message id we matched.
>        thread = GetThreadForThreadId(key);

Same thing - key changes from 0 to nsMsgKey_None, pre-existing logic looks wrong in any case.

@@ +4582,2 @@
>      if (NS_SUCCEEDED(hdrRow->GetOid(GetEnv(), &outOid)))
>        key = outOid.mOid_Id;

same

@@ +4654,2 @@
>      if (NS_SUCCEEDED(hdrRow->GetOid(GetEnv(), &outOid)))
>        key = outOid.mOid_Id;

same

::: mailnews/imap/src/nsImapUtils.cpp
@@ +246,5 @@
>  }
>  
>  // use the flagState to determine if the gaps in the msgUids correspond to gaps in the mailbox,
>  // in which case we can still use ranges. If flagState is null, we won't do this.
> +void AllocateImapUidString(nsMsgKey *msgUids, uint32_t &msgCount, 

Trailing white space

@@ +262,5 @@
>    
>    for (uint32_t keyIndex = 0; keyIndex < total; keyIndex++)
>    {
> +    nsMsgKey curKey = msgUids[keyIndex];
> +    nsMsgKey nextKey = (keyIndex + 1 < total) ? msgUids[keyIndex + 1] : 0xFFFFFFFF;

nsMsgKey_None in place of 0xFFFFFFFF here

Comment 54

[David :Bienvenu]

Comment on attachment 731457 [details] [diff] [review]
patch for nsMsgKey - mark nsMsgKey arguments v4

it all looks reasonable except for some of the imap stuff. Imap UIDs really are unsigned 32 bit ints, e.g.,

-void AllocateImapUidString(uint32_t *msgUids, uint32_t &msgCount, 
+void AllocateImapUidString(nsMsgKey *msgUids, uint32_t &msgCount, 
                            nsImapFlagAndUidState *flagState, nsCString &returnString)

and a few of the other changes in that same file are probably not right.

Comment 55

[:aceman]

(In reply to Irving Reid (:irving) from comment #53)
> ::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
> @@ +2823,5 @@
> >        return rv;
> >      }
> >      //Get key from row
> >      mdbOid outOid;
> > +    nsMsgKey key = nsMsgKey_None;
> 
> I'm concerned that changing the initializer from 0 to nsMsgKey_None changes
> semantics rather than just type declarations; that said, here and in the
> following examples I don't think the existing error handling is correct (we
> keep going and use key, even if the GetOid() call fails)
> 
> @@ +4378,4 @@
> >        if (NS_SUCCEEDED(threadRow->GetOid(GetEnv(), &outOid)))
> >          key = outOid.mOid_Id;
> >        // find thread header for header whose message id we matched.
> >        thread = GetThreadForThreadId(key);
> 
> Same thing - key changes from 0 to nsMsgKey_None, pre-existing logic looks
> wrong in any case.
> 

This is intentional. Using nsMsgKey_None as default initializer and then if we fail to get the real key from mOid makes us keep nsMsgKey_None and then if we use it some of the functions will assign a new proper key (or at least we will see an invalid key is being used). However using the initial 0 is bad I think, as 0 is a valid key, we may overwrite an existing message or we will never see later that the key was generated wrongly.

> @@ +262,5 @@
> >    
> >    for (uint32_t keyIndex = 0; keyIndex < total; keyIndex++)
> >    {
> > +    nsMsgKey curKey = msgUids[keyIndex];
> > +    nsMsgKey nextKey = (keyIndex + 1 < total) ? msgUids[keyIndex + 1] : 0xFFFFFFFF;
> 
> nsMsgKey_None in place of 0xFFFFFFFF here
I'm sure I changed this somewhere (maybe in the other patch). I'll check it out.

Comment 56

[:aceman]

Or we can just fail the function if GetOid fails so that the problem is immediately seen. Then the initializer value does not matter.

Comment 57

[ISHIKAWA, Chiaki]

(In reply to David :Bienvenu from comment #54)
> Comment on attachment 731457 [details] [diff] [review]
> patch for nsMsgKey - mark nsMsgKey arguments v4
> 
> it all looks reasonable except for some of the imap stuff. Imap UIDs really
> are unsigned 32 bit ints, e.g.,
> 
> -void AllocateImapUidString(uint32_t *msgUids, uint32_t &msgCount, 
> +void AllocateImapUidString(nsMsgKey *msgUids, uint32_t &msgCount, 
>                             nsImapFlagAndUidState *flagState, nsCString
> &returnString)
> 
> and a few of the other changes in that same file are probably not right.

Maybe :acman may want to define nsImapUID or some such data type and
use that instead of unit32_t. Just an idea.

Comment 58

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

(In reply to :aceman from comment #56)
> Or we can just fail the function if GetOid fails so that the problem is
> immediately seen. Then the initializer value does not matter.

I think that's best; looking into the implementations of GetOid(), they either never fail or they fail in strange ways that make me not trust the value set in outOid.

Comment 59

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

Comment on attachment 728805 [details] [diff] [review]
better separate offset/envelope_pos and msgKey

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

The bulk of this looks great to me; my only concern is with nsMsgDatabase::CreateNewKeyByOffset(). I'm going to defer to David :Bienvenu for final review of this change.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +785,5 @@
>      return rv;
>    }
>  
> +  nsMsgHdr *msgHdr = new nsMsgHdr(this, hdrRow, key);
> +  NS_ENSURE_TRUE(msgHdr, NS_ERROR_OUT_OF_MEMORY);

We get a C++ exception (and application crash) when new fails, so this check is not necessary

@@ +3352,5 @@
>    NS_ADDREF(*result = e);
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP nsMsgDatabase::CreateNewKeyByOffset(uint64_t aMsgOffset, nsMsgKey *newKey)

This doesn't feel good to me. I don't know this code nearly as well as David, so I'll defer to him for final decisions, but it seems wrong to me that we're going back into the database to find a message key that we've previously created corresponding to a particular message offset. I'd expect that we attach the key to the message the first time we create it, so we shouldn't need to look it up again.

::: mailnews/local/public/nsIMsgLocalMailFolder.idl
@@ +102,5 @@
>     */
>    [noscript] void getFolderScanState(in nsLocalFolderScanState aState);
>    [noscript] void getUidlFromFolder(in nsLocalFolderScanState aState, in nsIMsgDBHdr aMsgHdr);
>  
> +  boolean warnIfLocalFileTooBig(in nsIMsgWindow aWindow, in long long aSpaceRequested);

You could make this backwards compatible by making aSpaceRequested optional and default to zero.

Comment 60

[:aceman]

(In reply to :Irving Reid from comment #59)
> This doesn't feel good to me. I don't know this code nearly as well as
> David, so I'll defer to him for final decisions, but it seems wrong to me
> that we're going back into the database to find a message key that we've
> previously created corresponding to a particular message offset. I'd expect
> that we attach the key to the message the first time we create it, so we
> shouldn't need to look it up again.

I also would be happy without this, but I needed to specifically add it for the
GetNewMsgOutputStream and parseMailMessageState::FinalizeHeaders case. It just used the offset directly without storing the key (it assumed offset is the key). So maybe if we can rewrite that part, we can remove this hack. Without this hack we would create a new key for the same message in FinalizeHeaders.

Comment 61

[David :Bienvenu]

(In reply to :Irving Reid from comment #59)
it also seems wrong that the db should know about auto key numbering done by berkeley mailbox, when that's really a property of the msg store, not the generic db code.

Comment 62

[:aceman]

(In reply to :aceman from comment #60)
> (In reply to :Irving Reid from comment #59)
> > This doesn't feel good to me. I don't know this code nearly as well as
> > David, so I'll defer to him for final decisions, but it seems wrong to me
> > that we're going back into the database to find a message key that we've
> > previously created corresponding to a particular message offset. I'd expect
> > that we attach the key to the message the first time we create it, so we
> > shouldn't need to look it up again.
> 
> I also would be happy without this, but I needed to specifically add it for
> the
> GetNewMsgOutputStream and parseMailMessageState::FinalizeHeaders case. It
> just used the offset directly without storing the key (it assumed offset is
> the key). So maybe if we can rewrite that part, we can remove this hack.
> Without this hack we would create a new key for the same message in
> FinalizeHeaders.

If you see the original code in createNewHdr:
err = m_mdbStore->GetRow(GetEnv(), &allMsgHdrsTableOID, &hdrRow);
if (!hdrRow)
  err  = m_mdbStore->NewRowWithOid(GetEnv(), &allMsgHdrsTableOID, &hdrRow);

It looked if the key existed and used it if yes. If no, it created a new row with that key. That is where I took this approach. If you instrument those functions and run test_over2GBMailboxes.js you'll see that the test depends on this behaviour. I'd be glad to remove this and move CreateNewKey into the msg store as David suggests. I just am not sure how to pass the created key from GetNewMsgOutputStream to parseMailMessageState::FinalizeHeaders.

Comment 63

[:aceman]

(In reply to ISHIKAWA, chiaki from comment #57)
> Maybe :acman may want to define nsImapUID or some such data type and
> use that instead of unit32_t. Just an idea.

Imap UIDs were passed in as msg keys so I assumed they are semantically the same. But I can separate them, thanks for the suggestion.

Comment 64

[:aceman]

(In reply to David :Bienvenu from comment #61)
> (In reply to :Irving Reid from comment #59)
> it also seems wrong that the db should know about auto key numbering done by
> berkeley mailbox, when that's really a property of the msg store, not the
> generic db code.

That was the original implementation that I just preserved. So I can try to move the key generation to the store (but maybe then call CreateNewKey before all callers that just call CreateNewHdr or CopyHdrFromExistingHdr with a nsMsgkey_None). But I need  explicit confirmation for this:
- if we still need to preserve key = offset for any range of offsets. Or can we just start at the highest current key and increment by 1? Of course, accept any key that old existing databases return us (which were created as key=offset).
- what is the difference between envelope_pos and offset.

Comment 65

[:aceman]

Attachment: harden nsParseNewMailState::MoveIncorporatedMessage

OK, let's split the patches into more logical groups of changes. This makes nsParseNewMailState::MoveIncorporatedMessage more robust.

Not sure how to make the argument optional, as I can't make only the second argument optional in c++, when there are 3.

Comment 66

[:aceman]

Attachment: patch for hasSpaceAvailable to allow >4GB v3

This can now be simplified after the relevant part of bug 789679 is in.
(It is not to be reviewed).

Comment 67

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #66)
> patch for hasSpaceAvailable to allow >4GB v3

As you already know, JavaScript doesn't support 64bits value, because "double-precision 64-bit format IEEE 754" is used for number. Please be careful when enhance XPCOM Wrapped Object's property to 64bits.

Comment 68

[:aceman]

Yes, but we can't do anything better in C++. We must move variables to int64_t. Yes, we can then put the 53-bit limit into hasSpaceAvailable so that we do not cross the Javascript limit.
Anyway, this patch is only temporary and to be applied by those who want to test the other patches. Without it, the other patches have no useful effect as TB does not cross 4GB.

Comment 69

[Joshua Cranmer [:jcranmer]]

(In reply to :aceman from comment #68)
> Yes, but we can't do anything better in C++. We must move variables to
> int64_t. Yes, we can then put the 53-bit limit into hasSpaceAvailable so
> that we do not cross the Javascript limit.

Actually, you can squeeze out 54-bits from a double IIRC (there's an implied `1' at the beginning of the mantissa unless the exponent is 0). But 53 bits corresponds to an 8 PiB file. The supercomputer down the street has a total storage of 360 PB give or take a few dozen, with only 26.4 PB online. This kind of storage is unavailable except to extremely large enterprise/research-oriented databases, so I don't think there's a lot of danger in letting these numbers be doubles.

Comment 70

[WADA:World Anti-bad-Duping Agency]

If 64bits object property is error prone in JavaScript and is dangerous in some cases, "split to two 32bits" may be required.
Just an idea.
- in C++ code, use 64bits integer as size, offset, freely.
- when store/get to/from Object property, split to to 32bits.
  - Offset of mail data
    - left  32 bits == 4GB-block #
    - right 32 bits == offsets in the last 4GB block
  - Size : actual size == "left  32 bits" * 4GB + "right 32 bits"
- Or, provide wrapper function for JavaScript to get each 32 bits part
  of 64bits property data.
  GetUint64PropertyWithToken like one is a candidate.
- in JavaScript, issue error message if left 32 bits is too　large.
  I think "Javascript limit" is better processed by JavaScript code
  himself, instead of by C++ code side.

Comment 71

[:aceman]

As said, 53bits is enough for the near future. Who would want to loose 8 PiB of mbox file in one hit :) For such mailbox sizes hopefully maildir will be ready by then.

So if we lift the limit of 4GB in hasSpaceAvailable, we can move the limit to 8PiB (53bits). Then no special handling of >53bit will be needed as the mbox never grows to such big sizes.

Comment 72

[Jim Michaels]

Comment on attachment 751853 [details] [diff] [review]
harden nsParseNewMailState::MoveIncorporatedMessage

are you familiar with the c++/c thing called ... ?
http://www.cprogramming.com/tutorial/lesson17.html
make them all variable and in your function simply make one of the arguments required by making the arg count >= 2 etc. treat this like argv in main as if you were processing commandline switches in a program, without the switches. just force one of them to be an argument - it's doable.

Comment 73

[:aceman]

We can't use va_list here. The function arguments are auto-generated from the idl description. At most we can set their default values (as in int func(a, b=0, c=1) ).

Comment 74

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

Comment on attachment 751853 [details] [diff] [review]
harden nsParseNewMailState::MoveIncorporatedMessage

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

As long as removing the null check is safe, r+ after you add the comment I ask for.

::: mailnews/local/public/nsIMsgLocalMailFolder.idl
@@ +103,5 @@
>    [noscript] void getFolderScanState(in nsLocalFolderScanState aState);
>    [noscript] void getUidlFromFolder(in nsLocalFolderScanState aState, in nsIMsgDBHdr aMsgHdr);
>  
> +  boolean warnIfLocalFileTooBig(in nsIMsgWindow aWindow,
> +                                [optional] in long long aSpaceRequested);

I know it was like this when you found it, but can you please add a usage comment on this method?

::: mailnews/local/src/nsParseMailbox.cpp
@@ -2493,5 @@
>    }
>  
> -  nsCOMPtr <nsIMsgLocalMailFolder> destLocalFolder = do_QueryInterface(destIFolder);
> -  if (destLocalFolder)
> -  {

Is it really OK to remove the null check on the result of the QueryInterface call here?

Comment 75

[:aceman]

Attachment: harden nsParseNewMailState::MoveIncorporatedMessage v2 [checked in]

OK, I put the if (localFolder) back as many other places in the file use it too.

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/064f4b77d8cd

Comment 77

[:aceman]

Sorry, this is far from finished, forgot to mark the whiteboard.
The patches are not obsolete yet (but will become when I create new split versions).

And I still need info from David Bienvenu to comment 60.

Comment 78

[:aceman]

But we should have reliable test runs on the servers before we push these dangerous changes.

Comment 79

[:aceman]

Attachment: mark nsMsgKey arguments v5

So this splits out the changes of uint32_t to nsMsgKey where appropriate (I removed the IMAP changes). This also changes some 0xffffffff and PR_UINT32_MAX to nsMsgKey_None. This should not change any behaviour.

Comment 80

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

Comment on attachment 790432 [details] [diff] [review]
mark nsMsgKey arguments v5

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

Man, the sheer number of empty implementations of SetMessageKey() make me think it's not defined on the right interface... The rest looks fine, but I'd like to be sure we're OK on the URL builder and parser before r+.

::: mailnews/base/src/nsMsgFolderCompactor.cpp
@@ +90,4 @@
>  {
>    uri.Append(baseURI);
>    uri.Append('#');
>    uri.AppendInt(key);

Make sure the right coercion happens here, and when we parse the message URI to get the key back out.

::: mailnews/compose/src/nsMsgCopy.h
@@ +44,1 @@
>    

Might as well remove the trailing white space on the next line

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +7839,5 @@
>    return NS_ERROR_NOT_IMPLEMENTED;
>  }
>  
>  /* void SetMessageKey (in uint32_t aKey); */
> +NS_IMETHODIMP nsImapFolderCopyState::SetMessageKey(nsMsgKey aKey)

Remove the comment (or fix it)

::: mailnews/local/src/nsLocalUtils.cpp
@@ +204,5 @@
>                           keyEndSeparator - (keySeparator + 1));
>      else
>        keyStr = StringTail(uriStr, uriStr.Length() - (keySeparator + 1));
>  
> +    *key = (nsMsgKey) ParseUint64Str(keyStr.get());

... Here's the other side of my earlier review comment; we're appending it as "Int" but parsing as Uint64 (and then coercing the result down to uint32 in *key). Is this going to be OK for keys > 2^31, if we ever make any? How about for nsMsgKey_none, does it come through as uri ...#-1 or ...#(2^32-1 in base 10)?

::: mailnews/local/src/nsLocalUtils.h
@@ +20,3 @@
>  
>  nsresult 
> +nsBuildLocalMessageURI(const char* baseURI, nsMsgKey key, nsCString& uri);

Might as well take off the trailing white on the line above

::: mailnews/news/public/nsINNTPArticleList.idl
@@ +10,5 @@
>  
>  [scriptable, uuid(921AC214-96B5-11d2-B7EB-00805F05FFA5)]
>  interface nsINNTPArticleList : nsISupports {
>      void initialize(in nsIMsgNewsFolder newsFolder); 
> +    void addArticleKey(in nsMsgKey key);

Trailing whitespace on preceding line

Comment 81

[JD]

Please add CC: [e-mail]

Comment 82

[David :Bienvenu]

I still feel that the mailbox store, not the nsMsgDatabase, should be determining the msg key.

Comment 83

[:aceman]

Thanks David, we are in agreement about that. But what do you think about comment 60 and 64 ?

Comment 84

[:aceman]

(In reply to :Irving Reid from comment #80)
> Comment on attachment 790432 [details] [diff] [review]
> mark nsMsgKey arguments v5
> 
> Review of attachment 790432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Man, the sheer number of empty implementations of SetMessageKey() make me
> think it's not defined on the right interface... The rest looks fine, but
> I'd like to be sure we're OK on the URL builder and parser before r+.
> 
> ::: mailnews/base/src/nsMsgFolderCompactor.cpp
> @@ +90,4 @@
> >  {
> >    uri.Append(baseURI);
> >    uri.Append('#');
> >    uri.AppendInt(key);
> 
> Make sure the right coercion happens here, and when we parse the message URI
> to get the key back out.

> ::: mailnews/local/src/nsLocalUtils.cpp
> @@ +204,5 @@
> >                           keyEndSeparator - (keySeparator + 1));
> >      else
> >        keyStr = StringTail(uriStr, uriStr.Length() - (keySeparator + 1));
> >  
> > +    *key = (nsMsgKey) ParseUint64Str(keyStr.get());
> 
> ... Here's the other side of my earlier review comment; we're appending it
> as "Int" but parsing as Uint64 (and then coercing the result down to uint32
> in *key). Is this going to be OK for keys > 2^31, if we ever make any? How
> about for nsMsgKey_none, does it come through as uri ...#-1 or ...#(2^32-1
> in base 10)?
nsMsgKey_None is encoded as 4294967295 in nsMsgFolderCompactor.cpp above. And the reverse (via ParseUint64Str seems to work too.

Anyway, I do not change the type of nsMsgKey, it is still uint32_t. So if there are any bugs, I do not cause them.

Comment 85

[:aceman]

Attachment: mark nsMsgKey arguments v5.1

I refreshed the patch on top of bug 813459 and bug 789679, but it is not sure they overlap.

Comment 86

[:aceman]

Attachment: fail if keys not found in msgDatabase

This should address findings in comment 53 and up.

Comment 87

[Kent James (:rkent)]

I'd like to weigh in on these discussions since I seem to be getting involved in reviews.

In answer to comment 64 "need to preserve key = offset for any range of offsets? Or can we just start at the highest current key and increment by 1?" The issue here is what happens when someone with an older TB tries a newer TB, and then tries to use the same profile on an older TB. The older profile would assume that offset = key, and try to access the mbox message according to an incorrect offset, getting garbage.

Pluggable stores landed in Thunderbird 12, so I believe (but have not tested) that Thunderbird 12 or later will properly handle a message where the offset is stored, and the key does not equal the offset. Interoperation of TH 17, 24, 31, and 38 to me is sufficient (in fact I would be willing to drop TB 17). There is no need to preserve forward-to-backward compatibility between TB 38 and TB 3 or 10. That might be worth a release note, however. So messages added in TB 38 would not be readable in TB 3, but messages from TB 3 would be readable in TB 38.

Conclusion: just increment the key by 1 with no further correlation with offset.

So I don't really understand the logic behind "the mailbox store, not the nsMsgDatabase, should be determining the msg key." in comment 82. We should be moving away from message keys that have significance outside of the database. It is this conflation of meaning (key is sometimes offset, sometimes imap UID, sometimes insignificant) that makes it so darn hard to change anything in mailnews code without causing a regression someplace else. It is the database that knows the next available key, not the store.

The key is simply an integer that lets you access a particular message in the nsIMsgDBHdr. Anything needed to relate that message to the msgStore (storeToken) or server (UID) is stored in the database. Let the database generate keys, simply incrementing from the last available key (which mork will do passing in unknown key).

Unfortunately for legacy reasons, we need to support older significant keys. Those legacy conversions largely exist already (offset = key if no offset, store = offset if no store) but we should not be adding new keys that use those values.

Comment 88

[Kent James (:rkent)]

Comment on attachment 8516983 [details] [diff] [review]
mark nsMsgKey arguments v5.1

I'm seeing the following error on a try server build https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=56be541c2e3a

Log

TB Linux x86-64 try-comm-central leak test build on 2014-11-11 13:20:29 PST for push 56be541c2e3a

/builds/slave/tb-try-c-cen-l64-d-00000000000/build/mailnews/base/util/nsMsgUtils.cpp:2453:11: error: redefinition of 'nsMsgKey msgKeyFromInt(uint64_t)'
/builds/slave/tb-try-c-cen-l64-d-00000000000/build/mailnews/base/util/nsMsgUtils.cpp:2448:11: error: 'nsMsgKey msgKeyFromInt(long unsigned int)' previously defined here

Comment 89

[:aceman]

That is kinda strange as it worked fine for me on gcc 4.8.3 (seems try server is on 4.7).
Anyway I created those wrapped functions to indicate we should not assume the key is an integer that can be handled as a number. Currently it works but should not be expected. So I had those 3 functions do the needed conversion from int to key (currently it is mostly a noop). But imagine nsMsgKey would be an object (I used that for debugging in the past).
So I depends if anybody thinks this is a good idea.

Comment 90

[:aceman]

(In reply to Kent James (:rkent) from comment #87)
> I'd like to weigh in on these discussions since I seem to be getting
> involved in reviews.
> 
> In answer to comment 64 "need to preserve key = offset for any range of
> offsets? Or can we just start at the highest current key and increment by
> 1?" The issue here is what happens when someone with an older TB tries a
> newer TB, and then tries to use the same profile on an older TB. The older
> profile would assume that offset = key, and try to access the mbox message
> according to an incorrect offset, getting garbage.
> 
> Pluggable stores landed in Thunderbird 12, so I believe (but have not
> tested) that Thunderbird 12 or later will properly handle a message where
> the offset is stored, and the key does not equal the offset. Interoperation
> of TH 17, 24, 31, and 38 to me is sufficient (in fact I would be willing to
> drop TB 17). There is no need to preserve forward-to-backward compatibility
> between TB 38 and TB 3 or 10. That might be worth a release note, however.
> So messages added in TB 38 would not be readable in TB 3, but messages from
> TB 3 would be readable in TB 38.
> 
> Conclusion: just increment the key by 1 with no further correlation with
> offset.
This analysis seems correct to me.

> So I don't really understand the logic behind "the mailbox store, not the
> nsMsgDatabase, should be determining the msg key." in comment 82. We should
> be moving away from message keys that have significance outside of the
> database. It is this conflation of meaning (key is sometimes offset,
> sometimes imap UID, sometimes insignificant) that makes it so darn hard to
> change anything in mailnews code without causing a regression someplace
> else. It is the database that knows the next available key, not the store.
There is probably the option that each store may want to create keys in different ways.
Then it stores the key+storeToken in the database. The DB does not need to know how the key was created.

> The key is simply an integer that lets you access a particular message in
> the nsIMsgDBHdr. Anything needed to relate that message to the msgStore
> (storeToken) or server (UID) is stored in the database. Let the database
> generate keys, simply incrementing from the last available key (which mork
> will do passing in unknown key).
I am not sure about the IMAP keys (UIDs?) but maybe they have different schema. So then we would have 1 key from db (the autogenerated one), the IMAP UID (a new field) and storeToken (pointer to the message body on the store). 3 fields instead of current 2.

Do I understand it corrently, that for mbox the storeToken is currently the offset into the file?

Comment 91

[:aceman]

Attachment: mark nsMsgKey arguments v5.2 [checked in - comment 99]

Thanks to jcranmer for hints how to fix this and remove the conflicting function.

Comment 92

[Kent James (:rkent)]

(In reply to :aceman from comment #90)
> 
> > So I don't really understand the logic behind "the mailbox store, not the
> > nsMsgDatabase, should be determining the msg key." in comment 82. We should
> > be moving away from message keys that have significance outside of the
> > database. It is this conflation of meaning (key is sometimes offset,
> > sometimes imap UID, sometimes insignificant) that makes it so darn hard to
> > change anything in mailnews code without causing a regression someplace
> > else. It is the database that knows the next available key, not the store.
> There is probably the option that each store may want to create keys in
> different ways.
> Then it stores the key+storeToken in the database. The DB does not need to
> know how the key was created.
>

After our IRC discussions, let's clearly separate the concept of store ID (offset in mbox, filename in maildir) from the concept of server ID (IMAP UID, EWS item ID) and message key (uint32_t as MsgKey in mork). The local folder is made more complicated because the store is also the server.

In general, we do not want to conflate the meanings of database ID, store ID, and server ID. For practical reasons though, we don't see a way to eliminate the conflation of server ID and db ID in the Thunderbird 38 cycle, at least for IMAP and News. For Local, the work is mostly done to eliminate that conflation, and maildir forces us anyway since its store ID is not representable as a uint32_t key.

So for now, we need a way for new db entries to be created for specific messages with a specific value of the key. But that key will come from server information, not from store information. There is no need for us to get any information from the store to define a key. The decision of what key to use is made at the folder level.

From the db perspective, new entries can be made using either nsMsgKey_None (which will auto assign the key by incrementing from the last key), or my specifying a specific key. We should try to move to nsMskKey_None in the long run to reduce the conflation and its inherent complexity. For the specific case of Local Folders, we should be able to use entirely auto-assigned keys now.

So I keep hearing things like "each store may want to create keys in different ways" that I object to. I agree with "each folder may want to create keys in different ways"

> > The key is simply an integer that lets you access a particular message in
> > the nsIMsgDBHdr. Anything needed to relate that message to the msgStore
> > (storeToken) or server (UID) is stored in the database. Let the database
> > generate keys, simply incrementing from the last available key (which mork
> > will do passing in unknown key).
> I am not sure about the IMAP keys (UIDs?) but maybe they have different
> schema. So then we would have 1 key from db (the autogenerated one), the
> IMAP UID (a new field) and storeToken (pointer to the message body on the
> store). 3 fields instead of current 2.

I'll concede that in the short run we need to conflate IMAP UID with db key. But three keys instead of two represents the actual situation, and attempts to combine these into two in various ways are what is causing this to be so hard. Let's move away from that.

> 
> Do I understand it corrently, that for mbox the storeToken is currently the
> offset into the file?

Yes that is correct. See http://hg.mozilla.org/comm-central/annotate/c3c1bc2ecb51/mailnews/local/src/nsMsgBrkMBoxStore.cpp#l682 The integer offset is set, as well as storeToken as a string, to the offset value. Though in many cases key=offset=storeToken in mbox, these are all different in maildir, so do not confuse the three outside of the store code. storeToken accesses a particular message in the store, key access a particular message in the db, and offset is used to read the raw message from a stream.

Comment 93

[Kent James (:rkent)]

Comment on attachment 8521016 [details] [diff] [review]
mark nsMsgKey arguments v5.2 [checked in - comment 99]

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

OK looks good. Thanks! (Can't land though until upstream patch is landed)

Comment 94

[:aceman]

Blocking patch was landed.

Comment 95

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

Comment on attachment 8517026 [details] [diff] [review]
fail if keys not found in msgDatabase

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

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +2910,5 @@
>      //Get key from row
>      mdbOid outOid;
> +    nsMsgKey key = nsMsgKey_None;
> +    rv = hdrRow->GetOid(mDB->GetEnv(), &outOid);
> +    if (NS_FAILED(rv))

NS_WARN_IF(NS_FAILED(rv)) so that we get a warning in debug builds, unless it's normal for GetOid() to return NS_FAILED status values.

@@ +2917,5 @@
>  
>      rv = mDB->GetHdrFromUseCache(key, getter_AddRefs(mResultHdr));
>      if (NS_SUCCEEDED(rv) && mResultHdr)
>        hdrRow->Release();
> +    else {

Re-order the logic here to:

if (NS_WARN_IF(NS_FAILED(rv)) {
  return rv;
}
if (mResultHdr) {
 ...
} else {
 ...
}

Again, if it's normal for GetHdrFromUseCache to fail, don't use NS_WARN_IF

@@ +4599,4 @@
>      rv = GetHdrFromUseCache(key, &msgHdr);
>      if (NS_SUCCEEDED(rv) && msgHdr)
>        hdrRow->Release();
> +    else {

Same logic change as above - check NS_FAILED first

@@ +4640,5 @@
>      nsMsgKey key = outOid.mOid_Id;
>      rv = GetHdrFromUseCache(key, &msgHdr);
>      if ((NS_SUCCEEDED(rv) && msgHdr))
>        hdrRow->Release();
> +    else {

Again, check NS_FAILED first

@@ +4684,2 @@
>        rv = CreateMsgHdr(hdrRow, key, &msgHdr);
> +      if (NS_FAILED(rv))

And again

Comment 96

[:aceman]

(In reply to :Irving Reid (No longer working on Firefox) from comment #95)
> @@ +2917,5 @@
> >  
> >      rv = mDB->GetHdrFromUseCache(key, getter_AddRefs(mResultHdr));
> >      if (NS_SUCCEEDED(rv) && mResultHdr)
> >        hdrRow->Release();
> > +    else {
> 
> Re-order the logic here to:
> 
> if (NS_WARN_IF(NS_FAILED(rv)) {
>   return rv;
> }
> if (mResultHdr) {
>  ...
> } else {
>  ...
> }
> 
> Again, if it's normal for GetHdrFromUseCache to fail, don't use NS_WARN_IF

I don't understand this. If it fails, we fallback to CreateMsgHdr(). Why do you propose existing the function instead? I only push the check inside the else block because it seems to me checking rv unconditionally (previous state) s redundant as we already checked for NS_SUCCEEDED(rv).

Comment 97

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

Comment on attachment 8517026 [details] [diff] [review]
fail if keys not found in msgDatabase

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

(In reply to :aceman from comment #96)
> (In reply to :Irving Reid (No longer working on Firefox) from comment #95)
> > @@ +2917,5 @@
> > Re-order the logic here to:
> > 
> > if (NS_WARN_IF(NS_FAILED(rv)) {
> >   return rv;
> > }
> > if (mResultHdr) {
> >  ...
> > } else {
> >  ...
> > }
> > 
> > Again, if it's normal for GetHdrFromUseCache to fail, don't use NS_WARN_IF
> 
> I don't understand this. If it fails, we fallback to CreateMsgHdr(). Why do
> you propose existing the function instead? I only push the check inside the
> else block because it seems to me checking rv unconditionally (previous
> state) s redundant as we already checked for NS_SUCCEEDED(rv).

Ah, you're right. Sorry about that.

Comment 98

[:aceman]

Attachment: fail if keys not found in msgDatabase v1.1 [checked in - comment 99]

OK, added the NS_WARN_IF()s where useful. Carrying over r=irving. xpcshell tests pass for me locally.

Comment 99

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/34afccc6f8f7
https://hg.mozilla.org/comm-central/rev/37bc8ee327c6

Comment 100

[:aceman]

Unfortunatelly this is not done. The main patch is to be uploaded yet (it is in the obsolete ones, but needs to be refreshed).

Comment 101

[Kent James (:rkent)]

Why is the "The main patch is to be uploaded yet (it is in the obsolete ones ... "  Can you unobsolete the main patch? I can't figure out the status of this now.

Comment 102

[:aceman]

(In reply to Kent James (:rkent) from comment #101)
> Why is the "The main patch is to be uploaded yet (it is in the obsolete ones
> ... "  Can you unobsolete the main patch? I can't figure out the status of
> this now.

Yeah, sorry about that. I can upload the patch, but it is incomplete yet and fails many tests. I have refreshed it, but need to solve the problem of comment 60. The passing of the key between GetNewMsgOutputStream and parseMailMessageState::FinalizeHeaders. That happened automatically and silently till now due to using the offset (as the key) that has the same value in both functions (when they were called separately from outside). If we now remove any significance from the offset, we need to pass the msgkey (or msghdr). I am working on solving that, I just was stuck due to xpcshell tests not working for some weeks now (bug 1117543).

Comment 103

[:aceman]

Attachment: WIP main patch

This is the main patch, not fully working.

Comment 104

[Kent James (:rkent)]

See Bug 1122346 - Check if draft message is in db before deleting

Comment 105

[:aceman]

rkent, do you think after bug 1122346 is solved that this patch is ready for review? Should I clean it up?

Comment 106

[Kent James (:rkent)]

(In reply to :aceman from comment #105)
> rkent, do you think after bug 1122346 is solved that this patch is ready for
> review? Should I clean it up?

I did not see any glaring errors the last time I looked at it, so yes I think you should do whatever cleanup you think is appropriate, and let's try to get it reviewed.

Comment 107

[:aceman]

Attachment: WIP main patch v2

This should reduce the number of test failures. But I get inconsistent results in local runs. So Aryx, please push to try server, all tests.

Comment 108

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

Pushed as https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b564b885ec81

Comment 109

[:aceman]

The test failures:
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_index_messages_imap_online.js | test_message_moving - [test_message_moving : 1123] 3 == 4

The test expects a specific key to be produced. I need to find out if it is right to do so. As in IMAP keys are not arbitrary as in POP3, but represent the UIDs on the server, even though in cases of local copies there are some "fake keys" being used. I know nothing about that so need to look at that more.

TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapUndo.js | verifyFolders - [verifyFolders : 103] 5 == 4

The test expects LESS messages to be created than happen. Either the logic of the test fails somewhere sooner, or I now really create more messages, e.g. due to no longer finding out a message worked on in different parts of code is the same via the offset. Similar to what I noticed around comment 62 in POP3 case. So this again needs further analysing.

The mozmill failures may also be caused by this patch, but I think we should first fix up the xpcshell ones.

Comment 110

[Kent James (:rkent)]

AFAICT this bug is the last remaining significant blocker to >4GB mbox folders. Let's try to get this landed in the next three weeks before we freeze interfaces for Thunderbird 38. Or will there still be more needed?

Comment 111

[:aceman]

For mbox, nothing else is known to be needed. But this particular bug still needs to fix some IMAP cases (as seen in the try run in comment 108).

But I think this is a very risky change with high dataloss potential. I would not like it to be rushed at this stage, then get low testing and hotfixing it in release. Even if the test failures are solved I still do not feel the patch is ready. I do not yet understand why in this patch I do not see problem of comment 60 and up (maybe the bug is fixed, or just hidden). If possible, I would like to work on it continually in the next cycles, but not push it to 38.

But this bug is for mbox only. I think you could still do >4GB for maildir in bug 1078367 if you want. I assume in maildir you never get offset or envelopePos > 4GB so you can happily do with current 32bit values (where still used).

Comment 112

[Kent James (:rkent)]

So not in Thunderbird 38 :(

Comment 113

[:aceman]

I think I should split the key generation affecting parts of this into bug 1144020. Then the less problematic changes (upping envelope to 64bit) can hopefully pass here.

Comment 114

[:aceman]

Attachment: patch v3

Looks like the key/offset separation is solved in bug 1183490 so I could reduce this to the 64bit envelope problem.

Comment 115

[Kent James (:rkent)]

So close ... can we try to get this in tb45?

Comment 116

[:aceman]

Rkent, you can review it instead of Neil to speed it up.

Comment 117

[Kent James (:rkent)]

OK I'll set myself as a reviewer.

Note though that the upcoming Dec 14 deadline is for strings, and not necessarily for interfaces. We can do interface changes for at least several more weeks.

Comment 118

[Kent James (:rkent)]

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

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

As you say in comment 113, there's not much left here, just some upgrades to 64 bit of types. LGTM.

Comment 119

[:aceman]

Attachment: patch v3.1 [checked in - comment 120]

There was some bitrot from the other bug so this is the version that will be checked in.

Comment 120

[:aceman]

https://hg.mozilla.org/comm-central/rev/9ca01faee9dd