Last Comment Bug 747219 - decomtaminate GetCellAt() on accessible tables
: decomtaminate GetCellAt() on accessible tables
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on: 739882
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2012-04-19 15:50 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-05-23 08:03 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (13.15 KB, patch)
2012-05-15 12:22 PDT, Mark Capella [:capella]
surkov.alexander: feedback+
Details | Diff | Review
Patch (v2) (13.16 KB, patch)
2012-05-16 03:47 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
Details | Diff | Review
Patch (v3) (13.10 KB, patch)
2012-05-18 08:32 PDT, Mark Capella [:capella]
no flags Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-04-19 15:50:16 PDT
same as bug 739882, bug 739883, and bug 739885
Comment 1 Mark Capella [:capella] 2012-05-15 12:22:34 PDT
Created attachment 624143 [details] [diff] [review]
Patch (v1)

Asking mentor for initial feedback ... this builds and passes all mochitest-a11y's locally ... let me know what you find, and I'll fix it -- mark
Comment 2 alexander :surkov 2012-05-15 20:02:56 PDT
Comment on attachment 624143 [details] [diff] [review]
Patch (v1)

Review of attachment 624143 [details] [diff] [review]:
-----------------------------------------------------------------

stealing feedback request from Trevor, asking him for review

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +882,3 @@
>    nsCOMPtr<nsIDOMElement> cellElement;
> +  GetCellAt(aRowIndex, aColumnIndex, *getter_AddRefs(cellElement));
> +  NS_ENSURE_TRUE(cellElement, nsnull);

no warning, just return null

@@ +886,5 @@
>    nsCOMPtr<nsIContent> cellContent(do_QueryInterface(cellElement));
> +  if (!cellContent)
> +    return nsnull;
> +
> +  NS_ENSURE_TRUE(mDoc, nsnull);

it shouldn't be null, so do not add it

::: accessible/src/xpcom/xpcAccessibleTable.cpp
@@ +60,5 @@
> +  if (aRowIndex < 0 || aRowIndex >= mTable->RowCount() ||
> +      aColumnIndex < 0 || aColumnIndex >= mTable->ColCount())
> +    return NS_ERROR_INVALID_ARG;
> +
> +  NS_IF_ADDREF(*aCell = mTable->CellAt(aRowIndex, aColumnIndex));

I think I'd check *aCell instead of row and column indices and it's null then return invalid_arg

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +289,4 @@
>  
>    nsCOMPtr<nsIDOMXULSelectControlItemElement> item;
> +  control->GetItemAtIndex(aRowIndex, getter_AddRefs(item));
> +  NS_ENSURE_TRUE(item, nsnull);

no warning pls

@@ +297,2 @@
>  
> +  NS_ENSURE_TRUE(mDoc, nsnull);

no check

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +333,5 @@
>  
> +nsAccessible*
> +nsXULTreeGridAccessible::CellAt(PRUint32 aRowIndex, PRUint32 aColumnIndex)
> +{ 
> +  nsAccessible* rowAccessible = GetTreeItemAccessible(aRowIndex);

rowAccessible -> row please

@@ +345,4 @@
>  
>    nsRefPtr<nsXULTreeItemAccessibleBase> rowAcc = do_QueryObject(rowAccessible);
> +  if (!rowAcc)
> +    return nsnull;

would you mind to file a bug to add a downcasting to nsXULTreeItemAccessibleBase (or alternatively to make GetTreeItemAccessible to return nsXULTreeItemAccessibleBase*)
Comment 3 Mark Capella [:capella] 2012-05-15 22:28:26 PDT
FYI, I had to leave the row and column indices check in the xpcAccessibleTable.cpp member function GetCellAt(), that I'd copied from the existing GetCellIndexAt() member. 

Without the check, we fail getCellAt() at line number 79 with NS_ERROR_ILLEGAL_VALUE ...
http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/table/test_table_2.html?force=1#77
Comment 4 alexander :surkov 2012-05-15 22:33:25 PDT
(In reply to Mark Capella [:capella] from comment #3)

> Without the check, we fail getCellAt() at line number 79 with
> NS_ERROR_ILLEGAL_VALUE ...
> http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/
> table/test_table_2.html?force=1#77

I'm curious what returns this value?
Comment 5 Mark Capella [:capella] 2012-05-15 22:40:13 PDT
I had changed all the nits above as requested, and found that one item to fail out. Putting it back causes the test to pass. 

Now there is an additional issue where test_general.html hangs ... I'm putting things back one at a time and testing to see what's going on.
Comment 6 alexander :surkov 2012-05-15 22:45:01 PDT
(In reply to Mark Capella [:capella] from comment #5)
> I had changed all the nits above as requested, and found that one item to
> fail out. Putting it back causes the test to pass. 

Yes, I get this but I don't understand the cause. I didn't see where you can return this error value.

> Now there is an additional issue where test_general.html hangs ... I'm
> putting things back one at a time and testing to see what's going on.

it shouldn't be related, btw, which test_general.html?
Comment 7 Mark Capella [:capella] 2012-05-16 00:14:21 PDT
My mistake, the hanging test is treeupdate/test_imagemap.html ... if I pull all my patches out of the queue and re-build and re-test, the problem is still there so my patch can't be responsible.

fwiw, if while the test is hanging, I click on a background window (shift the focus?) the test then continues, and runs to completion successfully.
Comment 8 Mark Capella [:capella] 2012-05-16 03:47:02 PDT
Created attachment 624336 [details] [diff] [review]
Patch (v2)

Next version of the patch with nits addressed, asking tbsaunde for review?.

FYI, a fresh pull, and a non-debug / clobber build resolved all test issues :)
Comment 9 Trevor Saunders (:tbsaunde) 2012-05-16 12:05:59 PDT
Comment on attachment 624336 [details] [diff] [review]
Patch (v2)

>+nsHTMLTableAccessible::CellAt(PRUint32 aRowIndex, PRUint32 aColumnIndex)
>+{ 
>   nsCOMPtr<nsIDOMElement> cellElement;
>-  nsresult rv = GetCellAt(aRow, aColumn, *getter_AddRefs(cellElement));
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  GetCellAt(aRowIndex, aColumnIndex, *getter_AddRefs(cellElement));
>+  if (!cellElement)
>+    return nsnull;
> 
>   nsCOMPtr<nsIContent> cellContent(do_QueryInterface(cellElement));
>+  if (!cellContent)
>+    return nsnull;
>+
>   nsAccessible* cell = mDoc->GetAccessible(cellContent);
>+  if (!cell)
>+    return nsnull;

you never derefrence it after this so no need for a check.

> 
>-  if (!cell) {
>-    return NS_ERROR_INVALID_ARG;
>+  if (cell == this) {
>+    // XXX bug 576838: crazy tables (like table6 in tables/test_table2.html) may
>+    // return itself as a cell what makes Orca hang.
>+    return nsnull;
>   }

nit, be nice to do

// XXX blah blah
// blahh
if (cell == this)
  return nsnull;

return cell;

>+  if (aRowIndex < 0 || aRowIndex >= mTable->RowCount() ||
>+      aColumnIndex < 0 || aColumnIndex >= mTable->ColCount())

you need to case aRowIdx / aColIdx to PRUint32 before comparing to RowCount() / ColCount() to keep gcc happy

>+nsXULListboxAccessible::CellAt(PRUint32 aRowIndex, PRUint32 aColumnIndex)
>+{ 
>   nsCOMPtr<nsIDOMXULSelectControlElement> control =
>     do_QueryInterface(mContent);
>+  if (!control)
>+    return nsnull;

you probably should warn here since mContent should have the right type given we created this type of accessible.
Comment 10 Mark Capella [:capella] 2012-05-17 08:10:35 PDT
Re: 
>+  if (aRowIndex < 0 || aRowIndex >= mTable->RowCount() ||
>+      aColumnIndex < 0 || aColumnIndex >= mTable->ColCount())
you need to case aRowIdx / aColIdx to PRUint32 before comparing to RowCount() / ColCount() to keep gcc happy

This code was cloned from existing member function GetCellIndexAt()
http://mxr.mozilla.org/mozilla-central/source/accessible/src/xpcom/xpcAccessibleTable.cpp#51

I don't see why gcc would complain about one and not the other, but I'm a WIN guy so it may not be obvious to me.

Can you elaborate?
Comment 11 Mark Capella [:capella] 2012-05-18 08:32:10 PDT
Created attachment 625119 [details] [diff] [review]
Patch (v3)

FYI, we've got feedback+ and review+... this would be the final version unless Trev has further details for the gcc comments.
Comment 12 Mark Capella [:capella] 2012-05-22 04:51:30 PDT
Push to inbound TRY
https://tbpl.mozilla.org/?tree=Try&rev=eea7a9606f4e
Comment 13 Mark Capella [:capella] 2012-05-22 09:53:17 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7a6feee6c13c
Comment 15 Trevor Saunders (:tbsaunde) 2012-05-22 11:17:34 PDT
(In reply to Mark Capella [:capella] from comment #10)
> Re: 
> >+  if (aRowIndex < 0 || aRowIndex >= mTable->RowCount() ||
> >+      aColumnIndex < 0 || aColumnIndex >= mTable->ColCount())
> you need to case aRowIdx / aColIdx to PRUint32 before comparing to
> RowCount() / ColCount() to keep gcc happy
> 
> This code was cloned from existing member function GetCellIndexAt()
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/xpcom/
> xpcAccessibleTable.cpp#51
> 
> I don't see why gcc would complain about one and not the other, but I'm a
> WIN guy so it may not be obvious to me.

it does complain about the other, but its only a warning.
Comment 16 Ed Morley [:emorley] 2012-05-23 08:03:08 PDT
https://hg.mozilla.org/mozilla-central/rev/7a6feee6c13c

Note You need to log in before you can comment on or make changes to this bug.