107369 - Crash in M096 Trunk [@ nsMsgThreadedDBView::ExpandAll]

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P1)

Description

[scottputterman]

using the 10/26 build on Win 2000 I crashed twice trying to open
n.p.m.mail-news.  The top of the stack looks like:

nsMsgThreadedDBView::ExpandAll()
nsMsgThreadedDBView::InitSort()
nsMsgThreadedDBView::InitThreadedView()
nsMsgThreadedDBView::Open()

Looking at talkback, I see this happening on the 10/28 build as well. Here are
some talkback ids:

  37348018
  37347898    
  37349090
  37255134

Comment 1

[David :Bienvenu]

Adding Navin in case this might be fallout from the quick search.

Comment 2

[R.K.Aa.]

on linux it crashes every time i click a twisty in mailnews
(workaround: double-click on account-text instead)

2001102821. Still crashes in a fresh cvs.

Comment 3

[scottputterman]

This seems to have hit a bunch of people over a few days and then stoppped. 
Let's keep an eye on it.

Comment 4

[Bernd]

I just filed several talkbacks with 2001-11-09-09 build under Win98. I changed 
my header structure yesterday. I removed the flags and added the size. It 
chrashes at 

PRUint32	flags = m_flags[i];

this is what I have inside m_flags, m_pData is NULL while the size is 4251.

-	m_flags	{...}
-	__vfptr	0x040dc06c const  nsUInt32Array::`vftable'
	[0]	0x04092bdf nsUInt32Array::`vector deleting destructor'(unsigned 
int)
	m_nSize	4251
	m_nMaxSize	0
	m_nGrowBy	0
-	m_pData	0x00000000
		CXX0030: Fehler: Ausdruck kann nicht ausgewertet werden

Comment 5

[David Einstein]

This is caused, at least on my machine when in
nsMsgThreadedDBView::InitThreadedView AddKeys is called with numListed 0.  This
clobbers the m_keys, m_flags, and m_levels arrays.  I am not sure whether this is
caused by a problem in my mail db or not.  

Comment 6

[David :Bienvenu]

Looks like Cavin's change for speeding up view loading. Re-assigning.

Comment 7

[David Einstein]

Attachment: A patch that seems to fix things

Warning:  There is not a line of code in the vicinity of this patch that I
would swear in a court of law that I knew exactly what it did.
It is clear to me that calling AddKeys with no keys to add doesn't quite do
what is expected of it, but I am guesssing that nothing is the right thing to
do.
It works on my machine. YMMV.

Comment 8

[Cavin Song]

We should return right away in AllocateSpace() if nNewSize is set to 0. This 
will avoid calling SetSize(0) which removes/initializes all elements 
(i.e., m_pData=null). A patch is coming.

Comment 9

[Cavin Song]

Attachment: Check nNewSize=0 in AllocateSpace().

Comment 10

[David Einstein]

I agree that allocate space should not clobber the array when passed 0.  It
places the array in an inconsistent state. However, on further consideration I
think that we are passing AllocateSpace the wrong value.  Where we have

  m_keys.AllocateSpace(numKeysToAdd);

we should have

  m_keys.AllocateSpace(numKeysToAdd+m_keys.GetSize());

since we want to allocate space for numKeys to add more keys.

On my machine AddKeys is first called with numKeysToAdd = 400  AllocateSpace
allocates space for 400 keys and AddKeys adds 1311 keys (additional space
allocated as needed).  AddKeys is called again with numKeysToAdd = 400; the call
to allocate space just sets m_nSize to 400 and back to 1311 add keys then adds
another thousand or so keys.  What I think that we wanted to do is allocate
space for 400 new keys.  

Comment 11

[David Einstein]

Attachment: Allocate _additional_ space.

See above

Comment 12

[Cavin Song]

Agree and another patch is coming.

Comment 13

[Cavin Song]

Attachment: Added current size to AllocateSpace() calls.

Comment 14

[Jay Patel [:jay]]

Adding topcrash keyword and Trunk [@ nsMsgThreadedDBView::ExpandAll] to summary
for tracking.  This has been a topcrasher with recent MozillaTrunk builds. 
Here's the Talkback info in case anyone wants to take a look:

nsMsgThreadedDBView::ExpandAll   37 
BBID range: 37710158 - 37908144
Min/Max Seconds since last crash: 5 - 81734
Min/Max Runtime: 20 - 201744
Crash data range: 2001-11-07 to 2001-11-12
Build ID range: 2001110612 to 2001111205
Keyword List : mail(19), start(5), 
Stack Trace: 

	 nsMsgThreadedDBView::ExpandAll
[d:\builds\seamonkey\mozilla\mailnews\base\src\nsMsgThreadedDBView.cpp  line 405]
	 nsMsgThreadedDBView::InitSort
[d:\builds\seamonkey\mozilla\mailnews\base\src\nsMsgThreadedDBView.cpp  line 507]
	 nsMsgThreadedDBView::InitThreadedView
[d:\builds\seamonkey\mozilla\mailnews\base\src\nsMsgThreadedDBView.cpp  line 162]
 
 	Source File :
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/src/nsMsgThreadedDBView.cpp
line : 405
     (37908144)	Comments: Switching mail accounts.
     (37871977)	Comments: Trying to get to my pop mailbox.I was looking at my IMAP account 
and switched to another account using a pop server.
     (37820403)	URL: going to n.p.m.builds
     (37820403)	Comments: next mail crash
     (37820384)	URL: going to n.p.m.builds
     (37820384)	Comments: can't open mail without a crash
     (37820371)	URL: going to n.p.m.builds
     (37820371)	Comments: crash on mail lunch
     (37820341)	URL: going to n.p.m.builds
     (37820341)	Comments: 1. start mozilla2. hit the mail button to start mail3. select the
tinderbox link4. the mail window opens5. crash
     (37812087)	Comments: Selecting SENT folder
     (37812068)	Comments: Changing folder
     (37810658)	Comments: clicking on Inbox in mail
     (37796097)	Comments: getting to my mail account
     (37792700)	Comments: Deleted an email message and switched to another folder
     (37790587)	Comments: trying to get to my pop mailbox
     (37762533)	Comments: Crash on mail startup
     (37762429)	Comments: Crashed starting mail
     (37762364)	Comments: Crashed starting mail
     (37744627)	Comments: switchin mail
     (37744367)	Comments: switching to my pop mail server
     (37743546)	Comments: Opening Mail & News.
     (37741143)	Comments: Opening Mail&News
     (37733335)	Comments: The application crashed when I started it after the installation
     (37730262)	Comments: I loaded my second profile into browser mode first  then clicked
onthe Mail/Navigator button to get to my second set of email and thiscrash came
up.  My normal profile which uses Modern Theme worksfine  however this profile
is using Classic Theme.
     (37717132)	Comments: trying to get mail

Comment 15

[Cavin Song]

Seth and David, can you review the patch so that I can submit it soon? Thanks.

Comment 16

[David :Bienvenu]

Comment on attachment 57693 [details] [diff] [review]
Added current size to AllocateSpace() calls.

r=bienvenu (the other alternative is just to take out these calls to
AllocateSpace completely since they're not helping performance in any
measurable way)

Comment 17

[David Einstein]

Attachment: Allocate the right amount of space

OK, I'm beginning to comprehend this.  I found bug 74955 which desribes the
reasoning that led to this bug.  Unfortunately, because pop mailboxes are
threaded (god knows why, or how,) but not displayed that way we end up
inserting many more headers than threads.  Since  ListThreadID's computes (and
we ignore)  the number of headers it seems that we want to allocate enough
space for what we are going to store.  The attached patch accomplishes that. 
It may in some situations allocate too much space, but I haven't been able to
make it do so.	On loading my pop mailbox this reduces ~17 allocations to 2.
Luckily AddKeys is not part of an XPCom interface.

It would also seem to rename AllocateSpace to AllocateMoreSpace (or something
like that) and pushing the additon of size into the renamed function. YMMV.

Comment 18

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

Comment on attachment 57693 [details] [diff] [review]
Added current size to AllocateSpace() calls.

sr=sspitzer

Comment 19

[Cavin Song]

David Einstein, can you file a separate bug for the issue of not allocating the 
right amount of space for the headers to be added?  It seems to be a different 
issue than this one and we can close the crash bug first.

Comment 20

[David Einstein]

Extraneous stuff moved to new Bug 110121 . Feel free to dup it into bug 74955 if
that feels more appropriate.  

Comment 21

[Cavin Song]

Fix checked in.

Comment 22

[laurel]

Works OK for me using nov19 commercial trunk build: win98, mac OS X, linux rh6.2

Comment 23

[David :Bienvenu]

Hmm, see TB incident 38326667 - it's the same stack trace, and occurs in a build
after 11/13, 2001112003 (though perhaps that's a .9.6 branch?) Never mind, perhaps. 

Comment 24

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

Attachment: Talkback results (all branch)

Comment 25

[David Einstein]

Would it be possible to get a stack trace? 
Has this been going on consistently?

Comment 26

[David :Bienvenu]

Cavin, this was never checked in on the branch, was it?

Comment 27

[greer]

Reopening. M096 has 115 incidents with this signature. I looks like this was 
not checked into the M096 branch. (See comment #26 above). We need to reverify 
with M097.

Comment 28

[scottputterman]

Cavin checked this in 11/14.  the branch build date is 11/12.  Therefore this
didn't make the branch.  Marking fixed again.  If we get 0.9.7 data that this
isn't fixed then we can reopen.

Comment 29

[Jay Patel [:jay]]

I'm verifying this fixed again.  Talkback data currently shows this crashing
only with M096 builds.  There have been no incidents on any MozillaTrunk builds
after th fix went in.  If this pops up again, feel free to reopen...but for now,
this baby is verified!

Comment 30

[Boris Zbarsky [:bzbarsky]]

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

Comment 31

[Boris Zbarsky [:bzbarsky]]

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

Comment 32

[Ashley Bischoff (blog at handcoding.com)]

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