323875 - Undo Delete Message (Ctrl+Z) erases messages from trash instead of restoring them

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Stefan A. Möller]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051219 SeaMonkey/1.0b
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051219 SeaMonkey/1.0b

The command Undo Delete Message (Ctrl+Z) completely erases messages from the trash folder instead of restoring them. This happens (a) if you changed the folder between delete and undo, or (b) if you have deleted more than one message at a time.


Reproducible: Always

Steps to Reproduce:
Case (a):
1. Delete a message from your inbox.
2. Change to the trash folder. The deleted message shows up there. That's ok.
3. Press Ctrl+Z. The previously deleted message disappears from the trash folder, and is supposed to reappear in the inbox.
4. Change back to the inbox folder.

Actual Results:
The message has been gone.

Expected Results:
Message should be back in the inbox again.

Case (b):
1. Delete two (or more) messages from your inbox.
2. Press Ctrl+Z twice (or more). Note: The menu entry "Undo Delete Message" is always enabled.

Actual Results:  
Only the last one of the previously deleted messages reappears within the inbox. The others are lost completely, they are neither in trash nor in inbox.


Expected Results:  
All messages should be moved back into the inbox. Or, at least, one message should be moved back into the inbox, *and* the others should stay in trash, so that they can be recovered manually.


It's an IMAP account, and I'm accessing it online.

Comment 1

[Karsten Düsterloh]

Just to narrow the scope: WFM with Linux GTK1 SM 1.0b for POP3 and Local Folders.

Comment 2

[Stefan A. Möller]

Um... I can narrow the scope somewhat further...

I inadvertently set mail.server.server1.max_cached_connections to 1 for that particular IMAP server. Raising the value to at least 2 solves the problem. So this bug seems to be invalid, actually.

Should the prefs UI allow to set max_cached_connections to a value lower than 2, then? I cannot find a corresponding bug report for such an issue (only the opposite of it: bug 245258).

Comment 3

[Stefan A. Möller]

Just did some more testing: Raising the max_cached_connections value does not always help, it just makes the problem less visible.

I'm still able to reproduce it in certain circumstances, i.e.:

1. Leave max_cached_connections to the default value 5.
2. Delete one or more messages from your sent folder (strange to say, it does only happen in sent, but not in inbox).
3. Visit several other IMAP folders (more than 5?)
4. Go to the trash folder.
5. Press Ctrl+Z.
6. Message disappeared into Nirvana.

Comment 4

[Frank Wein [:mcsmurf]]

Can you create a IMAP log as described under http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html#imap?

Comment 5

[Andrew Schultz]

==> IMAP

Comment 6

[Andrew Schultz]

confirmed with linux seamonkey trunk 2006011804

Comment 7

[Stefan A. Möller]

Attachment: IMAP log

max_cached_connections = 1

Deleted 5 messages (subjects: test 1 to test 5) from inbox.
Pressed Ctrl+Z five times.
Only 1 message restored.

Comment 8

[Stefan A. Möller]

Attachment: another IMAP log

max_cached_connections = 5

Deleted 1 message (subject: test 1) from sent.
Switched between several IMAP folders.
Switched to trash folder.
Pressed Ctrl+Z.
Message disappeared.

Comment 9

[Adam Pierce]

I can confirm this same bug happened to me. Not only did it delete the message I was trying to move out of Trash, it deleted all messages in Trash.

I am running T-Bird version 2.0.0.9 (20071031) on Windows XP-SP1. I am using IMAP and my Maximum number of connections to cache is set to 1.

Comment 10

[Wayne Mery (:wsmwk)]

Thoughts on this and bug 245258?

see also bug 209189

Comment 11

[Emre Birol]

Mac OS X 10.5 with TB version 3.0a1pre (2008041703), using gmail imap account;
Ctrl-Z doesn't have any effect. max_cached_connections is default value.  

As I understand it, in TB,  max_cached_connections is the maximum number of sessions opened to the imap server per selected folder. It means that whenever you select a folder (inbox, trash, etc..), imapincomingserver class spawns a new thread to manage this connection as long as the number of active threads/connections < max_cached_connections. When this limit is reached, imap incoming server starts to recycle the threads. Recycling a connection causes an internal expunge command on the server that makes it to delete the messages marked as "deleted" during the recently disconnected session, forever - this behavior is subject to server implementation I guess, but at least imap protocol states expunge command like this. After that point, Ctrl-Z can't undelete the messages, unless TB locally store them that I am not sure about.

In other word, IMHO, this behavior has to do with the number of "unique" folders selected and the value of max_cached_connections and possibly the server implementation of imap protocol.

** Reassigning to myself since it is related to what I am working on right now **

Comment 12

[Adam Nielsen]

Just came across this bug with Thunderbird 2.0.0.19.  My max_cached_connections is set to 2, and if I delete a message from my inbox then immediately choose Undo (without changing the folder) then the message is lost.  If I delete a message then look in the Trash folder it's there, but then pressing Undo deletes it from the Trash folder.

Comment 13

[Jens Berger]

This bug happened to me in Version 2.0.0.21 (20090302)/WinXP: Deleted multiple mails from IMAP Drafts folder, opened Trash, Edit->Undo delete, mails are gone.
max_cached_connections = 3

Comment 14

[Matt]

This is an issue with Thunderbird 3.0b2.  I can delete a message from the Inbox, then hit Ctrl-Z.  The message reappears, but then disappears again (gone forever).  My max_cached_connections is 5, expunge_after_delete is set to true, and I have deleted messages moved to a folder.

Comment 15

[David :Bienvenu]

(In reply to comment #14)
> This is an issue with Thunderbird 3.0b2.  I can delete a message from the
> Inbox, then hit Ctrl-Z.  The message reappears, but then disappears again (gone
> forever).  My max_cached_connections is 5, expunge_after_delete is set to true,
> and I have deleted messages moved to a folder.

What kind of imap server? There's a known issue with gmail where it expunges on every delete, which causes problems. There's a gmail setting that turns off the server behavior.

Comment 16

[Matt]

It's an exchange imap server.

Comment 17

[David :Bienvenu]

Stefan's log shows us doing a CLOSE when switching between selected folders. That causes an implicit expunge, which breaks UNDO.

IMAP Undo needs to learn how to copy messages back out of the trash. This is especially important for gmail, because gmail does an immediate expunge on the inbox when a message is copied to the trash (even before we try to store the deleted flag on the source message).

Comment 18

[David :Bienvenu]

This is sufficiently complicated that I may want to put this logic in nsImapProtocol - otherwise, I'd have to potentially chain several imap urls together. But basically, the logic should be something like this:

1. Check the source folder to see if the UID is still there, but with the deleted flag set. If so, remove the deleted flag, and add the deleted flag on the message in the target folder/trash. Which is what we do today, except that we don't check (and in fact, it's rather hard to do that check since we need access to the flag state on the protocol object, which may mean we'll need to select the folder, which is async).

2. If the message is no longer in the source folder, move the message from the target folder back to the source folder. If we have the UID of the message in the target folder, we can do a simple UID move. But if we don't, we need to find the message via its message-id, and then do a UID move.

Comment 19

[Adam Nielsen]

Until this is fixed, perhaps the Undo function could be disabled for IMAP accounts so we don't accidentally lose messages?

Comment 20

[David :Bienvenu]

no, I don't think so - it's been this way forever, and it works in the normal (non gmail) situation of deleting a message and then doing an immediate undo.

Comment 21

[Adam Nielsen]

Really?  But I run Courier IMAP and undo deletes my messages, and Comment #16 is using MS Exchange IMAP with the same problem.  It looks like all IMAP accounts are affected.

(Yes it's been this way forever, but I've also had this problem forever...)  It only seems to work if you're not using IMAP.

Comment 22

[David :Bienvenu]

it only affects non-gmail imap servers if you select several other different folders between the time you do the delete, and the time you do the undo, which is why I said what I said in c#20.

Comment 23

[Adam Nielsen]

Well in Comment #12 I said that it affects Courier IMAP even if I undo *immediately* without switching folders...maybe the undo is happening in a separate connection?

Comment 24

[David :Bienvenu]

ah, if you've tweaked max_connections, you'll be more likely to run into issues. But, we try to keep the inbox connection selected on the inbox, so if you've set it to 2, I'm not sure why we would close the inbox connection - a protocol log might be interesting. Part of the undo happens on a second connection, because we have to go to the target folder to add the deleted flag there...and we do fire off the urls one after an other, immediately, which might behave a little differently.

Comment 25

[David :Bienvenu]

I've abandoned the idea of doing this all in one url, because the url would need to be in the selected state in two folders (the source and destination), and that could cause us to end up with two connections to the same folder. So I'm going to try to make the imap undo txn object a url listener and chain the necessary urls to implement the logic in c#18

Comment 26

[David :Bienvenu]

Attachment: prepatory cleanup work - checked in.

I needed to make the imap undo txn object a url listener, so I wanted to use NS_IMPL_ISUPPORTS_INHERITED1, which meant I got sucked into getting rid of the IID for a concrete object, and switched over to nsRefPtr, etc. This turned out to ripple through the code a bit, so in the interest of not having a giant patch later down the road, I thought I'd get a review on the prepatory work. This shouldn't affect the way imap undo works; that's coming later.

Comment 27

[David :Bienvenu]

Attachment: propagate extra status from imap protocol code to runner of url - checked in.

I decided a more general way of handling this was to add some extra status to nsIImapUrl that code that runs imap urls can access from OnStopRunningUrl. Then, imap undo can check if subtracting the flags really succeeded (i.e., the message was still in the source folder with the deleted flag set), and if it fails, can just move the message out of the trash back to the source folder. That will be the final patch for this bug, but I want to land the first two parts before writing the third part (at which point I'll add a test case as well)

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 389750 [details] [diff] [review]
prepatory cleanup work - checked in.


>-NS_DEFINE_STATIC_IID_ACCESSOR(nsImapMoveCopyMsgTxn, NS_IMAPMOVECOPYMSGTXN_IID)
>+//NS_DEFINE_STATIC_IID_ACCESSOR(nsImapMoveCopyMsgTxn, NS_IMAPMOVECOPYMSGTXN_IID)

nit: This can just be removed now.

r/sr=Standard8 with that fixed.

Comment 29

[David :Bienvenu]

first patch checked in..

Comment 30

[David :Bienvenu]

second patch checked in.

I'm probably going to add an imap url that does a move/copy based on message-ids in order to handle the case of moving a message out of the trash in the case that the server doesn't support COPYUID (e.g., gmail)

Comment 31

[David :Bienvenu]

Attachment: wip

this actually works with GMail, and should work with servers that support CopyUID. I need to clean up the patch a bit, and comment more about what's going on, and add a unit test (the unit test will be somewhat limited by what imap fakeserver supports/doesn't support).

Comment 32

[David :Bienvenu]

I've been fighting with a fake server bug where copying a message to a folder and then setting a flag on the original message makes the flag apply to the message in the target folder. I don't see what the code is doing wrong, but I'm sure it's doing it...so the tests are holding me up, but I should be able to finish today...

Comment 33

[David :Bienvenu]

Attachment: proposed fix

this fixes the bug, and adds a test case. It also has a couple conversions of GetParentMsgFolder to GetParent, but that's harmless.

The approach is to check if the subtraction of the delete flag from the inbox fails - if so, move the message back from the dest/trash to the source/inbox folder. If the server doesn't support COPYUID, we need to remember the message-ids in the undo object.

I'd like to add a test case for the COPYUID servers, since fakeserver has that as an option. But since gmail doesn't support COPYUID, fixing the non-COPYUID case is higher priority.

Comment 34

[Mark Banner (:standard8)]

Comment on attachment 392637 [details] [diff] [review]
proposed fix

>@@ -67,51 +70,56 @@ nsImapMoveCopyMsgTxn::Init(nsIMsgFolder*
...
>-  // ** jt -- only do this for mailbox protocol
>-  if (protocolType.LowerCaseEqualsLiteral("mailbox"))
>+  nsCOMPtr<nsIMsgDatabase> srcDB;
>+  rv = srcFolder->GetMsgDatabase(getter_AddRefs(srcDB));
>+  NS_ENSURE_SUCCESS(rv, rv);
>   {
>-    m_srcIsPop3 = PR_TRUE;
>     PRUint32 i, count = m_srcKeyArray.Length();

Do we need the indent here now?

>+NS_IMETHODIMP nsImapMoveCopyMsgTxn::OnStopRunningUrl(nsIURI *aUrl, nsresult aExitCode)
>+    nsresult rv;
>+    nsCOMPtr<nsIImapService> imapService = do_GetService(NS_IMAPSERVICE_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv,rv);

nit: space after comma.

>+        // If subtracting the deleted flag didn't work, try
>+        // moveing the message back from the target folder to the src folder

nit: moving.

>+      rv = dstFolder->GetMsgDatabase(getter_AddRefs(destDB));
>+      NS_ENSURE_SUCCESS(rv,rv);

nit: space after comma

>+  // this is used when we chain urls for imap undo, since "this" needs
>+  // to be the listener, but the folder may need to also be notified.
>+  nsCOMPtr<nsIUrlListener> onStopListener;

nit: please prefix with m_ so that we know it is a member (it got me confused when I was looking at one of the functions before reading the header changes).

>diff --git a/mailnews/imap/test/unit/test_imapUndo.js b/mailnews/imap/test/unit/test_imapUndo.js

This test leaks a whole bunch of items. From a quick look at the log some DBs are being left open at least.  Is it possible to do a quick fix for that?


Also, I've just been trying to test this, on first delete I'm getting:

Warning: reference to undefined property this.lastMessage.sourceFolder
Source File: file:///Users/moztest/comm/main/tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/activity/moveCopy.js
Line: 164

this is deleting a message on a gmail imap account. I'm guessing this breaks things as undo doesn't work at all with or without the patch.

Comment 35

[Mark Banner (:standard8)]

That is undo doesn't seem to work at all on gmail imap accounts. My courier imap server seems fine.

Comment 36

[David :Bienvenu]

Is you gmail account setup to use a trash folder, or to delete immediately?

Comment 37

[Mark Banner (:standard8)]

It uses the trash folder - I do see a mail go into the trash folder when I delete it.

Comment 38

[David :Bienvenu]

This is working for me here - which gMail folder are you undoing a delete from? Inbox? All Mail? Are you doing the undo immediately after the delete (i.e., before we playback the offline operation from the delete in background) or waiting a second? Is the deleted message still in the trash? (If so, that would be an improvement over the existing code :-) )

I think the activity manager warning is an issue with the moveCopy listener.

I saw the undo fail once, when I deleted the welcome to gmail message and tried to undo it. For every other message, undo has worked.

I'll look at the test leak. In the meantime, if you could generate an imap protocol log of a gmail undo delete failing, that would be helpful, thx!

Comment 39

[David :Bienvenu]

Attachment: fix addressing comments

this fixes the leaks, and addresses the other comments, except for the failure to undo with gMail, which I can't reproduce here.

There were at least two causes for the leaks - one was the offline sync object getting leaked when there was no playback to do, an other had to do with the pending playback operations for servers (like fakeserver, by default) that don't do COPYUID.

Re undo not working with gMail w/ or w/o the patch, the basic reason is that gMail does an expunge after every delete, which means we have to get the message out of the trash, which is what this patch does. So a protocol log of the failure to undo a delete w/ this patch might give me a clue as to where things are going wrong.

Oh, the change to nsMsgDatabase.cpp is debug-only; it helps me figure out which db's are actually leaking by fixing the loop variables to catch the case of just one db open.

Comment 41

[Mark Banner (:standard8)]

Comment on attachment 395424 [details] [diff] [review]
fix addressing comments

As discussed over irc the undo issue that I had seems just to be a folder display update issue on the trash folder.

This seems to work fine so r/sr=Standard8.

Comment 42

[David :Bienvenu]

fix checked in. I'll quickly try to recreate the issue with the trash folder and file a bug if I can reproduce it.

Comment 43

[David :Bienvenu]

 Bug 511646 filed for undo with trash selected issue.

Comment 45

[amorim.andre.89]

This bug is still here. I installed Thunderbird yesterday, fresh install and the bug still occured. Created a new thread as this one is 12 years old and is closed.
bug 1752569