457052 - try to repair cause of assertion - Borked message header

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[David :Bienvenu]

Attachment: proposed fix

Dan, especially, has been seeing a lot of these. The problem is that a message's thread parent isn't valid, i.e., it's not in the thread. The upcoming patch attempts to repair this situation, and changes the assertion to a warning.

The assertion appears harmless, but I think in some situations it could cause us not to display sub-messages in threaded view (not sure, though).

This patch repairs the problem for me. I still have to figure out why this is happening, however.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

I believe we decided we wanted this for 3.0b1; marking as such.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Running with this patch, I'm seeing a small number of the "Thread is a parent of itself" assertions (I was seeing them before, too, I think).  Should that really be an assertion, or should it be similar to the problem already solved in this patch and warn and rebuild the index instead...

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

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

This is making the method big enough that it's becoming difficult to digest.  How about splitting it apart?

Comment 4

[David :Bienvenu]

Attachment: proposed fix - checked in

ok, this moves the code into a new method. I don't have a test case anymore, though, so I can't swear that this works. Dan, can you try this out and see if you hit the code, and it fixes the problem?

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #1)
> I believe we decided we wanted this for 3.0b1; marking as such.

Correcting flag to get it onto the right radar.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 340749 [details] [diff] [review]
proposed fix - checked in

This looks ok to me.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 340749 [details] [diff] [review]
proposed fix - checked in

* It would be good to track stuff that should just warn and trigger database rebuilds once rkent's lossless-database-rebuild stuff is in, such as presumably the "Thread is parent of itself" assertion (and maybe the "no root hdr" assertion too?).  Please file a spinoff bug, if you would...

>+      nsMsgHdr* curMsgHdr = static_cast<nsMsgHdr*>(curHdr.get());      // closed system, cast ok

Please reformat so that this wraps at 80 columns.

sr=dmose

Comment 8

[David :Bienvenu]

fix checked in.

I'll file a spin-off bug, but I'm leaning towards repair as opposed to rebuilding.

Comment 9

[David :Bienvenu]

Attachment: fix some other related issues - checked in

This fixes a few other borked thread issues:

1. If a message is marked as the thread root key, but doesn't have a thread parent of -1, make GetRootHdr return the correct header (which will correct the thread root key)

2. If a message is in a thread object, but doesn't have its thread id set to the thread, correct it.

3. If a message is its own parent, reparent it in the thread.

4. In nsMsgThread::ReparentMsgsWithInvalidParent, make sure we don't set our parent to ourselves.

I'm not sure the last change is actually needed. The other changes fix the test cases I've got, I believe. I'll double check that.

Comment 10

[David :Bienvenu]

re-opening, and putting in beta 1.

Comment 11

[Ben Bucksch (:BenB)]

See bug 272527, which bienvenu thinks is fixed by the patch here.
There's an old attachment with an INBOX.msf, too.

Comment 12

[[e-mail]]

Comment on attachment 343621 [details] [diff] [review]
fix some other related issues - checked in

>+            PRInt32 resultIndex;
>+            nsMsgKey rootKey;
>+            nsCOMPtr <nsIMsgDBHdr> root;
>+            GetRootHdr(&resultIndex, getter_AddRefs(root));
>+            root->GetMessageKey(&rootKey);
sr=me if you replace this and just use m_threadRootKey instead.

Comment 13

[David :Bienvenu]

fix checked in.