Joanmarie Diggs Reporter
	Description • 17 years ago

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.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 1 • 17 years ago

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

	Joanmarie Diggs Reporter
	Comment 2 • 17 years ago

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!

	Aaron Leventhal
	Comment 3 • 17 years ago

#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.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 4 • 17 years ago

(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.

	Aaron Leventhal
	Comment 5 • 17 years ago

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

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 6 • 17 years ago

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

	Aaron Leventhal
	Comment 7 • 17 years ago

Of course! Also add info to XUL a11y docs. Otherwise why would anyone do it?

Anyway, back to the real bug :)

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 8 • 17 years ago

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?

	Aaron Leventhal
	Comment 9 • 17 years ago

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?

	Marco Zehe (:MarcoZ)
	Comment 10 • 17 years ago

(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!

	Marco Zehe (:MarcoZ)
	Comment 11 • 17 years ago

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. :-(

	Marco Zehe (:MarcoZ)
	Comment 12 • 17 years ago

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?

	Aaron Leventhal
	Comment 13 • 17 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 14 • 17 years ago

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.

	Aaron Leventhal
	Comment 16 • 17 years ago

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

	David :Bienvenu
	Comment 17 • 17 years ago

Comment on attachment 293121 [details] [diff] [review]
mail patch

most public methods already check, I believe, but they should be audited.

	alexander :surkov (:asurkov) Assignee
	Comment 18 • 17 years ago

like this?

	alexander :surkov (:asurkov) Assignee
	Comment 19 • 17 years ago

(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

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 20 • 17 years ago

Comment on attachment 293121 [details] [diff] [review]
mail patch

a=beltzner

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 21 • 17 years ago

Comment on attachment 293142 [details] [diff] [review]
patch

a=beltzner

	alexander :surkov (:asurkov) Assignee
	Comment 22 • 17 years ago

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

	Marco Zehe (:MarcoZ)
	Comment 23 • 17 years ago

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.

	Marco Zehe (:MarcoZ)
	Comment 24 • 17 years ago

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

	Marco Zehe (:MarcoZ)
	Comment 25 • 17 years ago

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

	Aaron Leventhal
	Comment 26 • 17 years ago

Comment on attachment 293367 [details] [diff] [review]
Follow up patch

We need to request from bienvenu since it's his module.

	Marco Zehe (:MarcoZ)
	Comment 27 • 17 years ago

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?

	Marco Zehe (:MarcoZ)
	Comment 28 • 17 years ago

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

	:Gijs (he/him)
	Comment 29 • 17 years ago

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.

	Marco Zehe (:MarcoZ)
	Comment 30 • 17 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 31 • 17 years ago

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).

	alexander :surkov (:asurkov) Assignee
	Comment 32 • 17 years ago

(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.

	alexander :surkov (:asurkov) Assignee
	Comment 33 • 17 years ago

(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 :)

	Aaron Leventhal
	Comment 34 • 17 years ago

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?

	alexander :surkov (:asurkov) Assignee
	Comment 35 • 17 years ago

(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? :)

	alexander :surkov (:asurkov) Assignee
	Comment 36 • 17 years ago

(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.

	Marco Zehe (:MarcoZ)
	Comment 37 • 17 years ago

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.

	Aaron Leventhal
	Comment 38 • 17 years ago

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

	Marco Zehe (:MarcoZ)
	Comment 39 • 17 years ago

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

	Reed Loden [:reed]
	Comment 40 • 17 years ago

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