Steps to reproduce: Set up Thunderbird so that the list of folders and the message list are showing but the preview pane is not. Ideally, have two folders next to one another, both of which contain a significant number of messages. Having done so, move focus to the list of folders. Then: 1. Tab to the list of messages and *immediately* press Down Arrow 2. Shift Tab to the list of folders, arrow to the other folder, repeat step 1. Keep doing this -- and doing it fast enough -- and eventually Thunderbird will crash with this assertion: ###!!! ASSERTION: array index out of bounds: 'nIndex < m_nSize', file nsUInt32Array.cpp, line 157 The trick seems to be to press Down Arrow before the list of messages has been fully populated. If you do the above steps nice and slow, it won't crash. :-) Also note that if I do not have a11y enabled, I cannot reproduce this. I got the best stack trace I can at the moment. It's attached.

Joanmarie, thanks for the report. Could you build a debug version of Thunderbird and recreate this to provide a more readable stack trace?

Attached is what I got from a build with shared libraries. Aside from the final thread, it didn't seem much better than the last. So then I built again with static and got even less. *sigh* If this does not contain what you need, please let me know what I should be doing to get you what you need. :-) Thanks!

#0 0xb4c8a582 in nsUInt32Array::GetAt (this=0x93b008c, nIndex=1) ---Type <return> to continue, or q <return> to quit--- at nsUInt32Array.cpp:158 #1 0xb4c895cc in nsUInt32Array::operator[] (this=0x93b008c, nIndex=1) at ../../../dist/include/msgbaseutil/nsUInt32Array.h:91 #2 0xb4c895f6 in nsMsgKeyArray::operator[] (this=0x93b008c, nIndex=1) at ../../../dist/include/msgbaseutil/nsMsgKeyArray.h:58 #3 0xb4c89620 in nsMsgKeyArray::GetAt (this=0x93b008c, nIndex=1) at ../../../dist/include/msgbaseutil/nsMsgKeyArray.h:64 #4 0xb4d43586 in nsMsgDBView::GetMsgHdrForViewIndex (this=0x93b0060, index=1, msgHdr=0xbf8e62b0) at nsMsgDBView.cpp:1448 #5 0xb4d56749 in nsMsgDBView::GetCellValue (this=0x93b0060, aRow=1, aCol=0x8d57450, aValue=@0xbf8e63b8) at nsMsgDBView.cpp:1508 #6 0xb604b55b in nsXULTreeitemAccessible::GetName (this=0x84da6f0, aName=@0xbf8e63b8) at nsXULTreeAccessible.cpp:605 #7 0xb6029189 in getNameCB (aAtkObj=0x93b0740) at nsAccessibleWrap.cpp:730 We're trying to compute the accessible name from GetValue() for a cell on row 1 (which is the 2nd row, because row 0 is the first row). In ::GetName() we first try GetCellText() and if that returns nothing we try GetCellValue() but the comment doesn't make it clear when we need to use the 2nd case. That's not necessarily related to the bug but it would be good to add a comment for that.

(In reply to comment #3) > In ::GetName() we first try GetCellText() and if that returns nothing we try > GetCellValue() but the comment doesn't make it clear when we need to use the > 2nd case. Graphical cells. We need tree/table view implementors to implement FooView::GetCellValue to return a meaningful string for cases where there is something shown in the cell (non-text) such as a star icon; in which case GetCellValue for that cell would return "starred" or "flagged" for example.

Makes sense, let's add a comment for that when we check this fix in.

Sounds good. We can put the comment in nsXULTreeAccessible.cpp. Should we also add an implementation comment in nsITreeView.idl?

Of course! Also add info to XUL a11y docs. Otherwise why would anyone do it? Anyway, back to the real bug :)

Looking at my schedule I'm doubtful I can get to this bug in the near term? I notice this is marked orca:urgent so I feel bad. Should I change the assignment or can we wait?

I think there's a good chance that Surkov's fix for bug 368835 fixes this: https://bugzilla.mozilla.org/show_bug.cgi?id=368835 Can someone test that and see?

(In reply to comment #9) > I think there's a good chance that Surkov's fix for bug 368835 fixes this: > https://bugzilla.mozilla.org/show_bug.cgi?id=368835 > > Can someone test that and see? I ran with a build that includes the most current patch for bug 368835 for over 15 minutes this evening. That is more than I ever was able to use Thunderbird on Linux so far. I also did some of the typical things that usually crashed me, like jumping forward to the next unread message using the N keystroke. Once all messages in a folder are read, Thunderbird asks whether I want to advance to the next folder with unread messages. Answering yes previously reliably caused a crash. With that fix in, it does no more. I will do some more heavy e-mailing tomorow morning and see if I'm still able to break it, or if the fix actually cures this bug too. Keep your fingers crossed!

Sorry, the problem has only been softened a bit. While folders with 40 or so messages no longer throw Thunderbird off-ballance with the patch for bug 368835, switching to a folder with 180 or so unread messages using the N keystroke, still causes the crash. So the patch only causes the bug to appear on a bigger number of messages, but it is still there. :-(

Joanie, the patch for bug 368835 has landed on the 2007-12-12 build of Thunderbird 3. Can you still reproduce a crash? I cannot get a crash with the signature we had previously, but only one of this sort: http://crash-stats.mozilla.com/report/index/a7c397e6-a8ce-11dc-b433-001a4bd43e5c This would indicate that we're passing an index to the "IsEmpty()" function that it doesn#t expect. This is in the MailNews module. Joanie, can you still repro the original crash now?

Surkov, this looks like something you should be able to fix even if you can't repro. It looks like we're firing an event for a tree item that still exists in our cache, but no longer in the message database. Either we shouldn't be asking about defunct objects, or the various tree implementations need to be more robust about indices they don't like, or both.

It's crash is not on our side. All we should do is mark accessible as defunct and AT should decide whether they will call methods or not. I think we shouldn't check whether object is defunct in the methods.

Surkov, do you need to add that check to any of the other methods of that class?

Comment on attachment 293121 [details] [diff] [review] mail patch most public methods already check, I believe, but they should be audited.

like this?

(In reply to comment #17) > (From update of attachment 293121 [details] [diff] [review]) > most public methods already check, I believe, but they should be audited. > I ran through them and didn't find

Comment on attachment 293121 [details] [diff] [review] mail patch a=beltzner

Comment on attachment 293142 [details] [diff] [review] patch a=beltzner

both patch are checked in. Joanie if you can reproduce original problem please reopen the bug.

Thunderbird is already much more stable with the first patch! However, another crash in the same area: http://crash-stats.mozilla.com/report/index/e1c8add9-abb3-11dc-b1c4-001a4bd43e5c?date=2007-12-16-08 It looks like the parameter aRow isn't being checked for being in a valid range before the query is executed. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/src/nsMsgDBView.cpp&rev=1.288&mark=1511#1511 Reopening for a follow-up patch.

This patch checks if ARow is a valid index in the method indicated in my Crash report.

Question: Should nsMsgDBView::GetImageSrc also be guarded against ACol or ARow being out of range? See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/src/nsMsgDBView.cpprev=1.288mark=1485#1485

Comment on attachment 293367 [details] [diff] [review] Follow up patch We need to request from bienvenu since it's his module.

Surkov, as I worked on this last patch, I was wondering why Accessibility is asking for invalid or out-of-range items in the first place. Is it because of asynchronous program flow that it is told the total number of rows the tree will eventually get, but when it uses this information, this information is actually not yet available/the rows not populated yet? Why else would the Accessibility module ask for rows that aren't there yet?

Requesting that Attachment 293367 [details] [diff] be checked in, I don't have CVS access myself. Thanks!

Checking in mozilla/mailnews/base/src/nsMsgDBView.cpp; /cvsroot/mozilla/mailnews/base/src/nsMsgDBView.cpp,v <-- nsMsgDBView.cpp new revision: 1.289; previous revision: 1.288 done Leaving this open because I'm not sure if you're all done now. Someone more knowledgeable than I am can close as FIXED if necessary.

This patch makes sure that nsXulTreeItemAccessible::GetName only deals with rows in a valid range. Question: Is it OK to simply return ns_OK here, or should something bee set to be defunct? I took Surkov's patch from above as a template for this one, but obviously don't have any AExtraState to defunct.

Ok we check everywhere whether object is defunct (check on mDOMNode) but we do not check this in treeitem accessbile. So either we should add IsDefunct method and call it everywhere before actual work or we shouldn't do anything (I'm about Marco's patch).

(In reply to comment #27) > Surkov, as I worked on this last patch, I was wondering why Accessibility is > asking for invalid or out-of-range items in the first place. Is it because of > asynchronous program flow that it is told the total number of rows the tree > will eventually get, but when it uses this information, this information is > actually not yet available/the rows not populated yet? Why else would the > Accessibility module ask for rows that aren't there yet? > Originally I thought this is the case about rows was removed but AT asks them. But if you think they aren't populated yet then we should ask mail guys how it's possible to return rowCount greater than possible index I guess.

(In reply to comment #25) > Question: Should nsMsgDBView::GetImageSrc also be guarded against ACol or ARow > being out of range? See > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/src/nsMsgDBView.cpprev=1.288mark=1485#1485 > it's easier to ask mail guys I guess, I tried but catched nothing :)

Comment on attachment 293409 [details] [diff] [review] Accessible part of recent crash report I suggest return NS_ERROR_INVALID_ARG instead. Are there any other methods in nsXULTreeitemAccessible that would benefit from this check?

(In reply to comment #34) > (From update of attachment 293409 [details] [diff] [review]) > I suggest return NS_ERROR_INVALID_ARG instead. > > Are there any other methods in nsXULTreeitemAccessible that would benefit from > this check? > Actually all of them. I suggested above to add IsDefunct method that should be added to the start of each method. Usually this method will check mDOMNode on nsnull. Treeitem accessible will override this method to add the check of row on validity. It will be huge patch. Do we want this in fx3 timeframe? And do we want this at all? :)

(In reply to comment #34) > (From update of attachment 293409 [details] [diff] [review]) > I suggest return NS_ERROR_INVALID_ARG instead. NS_ERROR_FAILURE would be better because usually we return this when accessible is defunct.

This patch is the same as the previous one, only that it returns NS_ERROR_FAILURE, as suggested by Surkov and Aaron. This is what should eventually be checked in once approval is granted.

Surkov, I'll rely on your judgment as to whether or not we should do this in Firefox 3 timeframe.

Requesting checkin of attachment 293469 [details] [diff] [review] because I don't have CVS access myself. Thanks!

Checking in accessible/src/xul/nsXULTreeAccessible.cpp; /cvsroot/mozilla/accessible/src/xul/nsXULTreeAccessible.cpp,v <-- nsXULTreeAccessible.cpp new revision: 1.69; previous revision: 1.68 done