90215 - M092 & Trunk crash pressing "n" in thread pane [@ MSVCRT.dll - morkNode::CutWeakRef]

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

This has been around since at least 0.9.1, and is still present on the 0.9.2
branch as well as the trunk.  I've got a particular mail folder where all the
messages except one are marked read.  I use the threaded view, and the one
unread message is a child in a collapsed thread.  When I open the folder and
press n, which should take me to the unread message, instead it crashes.  Top of
the stack trace is attached; the full stack is _extremely_ deep (at least 1500
frames, perhaps a lot more), some sort of incredibly deep recursion in
ListIdsInThreadOrder.  Note that the mailbox in question only has about 50
messages in it.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: top of the stack trace

Comment 2

[David :Bienvenu]

your mail folder database is corrupt - you'll need to delete it. If after the db
gets recreated, you still see this crash, then I'll look at it, but this db
corruption is an old bug that was fixed between 0.9.1 and 0.9.2 and regenerating
it should fix it.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Corrupt or not, the code should cope with this condition more gracefully than
just crashing.  Folks upgrading from previous versions (perhaps including NS
6.0x?) who have corrupt databases won't have any idea what to do.  

Comment 4

[David :Bienvenu]

Attachment: patch to try since I can't reproduce this

Comment 5

[David :Bienvenu]

Dan, can you apply the attached patch in mailnews/base/src and try it out? I
can't get a corrupted db anymore. The patch just checks if we've found more msgs
in a thread then we thought there were, and if so, invalidates the db and errors
out. Since I can't test this code, I don't know if it will work. I know that I'm
not hitting the assert so I don't think it will cause any problems with db's
that are not corrupt.

Comment 6

[David :Bienvenu]

And, Dan, you could either attach the corrupt .msf file to this bug, along with
the folder it is belongs to, or send them to m privately. Assuming you haven't
deleted the .msf file, of course. Assuming it's not an IMAP folder, that is,
though I might be able to  fake my way through with just the .msf file if it was.

Comment 7

[David :Bienvenu]

Seth and Navin, can I get reviews for this patch? Lisa sent me a corrupted news
database and I verified that we don't crash and that the next time you open the
newsgroup, all the headers are re-downloaded and the db is no longer corrupt.
Thanks.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Excellent; thanks Lisa & David!  As David guessed, I made the dumb move of
deleting my .msf file already, so I can no longer verify.  

Comment 9

[Navin Gupta]

makes sense, r=naving. 

Comment 10

[(not reading, please use [e-mail] instead)]

looks good, two minor comments / questions:

1)

+  threadHdr->GetNumChildren(&numChildren);

do we want to check the return result of that?

rv = threadHdr->GetNumChildren(&numChildren);

2)

+  while (NS_SUCCEEDED(rv) && NS_SUCCEEDED(rv =
msgEnumerator->HasMoreElements(&hasMore)) && (hasMore == PR_TRUE))

"&& (hasMore == PR_TRUE)" should be "&& hasMore"

sr=sspitzer

Comment 11

[David :Bienvenu]

GetNumChildren is just an accessor for a member variable - it won't fail.  I'll
change the hasMore part. Thanks for the review.

Comment 12

[David :Bienvenu]

fix checked in.

Comment 13

[lchiang]

wonderful, david!

Jay - can you see if this stack trace is a topcrash?  
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=38533

Thanks

Comment 14

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 89506 has been marked as a duplicate of this bug. ***

Comment 15

[Jay Patel [:jay]]

Adding topcrash keyword and Trunk & M092, [@ MSVCRT.dll - morkNode::CutWeakRef]
to summary for tracking. 

Lisa, this indeed is a topcrasher for Mozilla 0.9.2 and I also found a few 
crashes on the Trunk. I'll keep any eye out for anymore of these crashes to see 
if we can verify the fix.  I'm pretty sure this crash is also happening on the 
branch, but since we don't have a lot of Talkback data on those builds, it's 
hard to know for sure.  stephend, could you try to reproduce this with builds 
prior to the fix and then see if you can verify the fix with the latest builds 
on the trunk and branch?  If you crash, please enter the bug number somewhere so 
I can easily find it in the Talkback reports...thanks.

Comment 16

[Jay Patel [:jay]]

Changing OS to All since the majority of the Talkback data shows this crash 
occurring on Win32 builds.

Comment 17

[lchiang]

thanks, Jay.  I'm pretty sure this crash is on the branch also as I had reported 
a similar bug using branch.  (David - pls correct if I'm wrong.)

David and Scott P.  Since this is a topcrash, how do you feel about the risk 
factor of the fix to see if we can get this into the branch?  If ok, then I will 
try to run this by the rest of PDT.  Thanks.

Comment 18

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 88471 has been marked as a duplicate of this bug. ***

Comment 19

[scottputterman]

It looks like none of my changes to this bug yesterday made it.  Stephen, can
you test around this bug (it sounds like we don't have a reproduceable case, but
we should at least make sure that nothing else has broken).  Adding the nsbranch
as a candidate.

Comment 20

[[e-mail] (gone - use [e-mail] instead)]

Disclaimer: I think this bug might be slightly more complex (or require numerous
DB/msf syncs, etc.).  The limited testing that I did was as follows (and please
let me know if there are additional steps, or if I should omit some.)  

1.  Marked all of netscape.test read (news.mozilla.org).
2.  Replied to a thread's top level posting.
3.  Did a "GetMsg" to get the new posting.  (Didn't read the posting.)
4.  Did a "ReplyTo" on the posting, thereby creating a child of the child I
created in step #2.
5.  Now that we are 2 levels deep in hierarchy, went back to the newsgroup with
the 2 unread messages and used View | Messages | Thread with Unread, and pressed
n twice, effectively reading both of the sibling messages that I created in
steps #2 and #4.

The truly 100% method of verifying this fix is to check with people (like
Bradley Baetz, Dan Mosedale, etc.) and to also check the Talkback reports
(assuming people who crash with this stack are using Talkback for those
instances.)  Anyway, I'll go ahead and say that my rudimentary testing is done
(for now.)

Trunk builds:

Mac OS 9.1 - 2001-07-13-09
Windows 2K - 2001-07-13-04
RedHat 7.1 - 2001-07-13-08

Comment 21

[scottputterman]

In today's PDT meeting, lchiang mentioend she has a reproduceable case. Lisa,
can you try this out on the trunk?

Comment 22

[[e-mail] (gone - use [e-mail] instead)]

Well, since it's impossible to reply to a message without selecting it (and
therefore reading it ;-)  I meant that I changed the diamond icon in the
threadpane (I do this automatically, so much so that I don't think about it,
sorry about that.)  So basically, I've altered by hand the unread status of
those two messages that I replied to.  But that shouldn't (and didn't in this
case) cause me to crash.

Comment 23

[lchiang]

My reproducible case is for bug 
http://bugzilla.mozilla.org/show_bug.cgi?id=86016 which David says has the same 
fix as this bug report.  Here are my findings:

1.  2001-07-13-06-0.9.2 branch build.  Selecting the newsgroup for my particular 
case as described in bug 86016 causes a crash.
2.  2001-07-13-19-trunk build.  Selecting the newsgroup for my particular case 
as described in bug 86016 no longer crashes.  The news messages appear in the 
thread pane.

David, I did have one question though:  I went back to the branch build, 
following the correction of the .msf file in step 2 by your fix, and the crash 
still occurs.  Is this expected if the fix were on the trunk only?  thanks.

Comment 24

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 90812 has been marked as a duplicate of this bug. ***

Comment 25

[[e-mail] (gone - use [e-mail] instead)]

I've run through Lisa's steps on:

Windows 2000 - 2001-07-15-08
MacOS 9.1 - 2001-07-13-09
RedHat 7.1 - 2001-07-13-21

I ran into no crashers, or any other odd occurances.  Still, I'd like to wait a
while and keep checking this stack on the Talkback server.  Reason being, many
of the crashes we have seen were with older profiles.  If this fix on the trunk
works, even if they get a corrupted .msf, we will rebuild it, and they shouldn't
crash.  

Comment 26

[David :Bienvenu]

Lisa, to answer your question, I belive it means there still is some sort of
threading problem that's not fixed, that causes this problem. I think we
suspected this because you, among others, reported that they could recreate this
crash with builds after my previous threading fix. The trick is for me to
reproduce this and get a minimal case. I could not reproduce it on the newsgroup
you mentioned - how many headers did you download again?

Comment 27

[lchiang]

100, I believe.  Marking rest as read.
I can show you on Tuesday.

Comment 28

[selmer (gone)]

PDT+

Comment 29

[David :Bienvenu]

fix checked into branch as well (sorry, I haven't a clue what keyword/status
whiteboard mumbo jumbo I should use to indicate that this is now fixed on the
branch)

Comment 30

[lchiang]

You don't need to know all the keyword stuff, david :-)

mark vbranch for verification on branch since stephen has already tested on trunk.

Comment 31

[[e-mail] (gone - use [e-mail] instead)]

I've run through the testcase that Lisa provided, as well as ad-hoc testing of
my own, along with the comments in this bug.  Still no crashing.  Keeping my
fingers crossed though, and I'll wait a while before I verify this.

Builds:

2001-07-20-06-0.9.2 Windows 2000
2001-07-20-03-0.9.2 Mac OS 9.1
2001-07-20-04-0.9.2 RedHat Linux 7.1

Comment 32

[[e-mail] (gone - use [e-mail] instead)]

I did a stack trace search on http://climate (internal Talkback servers) for
MSVCRT.dll - morkNode::CutWeakRef and found nothing.  Since I've already checked
this on both trunk and branch, and was just waiting to check Talkback, I'm
confident that this is now fixed.  Certainly will file again if I/others see this.

Comment 33

[Jay Patel [:jay]]

stephend, if you run a talkback query for the string "MSVCRT.dll -
morkNode::CutWeakRef", you will never get any data back...since the "-
morkNode::CutWeakRef" part is the second frame in the stack for the stack
signature "MSVCRT.dll".  In the future, the easiest way to verify crashes like
this (.DLL and .so crashes), just look at the detailed topcrash report and click
through all the stack traces for the given stack signature and see if the second
(sometimes 3rd or 4th) frame in the stack isn't there.  If you need more
clarification, contact me offline.