409439 - Crash @ nsHTMLTableAccessible::GetRowAtIndex(int, int*) with certain pages if Orca is running

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P5)

Description

[Joanmarie Diggs]

Attachment: test case

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.

Comment 1

[Marco Zehe (:MarcoZ)]

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.

Comment 2

[Marco Zehe (:MarcoZ)]

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

Comment 3

[Marco Zehe (:MarcoZ)]

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.

Comment 4

[Marco Zehe (:MarcoZ)]

Attachment: WIP

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

Comment 5

[Joanmarie Diggs]

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.

Comment 6

[alexander :surkov (:asurkov)]

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

Comment 7

[Marco Zehe (:MarcoZ)]

Attachment: Patch

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

Comment 8

[alexander :surkov (:asurkov)]

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

Comment 9

[alexander :surkov (:asurkov)]

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

Comment 10

[Marco Zehe (:MarcoZ)]

Attachment: Patch v2

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

Comment 11

[alexander :surkov (:asurkov)]

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?

Comment 12

[Marco Zehe (:MarcoZ)]

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

Comment 13

[alexander :surkov (:asurkov)]

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.

Comment 14

[Marco Zehe (:MarcoZ)]

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

Comment 15

[alexander :surkov (:asurkov)]

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.

Comment 16

[Marco Zehe (:MarcoZ)]

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

Comment 17

[alexander :surkov (:asurkov)]

(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

Comment 18

[Marco Zehe (:MarcoZ)]

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

Comment 19

[Marco Zehe (:MarcoZ)]

Attachment: Removes the checks on valid columns/rows for the GetRowAtIndex and GetColumnAtIndex functions for now.

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 20

[alexander :surkov (:asurkov)]

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 21

[Marco Zehe (:MarcoZ)]

Attachment: Patch V4, addresses Surkov's comments

Comment 22

[alexander :surkov (:asurkov)]

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 23

[Marco Zehe (:MarcoZ)]

Attachment: Patch V5, address Surkov's comments

Comment 24

[alexander :surkov (:asurkov)]

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 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 294727 [details] [diff] [review]
Patch V5, address Surkov's comments

a=beltzner for 1.9

Comment 26

[Marco Zehe (:MarcoZ)]

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!

Comment 27

[Reed Loden [:reed]]

Shouldn't you address comment #24 first?

Comment 28

[alexander :surkov (:asurkov)]

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

Comment 29

[Marco Zehe (:MarcoZ)]

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

Comment 30

[Reed Loden [:reed]]

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

Comment 31

[Marco Zehe (:MarcoZ)]

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

Comment 32

[Damon Sicore (:damons)]

+'ing with a P5 priority.

Comment 33

[alexander :surkov (:asurkov)]

Marco, I think this bug should be closed?

Comment 34

[Marco Zehe (:MarcoZ)]

Fixed by landing of bug 410052.