User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0.1 Build ID: 20120615112143 Steps to reproduce: deleting messages or moving messages to a different folder Actual results: messages don't get removed from the cur folder Expected results: messages should have been deleted from the filesystem

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120907 Thunderbird/15.0.1 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0a2 I confirme this bug. Really anoying : my .thunderbird grows, and grows… I don't know where is my Build ID but if you tell me where it is, I could provide it.

Can you guys compact the offending folders ? (does that work )

I did it. It's not working. Mail files are stil in folders where there is no more mails.

I can't reproduce this with 18.0a1. I've tried with an IMAP account with maildirstore and with a Local Folders account with maildirstore. In the IMAP case messages don't end up on the disk in the first place and in the local folders case the deleted messages are properly moved to Trash and are gone if I empty the Trash. No stale files remain in the cur folder. What kind of account have you tested? POP3? Can you provide more precise steps to reproduce this beginning with a fresh Thunderbird profile.

> In the IMAP case messages don't end up on the disk in the first place I use IMAP too but download messages for offline access. And they still are on my disk I am now in 16.0.1 version. I tried rm on my INBOX folder and INBOX.msf (cleaning the old messages — which should not be here since I'm a ZeroInbox guy), copying a message in the cleaned INBOX. The message appears on the filesystem. I delete it via Thunderbird. Still on the filesystem.

Okay, so this is about Offline mode. That's the information missing from the bug report. Yes, I can reproduce the problem with Offline mode. Messages downloaded for offline viewing into a maildirstore never get deleted from the filesystem even if they are deleted in Thunderbird. That behaviour probably carries over from the standard berkeleystore behaviour where messages are only deleted when folders are compacted. But folder compaction does not work for maildirstore (you can select it from the menu, but the messages are not deleted from cur/).

What does it take to get the status changed to CONFIRMED? The problem still exists with User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/19.0 Thunderbird/19.0a1 and is confirmed by at least two more people here. Also my Trash folder keeps filling up with empty directories AND files. In it's current form, Maildir (Lite) is unusable.

The implementation of maildir message delete at http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMsgMaildirStore.cpp#810 surely has some glitches. There is a duplicate fileName.IsEmpty() check and also the tail is strange ('exists' is not used and no error checking): 838 path->Append(NS_LITERAL_STRING("cur")); 839 path->AppendNative(fileName); // let's check if the folder exists 842 bool exists; 843 path->Exists(&exists); 844 path->Remove(false); Of course, I am not sure if this function comes into play here, need to set up a maildir account yet.

Compact in IMAP consists of "request expunge" and "removal of unused mail data from offline-stor file". So, in berkleystore/IMAP/Offline-Use=Off, "removal of unused mail data from offline-stor file" is job of Compact. Culprit is "removal of Compact from maildir even though IMAP", isn't it?

"Compact" in maidirstore/IMAP/Offline-Use=On may be; - Expunge menu to issue EXPUGE command. Expunge in conext menu of message if "selective expunge" will be suported, in addition to "per mail Undelete". (per mail re-fetch header and body may be needed) - Automatic removel of associated file when "Expunged" is detected. Because msDBHdr for "Flaged as \Deleted" mail is removed from Msgdatabase when "Move to trash" model and "Remove immediately" model, relation between UID(currently messageKey is UID, perhaps) and "file name under cur"(storeToken) is lost by "store +Flag \Deleted". So, new mechinis to keep relation between UID and filename is needed. - Mbox/cur/nnnnnn.UID (if gmail IMAP, Mbox/cur/nnnnnn.(X-GM-MSGID) - Mbox/UID directory in addition to cur and tmp. filename=UID, file content=storeToken value of mail with \Deleted flag This is similar to popstate.dat in POP3. If relation is kept at somewhere consistently, file under Mbox/cur can be deleted when "Expunged" is detected. However, I think mechanism of "clean up of garbage" is needed in case of Mbox.msf corruption or intensional deletion of Mbox.msf by user. Rebuild Index, Compact, and Force Re-Fetch for IMAP, in maildirstore. - Repair Folder : - local folder : Re-creation of Mbox.msf content from existent Mbox/cur files, with reparsing message headers. - IMAP/Offline-Use=On : Re-creation of Mbox.msf content from existent Mbox/cur files, with reparsing message headers. - Repair Folder : - local folder : Re-creation of Mbox.msf content from existent Mbox/cur files, with reparsing message headers. - IMAP/Offline-Use=On : Re-creation of Mbox.msf content from existent Mbox/cur files, with reparsing message headers. (similar to Don't RETRIEVE if states of UIDL is "k". - Compact Folder : If IMAP, issue EXPUGE command. Based on Mbox.msf content, clean up Mbox/cur. - Force Refetch in IMAP(New. per account, per folder, per mail) Discard current data relevant to an UID, fetch header, and fetch message body if required.

supportsCompaction=false was set in nsIMsgPluggableStore of maidirstore/IMAP/Offline-Use=On folder. Dump of property of msgStore. > msgFolder.msgStore : [xpconnect wrapped nsIMsgPluggableStore] > msgFolder.msgStore / Property > supportsCompaction = false As for Compact, problem was "Compact is completly killed in maildirstore even though IMAP", instead of "Compact menu is killed or hidden". "Delete" case and "Move" and is slightly different, although "Mbox/cur/nnnn is not deleted" is same. In any case, MoveSourceFolder==maildirstore/IMAP/Offline-Use=On folder. (a-1) Delete, IMAP delete model : Move to Tash, Remove immediately "uid xx store *Flags \Deleted" is normally issued. msDBHdr is removed from Msgdatabase. However, Mbox/cur/nnnn is not deleted. (a-2) Delete, IMAP delete model : Just mark it as deleted. "uid xx store *Flags \Deleted" is normally issued. msDBHdr is NOT removed from Msgdatabase. Instead, IMAPDeleted flag of msgDBHdr is set to true. Message is still shown with strike-thru line, Undelete is possible. So, Mbox/cur/nnnn is kept correctly. (b-1) Move to maildirstore/IMAP/Offline-Use=Off, maildirstore/local folder berkleystore/any folder type For "Move Source Folder", same as "Delete". (b-2) Move to maildirstore/IMAP/Offline-Use=On IMAP delete model : Move to Tash, Remove immediately Mbox/cur/nnnn is moved to MoveTarget/cur. At thread pane, mail disappears once. "uid xx store *Flags \Deleted" is not issued. By folder access, message body is fetched to Mbox/tmp/nnnn and moved to Mbox/tmp/nnnn, msgDBHdr is updated, and shown again. ==> Delete operation at Move Source is not invoked as expected. (b-3) Move to maildirstore/IMAP/Offline-Use=On IMAP delete model : Jus mark it as deleted Mbox/cur/nnnn is moved to MoveTarget/cur. At thread pane, mail disappears once. "uid xx store *Flags \Deleted" is not issued. By folder access, message body is fetched to Mbox/tmp/nnnn and moved to Mbox/tmp/nnnn, msgDBHdr is updated, and shown again. IMAPDeleted flag of msgDBHdr is not set true. ==> Moved mail is not show as "Deleted". So, even if "delete of Mbox/cur/nnnn" will be executed upon end of Delete operation or end of Move operation, problem of "Mbox/cur/nnnn will remain. I think followig is an simplest way. (1) Keep relation between UID and cur/nnnn file(storeToken) somewhere, as postate.dat holds "k", "p", "d" states. (2) Upon folder re-sync(folder open), delete all cur/nnnn files(and directory, garbage) which is not pointed by existent UID(call Table) 1. Scan msgDBHdrs, add UID to Table if UID is not held in Table. 2. Delete all uknown cur/nnnn based on Table. 3. Refresh Table with current states to keep consistency with msgDB. This is same as "issue RETR if UIDL=k/p is not held in popsate.dat. (3) Upon unsol "EXPUNGED xxx" response, delete obsolete cur/nnnn file used for the UID. (4) In case of garbage, and for explicit expunge, provide a way to force removal of unknown cur/nnnn files. Name of "Compact" is not appropriate for maildirstore. And "Compact Folders" like capability(do on all folders of the account at once) is not mandatory. How about "Expunge Folder" button near"Repair Folder" in Folder Property/General?

For "Table" I call in previous comment. If implemnted by JavaScript, I think JavaScript Object, saved as JSON data which is held in BLOB data of, SQLite DB with utilizing journal, is most convenient one. No need to care for file loss due to safe but not-absolutely-safe file writing, no need to parse not-so-well structured text data, no need to use Mork, no need to use RDF, ...

FYI. As for "move from maildirstore/IMAP/Offline-Use=On to maildirstore/LocalMailFolder" case, phenomenon was never "IMAP/cur/nnnn is not removed by delete of mail", as seen in bug 856532. Phenomenon was; - IMAP/cur/nnnn file is successfully moved to Target/cur/nnnn because it's "Move", and msgDBHdr is successfully removed from msgDatabase for maildirstore/LocalMailFolder because it's "Move". i.e. IMAP/cur/nnnn file is pretty normally deleted by "Move". - But, "uid xx store +Flags \Delete" is not issued even though IMAP. (Perhaps, because of "Move" != "Delete") So, upon folder open, mail data of the unknown UID is fetched, then stored in offline-store file(IMAP/cur/xxxx) if Offline-use=On. Because IMAP/cur/nnnn file for deleted mail or moved mail should be kept with IMAPDeleted flag of msgDBHddr.flags = true when Offline-use=On and IMAP delete model = Just mark it as deleted, removal of IMAP/cur/nnnn file should be executed according to EXPUNGE of an UID at server's Mbox. This is difference of "Just mark it as deleted" case from problem of "IMAP-OfflineUseOn/cur/nnnn file is not removed by Delete of mail" with "Move to trash" model and "Remove it immediately" model.

I think the handling of MOVE with IMAP involved here is fundamentally wrong. MOVE is not COPY+DELETE from the user perspective, even though that may be the implementation in some backend combinations. MOVE was always "make it appear in folder2, not folder1, make sure mail is never lost in the process. The direct comparison is that in the file system, move is a different operation from copy+delete on all current file systems (including FAT since 1983), and the trash folders in file UIs only contains files that were deleted, not ones that were moved and survive in their new home. So MOVE on IMAP should be (regardless of delete mode): - If same account and server supports the extension, execute the move server side. - Otherwise copy the message, retrieve its header back to confirm copy successfully committed server side, then delete and SELECTIVELY EXPUNGE (do not expunge any pending delete mails, those need to be preserved for user undelete until use empties the trash). This needs to be efficiently batched when doing multiple mails, especially when moving a large batch (such as moving all mailing list threads from 2012 to a "2012" folder). - If success mark action as locally complete (e.g. by moving file if maildirstore). - If something goes wrong, refresh folder contents of both folders to find out what is really where now. COPY on IMAP should be (regardless of delete mode): - If same account and server supports the extension, execute the copy server side. - Otherwise copy by retrieve and upload, the retrieve back headers to confirm successfully committed server side. This needs to be efficiently batched when doing multiple mails, especially when copying a batch (such as all correspondence on a case being copied to or from a shared folder). - If success mark action as locally complete (e.g. by copying file if maildirstore). - If something goes wrong, refresh folder contents of both folders to find out what is really where now. USER DELETE on IMAP with just-mark-as-delete should be - Execute UID delete. - If success mark action as locally complete (e.g. by updating the deleted flag in local index). - If something goes wrong, refresh folder contents to find out what is really where now. USER DELETE on IMAP with move-to-trash should be - Follow procedure (and code) for regular message move (see above). USER DELETE on IMAP with delete-flag-as-trash should be - Execute UID delete. - If success mark action as locally complete (e.g. by moving file if maildirstore). - If something goes wrong, refresh folder contents to find out what is really where now. "delete-flag-as-trash" would be a new IMAP mode, where mails with the "Deleted" IMAP flag are shown in a corresponding virtual Trash folder in the UI, but remain in their original location server side. Emptying the virtual trash folder executes a server side expunge for its associated non-trash folder. But adding such a mode is a topic for another bug (if somebody cares to file it).

xref bug 302205 Need IMAP Purge/Expunge (without compact)

FYI. nsMsgMaildirStore::GetSupportsCompaction() always returns false. http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsMsgMaildirStore.cpp#1087 So msgFolder.msgStore.supportsCompaction = false is always returned if Maildir, then "Compact of Maildir" is always killed.

Confirming that this still occurs even after the fixes for bug 1113275 STR: 1) In maildir IMAP with offline storage enabled, and IMAP delete model set to move to Trash. Select one or more messages in an IMAP folder, and delete. Expected: 1) messages disappear from the source folder, appear in the Trash folder 2) in the maildir offline file storage, the files in the source folder disappear Actual: 1) works, but 2) the files continue to appear in the source folder.

I could find no plan to delete store messages for the non-compacting maildir. For mbox, we do not need to delete messages, because the status is stored in the msgHdr, and "deleting" does really affect the offline store until compact. So this is an implementation of a store delete for the offline store, done at the time that the message is deleted from the database.

This patch is applied on top of the current patches for bug 1113275 (and also bug 856519 though I don't think that one matters here.)

Comment on attachment 8543574 [details] [diff] [review] Implement DeleteStoreMessages for imap offline stores >+ // I believe we only reach here for same-server operations, That would be the !m_isCcrossServerOp check above. >+ if (!supportsCompaction & numHdrs) && >+ void DeleteStoreMessages(nsTArray<nsMsgKey> &aMessages, nsIMsgFolder* aFolder); Nit: static > // let's check if the folder exists > bool exists; > path->Exists(&exists); >- path->Remove(false); >+ rv = path->Remove(false); >+ if (!exists || NS_FAILED(rv)) >+ { >+ PR_LOG(MailDirLog, PR_LOG_ALWAYS, >+ ("DeleteMessages - file does not exist !!\n")); >+ // perhaps an offline store has not downloaded this particular message >+ continue; >+ } Not sure what you're trying to achieve here with the rv check. (Not sure what the old code was trying to achieve with the exists check!)

(In reply to neil@parkwaycc.co.uk from comment #21) > Comment on attachment 8543574 [details] [diff] [review] > > // let's check if the folder exists > > bool exists; > > path->Exists(&exists); > >- path->Remove(false); > >+ rv = path->Remove(false); > >+ if (!exists || NS_FAILED(rv)) > >+ { > >+ PR_LOG(MailDirLog, PR_LOG_ALWAYS, > >+ ("DeleteMessages - file does not exist !!\n")); > >+ // perhaps an offline store has not downloaded this particular message > >+ continue; > >+ } > Not sure what you're trying to achieve here with the rv check. (Not sure > what the old code was trying to achieve with the exists check!) The previous path->Exists() I believe was pointless, probably a leftover copy-and-paste. What I am trying to accomplish is the correct balance of error reporting. I can imagine cases in which we would reach the previous code (fileName.IsEmpty()) under normal operation. So while it is an interesting error, it should not really be a warning. That's why I removed the NS_ERROR_FAILURE from it, but left the PR_LOG. For the ->Remove(), a failure is also interesting and should not just be neglected, though I am less sure of cases where it would be reached in normal operation. But it should not abort, since the method has accomplished its mission (delete the message) since the message does not exist. So I also added PR_LOG reporting here as well. (Though in practice, I have never used PR_LOG with maildir, but perhaps I would if the implementation was more complete).

(In reply to Kent James from comment #22) > (In reply to comment #21) > > (From update of attachment 8543574 [details] [diff] [review]) > > > path->Exists(&exists); > > (Not sure what the old code was trying to achieve with the exists check!) > The previous path->Exists() I believe was pointless, probably a leftover > copy-and-paste. > > What I am trying to accomplish is the correct balance of error reporting. OK but I think checking both exists and rv might be overkill in that case.

Are you done with your initial review, and waiting for me to implement the changes from your last comment? I'd like to get all of the maildir patches landed.

(In reply to Kent James from comment #24) > Are you done with your initial review, and waiting for me to implement the > changes from your last comment? Are you planning to implement any changes to the code that you requoted in comment #22?

(In reply to neil@parkwaycc.co.uk from comment #25) > (In reply to Kent James from comment #24) > > Are you done with your initial review, and waiting for me to implement the > > changes from your last comment? > > Are you planning to implement any changes to the code that you requoted in > comment #22? I'm happy to do the change you suggested in comment 23. I'll do a revised patch when you want, are you done with your initial review and ready for a revised patch?

This patch had bit-rotted significantly, so I fixed that as well as incorporated the suggested changes.

Comment on attachment 8560022 [details] [diff] [review] Un-bitrotted with requested changes >+ // I believe we only reach here for same-server operations, >+ // so we can assume that the src is also imap that uses >+ // offline storage. Revise this to mention m_isCcrossServerOp? >- if (notifier) >+ if (notifier && numHdrs) > { > // XXX Currently, the DeleteMessages below gets executed twice on deletes. > // Once in DeleteMessages, once here. The second time, it silently fails > // to delete. This is why we're also checking whether the array is empty. Hmm, I guess the comment still makes sense here. >+ if (exists) >+ { >+ path->Remove(false); >+ } >+ else >+ { >+ PR_LOG(MailDirLog, PR_LOG_ALWAYS, >+ ("DeleteMessages - file does not exist !!\n")); >+ // Perhaps an offline store has not downloaded this particular message. >+ continue; >+ } The way you've written this with the continue in the else block looks awkward. Could you change it to if (!exists) { ... continue; } path->Remove(false);

Pushed with nits fixed: https://hg.mozilla.org/comm-central/rev/baa8c010ffb3

Excuse-me about being naive. I'm interested and testing Thunderbird with Maildir-like storage and I found this bug on my testing. I'm surprised to see the milestone set to version 38, as we are at 31.5 at the moment. Why can't this fix be pushed before, let say for version 32? Testing and validation issues? By the way, big thanks to all developers that put some work to give us this function. It's getting close and I'm excited about the moment of switching to Thunderbird (We are blocked by the one file per mail functionality, a must for us).

There were many issues with maildir in TB 31, and fixing them required changes that were beyond what would be appropriate for back-porting. Current aurora Thunderbird 38 has all of the critical maildir patches, and I would encourage you to use that for testing. It is not trivial to setup though. Start a new profile, skip the add account dialog, and go to Tools/Options/Advanced/General and set "Message Store Type for New Accounts" to maildir.

Thanks for that answer! As I see we misunderstood each other. I wasn't talking about backporting to TB 31, but releasing the fixes for the next version TB 32. As I understand your answer, next version will be TB 38, and not 32. Excuse my ignorance about the version numbering. I suppose this jump is to have the same number as Firefox at the date of it's release, am I right? Many thanks again and congratulations for your hard work. Will have a try at the testing version. Cheers!

(In reply to macuserfr from comment #32) > Thanks for that answer! As I see we misunderstood each other. I wasn't > talking about backporting to TB 31, but releasing the fixes for the next > version TB 32. As I understand your answer, next version will be TB 38, and > not 32. Excuse my ignorance about the version numbering. I suppose this jump > is to have the same number as Firefox at the date of it's release, am I > right? Well, there is no jump, the versions of TB 32-37 do exist, but they can only be downloaded as "beta" state, never a stable "release". Yes, TB 38 will be released at the same time as Firefox 38. This is intentional and worked this way at least since TB 24 (the next release was then TB31).

Thanks again! Sorry about spamming Bugzilla, didn't find a better place to talk directly with people envolved. That's cool. So in 2 major releases we can expect to have maildir working in the stable version.

Hi there! I'm testing EarlyBird version 38.0a2 (2015-03-20) with maildir activated. Getting my IMAP account messages from the server -> OK (expected result: downloading all messages from server) Sending a message -> OK (Created a message in the Sent folder, sent the message to my test address) Moving a message to trash -> OK Emptying trash -> Fail, the message is still there in the filesystem, but not visible anymore in Thunderbird. What is curious is that I tested emptying the trash from another mail client connected to the IMAP account. After the other client empty the trash, Thunderbird synchronise the folder and delete the message in the filesystem. But it does not do it when emptying the trash from Thunderbird. In this case we get phantom files in the filesystem. Should the bug be already fixed in the version I tested? Or maybe it's another one?