Closed Bug 675861 Opened 9 years ago Closed 9 years ago

Crash [@ AccIterator::GetNext()]

Categories

(Core :: Disability Access APIs, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: mick, Assigned: surkov)

References

()

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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.
Mick, can't reproduce it. Stack?
bp-092a56e9-75e1-435a-acbb-c8b0c2110803

Str:
1. Visit http://docs.google.com/ with NVDA loaded.
2. Log in if requested.
Result: Crash!
Crash Signature: [@ AccIterator::GetNext()]
Component: Disability Access → Disability Access APIs
Keywords: crash
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis
Summary: Crash [@ AccIterator::GetNext] → Crash [@ AccIterator::GetNext()]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #550612 - Flags: review?(trev.saunders)
Attached patch patchSplinter Review
correct patch
Attachment #550612 - Attachment is obsolete: true
Attachment #550612 - Flags: review?(trev.saunders)
Attachment #550613 - Flags: review?(trev.saunders)
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?
(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?
(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
(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.
(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
(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
Attachment #550613 - Flags: review?(trev.saunders) → review+
Ok, I added test for <div role="grid" id="grid4"><div role="row"></div></div>
inbound landing http://hg.mozilla.org/integration/mozilla-inbound/rev/fff506a4889b
Flags: in-testsuite+
Whiteboard: [inbound]
landed http://hg.mozilla.org/mozilla-central/rev/fff506a4889b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.