The default bug view has changed. See this FAQ.

Crash [@ AccIterator::GetNext()]

RESOLVED FIXED in mozilla8

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Michael Curran, Assigned: surkov)

Tracking

({crash})

Trunk
mozilla8
x86_64
Windows 7
crash
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(1 attachment, 1 obsolete attachment)

2.55 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Mick, can't reproduce it. Stack?

Comment 2

6 years ago
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()]

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

6 years ago
Created attachment 550612 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #550612 - Flags: review?(trev.saunders)
(Assignee)

Comment 4

6 years ago
Created attachment 550613 [details] [diff] [review]
patch

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

Comment 6

6 years ago
(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?
(Assignee)

Comment 7

6 years ago
(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
(Assignee)

Comment 8

6 years ago
(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+
(Assignee)

Comment 11

6 years ago
Ok, I added test for <div role="grid" id="grid4"><div role="row"></div></div>
(Assignee)

Comment 12

6 years ago
inbound landing http://hg.mozilla.org/integration/mozilla-inbound/rev/fff506a4889b
Flags: in-testsuite+
Whiteboard: [inbound]
(Assignee)

Comment 13

6 years ago
landed http://hg.mozilla.org/mozilla-central/rev/fff506a4889b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.