435259 - playing back offline delete of unread IMAP message gets unread count in Trash wrong

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[David :Bienvenu]

Thx to Emre for pointing this out - I never noticed it before.

Sample steps to reproduce:

1. Go offline
2. Hide message pane (f8)
3. Select and delete an unread imap message
(at this point, the trash shows the correct unread count, i.e., 1 more than it was)
4. Go back online
At this point, the offline delete will get played back, and the unread trash count will revert to what it was before the whole process. Selecting the trash folder corrects the unread count.

Here's the stack trace where the count reverts back to the wrong value (found by just setting a breakpoint on nsDBFolderInfo::ChangeNumUnreadMessages)

 	msgdb.dll!nsDBFolderInfo::ChangeNumUnreadMessages(int delta=0xffffffff)  Line 516	C++
 	msgdb.dll!nsMsgDatabase::DeleteHeader(nsIMsgDBHdr * msg=0x07d78298, nsIDBChangeListener * instigator=0x00000000, int commit=0x00000000, int notify=0x00000001)  Line 1734	C++
>	msgdb.dll!nsMsgDatabase::DeleteMessage(unsigned int key=0x00000011, nsIDBChangeListener * instigator=0x00000000, int commit=0x00000000)  Line 1675 + 0x24 bytes	C++
 	msgimap.dll!nsImapOfflineSync::ProcessNextOperation()  Line 810	C++
 	msgimap.dll!nsImapOfflineSync::ProcessNextOperation()  Line 987	C++
 	msgimap.dll!nsImapOfflineSync::ProcessNextOperation()  Line 943	C++
 	msgimap.dll!nsImapOfflineSync::ProcessNextOperation()  Line 935 + 0xd bytes	C++
 	msgimap.dll!nsImapOfflineSync::ProcessNextOperation()  Line 928 + 0xd bytes	C++

I think we're deleting the "ghost" header that the offline operation created as a placeholder for the real message that we'll only find out about when we download headers for the trash folder. And since that ghost header is unread, we decrement the unread count for the folder. I'm not sure what the fix is. Since we're deleting the "ghost" header w/o updating the folder, it seems like we shouldn't be tweaking the counts at all.

Comment 1

[David :Bienvenu]

I'm thinking the correct way of doing this is to use the pending counts (total and unread) - so if we delete a fake header for an unread message, we increment both the total and unread pending counts for the folder. (This is what we do when you move/copy an imap message to an other folder - we don't download the headers for the dest folder, we just note that it now has one more message than we really know about). The nice thing about the pending counts is that they're cleared when the folder is synced with the server. My one worry is the scenario where we're playing back offline events after we've synced the folder, in which case adjusting the pending counts would be wrong. I'm not sure that scenario happens, but I need to check.

Comment 2

[David :Bienvenu]

Attachment: proposed fix

as I described above, this patch uses the pending count to correct the unread and total counts for imap folders after playing back offline events. I put the methods in nsIMsgImapMailFolder.idl because they're really only useful for imap. News also uses pending counts, but I don't think anyone's going to need to tweak them like this for news. If I'm wrong, we can move the methods; I just didn't want to add yet more methods to nsIMsgFolder.

Comment 3

[Mark Banner (:standard8)]

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

This seems to have been checked in already, but it looks fine and I couldn't break the checked in version.

Comment 4

[David :Bienvenu]

d'oh, sorry, looks like I got confused about which bugs I got an r/sr from Neil and which I didn't.