Steps to reproduce: 1. Get the latest Orca from svn trunk (this is necessary). 2. Load the attached test case and tab to the link. Expected results: Firefox wouldn't crash Actual results: Firefox crashes 100% of the time.

I cannot reproduce the crash using current nightly build. I'm using latest Orca Trunk (with the performance enhancements). The one thing I do notice is the fact that Orca does not speak the link when tabbing to it. But I don't get a crash.

OK, after updating to Orca Trunk revision 3400, I could reproduce the crash. A crash report is here: http://crash-stats.mozilla.com/report/index/58e6b4fa-b000-11dc-ac00-001a4bd46e84

From the looks of it, it appears that, in nsHTMLTableAccessible::GetRowAtIndex, ChildNode is being used without checking if it is valid. Also, before the line pointed to by the crash report, the "child" accessible is being interface-queried without checking if it is valid, and the AIndex is also not checked whether it is in a valid range. CC'ing Ginn because he wrote this code.

General idea of fix, have not made sure it compiles yet. Will do that tomorrow. Just wanted to save it here just in case.

Okay, I saw what we were doing to make Firefox unhappy, and I committed a new patch to Orca trunk to stop doing that. :-) Still, crashers are bad. Thanks for working on this Marco.

Marco, is it worth to use IsDefunct() approach from bug 408831?

Guarded against dealing with invalid indexes with row and/or column parameters in various functions of the nsHTMLTableAccessible class.

I would like to have isValidRow/isValidColumn, it should be a bit readable. Also return NS_ERROR_INVALID_ARG

You could do NS_ENSURE_TRUE(IsValidColumn(aColumn) && IsValidRow(aRow), NS_ERROR_INVALID_ARG);

Address Surkov's comments: Turned the function into two separate functions, and used NS_ENSURE_TRUE.

It sounds you got a mesh with index/row/columns, for example, nsHTMLTableAccessible::GetRowAtIndex(PRInt32 aIndex, PRInt32 *aRow) NS_ENSURE_TRUE(IsValidRow(aIndex), NS_ERROR_INVALID_ARG); here aIndex is not row actually, right?

(In reply to comment #11) > It sounds you got a mesh with index/row/columns, for example, > > nsHTMLTableAccessible::GetRowAtIndex(PRInt32 aIndex, PRInt32 *aRow) > NS_ENSURE_TRUE(IsValidRow(aIndex), NS_ERROR_INVALID_ARG); No, in this case, aRow is a pointer, not an index, that's why I have to use aIndex instead. aRow is checkecd if it is a valid pointer two lines above.

iirc, we have three things: index, row and column, index is unique pointer to cell on the given row and column. I mean index is not the same like is row or column. But there you check index as it was a row.

(In reply to comment #13) > iirc, we have three things: index, row and column, index is unique pointer to > cell on the given row and column. I mean index is not the same like is row or > column. But there you check index as it was a row. Yes, but in this case the function is called "GetColumnAtIndex", which means "get me the aIndex-th column". So the index has to be checked to be a valid column. Same goes for the GetRowAtIndex etc. functions.

Let consider the following example table cell (0) cell(1) cell(3) cell (4) cell(5) cell(6) GetColumnAtIndex(5) should return 1. But your check IsValidColumn(aIndex) return false and method fails.

(In reply to comment #15) > Let consider the following example > > table > cell (0) cell(1) cell(3) > cell (4) cell(5) cell(6) > GetColumnAtIndex(5) should return 1. But your check IsValidColumn(aIndex) > return false and method fails. Why? There arre 6 columns in your example, and index 5 would return that last column. And the IsValidColumn function checks whether the index is within the range of 0...ColumnCount-1. ColumnCount is 6, so the last allowed index is 5. Why should it return false?

(In reply to comment #16) > (In reply to comment #15) > > Let consider the following example > > > > table > > cell (0) cell(1) cell(3) > > cell (4) cell(5) cell(6) > > GetColumnAtIndex(5) should return 1. But your check IsValidColumn(aIndex) > > return false and method fails. > > Why? There arre 6 columns in your example, and index 5 would return that last > column. And the IsValidColumn function checks whether the index is within the > range of 0...ColumnCount-1. ColumnCount is 6, so the last allowed index is 5. > Why should it return false? > No, I meant 3 columns and 2 rows, 6 items

(In reply to comment #17) > No, I meant 3 columns and 2 rows, 6 items OK, but even then it'll not fail, because if the ColumnIndex is row-based, asking for column with index 5 in a single row will return FALSE, causing the function to return NS_E_INVALID_ARG. The original crash happened in the GetRowAtIndex function, which uses the same logic, and the patch fixes that crash.

After discussing the issue with Surkov on IRC, I've removed the checks on the GetRowAtIndex and GetColumnAtIndex for this patch. We found that the table interface seems broken, since GetIndexAt currently may return bogus values if there are RowAccessibles in the table (due to OnClicks). In consequence, this patch does not yet address the actual crasher, but should be checked in nevertheless since it ensures the validity of parameters in a lot of other places.

Comment on attachment 294721 [details] [diff] [review] Removes the checks on valid columns/rows for the GetRowAtIndex and GetColumnAtIndex functions for now. >Index: accessible/src/html/nsHTMLTableAccessible.cpp >=================================================================== >RCS file: /cvsroot/mozilla/accessible/src/html/nsHTMLTableAccessible.cpp,v >retrieving revision 1.56 >diff -p -u -5 -r1.56 nsHTMLTableAccessible.cpp >--- accessible/src/html/nsHTMLTableAccessible.cpp 12 Dec 2007 02:10:27 -0000 1.56 >+++ accessible/src/html/nsHTMLTableAccessible.cpp 28 Dec 2007 08:40:34 -0000 >@@ -505,10 +505,12 @@ nsHTMLTableAccessible::GetSelectedRows(P > > NS_IMETHODIMP > nsHTMLTableAccessible::CellRefAt(PRInt32 aRow, PRInt32 aColumn, > nsIAccessible **aTableCellAccessible) > { >+ NS_ENSURE_TRUE(IsValidColumn(aColumn) & IsValidRow(aRow), NS_ERROR_INVALID_ARG); >+ possibly logical & (&&) looks more correct here. nit: for beauty you could check row firstly :) > nsCOMPtr<nsIAccessible> child; > GetChildAt(aIndex, getter_AddRefs(child)); >+ NS_ENSURE_TRUE(child, NS_ERROR_FAILURE); > nsCOMPtr<nsPIAccessNode> childNode(do_QueryInterface(child)); >+ NS_ENSURE_TRUE(childNode, NS_ERROR_FAILURE); I think NS_ASSERTION would be enough here because accessible must implement nsPIAccessNode. >@@ -591,10 +603,12 @@ NS_IMETHODIMP > nsHTMLTableAccessible::GetRowExtentAt(PRInt32 aRow, PRInt32 aColumn, > PRInt32 *_retval) > { > nsresult rv = NS_OK; > >+ NS_ENSURE_TRUE(IsValidRow(aRow), NS_ERROR_INVALID_ARG); You forgot to check aColumn here. >+PRBool >+nsHTMLTableAccessible::IsValidColumn(PRInt32 aColumn) >+{ >+ PRInt32 colCount = 0; >+ GetColumns(&colCount); It's worth to check nsresult here too. >+nsHTMLTableAccessible::IsValidRow(PRInt32 aRow) >+{ >+ PRInt32 rowCount = 0; >+ GetRows(&rowCount); the same >+ // Checks whether the given row or column index is in a valid range >+ PRBool IsValidColumn(PRInt32 aColumn); >+ PRBool IsValidRow(PRInt32 aRow); I would like to have java doc style here for each method. Though if you think it's not reasonable then it's up to you.

Comment on attachment 294725 [details] [diff] [review] Patch V4, addresses Surkov's comments >@@ -575,10 +585,12 @@ NS_IMETHODIMP > nsHTMLTableAccessible::GetColumnExtentAt(PRInt32 aRow, PRInt32 aColumn, > PRInt32 *_retval) > { > nsresult rv = NS_OK; move rv declaration below, place it after NS_ENSURE_TRUE >+ NS_ENSURE_TRUE(IsValidRow(aRow) && IsValidColumn(aColumn), NS_ERROR_INVALID_ARG); > nsHTMLTableAccessible::GetRowExtentAt(PRInt32 aRow, PRInt32 aColumn, > PRInt32 *_retval) > { > nsresult rv = NS_OK; the same >+ NS_ENSURE_TRUE(IsValidRow(aRow) && IsValidColumn(aColumn), NS_ERROR_INVALID_ARG); >+ >+PRBool >+nsHTMLTableAccessible::IsValidColumn(PRInt32 aColumn) >+{ >+ PRInt32 colCount = 0; >+ nsresult rv = GetColumns(&colCount); >+ NS_ENSURE_SUCCESS(rv, rv); wrong: the method return PRBool but nsresult. You should do something like return NS_SUCCEEDED(rv) && (aColumn >= 0) && (aColumn < colCount); >+PRBool >+nsHTMLTableAccessible::IsValidRow(PRInt32 aRow) >+{ >+ PRInt32 rowCount = 0; >+ nsresult rv = GetRows(&rowCount); >+ NS_ENSURE_SUCCESS(rv, rv); the same >+ return ((aRow >= 0) && (aRow < rowCount)); >+} >+ > >+ /** >+ * Checks whe the given index is in the valid Column range. possibly, "Return true if the given index is valid column index". At least "whe" is misspeling and the first letter of "Column" is in upper case. >+ * >+ * @param aColumn - The index to check for validity. >+ */ nit: actually you shoudn't use '-', just two spaces is more correct, though previously we used that style. the rest looks good, I would like to look at new patch before r+

Comment on attachment 294727 [details] [diff] [review] Patch V5, address Surkov's comments >+PRBool >+nsHTMLTableAccessible::IsValidColumn(PRInt32 aColumn) >+{ >+ PRInt32 colCount = 0; >+ nsresult rv = GetColumns(&colCount); >+ return NS_SUCCEEDED(rv) && (aColumn >= 0) && (aColumn < colCount); nit I didn't notice previously. You could check aColumn >=0 before you call GetColumns() actually. >+} >+ >+PRBool >+nsHTMLTableAccessible::IsValidRow(PRInt32 aRow) >+{ >+ PRInt32 rowCount = 0; >+ nsresult rv = GetRows(&rowCount); >+ return NS_SUCCEEDED(rv) && (aRow >= 0) && (aRow < rowCount); the same

Comment on attachment 294727 [details] [diff] [review] Patch V5, address Surkov's comments a=beltzner for 1.9

Requesting checkin. Note to dev checking this one in: Please do not mark this bug as resolved yet. This is only the first part of the fix. Thanks!

Shouldn't you address comment #24 first?

(In reply to comment #27) > Shouldn't you address comment #24 first? > sorry, forgot to say, we talk with Marco on IRC. It's very rare case, so we shouldn't get real advantages but Marco likes the current syntax more. It's up to him.

(In reply to comment #27) > Shouldn't you address comment #24 first? As Surkov already stated: We talked on IRC about this and agreed that it is far more likely that an index be passed in that is greater than or equal to the actual count, rather than negative. So putting in this extra check would only give us an advantage in some rare cases and would make the code less readable, IMO.

Checking in accessible/src/html/nsHTMLTableAccessible.cpp; /cvsroot/mozilla/accessible/src/html/nsHTMLTableAccessible.cpp,v <-- nsHTMLTableAccessible.cpp new revision: 1.57; previous revision: 1.56 done Checking in accessible/src/html/nsHTMLTableAccessible.h; /cvsroot/mozilla/accessible/src/html/nsHTMLTableAccessible.h,v <-- nsHTMLTableAccessible.h new revision: 1.32; previous revision: 1.31 done

Reopening awaiting a second patch after fix for bug 410052. Thanks for checking in this first patch, Reed!

+'ing with a P5 priority.

Marco, I think this bug should be closed?

Fixed by landing of bug 410052.