User Agent: Mozilla/5.0 (X11; Linux i686; rv:9.0.1) Gecko/20100101 Firefox/9.0.1 (Beta/Release) Build ID: 20111222033305 Steps to reproduce: This is a repeat of bug Bug 236218 since we can no longer change the status of Bug 236218 from "expired". Existing in inbox full is at approximately 2GB in size. Try fetch email. Actual results: Error message "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." This message is somewhat confusing because you may have lots of hard drive space on your computer, but error message indicates you do not have enough space. Expected results: Thunderbird should be able to have inbox larger than 2GB in size, especially if the file system can handle files larger than 2GB in size. or, the error message should also suggest to keep your "inbox" much smaller than 2GB. My preferred suggestion is to allow Thunderbirds inbox to be able to grow much larger than 2GB in size. This is a repeat of Bug 236218

In bug 236218 you talk about a folder of 4GB. So how big is it really? There should be no problem at 2GB so please check it. But regardless of free disk space, TB does not support single folders larger than 4GB. That is being solved in bug 789679 (or in other bug where we finish maildir (one file per message, not file per folder)). Until then, you need to split the folder as you describe in bug 236218. But you are right, the message is misleading, saying "not enough disk space" even if it just hit the mailbox size limit. I can fix that.

This would be a nice polish for TB31 but I would probably need to change the signature of the nsIMsgPluggableStore.idl::hasSpaceAvailable function, so that it distinguishes between the 2 reasons of returning false: full disk OR 4GB reached. Instead of returning single boolean. Or something like that. Can you guys please check if any extension is calling this function? Or do you have some other clever idea?

There are no hits to "hasSpaceAvailable" in https://mxr.mozilla.org/addons

Hi Aceman, you are correct, 4GB, not 2GB. I looked at the earlier bug report. I've fixed a lot of code working with signed/unsigned 32bit, so I defaulted to 2GB :-) > so that it distinguishes between the 2 reasons of returning false: > full disk OR 4GB reached. Instead of returning single boolean. > Or something like that, I would recommend thinking further ahead, and instead of simply using boolean, return 0 for good, -1 for undefined error, and a negative number for the other errors, such as -2=full disk, -3=4GB reached, -4=unwriteable(ex:no_write_permissions), -6=media_missing (eg:android uses pluggable SD cards for extra memory)..... Functions such as fflush(), fclose(), fopen(), fprintf() have good ideas for reporting errors. A value has always had much more value than a boolean IMHO. > This would be a nice polish for TB31 but I would probably need to > change the signature of the nsIMsgPluggableStore.idl::hasSpaceAvailable function In some of the projects I worked on (libspiro 20130930, FontForge 20140101), they do an interesting thing which you can do too... int hasSpaceAvailable0() { do stuff; return( errorcode ); } boolean hasSpaceAvailable() { if ( hasSpaceAvailable0()<0 ) return( false_noMoreSpaceError); else return( good_youHaveSpaceAvailable ); } ...this way, functions that call the "old" hasSpaceAvailable() still work like before, while new functions that call hasSpaceAvailable0() are able to access 0=good or any error<0. If you want an example, look at libspiro's spiroentrypoints.h where you see the two extra enhanced functions. See here: https://github.com/fontforge/libspiro/blob/master/spiroentrypoints.h

(In reply to Jose Da Silva from comment #4) > int hasSpaceAvailable0() { > do stuff; > return( errorcode ); > } > > boolean hasSpaceAvailable() { > if ( hasSpaceAvailable0()<0 ) > return( false_noMoreSpaceError); > else > return( good_youHaveSpaceAvailable ); > } Yes, that is also a solution, but that means I need to change internal code to call hasSpaceAvailable0() and have this one unnecessary function. But yes, we can have different (ugly) code for TB31 and then do it right for TB32/trunk.

If it helps, you can think of it like this, which is, it's not a huge CPU time hit, and the resulting increase is not that big in terms of compiled code... You can convert code that you have control over, to use the better function, but for code you don't have control over, such as external plugins, you won't be breaking something you don't know about, and as we both know, Users will never look at code, but are the biggest source of complaints of how program X or Y is broken, but have almost no way of explaining what's broke, other than using words like "doesn't work" ;-P and Yes, it is ugly code, but sometimes we don't get it right the first time, or we find a better way to do something we did another way, so, yes, it is a kludge, but we don't break someone else's code. :-( In my hurry to give you this idea, I forgot to comment the old code so that developers that actually take time to look at code see the warning to please use the better function instead. Here is the routine a bit further optimized... boolean hasSpaceAvailable() { //Deprecated function: Please use hasSpaceAvailable0() return ( hasSpaceAvailable0() ? false_noMoreSpaceError : good_youHaveSpaceAvailable ); } as wordy and complicated as the above code looks like, an optimizing compiler will output something in the size of just a few CPU instructions if all you really need is to output 0 and +1. hasSpaceAvailable: call hasSpaceAvailable0 test ax,ax ;test ax and set CPU flags (good=0) mov ax,true ;ax=1, have space jz skip0 clr ax ;ax=0, no space skip0 return hope this suggestion helps. Anyhow, regardless of which choice you choose, I'm glad this bug report helps you fix/improve a function.

So I try to workaround this without changing of strings or interfaces. I make a better fix for trunk in a new bug, also encompassing fixing nsMsgLocalMailFolder::WarnIfLocalFileTooBig. We have 2 strings mailboxTooLarge and pop3OutOfDiskSpace in different .properties files and I want to use both at both places (in pop3Protocol and in nsMsgLocalMailFolder). I know it is ugly, but the whole code block will go away in the next version.

Comment on attachment 8431731 [details] [diff] [review] patch Review of attachment 8431731 [details] [diff] [review]: ----------------------------------------------------------------- Don't forget to file a follow-up bug to implement a cleaner fix after 31. ::: mailnews/local/src/nsPop3Protocol.cpp @@ +2942,4 @@ > > bool spaceAvailable; > // check if we have a reasonable amount of space left > rv = msgStore->HasSpaceAvailable(folder, m_totalDownloadSize, &spaceAvailable); Rather than re-open the file to check the size, change HasSpaceAvailable to fail with rv=NS_ERROR_FILE_TOO_BIG and check for that.

The return value is expected to be in the boolean value returned. Won't returning such an error in rv mean an exception in case somebody uses the function from JS? So yes, this would probably work without needing to change any interface and UUID, but it silently changes the API. So we decide it is fine because there were no callers found in extensions?

Comment on attachment 8431731 [details] [diff] [review] patch Review of attachment 8431731 [details] [diff] [review]: ----------------------------------------------------------------- Good point about JS callers experiencing thrown exceptions. Since there don't seem to be any callers outside our test suite, I think it's safe to change the error code from HasSpaceAvailable. If you're more comfortable with duplicating the size check as you've done in your patch, you can have an r+ on that.

I do not like the duplication as I have done in the patch. I will gladly do your version if it gets r+.

I like this one better but it causes the exception for hypothetical JS callers. But if you accept it, I am for this version to go in.

Comment on attachment 8431731 [details] [diff] [review] patch Not only here, but in CheckIfSpaceForCopy and WarnIfLocalFileTooBig, we have a problem with the API not matching the caller. As a workaround, I'd probably make this code use WarnIfLocalFileTooBig and then on trunk we can fix WarnIfLocalFileTooBig to do the right thing, which I suspect is to change bool hasSpaceAvailable(...) to void checkSpaceAvailable(..,) which throws either NS_ERROR_FILE_TOO_BIG or NS_ERROR_FILE_DISK_FULL. I don't like the patch v2 at all, by the way.

(In reply to neil@parkwaycc.co.uk from comment #13) > then on trunk we can fix WarnIfLocalFileTooBig to do the right thing, which > I suspect is to change bool hasSpaceAvailable(...) to void > checkSpaceAvailable(..,) which throws either NS_ERROR_FILE_TOO_BIG or > NS_ERROR_FILE_DISK_FULL. Probably not throw, but return those values in an out argument. > I don't like the patch v2 at all, by the way. Why?

(In reply to aceman from comment #14) > (In reply to comment #13) > > then on trunk we can fix WarnIfLocalFileTooBig to do the right thing, which > > I suspect is to change bool hasSpaceAvailable(...) to void > > checkSpaceAvailable(..,) which throws either NS_ERROR_FILE_TOO_BIG or > > NS_ERROR_FILE_DISK_FULL. > Probably not throw, but return those values in an out argument. What's wrong with throwing an error? > > I don't like the patch v2 at all, by the way. > Why? Because the two cases have to be treated differently. I think you'd get the most consistency if you always threw if there was a problem, which would actually make it more compatible, as an extension would be able to distinguish between an unknown problem (older version returning false) or a known problem.

OK, we can argue about the clean fix for trunk in the followup bug. But I do not understand what you want here.

(In reply to aceman from comment #16) > OK, we can argue about the clean fix for trunk in the followup bug. > But I do not understand what you want here. I'm just saying that while v1 is better than v2, I would have preferred to try to reduce the code duplication.

(In reply to neil@parkwaycc.co.uk from comment #17) > I'm just saying that while v1 is better than v2, I would have preferred to > try to reduce the code duplication. Yes, but I didn't yet find a way to do less duplication in v1. How do you propose to use WarnIfLocalFileTooBig there?

This really belongs in your followup bug, but something like this: m_nsIPop3Sink->GetFolder(getter_AddRefs(folder)); nsCOMPtr<nsILocalMailFolder> localFolder(do_QueryInterface(folder)); if (!localFolder) return -1; bool tooBig = true; localFolder->WarnIfLocalFileTooBig(msgWindow, m_totalDownloadSize, &tooBig); if (tooBig) return -1;

(In reply to neil@parkwaycc.co.uk from comment #19) > This really belongs in your followup bug, but something like this: Yes, and that is where I will rework it to have a common function displaying both messages and used from all 3 places. > m_nsIPop3Sink->GetFolder(getter_AddRefs(folder)); > nsCOMPtr<nsILocalMailFolder> localFolder(do_QueryInterface(folder)); > if (!localFolder) > return -1; > bool tooBig = true; > localFolder->WarnIfLocalFileTooBig(msgWindow, m_totalDownloadSize, &tooBig); > if (tooBig) > return -1; But how does this help? WarnIfLocalFileTooBig doesn't distinguish the 2 reasons for failure. I want to display "pop3OutOfDiskSpace" but that is in localMsgs.properties, while WarnIfLocalFileTooBig uses messenger.properties. Or do you propose to add the logic to display both messages into WarnIfLocalFileTooBig, but keep them in the 2 files as they are now? A plan to put them in one file in the followup.

Even in that case, how does it help with the needed change to the hasSpaceAvailable to report the 2 cases? I still have to opencode it as in patch v1, or make it return the rv as in patch v2.

(In reply to aceman from comment #21) > Even in that case, how does it help with the needed change to the > hasSpaceAvailable to report the 2 cases? I still have to opencode it as in > patch v1, or make it return the rv as in patch v2. It helps in patch v2 because you need to fix all of the callers to hasSpaceAvailable to handle the switch to use rv. If you reduce the number of callers then you reduce the number of times you have to code the fix.

(In reply to neil@parkwaycc.co.uk from comment #22) > It helps in patch v2 because you need to fix all of the callers to > hasSpaceAvailable to handle the switch to use rv. If you reduce the number > of callers then you reduce the number of times you have to code the fix. I thought the other callers do handle the switch. On NS_FAILED(rv) they show the "folder full" message (4GB), even if "full disk" happened (for simplicity I converted only the POP3 one). But at least they show any message. So do you want me to also convert them to show both messages, and pull in the pop3 string as in comment 20?

(In reply to aceman from comment #23) > for simplicity I converted only the POP3 one Sorry, I didn't realise that was deliberate (although I suppose you still have the option of using that disk space warning function to at least give you a consistent message across all of the checks).

(In reply to neil@parkwaycc.co.uk from comment #24) > (In reply to aceman from comment #23) > > for simplicity I converted only the POP3 one > Sorry, I didn't realise that was deliberate (although I suppose you still > have the option of using that disk space warning function to at least give > you a consistent message across all of the checks). I just didn't want to load bundle localMsgs.properties from inside WarnIfLocalFileTooBig and duplicating something equivalent to ThrowAlertMsg (because I could not use it as it uses other bundle).

So would this appeal to you?

Comment on attachment 8434414 [details] [diff] [review] patch v2 (alternative) Review of attachment 8434414 [details] [diff] [review]: ----------------------------------------------------------------- Aside from the comment below, which applies to both the v2 and v3 patches, I'll defer to Neil on the ongoing reviews since he has stronger opinions on how he'd like to see this done. ::: mailnews/local/src/nsMsgMaildirStore.cpp @@ +256,5 @@ > + > + // Allow the folder to only reach 0xFFC00000 = 4 GiB - 4 MiB for now. > + *aResult = (0xFFC00000ULL - aSpaceRequested > folderSize); > + if (!*aResult) > + return NS_ERROR_FILE_TOO_BIG; We don't want this; one of the advantages of maildir is that the total contents of a folder don't need to be limited by how large of a single file we can handle.

(In reply to :Irving Reid from comment #27) > ::: mailnews/local/src/nsMsgMaildirStore.cpp > @@ +256,5 @@ > > + > > + // Allow the folder to only reach 0xFFC00000 = 4 GiB - 4 MiB for now. > > + *aResult = (0xFFC00000ULL - aSpaceRequested > folderSize); > > + if (!*aResult) > > + return NS_ERROR_FILE_TOO_BIG; > > We don't want this; one of the advantages of maildir is that the total > contents of a folder don't need to be limited by how large of a single file > we can handle. We definitely want this to be there until we fix all the internal counters (e.g. folder->GetSizeOnDisk()) to be 64bit. You can see even in this patch that folderSize is uint32_t. So unless that is done (I'm on it), we need this limit even on maildir. See bug 789679 and bug 813459. I do not need to add this in this bug, but it should go in sometime. Or do I miss anything? There is also the possibility that we move this check up into WarnIfLocalFileTooBig as this is a limitation of internal folder implementation itself, not of the mailstore. In that case we didn't need to play with the 'return NS_ERROR_FILE_TOO_BIG' as HasSpaceAvailable would only really check free disk space. Now that we actually call WarnIfLocalFileTooBig everywhere instead of HasSpaceAvailable this could be done. What do you think?

(In reply to aceman from comment #28) > There is also the possibility that we move this check up into > WarnIfLocalFileTooBig as this is a limitation of internal folder > implementation itself, not of the mailstore. In that case we didn't need to > play with the 'return NS_ERROR_FILE_TOO_BIG' as HasSpaceAvailable would only > really check free disk space. Now that we actually call > WarnIfLocalFileTooBig everywhere instead of HasSpaceAvailable this could be > done. So you'd have three functions, One on the store to check for the mbox file getting too large, one somewhere to check for the amount of free disk space, and one to call those two and do the necessary alerting?

(In reply to neil@parkwaycc.co.uk from comment #29) > So you'd have three functions, One on the store to check for the mbox file > getting too large, one somewhere to check for the amount of free disk space, > and one to call those two and do the necessary alerting? Yes, something like that. There are 2 levels of limits: 1. 4GB limit somewhere on nsIMsgFolder as size can't be above 4GB due to folder listeners being 32-bit (and GetSizeOnDisk). This should apply to all folders, not only local ones. But I don't know yet where to plug this so is in HasSpaceAvailable (both maildir and mbox) for now, but could be at promoted at least into WarnIfLocalFileTooBig. Once the 32-bitness is fixed, we could increase the limit to ~50bits due to JS integer limits. 2. free disk space check, applies to mbox and maildir too (could be in WarnIfLocalFileTooBig but specific mailstores may know about their overhead so it is in HasSpaceAvailable, actually shared in DiskSpaceAvailableInStore() now). So yes, WarnIfLocalFileTooBig could be the arbiter for now, calling HasSpaceAvailable of the store and additionally check the 4GB limit.

(In reply to Irving Reid from comment #27) > (From update of attachment 8434414 [details] [diff] [review]) > ::: mailnews/local/src/nsMsgMaildirStore.cpp > @@ +256,5 @@ > > + > > + // Allow the folder to only reach 0xFFC00000 = 4 GiB - 4 MiB for now. > > + *aResult = (0xFFC00000ULL - aSpaceRequested > folderSize); > > + if (!*aResult) > > + return NS_ERROR_FILE_TOO_BIG; > > We don't want this; one of the advantages of maildir is that the total > contents of a folder don't need to be limited by how large of a single file > we can handle. And in fact maildir returns a bogus value for GetSizeOnDisk anyway.

Comment on attachment 8436061 [details] [diff] [review] patch v3 (alternative 2) > if (NS_FAILED(rv) || !spaceAvailable) > { >- ThrowAlertMsg("mailboxTooLarge", aWindow); >+ if (rv == NS_ERROR_FILE_TOO_BIG) { >+ ThrowAlertMsg("mailboxTooLarge", aWindow); >+ } else { On trunk I think this should probably say if (rv == NS_ERROR_FILE_TOO_BIG) { ThrowAlertMsg("mailboxTooLarge", aWindow); } else if (NS_FAILED(rv) || !spaceAvailable) { ThrowAlertMsg("outOfDiskSpace", aWindow); } else { *aTooBig = false; } >+ *aResult = ((fileSize + aSpaceRequested) < 0xFFC00000); >+ if (!*aResult) >+ return NS_ERROR_FILE_TOO_BIG; Don't store this in aResult, just test directly.

(In reply to neil@parkwaycc.co.uk from comment #31) > And in fact maildir returns a bogus value for GetSizeOnDisk anyway. Thanks, filed bug 1032360 for this. (In reply to neil@parkwaycc.co.uk from comment #32) > On trunk I think this should probably say > if (rv == NS_ERROR_FILE_TOO_BIG) { > ThrowAlertMsg("mailboxTooLarge", aWindow); > } else if (NS_FAILED(rv) || !spaceAvailable) { > ThrowAlertMsg("outOfDiskSpace", aWindow); > } else { > *aTooBig = false; > } I don't want to return rv != NS_OK on trunk. Or I would return both values (NS_ERROR_FILE_TOO_BIG, NS_ERROR_FILE_DISK_FULL) in rv as necessary. Or return them in an out argument. This cludge of one value returned via rv and other one via the return argument is only for release, but we probably already missed that. So you do not like the idea of promoting the 4GB check up to WarnIfLocalFolderTooBig? Then this whole cludge could go away. On the other hand, callers would need to then call WarnIfLocalFolderTooBig to get the full checking and they may not be content with the function showing dialogs. They may want a silent check as the current HasSpaceAvailable is. We could add a "beSilent" argument and rename the function.

(In reply to :aceman from comment #33) > This cludge of one value returned via rv and other one via the return > argument is only for release, but we probably already missed that. second beta has not yet spun up yet.

(In reply to neil@parkwaycc.co.uk from comment #32) > >+ *aResult = ((fileSize + aSpaceRequested) < 0xFFC00000); > >+ if (!*aResult) > >+ return NS_ERROR_FILE_TOO_BIG; > Don't store this in aResult, just test directly. This is intentional in case some caller does not check rv, just aResult. So we want to return 'false' there. We can rework it in the trunk version.

For as long as we're stuck with uint_32 for our folder size API, I'm fine with maildir lying about its total size so that it always reports less than 4GB; with that, we don't need to fail writes to maildir because the folder is getting too big. I'm OK with, or have no strong opinion on, the rest of the decisions and questions.

(In reply to aceman from comment #33) > So you do not like the idea of promoting the 4GB check up to > WarnIfLocalFolderTooBig? Then this whole cludge could go away. On the other > hand, callers would need to then call WarnIfLocalFolderTooBig to get the > full checking and they may not be content with the function showing dialogs. > They may want a silent check as the current HasSpaceAvailable is. We could > add a "beSilent" argument and rename the function. OK, so for branch, attachment 8436061 [details] [diff] [review] is good except for the maildir change, you'll have to come to an agreement with Irving to decide the way forward there. For trunk, I'd say it's fine for HasSpaceAvailable to be the "silent" version of WarnIfLocalFolderTooBig. I don't think WarnIfLocalFolderTooBig should have extra checks.

CRITICAL: I think it is time to muddy the waters on this issue as I am facing it now and it isn't behaving as everyone assumed to this point. I have encountered this problem too in the last 24 hours. I read this thread and it wasn't very helpful. BACKGROUND: My Inbox is actually shared among six different email addresses -- i.e., when I "Get Mail", it all goes into the same Inbox. The Inbox is quite large, as you might expect after several years of collecting. NEW BEHAVIOR: I am now seeing this message: "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." However, I am NOT NOT NOT seeing it on ALL of the email accounts that retrieve into the same Inbox. Three of the six return the error message, while the other three do not. CONCERN: This points to something far more disturbing than just an Inbox that bumps up against the 4GB limit. If it was simply that, then it would do that for ALL of my accounts that retrieve to the same Inbox. Ideas?

It also depends on the size of the incoming messages. Say the inbox is 4GB-10MB in size. 3 of your accounts retrieve message that are of some small size (up to 6MB in total) so that they are successful without any error. Each of the other 3 contain messages that are more than 6MB in total to be downloaded. All of them fail with the message. I don't say that is exactly your situation, but it is an explanation how you could get the observation you have.

@ aceman -- Thank you for that insight. RESULTS: Using my iPhone to review the contents for each mail account, then deleted spams and extraneous messages. Two of the three accounts are now working again. The third one remains hung up with a 29MB attachment that I left in place to test your theory -- thus, you are correct in your interpretation. FINAL QUESTION: So how to retrieve that message with its 29MB attachment?

There is currently no supported way to grow any folder (including Inbox) above 4GB-4MB. You need to move some old messages out of Inbox to some other folder (e.g. Archive or just Inbox2). If you frequently do operations on all the messages, you can create a Saved Search folder that covers both folders and matches all messages in them.

It looks like we missed it and I will not try to get this into TB31. So this is the full version for trunk.

Comment on attachment 8493238 [details] [diff] [review] patch v4 (for trunk) Review of attachment 8493238 [details] [diff] [review]: ----------------------------------------------------------------- Everything looks good except for the maildir "file size" limit I argued against before. If Neil and Mark overrule me, OK, but I'd rather not have the arbitrary limit on maildir size. ::: mailnews/local/src/nsMsgMaildirStore.cpp @@ +269,5 @@ > + > + // Allow the folder to only reach 0xFFC00000 = 4 GiB - 4 MiB for now. > + *aResult = (0xFFC00000ULL - aSpaceRequested > folderSize); > + if (!*aResult) > + return NS_ERROR_FILE_TOO_BIG; I still prefer that we fix this issue by allowing maildir folders to grow as long as there is space on the disk, never returning 'file too big'; to get around the problem of 32-bit APIs for reporting folder size, maildir folders can stop counting somewhere before 4GB and return a number that fits in the API. "The folder size for my ridiculously large folder isn't right" isn't as bad a failure as "I can't download new messages into my ridiculously large folder even though I have space". ::: mailnews/local/test/unit/test_over4GBMailboxes.js @@ +287,5 @@ > * so it will really take 4GiB of space in the filesystem and may be slow > * (e.g. it takes ~450s on TB-try). Therefore we run this part of the test randomly, > * only in 1 of 100 runs. Considering the number of times all the tests are run > * per check-in, this still runs this test after several check-ins.*/ > + if (Math.random() * 100 > 1) { Was this change intentional? It looks like now we run the compact 99 times out of 100.

Comment on attachment 8493238 [details] [diff] [review] patch v4 (for trunk) The argument for maintaining the limit seems reasonable for now - if it simply isn't supported/fully tested then its reasonable to do. I would think that it would be better to have a more descriptive comment in the code stating why the limit is there, and pointing to a new bug that will remove the limit, once the dependent bugs are fixed.

The hasSpaceAvailable function uses nsMsgLocalMailFolder::GetSizeOnDisk which returns a bogus value yet for mialdir (bug 1032360) so the limit will probably never kick in (until that bug is fixed). The 4GB limits should be removed via bug 789679 and its dependecies. (In reply to :Irving Reid from comment #45) > ::: mailnews/local/test/unit/test_over4GBMailboxes.js > @@ +287,5 @@ > > * so it will really take 4GiB of space in the filesystem and may be slow > > * (e.g. it takes ~450s on TB-try). Therefore we run this part of the test randomly, > > * only in 1 of 100 runs. Considering the number of times all the tests are run > > * per check-in, this still runs this test after several check-ins.*/ > > + if (Math.random() * 100 > 1) { > > Was this change intentional? It looks like now we run the compact 99 times > out of 100. No, this is just a remnant of my testing :)(In reply to :Irving Reid from comment #45) > I still prefer that we fix this issue by allowing maildir folders to grow as > long as there is space on the disk, never returning 'file too big'; to get > around the problem of 32-bit APIs for reporting folder size, maildir folders > can stop counting somewhere before 4GB and return a number that fits in the > API. "The folder size for my ridiculously large folder isn't right" isn't as > bad a failure as "I can't download new messages into my ridiculously large > folder even though I have space". So that would need to change nsMsgLocalMailFolder::GetSizeOnDisk to intentionally report this incorrect value. This code is not there yet. If you produce that patch, you can remove this limit :) Also, if the folder size suddenly stops growing even though new messages are downloaded into it, the user may think some messages are getting lost and will report a panicking bug. Assuming he has something like Extra folder columns installed and watches the sizes regularly.

Comment on attachment 8493238 [details] [diff] [review] patch v4 (for trunk) Review of attachment 8493238 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Mark Banner (:standard8) from comment #46) > Comment on attachment 8493238 [details] [diff] [review] > patch v4 (for trunk) > > The argument for maintaining the limit seems reasonable for now - if it > simply isn't supported/fully tested then its reasonable to do. > > I would think that it would be better to have a more descriptive comment in > the code stating why the limit is there, and pointing to a new bug that will > remove the limit, once the dependent bugs are fixed. OK, let's land this (with the test fixed). Aceman, please file a follow-up bug, depending on bug 789679, to remove the 4GB check in maildir once we can correctly report sizes.

OK, I filed the bug and mentioned it in the patch.

Neil, will you please look at this?

Comment on attachment 8500524 [details] [diff] [review] patch v4.1 I think we mostly need r+ for the Seamonkey strings here.

Comment on attachment 8500524 [details] [diff] [review] patch v4.1 r=me for the string changes

Comment on attachment 8500524 [details] [diff] [review] patch v4.1 We can use the review from Irving.

Comment on attachment 8500524 [details] [diff] [review] patch v4.1 a=Ratty SeaMonkey CLOSED TREE

Pushed as https://hg.mozilla.org/comm-central/rev/99137515f12e

Builds failed with /builds/slave/tb-c-cen-l64-00000000000000000/build/mailnews/local/src/nsMsgMaildirStore.cpp:269:42: error: no matching function for call to 'nsIMsgFolder::GetSizeOnDisk(uint32_t*)' Backed out https://hg.mozilla.org/comm-central/rev/ad8d68fdeee3

Patch did still apply but the size of the argument to GetSizeOnDisk has changed since the patch was done. Sorry about that, try this one.

Pushed as https://hg.mozilla.org/comm-central/rev/0e0fb54e1299