User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36 Steps to reproduce: I'm using maildir option in TB because for backup with rsync it's much better than berkley db. In the account settings I have in the "Server Settings" under "Message Storage" the option "Clean up ("Expunge") Inbox on Exit" enabled as well as the "Empty Trash on Exit" option. Also, I set the option to "When I delete a message:" "Move it to folder:" "Trash on user@domain.tld" Actual results: So, when I get an email now and delete it, it moves to the Trash folder in TB. When I empty the trash folder manually, it disappears from the message list in TB, however the mail is still there when I look under ~/.thunderbird/xxx.default/mail.domain.tld/Trash/cur This results in thousands of files still left over that should have been removed. Expected results: After emptying the trash in Thunderbird, the underlaying file in the Maildir storage should also go.

Something to look at when you have a spare five minutes ;-)

There are a number of maildir bugs that would be great to solve, but realistically I am not going to have the time in the foreseeable future to investigate.

Reporter, is this a pop account. If yes, then perhaps a duplicate of bug 1293770

It's IMAP

I have same issue. IMAP account, , set up as MailDir the ....TRASH\CUR desktop PC folder is not getting emptied, despite the THUNDERBIRD TRASH folders per each login account, in the mail program, ARE being emptied (both manually and automatically) I am currently emptying the TRASH\CUR directory manually just to decrease space but it would be good if the desktop PC folder emptied in conjunction with the actual account folder any suggestions for testing, let me know, with settings

Do you still see this when using version 60 ?

Still same behaviour in v60.

This is cheesy trivial fix, which will fix the problem, but doesn't really dig too deep. In particular, it'll work for the current mbox and maildir nsIMsgPluggableStore implementations, but it short-circuits the whole abstraction.

This one is an attempt to fix the problem properly. The problem should also affect the nntp folders, but I haven't addressed them in this patch. I _think_ the nntp folders use a pluggablemailstore, but I've not gone in and confirmed it yet.

Comment on attachment 9008960 [details] [diff] [review] bug1317117_proper_fix1.patch Review of attachment 9008960 [details] [diff] [review]: ----------------------------------------------------------------- You'll ask for feedback when you're ready? Or you rely on people to watch the bug? I think I like this better. As you said, the other patch covers News, so that would have to be added here. What about a local folder as Trash? Does that already work? ::: mailnews/imap/src/nsImapMailFolder.cpp @@ +1543,5 @@ > mDatabase = nullptr; > } > > + // Delete the .msf file. > + { That block would be removed in the final version.

(In reply to Jorg K (GMT+2) from comment #10) > I think I like this better. As you said, the other patch covers News, so > that would have to be added here. What about a local folder as Trash? Does > that already work? Yes, the local folder implementation already invokes the msgstore. Got an updated patch which makes the change for nsMsgNewsFolder too. > ::: mailnews/imap/src/nsImapMailFolder.cpp > @@ +1543,5 @@ > > mDatabase = nullptr; > > } > > > > + // Delete the .msf file. > > + { > > That block would be removed in the final version. Do you mean it shouldn't use scoping blocks like that, or that it shouldn't be deleting the .msf file? I'm assuming the former for my updated patch, but let me know if I'm reading that wrong.

Comment on attachment 9010140 [details] [diff] [review] bug1317117_proper_fix2.patch Review of attachment 9010140 [details] [diff] [review]: ----------------------------------------------------------------- Seems to work, but I see one small inconsistency: If I create a subfolder in trash, say foobar. When I do "Empty trash" the it deletes all the mails in trash, and subfolders. But the foobar.sbd and a msf file in it remain. "Empty trash" a second time will remove these remains also. ::: mailnews/news/src/nsNewsFolder.cpp @@ +511,5 @@ > + // Ask the msgStore to delete the actual storage (mbox,maildir whatever). > + nsCOMPtr<nsIMsgPluggableStore> msgStore; > + rv = GetMsgStore(getter_AddRefs(msgStore)); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = msgStore->DeleteFolder(this); NS_ENSURE_SUCCESS(rv, rv); here?

Comment on attachment 9010140 [details] [diff] [review] bug1317117_proper_fix2.patch Review of attachment 9010140 [details] [diff] [review]: ----------------------------------------------------------------- Code looks good, I didn't test, but Magnus did ;-) ::: mailnews/imap/src/nsImapMailFolder.cpp @@ +1551,5 @@ > + bool exists = false; > + rv = summaryFile->Exists(&exists); > + if (NS_SUCCEEDED(rv) && exists) { > + rv = summaryFile->Remove(false); > + NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Could not delete msg summary file"); M-C is moving to use: if (NS_WARN_IF(NS_FAILED(rv))) { @@ +1555,5 @@ > + NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Could not delete msg summary file"); > + } > + } > + > + // Ask the msgStore to delete the actual storage (mbox,maildir whatever). Nit: We like funny comments, but not, well, suboptimal comments. So there should be a space after the comma, and I'm not sure what "whatever" is referring to. I guess future types we might introduce. ::: mailnews/news/src/nsNewsFolder.cpp @@ +503,5 @@ > + bool exists = false; > + rv = summaryFile->Exists(&exists); > + if (NS_SUCCEEDED(rv) && exists) { > + rv = summaryFile->Remove(false); > + NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Could not delete msg summary file"); See above.

(In reply to Magnus Melin [:mkmelin] from comment #13) > Comment on attachment 9010140 [details] [diff] [review] > bug1317117_proper_fix2.patch > > Review of attachment 9010140 [details] [diff] [review]: > ----------------------------------------------------------------- > > If I create a subfolder in trash, say foobar. When I do "Empty trash" the it > deletes all the mails in trash, and subfolders. But the foobar.sbd and a msf > file in it remain. "Empty trash" a second time will remove these remains > also. Cool - I don't think I tried it with a subfolder, so I'll check that out. > ::: mailnews/news/src/nsNewsFolder.cpp > @@ +511,5 @@ > > + // Ask the msgStore to delete the actual storage (mbox,maildir whatever). > > + nsCOMPtr<nsIMsgPluggableStore> msgStore; > > + rv = GetMsgStore(getter_AddRefs(msgStore)); > > + NS_ENSURE_SUCCESS(rv, rv); > > + rv = msgStore->DeleteFolder(this); > > NS_ENSURE_SUCCESS(rv, rv); here? I left it out as the existing DeleteFolder() implementations aren't too specific about their returncodes. eg it's perfectly normal to empty trash on an imap folder which doesn't even have a local directory. Currently DeleteFolder() will return a missing-file error for this, but I think it should return NS_OK in such cases. So I shall pop back in, rework the patch and tighten things up.

(In reply to Ben Campbell from comment #15) > Currently DeleteFolder() will return a missing-file > error for this, but I think it should return NS_OK in such cases. In nsImapMailFolder::Delete() you have: rv = msgStore->DeleteFolder(this); return rv; So that would need reconsideration in view of what you said, right?

(In reply to Jorg K (GMT+2) from comment #16) > (In reply to Ben Campbell from comment #15) > In nsImapMailFolder::Delete() you have: > rv = msgStore->DeleteFolder(this); > return rv; > > So that would need reconsideration in view of what you said, right? Not in that exact snippet, but the msgStore DeleteFolder() implementations should (IMHO) be a little more rigorous about what they return (ie not return an error if the mbox or maildir doesn't exist on the filesystem, which can easily happen, especially with imap folders)

This latest one is a bit more intrusive, but it: - is more careful with returncodes - factors out a goodly chunk of code which was replicated over multiple folder implementations - should fix the not-completely-removing-subfolders issue Magnus ran into (the old patch didn't attempt to force-close databases for subfolders, but this new one does)

Comment on attachment 9010508 [details] [diff] [review] bug1317117_proper_fix3.patch Review of attachment 9010508 [details] [diff] [review]: ----------------------------------------------------------------- Good if we can unify the the implementation. But it doesn't seem to fix the problem I saw earlier. Didn't notice earlier, and somewhat unrelated, but for IMAP Empty Trash fails for subfolders in that they still show up after restart. (Seems this is the case on trunk too.) Manually deleting each of them does the trick. ::: mailnews/local/src/nsMsgBrkMBoxStore.cpp @@ +305,5 @@ > nsCOMPtr<nsIFile> pathFile; > nsresult rv = aFolder->GetFilePath(getter_AddRefs(pathFile)); > NS_ENSURE_SUCCESS(rv, rv); > > + exists = false; I'd move the bool exists declaration down to here where it's first used ::: mailnews/local/src/nsMsgMaildirStore.cpp @@ +328,5 @@ > nsCOMPtr<nsIFile> pathFile; > nsresult rv = aFolder->GetFilePath(getter_AddRefs(pathFile)); > NS_ENSURE_SUCCESS(rv, rv); > > + exists = false; same here

Question - are deletes well tested for the different storage types?

(In reply to Magnus Melin [:mkmelin] from comment #19) > But it doesn't seem to fix the problem I saw earlier. Oh, it seemed to work for me (I was using Imap folders). Will take a closer look. > Didn't notice earlier, and somewhat unrelated, but for IMAP Empty Trash > fails for subfolders in that they still show up after restart. (Seems this > is the case on trunk too.) Manually deleting each of them does the trick. Hmm. I could easily imagine subfolders getting reinstated because they were never properly deleted from the IMAP server. Will investigate (although my understanding of the IMAP<->folder interaction is still a bit fuzzy) On folder deletion in general, my first impressions are that it's a little muddled: nsIMsgFolder has 4 functions which deal with folder deletion (Delete(), deleteSubFolders(), propogateDelete() and recursiveDelete()), which strikes me as a likely candidate for some simplification/clarification at some point...

(In reply to Wayne Mery (:wsmwk) from comment #20) > Question - are deletes well tested for the different storage types? There don't seem to be any specific tests to cover it: https://dxr.mozilla.org/comm-central/source/comm/mailnews/local/test/unit/test_nsIMsgPluggableStore.js (assuming I'm looking in the right place!)

Comment on attachment 9010508 [details] [diff] [review] bug1317117_proper_fix3.patch Review of attachment 9010508 [details] [diff] [review]: ----------------------------------------------------------------- We're getting further and further away from the "cheesy" one line fix now cutting quite deep into established code. Please do two things: 1) A try run. Three platforms will do. Due to different file locking mechanisms, please include Windows. I suggest: hg qnew -m "try: -b do -p linux64,macosx64,win64 -u all" try && hg push -f -r tip cc-try && hg qpop && hg qdelete try 2) Test your changes also on virtual/search folders. That's enough feedback for now, not sure yet whether it's good or bad ;-) ::: mailnews/base/util/nsMsgDBFolder.cpp @@ +3639,5 @@ > + // else may be supported in future). > + nsCOMPtr<nsIMsgPluggableStore> msgStore; > + rv = GetMsgStore(getter_AddRefs(msgStore)); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = msgStore->DeleteFolder(this); I think you said that this can fail for various reasons. ::: mailnews/local/src/nsLocalMailFolder.cpp @@ -772,5 @@ > - > - rv = msgStore->DeleteFolder(this); > - if (rv == NS_ERROR_FILE_NOT_FOUND || > - rv == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) > - rv = NS_OK; // virtual folders do not have a msgStore file Where did this code go? Did you try your patch with a "virtual" folder, that is, a search folder? Search messages, then "Save as Search Folder". ::: mailnews/local/src/nsMsgBrkMBoxStore.cpp @@ +318,5 @@ > + exists = false; > + pathFile->Exists(&exists); > + if (exists) { > + rv = pathFile->Remove(true); > + NS_ENSURE_SUCCESS(rv, rv); I've looked at the new code for a while. Basically you transplanted the maildir code to the mailbox code here. Somehow you changed the semantics a bit. Before the code tried to delete the 'pathFile' without checking. Then if the 'pathFile' was already a directory, and most likely the prior deletion failed, it removed the directory recursively. If the 'pathFile' wasn't a directory, it deleted 'pathFile.sdb'. That old processing looks a little strange. However, it's been there for decades, so it might have had a reason. Can GetFilePath() ever return a directory to start with?

(In reply to Jorg K (GMT+2) from comment #23) > We're getting further and further away from the "cheesy" one line fix now > cutting quite deep into established code. Afraid so :-( > Please do two things: > 1) A try run. Three platforms will do. Due to different file locking > mechanisms, please include Windows. I suggest: > hg qnew -m "try: -b do -p linux64,macosx64,win64 -u all" try && hg push > -f -r tip cc-try && hg qpop && hg qdelete try Running now: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=68928a9834b60fd11d0195dcf1fecae5b2a2595a > 2) Test your changes also on virtual/search folders. Will do (and also the ongoing subfolder issues mkmelin reported in comment #19). > That's enough feedback for now, not sure yet whether it's good or bad ;-) It's all good feedback! > ::: mailnews/base/util/nsMsgDBFolder.cpp > @@ +3639,5 @@ > > + // else may be supported in future). > > + nsCOMPtr<nsIMsgPluggableStore> msgStore; > > + rv = GetMsgStore(getter_AddRefs(msgStore)); > > + NS_ENSURE_SUCCESS(rv, rv); > > + rv = msgStore->DeleteFolder(this); > > I think you said that this can fail for various reasons. I changed the two msgStore DeleteFolder() implementations to be tolerant of a missing mbox/maildir because that's not an error condition (they are created as needed). If they return anything other than NS_OK now, then that's an actual error. > ::: mailnews/local/src/nsLocalMailFolder.cpp > @@ -772,5 @@ > > - > > - rv = msgStore->DeleteFolder(this); > > - if (rv == NS_ERROR_FILE_NOT_FOUND || > > - rv == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) > > - rv = NS_OK; // virtual folders do not have a msgStore file > > Where did this code go? As above, this is now handled in the DeleteFolder() implementations, albeit by calling `file->Exists(&b)` first, rather than blinding attempting a delete then checking the return codes. Additionally, it doesn't make sense for users of DeleteFolder() to be checking for file-specific error codes like that. The aim is to abstract that kind of detail away behind the `nsIMsgPluggableStore` API. Currently the only two msgStore implementations _are_ filesystem-based, but the aspiration is to support more way-out-there stores. > I've looked at the new code for a while. Basically you transplanted the > maildir code to the mailbox code here. Yes, pretty much. The maildir does a recursive directory delete while mbox one just does non-recursive file delete, but otherwise they do exactly the same thing. > Somehow you changed the semantics a bit. Before the code tried to delete the > 'pathFile' without checking. Then if the 'pathFile' was already a directory, > and most likely the prior deletion failed, it removed the directory > recursively. If the 'pathFile' wasn't a directory, it deleted > 'pathFile.sdb'. That old processing looks a little strange. However, it's > been there for decades, so it might have had a reason. Can GetFilePath() > ever return a directory to start with? I was rather hoping you'd come back with a nice definitive "What? That's craziness! You've missed all these really obvious cases where it might be a directory!" :-) It was transplanted verbatim from nsMSgDBFolder when pluggable stores were added in 2011. My take is that it's vestigial code (perhaps left over from a previous musing on supporting both maildir and mbox). I think GetFilePath() (and nsMsgDBFolder::parseURI()) is agnostic on the whole file/dir thing. I think the actual file/directory creation is handled by the msgStore anyway. I'll dig back into this again next week.

All sounds reasonable and no failures one the try run :-)

Ahh, such a can of worms, this bug! This revised patch contains one extra change for imap folders, to delay the deletion of the .msf & storage until after all the subfolders have been deleted. Without this, a warning was triggering on virtual folders under Trash. (the warning was that the .msf file had gone). This patch seems to fix the issue for me: - after "Empty Trash", the maildir on disk is empty. - subfolders are deleted. - virtual subfolders seem to be deleted fine. - exiting and restarting the app doesn't cause the folders to reappear. Magnus: does fix it for you? If so, I'll kick off a full try run+tests and flag Jorg to review it (Again. Sorry Jorg!)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=202863330&revision=ad657a76acdbd30c55e172a9547ec277e1957403 is what we call "green", so no test failures produced by this patch.

Comment on attachment 9013478 [details] [diff] [review] bug1317117_proper_fix4.patch Review of attachment 9013478 [details] [diff] [review]: ----------------------------------------------------------------- The original problem in this bug seems fixed. But I don't see a change in what I saw earlier. I think subfolder deleting is missing unsubscribing so therefore do not really disappear (reappear after next restart). What I see is that A) if the trash/foo folder had messages, foo.msf will still be there B) if the trash/foo folder didn't have messages, everting, including foo.msf will be gone, locally In both cases the folders will reappear after restart, since they still exist on the server. Reappear, but apparently not usable (gray, can't file messages into them) ::: mailnews/base/util/nsMsgDBFolder.cpp @@ +3624,5 @@ > + // NOTE: this doesn't remove .msf files in subfolders, but > + // both nsMsgBrkMBoxStore::DeleteFolder() and > + // nsMsgMaildirStore::DeleteFolder() will remove those .msf files > + // as a side-effect of deleting the .sbd dir. > + nsCOMPtr <nsIFile> summaryFile; Nit: I think nsCOMPtr<nsIFile> is more readable (no space before the typing) ::: mailnews/imap/src/nsImapMailFolder.cpp @@ +1512,5 @@ > } > } > > + nsCOMPtr <nsIDBFolderInfo> transferInfo; > + rv = trashFolder->GetDBTransferInfo(getter_AddRefs(transferInfo)); check error? @@ +1515,5 @@ > + nsCOMPtr <nsIDBFolderInfo> transferInfo; > + rv = trashFolder->GetDBTransferInfo(getter_AddRefs(transferInfo)); > + // Bulk-delete all the messages by deleting the msf file and storage. > + // This is a little kludgy. > + rv = trashFolder->Delete(); here too?

(In reply to Magnus Melin [:mkmelin] from comment #28) > But I don't see a change in what I saw earlier. I think subfolder deleting > is missing unsubscribing so therefore do not really disappear (reappear > after next restart). > > What I see is that > A) if the trash/foo folder had messages, foo.msf will still be there > B) if the trash/foo folder didn't have messages, everting, including foo.msf > will be gone, locally > > In both cases the folders will reappear after restart, since they still > exist on the server. Reappear, but apparently not usable (gray, can't file > messages into them) Hmm. I still can't replicate this here (Imap account, maildir). I wonder if there's some timing issue involved... I'll take a look at what IMAP commands it's sending. For reference, here's the procedure I'm using to try it out: 1) create a subfolder `foo` inside `Trash`. 2) copy some messages into `foo` (using ctrl+drag) 3) view one of the copied messages to force it to download 4) right click on Trash and select "Empty Trash" 5) exit the app and restart After this, it all looks fine in the gui - the contents of the trash folder are gone. Looking at the filesystem... After step 3, I see what I'd expect: Trash/ cur/ tmp/ Trash.msf Trash.sbd/ foo/ cur/ ...nnnnnnn... tmp/ foo.msf Immediately after step 4, all I see is: Trash.msf (timestamped to when "Empty Trash" was performed) Same with step 5 (the actual maildir isn't created on disk until a message or subfolder is added).

It's worth noting that my patch - despite a little refactoring - is really just picking away at the edges. The EmptyTrash path seems to cut across a lot of systems, and I think there's a lot of potential for more nasty little corner-cases to pop up (eg like the ordering issue with virtual folders, in comment 26) I can't help thinking that it should all be simpler than this, with EmptyTrash() just automatically performing the steps a user would do manually, rather than all this messing about with message databases and invoking the msgStore directly. It'd be the same code for all folder types, and the IMAP/POP/Local details would all be handled automatically. I'll take a look into this.

I suppose it's also possible this server is goofy, and many other servers unsubscribe automatically for folder delete. Or something. So let's not let this block general progress here.

New patch to tidy up the issues in comment 28. Magnus: do you see the folder-reappearing issue without this patch? If not, then I've introduced a new bug, and we should hold off applying this patch. If the issue was already there, then I'd be happy for this patch to proceed (and we should probably log another bug for the folder-reappearing)

> Magnus: do you see the folder-reappearing issue without this patch? Yes looks like the patch doesn't make it worse.

(In reply to Magnus Melin [:mkmelin] from comment #33) > > Magnus: do you see the folder-reappearing issue without this patch? > > Yes looks like the patch doesn't make it worse. hehe - a ringing endorsement if ever there was one! One day I'd like to revisit this in the wider context of firming up the nsIMsgPluggableStore/nsIMsgFolder separation of responsibilities. But for now I shall leave it there and just request application.by pestering Jorg again :-)

Has that been on a try run on all three platforms? I didn't see one, so: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=68a9025dace9bdc54646bb6d017ed1e936218cad

Oops, thanks Jorg!

Comment on attachment 9014196 [details] [diff] [review] bug1317117_proper_fix5.patch Review of attachment 9014196 [details] [diff] [review]: ----------------------------------------------------------------- Sorry about the delay here. I've seen a few iterations of this and it looks fine. Personal note: The review delay is proportional to the felt riskiness. This does make quite some deep changes, so who knows what will break ;-) ::: mailnews/base/util/nsMsgDBFolder.cpp @@ +3623,5 @@ > + // Delete the .msf file. > + // NOTE: this doesn't remove .msf files in subfolders, but > + // both nsMsgBrkMBoxStore::DeleteFolder() and > + // nsMsgMaildirStore::DeleteFolder() will remove those .msf files > + // as a side-effect of deleting the .sbd dir. Nit: I'm not a great friend of abbreviations. I'll change this to 'directory' before landing.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/930dfab9478d call msgStore folder deletion during imap/news empty trash. r=jorgk

Bugs which are confirmed New or Fixed should not be left in the untriaged component. Please move it to a more appropriate component. Thanks

Not sure "General" is the best place for it, but I didn't see a more appropriate component...

Ahh - much better. Thanks Jorg!