Last Comment Bug 675861 - Crash [@ AccIterator::GetNext()]
: Crash [@ AccIterator::GetNext()]
Status: RESOLVED FIXED
: crash
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla8
Assigned To: alexander :surkov
:
:
Mentors:
http://docs.google.com/
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-01 21:52 PDT by Michael Curran
Modified: 2011-08-05 08:51 PDT (History)
3 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (940 bytes, patch)
2011-08-03 23:34 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch (2.55 KB, patch)
2011-08-03 23:35 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Michael Curran 2011-08-01 21:52:41 PDT
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 2011-08-01 21:58:56 PDT
Mick, can't reproduce it. Stack?
Comment 2 James Teh [:Jamie] 2011-08-03 22:11:04 PDT
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 2011-08-03 23:34:23 PDT
Created attachment 550612 [details] [diff] [review]
patch
Comment 4 alexander :surkov 2011-08-03 23:35:45 PDT
Created attachment 550613 [details] [diff] [review]
patch

correct patch
Comment 5 Trevor Saunders (:tbsaunde) 2011-08-04 13:08:24 PDT
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 2011-08-04 21:21:53 PDT
(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 2011-08-04 21:39:51 PDT
(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 2011-08-04 21:45:58 PDT
(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) 2011-08-04 23:26:55 PDT
(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) 2011-08-04 23:30:22 PDT
(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 2011-08-05 01:09:10 PDT
Ok, I added test for <div role="grid" id="grid4"><div role="row"></div></div>
Comment 12 alexander :surkov 2011-08-05 01:13:14 PDT
inbound landing http://hg.mozilla.org/integration/mozilla-inbound/rev/fff506a4889b
Comment 13 alexander :surkov 2011-08-05 08:51:54 PDT
landed http://hg.mozilla.org/mozilla-central/rev/fff506a4889b

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