475570 - Messages deleted while offline are not deleted from server when going online

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Martin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; sv-SE; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shredder/3.0b2pre

In my IMAP account there are two email messages. One in Inbox and one in Inbox\SomeOtherFolder. If I go offline, delete both of them and then go back online, only the message in Inbox\SomeOtherFolder is deleted. The message in the Inbox disappears from the list in Thunderbird but Thunderbird never tells the IMAP server to set the \Deleted flag.

Reproducible: Always

Steps to Reproduce:
0) Make sure one message exists in the Inbox and one in Inbox\SomeOtherFolder.
1) Start Thunderbird
2) Select File->Offline->Work offline.
3) While in offline mode, I delete the two messages (by selecting them and pressing Delete on the keyboard)
4) De-select File->Offline->Work offline
Actual Results:  
Only the message in Inbox\SomeOtherFolder is deleted.

Expected Results:  
I expect that the message in the Inbox would be deleted as well.

- I've tested with version 2.0.0.16 (20080708), 2.0.0.17-pre and 2.0.0.20-pre as well and got the same result.
- I understand it's somewhat irrelevant, but when doing the same thing using Outlook Express it works fine, which would indicate that my brain isn't entirely broken and that the message file can actually be deleted.
- The attachment offline_logs.txt contains server logs for the individual steps mentioned under "Steps to reproduce". The log shows that Thunderbird makes no attempt to delete the message in the inbox when going online.
- The attachment (in the end) also contains a log snippet from Thunderbird when the problem occurs. When the problem occurs, Thunderbird reports "failed creating protocol instance to play queued" into the log file. Not sure if it's relevant but it sounds suspicious.

Comment 1

[Martin]

Attachment: Log files from the IMAP server and Thunderbird when the problem occured

Comment 2

[hansen]

Can you test again with the newest nightly? There were some checkins around that time for offline IMAP.

Comment 3

[Wayne Mery (:wsmwk)]

Martin, your retest will help.  
 http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-trunk/

Comment 4

[Kent James (:rkent)]

I'm seeing this quite reliably in my recent TB3 / 1.9.1 debug builds. My STR are simpler:

1) Have a single message in an IMAP folder (in my case, in a subfolder of the inbox. The original message was copied from the INBOX. Keep messages offline is selected for the server)

2) Start TB, selecting Work Offline at startup

3) Delete the message in the subfolder.

4) Quit TB (I always do it fairly quickly, not sure if that matters)

5) Restart TB online

6) Select the folder from which the email was deleted offline.

Expected: email is gone
Actual: email is there.

I don't normally use offline, I was simply testing it while working with bug 340886. But these tests are on a build without any of my patches.

Comment 5

[David :Bienvenu]

I'm going to take this and mark it blocking 3.0 b4 because Standard8 says he can reproduce it. I'll try to look at it for b3, but no promises.

Comment 6

[David :Bienvenu]

I can reproduce this - I hope the fix for bug 480090 didn't regress this - it landed on 6/8, which makes it seem unlikely, though possible, depending on what rkent meant by recent debug builds...

Comment 7

[David :Bienvenu]

The other possibly related bug is Bug 364082

Comment 8

[David :Bienvenu]

Kent's STR don't quite work/fail anymore because of tab session restore. A step 3.5 needs to be added, to select a different folder before shutting down.

Next, the lite select that offline playback attempts is failing, at logon time (perhaps due to some of the recent imap password/logon changes). We note this error and advance to the next folder, but the offline sync playback machine doesn't clear m_currentDB when advancing to the next folder. This causes a mismatch between the uid validities between the folder and db (because they're not for the same folder), which causes us clear all the offline ops from the db, which is pointed at the folder we failed to logon to.

So I'll fix the latter problem first, and then fix the logon problem, which is fortuitous in one sense, since it made the latter problem easy to reproduce, but pretty painful in all other senses.

Comment 9

[David :Bienvenu]

Attachment: fix handling of error running offline playback url

When advancing past the current folder because of an error running the url, we need to clear out the m_currentDB so we'll re-open it with the next folder.

Now I need to figure out why the logon is failing - unfortunately, it's pretty specific to startup, which makes it challenging to develop a unit or mozmill test.

Comment 10

[David :Bienvenu]

Attachment: fix logon issue

I suspect this was a regression from some of the password fixes.

It should be OK not to have a msgWindow when trying to get a password. We can get it from the password manager for users who have remember password turned on.

This could fix a few issues - w/o this fix, I believe any imap url that runs w/o a msg window will fail if the user hasn't clicked on a folder in that account.  This would include biff, autosync, and offline playback.

I'm treating the case where trying to get a password when we need to prompt, but there's no msg window, as an error. I think that should be OK...

The third part of this fix is going to be to make liteSelect for offline playback pass in a msg window, so it isn't vulnerable to these kinds of bugs.

Comment 11

[David :Bienvenu]

Attachment: make liteSelect take a msgWindow

pass in a msg window (can be null) to liteSelect, and add comments to idl.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 387095 [details] [diff] [review]
fix logon issue

> nsresult nsImapProtocol::GetPassword(nsCString &password)
> {
>   nsCOMPtr<nsIMsgWindow> msgWindow;
> 
>   if (password.IsEmpty() && m_imapServerSink &&
>       !(m_useSecAuth && GetServerStateParser().GetCapabilityFlag() & kHasAuthGssApiCapability))
>   {
>-    nsresult rv = GetMsgWindow(getter_AddRefs(msgWindow));
>-    if (NS_FAILED(rv)) return rv;
>+    GetMsgWindow(getter_AddRefs(msgWindow));

Whilst you're here can you move the nsCOMPtr msgWindow inside the if, no point in creating it if we're not going to use it.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 387113 [details] [diff] [review]
make liteSelect take a msgWindow

>   nsIURI playbackOfflineFolderCreate(in AString folderName, in nsIMsgWindow aWindow);
>-  void liteSelect(in nsIUrlListener aUrlListener);
>+  /**
>+   * Select this folder on the imap server without doing a sync of flags or

nit: blank line before comment please.

>+  nsCOMPtr<nsIImapService> imapService = do_GetService(NS_IMAPSERVICE_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv,rv);

nit: space after comma.

>@@ -217,16 +217,17 @@ nsresult nsImapOfflineSync::AdvanceToNex
...
>+  m_currentDB = nsnull;

(already reviewed in the first patch).

Comment 14

[David :Bienvenu]

nits addressed, all three patches landed.