1293770 - POP3 delete on move does not work with 'maildir' only with 'mbox'

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, defect)

Description

[detlefpilzecker]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.82 Safari/537.36 OPR/39.0.2256.48

Steps to reproduce:

A) I set up Thunderbird (TB) Portable 45.2.0 using the account saving method 'maildir'.
I set up an POP3 mail account for email-address [e-mail].
I selected the option to delete the mail at the server if I move it from In-folder.

B) Same as above, but using the account saving method 'mbox'.


Actual results:

A) Mail wasn't deleted from server if moved or even deleted at TB.

B) Mail was deleted from server if moved in TB.


Expected results:

A) Mail should be deleted if I move it from In-folder.

B) Worked as expected.

I expect that the function works with both saving methods!!! Or a warning popup informing me about the consequences using 'maildir'.

Comment 1

[detlefpilzecker]

This curious behaviour I noticed with the freemail provider http://www.web.de. I also found it described by pop3 web.de users in some forums in the net. In non of this forums I found a solution for the problem.
Using my own mail server running Plesk I can not reproduce this behaviour. This accounts behave with TB mbox or maildir as expected.
So please test it with the popular German freemail provider web.de. Thanks.

Comment 2

[Jorg K (CEST = GMT+2)]

We have a duplicate of this where I confirmed the behaviour:
Bug 1381249 comment #1.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1293770-maildir-popstate.patch - WIP *not* working

Kent, how is this meant to work? By the looks of it, updating popstate.dat was simply *NOT* implemented at all for maildir. How is it possible to declare maildir fit for prime-time use without it??

In the patch I copied a bunch of stuff from nsLocalMailFolder.cpp to nsMsgMaildirStore.cpp. The patch compiles but it doesn't work, debug shows a continue at c1 is executed since GetUidlFromFolder() does not work for maildir storage.

So what's the way forward here? Am I mistaken that nsLocalMailFolder.cpp should be able to handle both mbox and maildir? In this case what I've done is on the right track and I still need to fix GetUidlFromFolder()?

Or should I create a
nsMsgMaildirStore::MarkMsgsOnPop3Server() and I
nsMsgMaildirStore::GetUidlFromMessage()?

Perhaps f? and NI gets me a faster response ;-(

Comment 5

[Jorg K (CEST = GMT+2)]

Reading comment #0 again, I think it's a little confused. There is no option "delete the mail at the server if I move it from In-folder." The only option is to delete it from the server if deleted from TB.

Deleting from TB is defined as being put/moved into the trash, either be hitting delete or moving it into the trash. Moving a message to a non-trash folder doesn't delete it from the server as far as I can see. So the aim is to make the system behave in the same way for mbox and maildir.

Comment 6

[Kent James (:rkent)]

(In reply to Jorg K (GMT+2) from comment #4)
> Created attachment 8886891 [details] [diff] [review]
> 1293770-maildir-popstate.patch - WIP *not* working
> 
> Kent, how is this meant to work? By the looks of it, updating popstate.dat
> was simply *NOT* implemented at all for maildir.

Updating of popstate.dat should not be the responsibility of the message store, so I would not expect it to be implemented in maildir.

> 
> In the patch I copied a bunch of stuff from nsLocalMailFolder.cpp to
> nsMsgMaildirStore.cpp. The patch compiles but it doesn't work, debug shows a
> continue at c1 is executed since GetUidlFromFolder() does not work for
> maildir storage.

You should not be adding a dependency on type of mail folder (nsCOMPtr <nsIMsgLocalMailFolder> localSrcFolder = do_QueryInterface(srcFolder); if (localSrcFolder)) in the message store. Any existing such calls are signs of poor design, and adding another one would require a VERY strong justification to add another gotcha instead of trying to figure out how to fix the design.

> 
> So what's the way forward here? Am I mistaken that nsLocalMailFolder.cpp
> should be able to handle both mbox and maildir?

Yes, nsLocalMailFolder.cpp should work with both mbox and maildir. The way forward that I would do is to figure out the call sequence for MarkMsgsOnPop3Server in the mbox case, and figure out why that does not also work in the maildir case. Currently the calls to MarkMsgsOnPop3Server are all made in either filters or nsMsgLocalMailFolder, so I would expect that the fixes are also in nsMsgLocalMailFolder.

 
> Or should I create a
> nsMsgMaildirStore::MarkMsgsOnPop3Server() and I
> nsMsgMaildirStore::GetUidlFromMessage()?

No, that does not sound like the correct direction. The message store should not have to know about server-specific issues like this.

Comment 7

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #6)
> The way
> forward that I would do is to figure out the call sequence for
> MarkMsgsOnPop3Server in the mbox case, and figure out why that does not also
> work in the maildir case.
Did that already a few days ago (and saved the result):
nsMsgLocalMailFolder::MarkMsgsOnPop3Server(nsIArray * aMessages, int aMark) Line 2954	C++
nsMsgLocalMailFolder::EndMove(bool moveSucceeded) Line 2640	C++
nsCopyMessageStreamListener::EndCopy(nsISupports * url, nsresult aStatus) Line 135	C++
nsMsgProtocol::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult aStatus) Line 340	C++
nsMailboxProtocol::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult aStatus) Line 380	C++

It's called from EndMove. But no EndMove in maildir. The maildir store handles the move by moving files around. Hence my attempt.

So how do we move forward?

Comment 8

[Kent James (:rkent)]

"But no EndMove in maildir." There also is no EndMove in mbox, so I don't understand why you would expect one in maildir. Or are you saying that endmove is not being called in maildir? If not, why not?

We move forward by understanding why the existing design is calling  MarkMsgsOnPop3Server in the mbox case but not the maildir case, and figuring out the correct change to fix that. Maybe I do not understand the question.

Comment 9

[Jorg K (CEST = GMT+2)]

Well, deleting a message in maildir doesn't call nsMsgLocalMailFolder::MarkMsgsOnPop3Server(). That's observed behaviour. I also know that if it were called, like in my patch, it wouldn't work.

I don't know why you're saying "There also is no EndMove in mbox" when I have a stack that shows that yes, in a mailbox move, nsMsgLocalMailFolder::EndMove() gets called.

Once again, maildir seems to handle moves in the nsMsgMaildirStore.cpp.

I think, "understanding why the existing design is calling  MarkMsgsOnPop3Server in the mbox case but not the maildir case" is a nice question to ask, but more interesting is the question how the whole thing hangs together, so what is the design of moving messages in the two cases, where in the maildir case we only need to move/rename a file.

Comment 10

[Jorg K (CEST = GMT+2)]

And - as I understand it - you were behind getting maildir ready for prime time (like you said: I've used it for a while in TB 38 and saw no problems), maybe you can enlighten me here.

Further to comment #9:
I also know that if it (nsMsgLocalMailFolder::MarkMsgsOnPop3Server()) were called, like in my patch, it wouldn't work ... since GetUidlFromFolder() assumes mbox.

Comment 11

[Kent James (:rkent)]

"maybe you can enlighten me here" I'm afraid I have no significant insights. I've tried to tell you what I would do to approach rying to fix the bug, but I am not volunteering to fix the bug myself.

Comment 12

[Jorg K (CEST = GMT+2)]

Let me ask differently then. What do you know about the architecture for moving messages? For the mbox case there seems to be a listener/OnStopRequest involved and code in nsMsgLocalMailFolder.cpp. Do you know how maildir was bolted onto the existing mbox code?

Anything useful. So far I'm seeing mbox move go through nsMsgLocalMailFolder::EndMove() and maildir move does its own thing in nsMsgMaildirStore.cpp. Hence my patch which manages to execute MarkMsgsOnPop3Server() but doesn't work.

"I would expect that the fixes are also in nsMsgLocalMailFolder" is a nice wish, but I can't fix a code path that only goes through nsMsgMaildirStore with changes in nsMsgLocalMailFolder.

You don't have to fix the bug, but a few people share the mailnews/ load, and I really have little time to investigate something you might already know.

Comment 13

[Jorg K (CEST = GMT+2)]

OK, here's a stack for deleting a message in a maildir store:
nsMsgMaildirStore::CopyMessages() Line 971	C++
nsMsgLocalMailFolder::CopyMessages() Line 1573	C++
nsMsgCopyService::DoNextCopy() Line 316	C++
nsMsgCopyService::DoCopy() Line 260	C++
nsMsgCopyService::CopyMessages() Line 543	C++
nsMsgLocalMailFolder::DeleteMessages() Line 1203	C++
nsMsgDBView::DeleteMessages() Line 3265	C++

So deletion is mainly a move/copy to trash.

As expected the deletion from a mbox store also goes through nsMsgLocalMailFolder::CopyMessages() calling msgStore->CopyMessages(), but the EndMove() happens async after that method has already returned.

So one approach of fixing the problem in nsMsgLocalMailFolder would be to update popstate file after the call to msgStore->CopyMessages(), but for mbox that hasn't actually completed the move to trash yet.

I haven't been able to work out how this call stack is executed for mbox only.
nsMsgLocalMailFolder::MarkMsgsOnPop3Server(nsIArray * aMessages, int aMark) Line 2954	C++
nsMsgLocalMailFolder::EndMove(bool moveSucceeded) Line 2640	C++
nsCopyMessageStreamListener::EndCopy(nsISupports * url, nsresult aStatus) Line 135	C++
nsMsgProtocol::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult aStatus) Line 340	C++
nsMailboxProtocol::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult aStatus) Line 380	C++
But then I didn't look very hard since it's 2 AM here now.

Comment 14

[Kent James (:rkent)]

"What do you know about the architecture for moving messages?"

In a general sense, there is a nsCopyRequest that is created for a copy or move, and is supposed to be the holder of state information about the move.

But the architecture is poorly defined. nsICopyMessageListener for example has no documentation on the differences between endMessage, endCopy, and endMove, and what is supposed to happen when they are called, or the order they should be called. Then we have issues of poor design leading to code like https://dxr.mozilla.org/comm-central/source/mailnews/base/src/nsCopyMessageStreamListener.cpp#125-135 that claims it is wrong, then inserts a dependency on the folder type in a copy service where it does not belong.

It is really hard to get right, and really easy to create unexpected regressions. Like most of our code unfortunately.

Comment 15

[Jorg K (CEST = GMT+2)]

Just confirming: nsMsgLocalMailFolder::EndMove() is *NOT* called for maildir deletions. So someone arranges something for mbox but not maildir. Perhaps you know who/where that something is.

Comment 16

[Jorg K (CEST = GMT+2)]

So just to make the question very clear:
Do you know or know how to find out how/where this call "cascade" is arranged for in the mbox case:

nsMsgLocalMailFolder::MarkMsgsOnPop3Server(nsIArray * aMessages, int aMark) Line 2954	C++
nsMsgLocalMailFolder::EndMove(bool moveSucceeded) Line 2640	C++
nsCopyMessageStreamListener::EndCopy(nsISupports * url, nsresult aStatus) Line 135	C++
nsMsgProtocol::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult aStatus) Line 340	C++
nsMailboxProtocol::OnStopRequest(nsIRequest * request, nsISupports * ctxt, nsresult aStatus) Line 380	C++

Quoting from comment #13:
As expected, the deletion from a mbox store also goes through nsMsgLocalMailFolder::CopyMessages() calling msgStore->CopyMessages(), but the EndMove() happens async after that method has already returned.

Comment 17

[Kent James (:rkent)]

"Do you know or know how to find out how/where this call "cascade" is arranged for in the mbox case"

No, I do not know. Figuring this out involves adding printf statements and using the debugger.

Comment 18

[Jorg K (CEST = GMT+2)]

OK, comparing deletion of a message with maildir storage gives:
nsMsgMaildirStore::CopyMessages() Line 971	C++
nsMsgLocalMailFolder::CopyMessages() Line 1573	C++
nsMsgCopyService::DoNextCopy() Line 316	C++
nsMsgCopyService::DoCopy() Line 260	C++
nsMsgCopyService::CopyMessages() Line 543	C++
nsMsgLocalMailFolder::DeleteMessages() Line 1203	C++
nsMsgDBView::DeleteMessages() Line 3265	C++

I get the same for mbox:
nsMsgBrkMBoxStore::CopyMessages() Line 773	C++
nsMsgLocalMailFolder::CopyMessages() Line 1573	C++
nsMsgCopyService::DoNextCopy() Line 316	C++
nsMsgCopyService::DoCopy() Line 260	C++
nsMsgCopyService::CopyMessages() Line 543	C++
nsMsgLocalMailFolder::DeleteMessages() Line 1203	C++
nsMsgDBView::DeleteMessages() Line 3265	C++

where nsMsgLocalMailFolder::CopyMessages() Line 1573 calls msgStore->CopyMessages().

nsMsgBrkMBoxStore::CopyMessages() returns *aCopyDone = false; whereas returns nsMsgMaildirStore::CopyMessages() *aCopyDone = true;

nsMsgLocalMailFolder::CopyMessages() on receiving 'true' exists pretty much straight away, for 'false' it kicks on and ends up calling nsMsgLocalMailFolder::CopyMessageTo(), which creates a bunch of listeners, etc. and with these calls nsMailboxService::CopyMessage() which fetches the message and like that "start request", "stop request" and "end move" are being called.

So the obvious fix is to put the popstate.dat refresh into the branch of "copy done == true".

P.S.: I didn't use a single print statement ;-)

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1293770-maildir-popstate2.patch - WIP *not* working.

OK, so this solution is in nsLocalMailFolder.cpp as you postulated.

This fails, as we know, in GetUidlFromFolder() which is called by MarkMsgsOnPop3Server(). The call to nsMsgDBFolder::GetMsgInputStream() fails in nsMsgMaildirStore::GetMsgInputStream().

More debugging to follow, but we're on the right track here, right?

Comment 20

[Jorg K (CEST = GMT+2)]

OK, the problem is that by the time I want to read the file to get the UID, it has already been moved and the access fails.

So we need to do it before the call that tells us whether the copy/move was done. Tricky.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1293770-maildir-popstate2.patch (v1).

This is working. Tested with mbox and maildir.

I've just shifted the call to MarkMsgsOnPop3Server() from EndMove() (only executed by mbox) to common code executed by mbox and maildir.

The attempt to copy the code into the maildir branch (attachment 8887633 [details] [diff] [review]) failed since the message file is already moved when we come to open it to read the UID.

Comment 22

[Kent James (:rkent)]

Comment on attachment 8887643 [details] [diff] [review]
1293770-maildir-popstate2.patch (v1).

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

The problem with your proposal is that you are setting up a delete before you know that a move succeeded. As I understand this, the user is copying messages to her local trash folder, and expects the message to be removed from the server because she has a copy of the message in trash locally. If that copy fails, then no message in trash, no message on server, and the message is lost. Sad!

I really despise working with all of this copy stuff because it is such a collection of special cases that are not well thought out. But given that caveat, it seems to me that what you want to do is to take the stuff that you added to nsMsgLocalMailFolder::CopyMessages, and only do this after if (storeDidCopy) and only if the CopyMessages succeeded from the store. Also then leave the existing stuff in EndMove for the !storeDidCopy case.

All of this special casing we are doing is a sure sign that this whole message copying API is terrible and badly needs rewriting. I'm scared we are creating new regressions when we do this.

Comment 23

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #22)
> it seems to me that what you want to do is to take the stuff that
> you added to nsMsgLocalMailFolder::CopyMessages, and only do this after if
> (storeDidCopy) and only if the CopyMessages succeeded from the store.
Looks like you didn't read my detailed comment before.

Once the msgStore->CopyMessages() call returns with success and storeDidCopy==true, the message file has been moved in the maildir case and the UID cannot be retrieved any more. Too late, mate.

What you're asking for is attachment 8887633 [details] [diff] [review] and that DOES NOT WORK. Quoting comment #21:
The attempt to copy the code into the maildir branch (attachment 8887633 [details] [diff] [review]) failed since the message file is already moved when we come to open it to read the UID.

So what's next?

Comment 24

[Jorg K (CEST = GMT+2)]

Also see comment #19 and comment #20.

Comment 25

[Jorg K (CEST = GMT+2)]

In other words: For maildir I need to process the message before they get moved otherwise I can't access them any more.

The alternative is to move the code into nsMsgMaildirStore.cpp and call MarkMsgsOnPop3Server() from nsMsgMaildirStore::CopyMessages() where I have variable 'dstHdrs' set. That's like the very first patch, attachment 8886891 [details] [diff] [review].

So three options:
1) Do the notification from nsMsgMaildirStore.cpp using 'dstHdrs', idea got r-
2) Do the notification from nsMsgLocalMailFolder::CopyMessages() before the copy, idea got r-
3) Do the notification from nsMsgLocalMailFolder::CopyMessages() after the copy: Doesn't work.

So what's next?

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8886891 [details] [diff] [review]
1293770-maildir-popstate.patch - WIP *not* working

This would work changing
localSrcFolder->MarkMsgsOnPop3Server(aHdrArray, POP3_DELETE);
to
localSrcFolder->MarkMsgsOnPop3Server(dstHdrs, POP3_DELETE);
since the original messages have been moved, so we need to access the moved ones and retrieve their UID.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 1293770-maildir-popstate.patch - original approach, working.

OK, I made the original f- patch work. So we have two working patches you don't like and one non-working patch that does what you asked for.

Comment 28

[Jorg K (CEST = GMT+2)]

To make option 3) from comment #25 work, I'd have to change nsIMsgPluggableStore::CopyMessages() and return the result messages if storeDidCopy==true. Then I can do the marking after the call.

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: 1293770-maildir-popstate3.patch (v3).

OK, so marking the messages after the copy went through. Tested and working.

Comment 30

[Kent James (:rkent)]

Comment on attachment 8887707 [details] [diff] [review]
1293770-maildir-popstate3.patch (v3).

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

OK this is a reasonable approach. r+ with my one issue fixed (but if it causes problems feel free to land as is without the fix).

::: mailnews/base/public/nsIMsgPluggableStore.idl
@@ +224,5 @@
>    boolean copyMessages(in boolean isMove,
>                         in nsIArray aHdrArray,
>                         in nsIMsgFolder aDstFolder,
>                         in nsIMsgCopyServiceListener aListener,
> +                       out nsIMutableArray aDstHdrs,

One little detail: really this should just be an nsIArray, as the list of dbHdrs would be considered immutable when received by copyMessages.

You would then remove the earlier "interface nsIMutableArray" and fix other locations. Not sure if you need a QI at the .forget or not.

Comment 31

[Kent James (:rkent)]

Comment on attachment 8887688 [details] [diff] [review]
1293770-maildir-popstate.patch - original approach, working.

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

Other patch is preferred.

Comment 32

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/c4c7c0e2a1c64291692fff85f81f41a22ffdc1ce

Landed with array changed to nsIArray as requested, which is of course the better way to pass it around. No QI required. Compiles and works.

Thanks for the quick review!

Comment 33

[Jorg K (CEST = GMT+2)]

Comment on attachment 8887707 [details] [diff] [review]
1293770-maildir-popstate3.patch (v3).

[Approval Request Comment]
Long-standing bug and missing functionality, should uplift.

Comment 34

[johannes]

Hi all,
thanks for this patch. I can confirm, it is now updating the popstate.dat after removing the mail to trash. Tested it with a the daily source code. 
But I can not evaluate [review] the quality of changes you made, I do not have this skills now.

Comment 35

[Jorg K (CEST = GMT+2)]

Comment on attachment 8887707 [details] [diff] [review]
1293770-maildir-popstate3.patch (v3).

Not doing another TB 55 beta.

Comment 36

[Jorg K (CEST = GMT+2)]

TB 52.3 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/e80e7b964bb0