436214 - Deleting an imap message offline can cause an assertion nsMsgHdr::SetThreadParent "can't be your own parent

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[David :Bienvenu]

Steps to reproduce:

1. Go offline.
2. Delete an imap message (using trash delete model)

Sometimes you'll see this assertion:

 	msgdb.dll!nsMsgHdr::SetThreadParent(unsigned int inKey=0x00000015)  Line 714 + 0x25 bytes	C++
 	msgdb.dll!nsMsgThread::AddChild(nsIMsgDBHdr * child=0x08d3ff38, nsIMsgDBHdr * inReplyTo=0x00000000, int threadInThread=0x00000000, nsIDBChangeAnnouncer * announcer=0x08c14e08)  Line 387	C++
 	msgdb.dll!nsMsgDatabase::AddToThread(nsMsgHdr * newHdr=0x08d3ff38, nsIMsgThread * thread=0x08d40830, nsIMsgDBHdr * inReplyTo=0x00000000, int threadInThread=0x00000000)  Line 3955 + 0x23 bytes	C++
 	msgdb.dll!nsMsgDatabase::AddNewThread(nsMsgHdr * msgHdr=0x08d3ff38)  Line 4129	C++
 	msgdb.dll!nsMsgDatabase::ThreadNewHdr(nsMsgHdr * newHdr=0x08d3ff38, int & newThread=0x0a4e6e38)  Line 3940 + 0x1a bytes	C++
 	msgdb.dll!nsMsgDatabase::AddNewHdrToDB(nsIMsgDBHdr * newHdr=0x08d3ff38, int notify=0x00000001)  Line 2953 + 0x18 bytes	C++
 	msgdb.dll!nsImapMailDatabase::AddNewHdrToDB(nsIMsgDBHdr * newHdr=0x08d3ff38, int notify=0x00000001)  Line 147 + 0x11 bytes	C++
 	msgdb.dll!nsMsgDatabase::CopyHdrFromExistingHdr(unsigned int key=0x00000015, nsIMsgDBHdr * existingHdr=0x0aba04d8, int addHdrToDB=0x00000001, nsIMsgDBHdr * * newHdr=0x0012bc04)  Line 3021 + 0x14 bytes	C++
 	msgimap.dll!nsImapMailFolder::CopyMessagesOffline(nsIMsgFolder * srcFolder=0x0493031c, nsISupportsArray * messages=0x07f9ba80, int isMove=0x00000001, nsIMsgWindow * msgWindow=0x0510a170, nsIMsgCopyServiceListener * listener=0x00000000)  Line 6212 + 0x58 bytes	C++
 	msgimap.dll!nsImapMailFolder::CopyMessages(nsIMsgFolder * srcFolder=0x0493031c, nsISupportsArray * messages=0x07f9ba80, int isMove=0x00000001, nsIMsgWindow * msgWindow=0x0510a170, nsIMsgCopyServiceListener * listener=0x00000000, int isFolder=0x00000000, int allowUndo=0x00000001)  Line 6337 + 0x1f bytes	C++
 	msgbase.dll!nsMsgCopyService::DoNextCopy()  Line 325 + 0x5c bytes	C++
 	msgbase.dll!nsMsgCopyService::DoCopy(nsCopyRequest * aRequest=0x07f9b948)  Line 267 + 0x8 bytes	C++
 	msgbase.dll!nsMsgCopyService::CopyMessages(nsIMsgFolder * srcFolder=0x0493031c, nsISupportsArray * messages=0x07f9b888, nsIMsgFolder * dstFolder=0x04924a84, int isMove=0x00000001, nsIMsgCopyServiceListener * listener=0x00000000, nsIMsgWindow * window=0x0510a170, int allowUndo=0x00000001)  Line 528 + 0xc bytes	C++
 	msgimap.dll!nsImapMailFolder::DeleteMessages(nsISupportsArray * messages=0x07f9b888, nsIMsgWindow * msgWindow=0x0510a170, int deleteStorage=0x00000000, int isMove=0x00000000, nsIMsgCopyServiceListener * listener=0x00000000, int allowUndo=0x00000001)  Line 2216 + 0x4f bytes	C++

I think this has to do with the fake msg header we put in the dest db (in this case, the trash). I'm not quite sure what the pattern is, but I think it has to do maybe not cleaning up the fake msg headers completely during previous runs. If I empty the trash (which blows away the db), then go offline and delete some messages, everything's ok. If I then restart and delete some more messages, the problem starts happening...

Comment 1

[David :Bienvenu]

it looks to me like we have some uncleaned up mork rows laying around, one for the message header and one for the thread. When we go to create the wrapping nsMsgHdr and nsMsgThread objects, and get the underlying mork row where we store attributes for the object, we're getting a left-over row, which has some bogus data in it. Mork is supposed to remove rows that aren't in any table, so I'm not sure what's going on. However, I bet this is the same as or very similar to the problem we have when we delete a message and then undo the delete, and sometimes get into problems.

Comment 2

[David :Bienvenu]

I think we're not cleaning up the dummy header in the dest folder until that folder is clicked on, which explains why the header and thread are still around.

Comment 3

[David :Bienvenu]

Attachment: proposed fix

boy, was I ever out in the weeds. The problem was simply that we weren't persisting the highwater mark for message keys for folders; the offline code was the only code that cared. This will fix the basic problem. I might add some code to double check that the highwater mark is really valid, though I don't want to have that be expensive...Emre, you could try the patch and see if it fixes the assertion for you. I'm not sure it's going to fix the blank line problem, however, but let me know. If not, steps to reproduce the blank line problem would be very helpful.

Comment 4

[David :Bienvenu]

I did see the blank line problem while running with this patch - it's easier to reproduce if you close the message pane, I think.

Comment 5

[David :Bienvenu]

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

I'm working on some tests for db/msgdb, but I don't want to hold this patch up on that...

Comment 6

[[e-mail]]

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

> NS_IMETHODIMP nsDBFolderInfo::SetHighWater(nsMsgKey highWater, PRBool force)
> NS_IMETHODIMP nsDBFolderInfo::SetHighWater(nsMsgKey highWater)
Eww... C++ doesn't guarantee vtable order for overloaded methods :-(

Comment 7

[David :Bienvenu]

Attachment: add tests

this adds some tests for the underlying problem. I also ended up cleaning up nsIDBFolderInfo.idl a bit while I was at it.

And thx to Neil for pointing out the problem with overloading methods in idl. It made xpcshell crash, so I reworked the idl so that we no longer overload the highwater methods.

Comment 8

[David :Bienvenu]

Attachment: remove stuff that didn't belong in previous patch

prev patch included Emre's offline work, and the start of my work on imap rfc 4551

Comment 9

[[e-mail]]

Comment on attachment 323133 [details] [diff] [review]
remove stuff that didn't belong in previous patch

>-  attribute long ImapUidValidity;
>-  attribute unsigned long Version;
>-  attribute long ImapTotalPendingMessages;
>-  attribute long ImapUnreadPendingMessages;
>-  attribute wchar IMAPHierarchySeparator;
>+  attribute long imapUidValidity;
>+  attribute unsigned long version;
>+  attribute long imapTotalPendingMessages;
>+  attribute long imapUnreadPendingMessages;
These changes look like they're from one of those IMAP patches too :-)

>+  // Create a local mail account (we need this first)
>+  acctMgr.createLocalMailAccount();
>+
>+  // Get the account
>+  var account = acctMgr.accounts.GetElementAt(0)
>+                       .QueryInterface(Components.interfaces.nsIMsgAccount);
>+
>+  // Get the root folder
>+  var root = account.incomingServer.rootFolder;
Why not acctMgr.localFoldersServer.rootFolder?

>+  var dbFolderInfo = db.dBFolderInfo;
What does this achieve?

>+  dbFolderInfo.highWater = 10;
>+  db.Close(true);
>+  db = dbService.openFolderDB(folder, true, true);
>+  do_check_true(db != null);
>+  do_check_eq(db.dBFolderInfo.highWater, 10);
You might want to consider testing onNewKey too while you're here ;-)

Comment 10

[David :Bienvenu]

> >+  attribute long imapTotalPendingMessages;
> >+  attribute long imapUnreadPendingMessages;
> These changes look like they're from one of those IMAP patches too :-)
Actually, that's just part of the cleanup of nsIDBFolderInfo - I was fixing the intercaps on the var names, and I noticed that the hierarchy delimiter stuff was never used.
> 
> >+  var dbFolderInfo = db.dBFolderInfo;
> What does this achieve?

nothing - I can get rid of the local var.
> 
> >+  dbFolderInfo.highWater = 10;
> >+  db.Close(true);
> >+  db = dbService.openFolderDB(folder, true, true);
> >+  do_check_true(db != null);
> >+  do_check_eq(db.dBFolderInfo.highWater, 10);
> You might want to consider testing onNewKey too while you're here ;-)

sure, I can do that.

Comment 11

[Emre Birol]

re comment #6

Just curious, how it affects XPCOM or xpcshell?

Comment 12

[Mark Banner (:standard8)]

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

So I think this patch has been superseded.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 323133 [details] [diff] [review]
remove stuff that didn't belong in previous patch

Index: db/msgdb/test/unit/tail_maildb.js

This file needs dropping following the changes I checked in the other day.

+  var acctMgr = Components.classes["@mozilla.org/messenger/account-manager;1"]
+                          .getService(Components.interfaces.nsIMsgAccountManager);
+
+  // Create a local mail account (we need this first)
+  acctMgr.createLocalMailAccount();
+
+  // Get the account
+  var account = acctMgr.accounts.GetElementAt(0)
+                       .QueryInterface(Components.interfaces.nsIMsgAccount);
+
+  // Get the root folder
+  var root = account.incomingServer.rootFolder;

If you want you can now use loadLocalMailAccount from http://mxr.mozilla.org/seamonkey/source/mailnews/test/resources/mailTestUtils.js

+  do_check_true(db != null);

I normally prefer do_check_neq(db, null); but I'm don't really mind either way.

r=me as long as you fix at least the first comment

Comment 14

[David :Bienvenu]

Attachment: patch for checkin

this patch addresses Neil and Standard8's comments, and is what I'll check in.

Comment 15

[Kent James (:rkent)]

I have a regression from this. You changed:

  attribute long NumUnreadMessages;
  attribute long NumMessages;

to:

  attribute long numUnreadMessages;
  attribute long numMessages;

But this change requires that you patch users of those attributes to match the case change. searchBar.js has lines like:

  dbFolderInfo.NumUnreadMessages = gNumUnreadMessages;
  dbFolderInfo.NumMessages = gNumTotalMessages;

which are now failing.

Comment 16

[David :Bienvenu]

oh geez, how awful. OK, I'll fix those. Thx for finding that.

Comment 17

[David :Bienvenu]

Attachment: [checked in]fix js consumers (TB and SM)

fix regression Kent pointed out.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 324520 [details] [diff] [review]
[checked in]fix js consumers (TB and SM)

I couldn't see any others that we may have missed either.

r=me.