675861 - Crash [@ AccIterator::GetNext()]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Michael Curran]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110801 Firefox/8.0a1
Build ID: 20110801030916



Actual results:

Crash when visiting docs.google.com (logged in) and running NVDA.


Expected results:

No crash.

Comment 1

[alexander :surkov (:asurkov)]

Mick, can't reproduce it. Stack?

Comment 2

[James Teh [:Jamie]]

bp-092a56e9-75e1-435a-acbb-c8b0c2110803

Str:
1. Visit http://docs.google.com/ with NVDA loaded.
2. Log in if requested.
Result: Crash!

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch

correct patch

Comment 5

[Trevor Saunders (:tbsaunde)]

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

>diff --git a/accessible/src/base/nsARIAGridAccessible.cpp b/accessible/src/base/nsARIAGridAccessible.cpp
>--- a/accessible/src/base/nsARIAGridAccessible.cpp
>+++ b/accessible/src/base/nsARIAGridAccessible.cpp
>@@ -101,17 +101,19 @@ nsARIAGridAccessible::GetColumnCount(PRI
> {
>   NS_ENSURE_ARG_POINTER(acolumnCount);
>   *acolumnCount = 0;
> 
>   if (IsDefunct())
>     return NS_ERROR_FAILURE;
> 
>   AccIterator rowIter(this, filters::GetRow);
>-  nsAccessible *row = rowIter.GetNext();
>+  nsAccessible* row = rowIter.GetNext();
>+  if (!row)
>+    return NS_OK;

btw I'll introduce some trivially fixable bit    rot  if I land 641838 first,  or you'll do the same to me either way :-)

>diff --git a/accessible/tests/mochitest/table/test_struct_ariagrid.html b/accessible/tests/mochitest/table/test_struct_ariagrid.html
>--- a/accessible/tests/mochitest/table/test_struct_ariagrid.html
>+++ b/accessible/tests/mochitest/table/test_struct_ariagrid.html

add bug link?

>+  <!-- Wrong markup ARIA grid -->
>+  <div role="grid" id="grid3"></div>
> </body>
> </html>

what about the cases of a table with a row but no cells, or cells directly under the table with no rows? are we protected from those, or do we test them somewhere else?

also why this file not test_struct_ariagrid.html or test_struct_ariatreegrid.html?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> btw I'll introduce some trivially fixable bit    rot  if I land 641838
> first,  or you'll do the same to me either way :-)

the existing patch of bug 641838 doesn't contain this check.

> add bug link?

ok

> >+  <!-- Wrong markup ARIA grid -->
> >+  <div role="grid" id="grid3"></div>
> > </body>
> > </html>
> 
> what about the cases of a table with a row but no cells, or cells directly
> under the table with no rows? are we protected from those, or do we test
> them somewhere else?

No possible crashes I can see, no tests but I think existing behavior is not correct, for example, having one row with no cells we return 1 as row count (while 0 make sense).

> also why this file not test_struct_ariagrid.html or
> test_struct_ariatreegrid.html?

why should it be?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> No possible crashes I can see, no tests but I think existing behavior is not
> correct, for example, having one row with no cells we return 1 as row count
> (while 0 make sense).

though maybe it's ok to expose what we have

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> > also why this file not test_struct_ariagrid.html or
> > test_struct_ariatreegrid.html?
> 
> why should it be?

maybe I interpreted your comment wrong. I added test to test_struct_ariagrid.html, I don't see a reason why it could be test_struct_ariatreegrid.html.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #8)
> (In reply to comment #6)
> 
> > > also why this file not test_struct_ariagrid.html or
> > > test_struct_ariatreegrid.html?
> > 
> > why should it be?
> 
> maybe I interpreted your comment wrong. I added test to
> test_struct_ariagrid.html, I don't see a reason why it could be
> test_struct_ariatreegrid.html.

ignore it, I read the diff header wrong

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to alexander surkov from comment #6)
> (In reply to comment #5)
> 
> > btw I'll introduce some trivially fixable bit    rot  if I land 641838
> > first,  or you'll do the same to me either way :-)
> 
> the existing patch of bug 641838 doesn't contain this check.

I was refering to renameing Getnext() -> Next()

> > >+  <!-- Wrong markup ARIA grid -->
> > >+  <div role="grid" id="grid3"></div>
> > > </body>
> > > </html>
> > 
> > what about the cases of a table with a row but no cells, or cells directly
> > under the table with no rows? are we protected from those, or do we test
> > them somewhere else?
> 
> No possible crashes I can see, no tests but I think existing behavior is not
> correct, for example, having one row with no cells we return 1 as row count
> (while 0 make sense).

well, maybe what we have makes sense, or  we can add tests that are expected to fail right? it would seem that's worth to do in case we fix accidentally we can just enable the test

Comment 11

[alexander :surkov (:asurkov)]

Ok, I added test for <div role="grid" id="grid4"><div role="row"></div></div>

Comment 12

[alexander :surkov (:asurkov)]

inbound landing http://hg.mozilla.org/integration/mozilla-inbound/rev/fff506a4889b

Comment 13

[alexander :surkov (:asurkov)]

landed http://hg.mozilla.org/mozilla-central/rev/fff506a4889b