415601 - Mailnews topcrashes [@ nsUInt32Array::GetAt] [@ nsMsgDBView::NonDummyMsgSelected] typically deleting message(s) or opening message in new tab, switching tabs, renaming folder

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

We're crashing in nsUInt32Array::GetAt again:

http://crash-stats.mozilla.com/report/list?range_unit=weeks&version=Thunderbird%3A3.0a1pre&range_value=2&signature=nsUInt32Array%3A%3AGetAt%28unsigned+int%29
http://crash-stats.mozilla.com/report/list?range_unit=weeks&version=Thunderbird%3A3.0a1pre&range_value=2&signature=nsUInt32Array%3A%3AGetAt%28unsigned+int%29+const

This time it looks like a regression from bug 258018 attachment 300330 [details] [diff] [review].

Typical windows stack:

0  	nsUInt32Array::GetAt(unsigned int)  	 mozilla/mailnews/base/util/nsUInt32Array.cpp:158
1 	nsMsgDBView::NonDummyMsgSelected(unsigned int*, int) 	mozilla/mailnews/base/src/nsMsgDBView.cpp:6095
2 	nsMsgDBView::SelectionChanged() 	mozilla/mailnews/base/src/nsMsgDBView.cpp:1027
3 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
4 	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2339
5 	XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, long*, long*) 	mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1470

Typical Linux/Mac stack:

0  	nsUInt32Array::GetAt(unsigned int) const  	 mozilla/mailnews/base/util/nsUInt32Array.cpp:155
1 	nsMsgDBView::SelectionChanged() 	mozilla/mailnews/base/src/nsMsgDBView.cpp:1027
2 	xptiInterfaceEntry::EnsureResolved(xptiWorkingSet*) 	
3 	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2339
4 	XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, long*, long*) 	mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1470
5 	js_Invoke 	mozilla/js/src/jsinterp.c:1044
6 	js_Interpret 	mozilla/js/src/jsinterp.c:3868
7 	js_Invoke 	mozilla/js/src/jsinterp.c:1061
8 	js_InternalInvoke 	mozilla/js/src/jsinterp.c:1117
9 	JS_CallFunctionValue 	mozilla/js/src/jsapi.c:4933
10 	nsJSContext::CallEventHandler(nsISupports*, void*, void*, nsIArray*, nsIVariant**) 	mozilla/dom/src/base/nsJSEnvironment.cpp:1941
11 	nsJSEventListener::HandleEvent(nsIDOMEvent*) 	mozilla/dom/src/events/nsJSEventListener.cpp:248
12 	nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsISupports*, unsigned int) 	mozilla/content/events/src/nsEventListenerManager.cpp:1081
13 	nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, nsISupports*, unsigned int, nsEventStatus*) 	mozilla/content/events/src/nsEventListenerManager.cpp:1183
14 	nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int) 	mozilla/content/events/src/nsEventDispatcher.cpp:206
15 	nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*) 	mozilla/content/events/src/nsEventDispatcher.cpp:264
16 	nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*) 	mozilla/content/events/src/nsEventDispatcher.cpp:479
17 	nsTreeSelection::FireOnSelectHandler() 	mozilla/layout/xul/base/src/tree/src/nsTreeSelection.cpp:839
18 	nsTreeSelection::SetSelectEventsSuppressed(int) 	mozilla/layout/xul/base/src/tree/src/nsTreeSelection.cpp:606
19 	nsMsgDBView::RestoreSelection(unsigned int, nsMsgKeyArray*) 	mozilla/mailnews/base/src/nsMsgDBView.cpp:829
20 	nsMsgDBView::SelectMsgByKey(unsigned int) 	mozilla/mailnews/base/src/nsMsgDBView.cpp:6279
21 	xptiInterfaceEntry::EnsureResolved(xptiWorkingSet*) 	
22 	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2339
23 	XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, long*, long*) 	mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1470
24 	js_Invoke 	mozilla/js/src/jsinterp.c:1044
25 	js_Interpret 	mozilla/js/src/jsinterp.c:3868
26 	js_Invoke 	mozilla/js/src/jsinterp.c:1061
27 	nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) 	mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1473
28 	nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) 	mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:556
29 	PrepareAndDispatch 	mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp:95
30 	nsMsgSearchSession::NotifyListenersDone(unsigned int) 	mozilla/mailnews/base/search/src/nsMsgSearchSession.cpp:607
31 	nsMsgSearchSession::TimerCallback(nsITimer*, void*) 	mozilla/mailnews/base/search/src/nsMsgSearchSession.cpp:541
32 	nsTimerImpl::Fire() 	mozilla/xpcom/threads/nsTimerImpl.cpp:400
33 	nsTimerEvent::Run() 	mozilla/xpcom/threads/nsTimerImpl.cpp:488
34 	nsThread::ProcessNextEvent(int, int*) 	mozilla/xpcom/threads/nsThread.cpp:510
35 	NS_ProcessNextEvent_P(nsIThread*, int) 	nsThreadUtils.cpp:227
36 	nsBaseAppShell::Run() 	mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:154
37 	nsAppStartup::Run() 	mozilla/toolkit/components/startup/src/nsAppStartup.cpp:181
38 	XRE_main 	mozilla/toolkit/xre/nsAppRunner.cpp:3135
39 	main 	mozilla/mail/app/nsMailApp.cpp:91

Comment 1

[[e-mail]]

(In reply to comment #0)
>0	nsUInt32Array::GetAt(unsigned int)	mozilla/mailnews/base/util/nsUInt32Array.cpp:158
>1	nsMsgDBView::NonDummyMsgSelected(unsigned int*, int)	mozilla/mailnews/base/src/nsMsgDBView.cpp:6095
I'd really like to know what those ints are...

Comment 2

[james davis]

This is getting to be a many times per day crash for me. The last one was
http://crash-stats.mozilla.com/report/index/9e4439c7-d666-11dc-8649-001a4bd43ef6

It seems hard to reproduce by doing only one activity. The last crash I wasn't
doing anything but it doesn't mean the backend wasn't receiving or moving something due to a filter.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008020603 Thunderbird/3.0a1pre ID:2008020603

Comment 3

[Robert Roessler]

These happen quite frequently for me also, and if you check the comments I diligently enter in the crash report that gets sent in, they *invariably* happen when I click the Junk mail folder in the mail client.

The typical scenario is mail comes in, some of it gets automagically classified as 'Junk' and moved to the Junk folder, and I may manually classify some mails as 'Junk' also.  Then I click the Junk folder to go and weed out any false positives, and wham - my SM nightly crashes...

To be clear, I don't get the crash EVERY time I click the Junk folder, but if I do crash, it WILL BE when the Junk folder is clicked.

Comment 5

[Jay Moran (jaym/lazvon)]

Okay, moving over to here by way of Bug 411145 which was by way of Bug 411140 ... anyway, attachment 302492 [details] is gdb attached with a back, a couple of up commands, and lists with each. Most importantly though, looks like folks were interested in the variables. For Frame 0, (nIndex=0) ... and for Frame 1 (indices=0xbfffb914, numIndices=1). I'm completely new to C++ debugging and an extreme amateur with C even... but I'll be happy to help anyway I can.

#0  0x0c065c6a in nsUInt32Array::GetAt (this=0x20d95d70, nIndex=0) at /Users/jay/mozilla/mailnews/base/util/nsUInt32Array.cpp:158
#1  0x0aa66bae in nsMsgDBView::NonDummyMsgSelected (this=0x20d95d30, indices=0xbfffb914, numIndices=1) at /Users/jay/mozilla/mailnews/base/src/nsMsgDBView.cpp:6095

Comment 6

[Jay Moran (jaym/lazvon)]

Attachment: Fresh gdb with symbols and variables to inspect.

A fresh attachment from just a couple minutes ago. Still have it in GDB actually if anyone would like something specific (will it die in ~30 minutes? though I need to get back to reading work email at some point soon).

Just for reference this happens to me when switching IMAP folders "quickly". It might be just the Junk Mail folder, I'm not sure about that. I do have an IMAP Server Side Rule that moves any mail with ****SPAM**** in the subject to the Junk Mail folder, but no Thunderbird local filters/rules that do anything with the Junk Folder (other than Thunderbird's auto-junking) that is setup for the account. Again, I'm not sure it is only when I'm clicking through to the Junk folder. I seem to remember it happenings with other folders as well. Always when I'm changing folders though, never when it is idle.

Comment 7

[Jay Moran (jaym/lazvon)]

BTW, my crashes started (again) about Feb 1st... so it certainly looks like a regression from bug 258018 attachment 300330 [details] [diff] [review].

Comment 8

[Jay Moran (jaym/lazvon)]

Attachment: gdb of crash, nothing to do with Junk Folder.

Okay, nothing to do with Junk Folder. Specific steps for this one were:

1) Delete Key on one and only message in IMAP Folder "Discussion Lists" after reading it.
2) Right clicked on "Discussion Lists" and clicked Compact
3) Left clicked on IMAP Folder "Daily Review" which became selected (I know now after restarting Tbird that there were no messages in the folder)... since there were no messages, it either locked up at this point, or the next action...
4) Right clicked, no context menu popped up, was going to click Compact (habit when I leave a folder, wish it was automatic)... either at that point, or right before I got the bouncing ball and it crashed which I attached GDB and did a back as attached.

Comment 9

[[e-mail]]

(In reply to comment #8)
>Specific steps for this one were:
>1) Delete Key on one and only message
Yay :-) After doing this I was able to reproduce the crash.

The problem here is that sometimes the last message loaded can be 0, even when there are no messages in the folder. We then select it, which is a good trick considering ;-) Things go downhill from here...

Comment 10

[Jay Moran (jaym/lazvon)]

Attachment: One last gdb

This one was a little different... no deleting or anything. Simply was in the Inbox then clicked on a different IMAP Folder (Discussion Lists, to be specific) and immediately got a crash (no beach ball like usually). This one in the first crash box also said something about a missing file, and referenced a lib... dymlib perhaps? I know I should have copied it down but thought I'd be able to find that somewhere else and started GDB. As you can see from the attachment, GDB looks very similar, just different circumstances getting us here. No delete or anything.

Comment 11

[[e-mail]]

If it is possible to open that folder without crashing, then the proof of the pudding would be that no message in that folder has the specific key from the crash backtrace [nsMsgDBView::SelectMsgByKey (this=0x20dce820, aKey=670)].

If you turn off the "Remember the last selected message" preference then you won't activate that particular call to SelectMsgByKey.

Message keys are displayed in the "Order Received" column.

Comment 12

[Jay Moran (jaym/lazvon)]

I just recompiled with fresh checkouts... and I've disabled "Remember the last selected message"... I'll see if I can make it crash this afternoon.

Next time (after recompile) I opened the folder, I did not crash. There were two messages in there, but they most likely came while I was recompiling, so they probably were not present the first time. Could the remember have been from the last time I was in there, deleted and compacted? Dunno. I'll continue using Tbird as I normally do and see if I can get another crash out of it.

Comment 13

[Jay Moran (jaym/lazvon)]

No crashes for the past couple of hours. I'm assuming that is from disabling "Remember the last selected message". I'll turn that back on and see what happens for the next hour or so before I leave. Not conclusive, but heck... at least I'll have a potential workaround for tomorrow.

Comment 14

[Jay Moran (jaym/lazvon)]

Wow! Okay. That was easy enough, turned the Preference back on and BOOM! Yeah, definitely that for the problem. Now I'll be turning that off and awaiting a fix. Thanks for the help and let me know if I can be of any assistance. I'm CC'd.

Comment 15

[[e-mail]]

Attachment: Proposed patch

The first hunk stops SelectMsgByKey from selecting an invalid key.
The second hunk is an unused variable removal I noticed.
The third hunk hardens GetNumSelected by comparison with GetSelectedIndices.
The fourth hunk stops SelectFolderMsgByKey from selecting an invalid key.
Please feel free to only allow specified hunks. In particular I've no idea what the point of SelectFolderMsgByKey is, it was added as part of tabbed mail.

Comment 16

[Jay Moran (jaym/lazvon)]

(In reply to comment #15)

Applied Neil's patch this morning, recompiled, and re-enabled the Remember Message preference. No more crashes after two hours. I've not had a two hour stretch of regular mail us in Thunderbird since Jan 31st. I'll post back later today before heading home, but looks great. Others should try it if they are tired of crashing! :)

Comment 17

[Jay Moran (jaym/lazvon)]

(In reply to comment #15)

This proposed patches very much seems to have solved my problems of crashing on changing folders with "Remember the last selected message" preference set. :) A full day of work emails and banging the heck out of Thunderbird with 70 or so emails coming in, filed, deleted, compacted... and a whole lot of clicking through all my folders as fast I could to stress things the best that I could. Never a hiccup.

Can't wait to see this patch land, but until it does land I'm not checking out or recompiling Thunderbird at all... since the 2008-2-12 7:30AM PST checkout with patch applied works perfectly for me. Thank you Neil!!!

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 302714 [details] [diff] [review]
Proposed patch

>Index: mailnews/base/src/nsMsgDBView.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/src/nsMsgDBView.cpp,v
>retrieving revision 1.296
>diff -u -p -r1.296 nsMsgDBView.cpp
>--- mailnews/base/src/nsMsgDBView.cpp	31 Jan 2008 15:41:14 -0000	1.296
>+++ mailnews/base/src/nsMsgDBView.cpp	12 Feb 2008 00:36:32 -0000
>@@ -810,7 +810,8 @@ nsresult nsMsgDBView::RestoreSelection(n
>   {
>     newViewPosition = FindKey(aMsgKeyArray->GetAt(index), PR_FALSE);
>     // add the index back to the selection.
>-    mTreeSelection->ToggleSelect(newViewPosition);
>+    if (newViewPosition != nsMsgViewIndex_None)
>+      mTreeSelection->ToggleSelect(newViewPosition);
>   }
> 
>   // make sure the currentView was preserved....

sr=dmose on this

>@@ -1110,7 +1111,7 @@ nsresult nsMsgDBView::GetSelectedIndices
>     selection.SetLength(count);
>     count = 0;
>     PRInt32 selectionCount;
>-    nsresult rv = mTreeSelection->GetRangeCount(&selectionCount);
>+    mTreeSelection->GetRangeCount(&selectionCount);
>     for (PRInt32 i = 0; i < selectionCount; i++)
>     {
>       PRInt32 startRange = -1;

sr=dmose on this

>@@ -5907,12 +5908,25 @@ nsMsgDBView::GetNumSelected(PRUint32 *nu

Should these just be assertions?  Or is it the case that extensions could cause them to be inconsistent? 

> NS_IMETHODIMP
>@@ -6242,7 +6256,10 @@ NS_IMETHODIMP nsMsgDBView::SelectFolderM
> 
>   nsMsgViewIndex viewIndex = FindKey(aKey, PR_TRUE /* expand */);
> 
>-  mTreeSelection->Select(viewIndex);
>+  if (viewIndex == nsMsgViewIndex_None)
>+    mTreeSelection->ClearSelection();
>+  else
>+    mTreeSelection->Select(viewIndex);
> 
>   if (mTree)
>     mTreeSelection->SetCurrentIndex(viewIndex);

sr=dmose on this, but it would nice to know if ClearSelection is the right thing or merely doing nothing.  As you pointed out, with no callers and no documentation it's hard to guess...

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

In fact, for the last hunk, I'd suggest documenting that behavior in the IDL if you decide to land it.

Comment 20

[Mark Banner (:standard8)]

With another bug 258018 checking (attachment 300713 [details] [diff] [review]), this bug has mutated into crashing at nsMsgDBView::NonDummyMsgSelected() - Neil confirmed on irc, it is due to one of the functions being inlined. Updating title.

Comment 21

[[e-mail]]

(In reply to comment #18)
>(From update of attachment 302714 [details] [diff] [review])
>>@@ -5907,12 +5908,25 @@ nsMsgDBView::GetNumSelected(PRUint32 *nu
>Should these just be assertions?
There already is an assertion in nsMsgDBView::SelectionChanged

> Or is it the case that extensions could cause them to be inconsistent? 
That is also possible.

Comment 22

[Philip Chee]

version 3.0a1pre (2008021403)
http://crash-stats.mozilla.com/report/index/2dd95a5c-db06-11dc-806b-001a4bd43ef6?date=2008-02-14-14

I have turned remember_selected_message off. I can reliably make Thunderbird crash by right clicking on a message in the thread pane and select "Open message in new tab".

Comment 23

[Joe Sabash [:JoeS1]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008021503 Thunderbird/3.0a1pre ID:2008021503
Still crashing in any state of "remember selected message" when attepting to open a message in a new tab.
http://crash-stats.mozilla.com/report/index/4b1f325c-dc29-11dc-8b8a-001a4bd43e5c?date=2008-02-16-00

Comment 24

[[e-mail]]

Whoops, I checked this in almost a month ago but forgot to mark it fixed :-(

Comment 25

[[e-mail]]

Sorry, got my patches mixed up.

Comment 26

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: gdb trace

smacked on me whilst on a debug build version 3.0a1pre 12 March 08, while opening a message in a new tab.

stack trace from gdb attached.

Comment 27

[[e-mail]]

Attachment: Make GetNumSelected not lie

The other approach is simply to stop using GetNumSelected...

Comment 28

[Wayne Mery (:wsmwk)]

#1 crash

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

With simple patches and a top crash, we should definite block 3.0a1 for this.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Neil, can you clarify exactly how you'd like to drive this forward?   Land (parts of) both patches?  Just the second patch?

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 309290 [details] [diff] [review]
Make GetNumSelected not lie

After discussion in IRC with Neil, we've agreed that it's worth looking for a bandaid fix that's less likely to impact performance than this one; marking attachment obsolete.

Comment 32

[[e-mail]]

Attachment: Just fix this one crash

This just fixes this crash. The other callers of GetNumSelected didn't crash in the "invalid selection" case; one in C++ is only used for an assertion, one in C++ double-checks the first selected index and the other callers are all in JS.

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 315723 [details] [diff] [review]
Just fix this one crash

The old code asserted but picked the less-reliable number, causing the crash.  This change picks the more reliable number, but removes the assertion.  Let's keep the assertion but pick the more reliable number.  Otherwise we'll lose track of the underlying problem here.  #ifdef DEBUG for the GetNumSelected() is probably reasonable.

Comment 34

[[e-mail]]

Attachment: Keep the assertion

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 315940 [details] [diff] [review]
Keep the assertion

r+sr=dmose; thanks for the quick turnaround!

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

Bonus points if you coalesce the GetNumSelected and NS_ASSERTION inside a single #ifdef DEBUG block with its own variable (and maybe a comment) so that it's more clear on first reading which bits of code go together and for what reason.

Comment 37

[[e-mail]]

Fix checked in.

Comment 38

[[e-mail]]

(In reply to comment #36)
> Bonus points if you coalesce the GetNumSelected and NS_ASSERTION inside a
> single #ifdef DEBUG block with its own variable (and maybe a comment) so that
> it's more clear on first reading which bits of code go together and for what
> reason.
Whoops, I didn't see this, but I quite liked not touching the existing code ;-)

Comment 39

[Joe Sabash [:JoeS1]]

That fixes the last remaining crash for me.
Open message in a new tab works fine now.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041616 Thunderbird/3.0a1pre ID:2008041616

Comment 40

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Verified on a debug trunk Thunderbird build compiled today on Leopard; though it asserts, it no longer crashes.