Bug 31035 – [GTK] some accessibility tests hitting assertion in debug builds

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Gustavo Noronha (kov)]

The following tests are hitting an assertion, which is being logged to stderr, on debug builds:

accessibility/aria-controls-with-tabs.html
accessibility/nochildren-elements.html
accessibility/non-data-table-cell-title-ui-element.html
accessibility/table-notbody.html

Here's the output:

** (DumpRenderTree:26309): CRITICAL **: AtkObject* webkit_accessible_ref_child(AtkObject*, gint): assertion `static_cast<size_t>(index) < coreObject->children().size()' failed

** (DumpRenderTree:26309): CRITICAL **: atk_object_ref_accessible_child: assertion `ATK_IS_OBJECT (accessible)' failed

** (DumpRenderTree:26309): CRITICAL **: atk_object_ref_accessible_child: assertion `ATK_IS_OBJECT (accessible)' failed

[Joanmarie Diggs]

Created attachment 42359 [details]
Replace assertions with sanity checks

===================
table-notbody.html
===================

The body elements consist of a single table which:

* Has no cells
* Has style "visibility: hidden;"

As a result it's ignored as an accessible object and the web view is childless. Then along comes the layout test:

  [...]
  body.focus();
  var table = accessibilityController.focusedElement.childAtIndex(0);
  result.innerText += "Test passes if there is no crash\n\n";
  [...]

Asking for a non-existent child triggers the assertion.

=========================
nochildren-elements.html
=========================

Similar deal.

[...]
<!-- This test makes sure that these types of elements DO NOT have children.
     The test will pass if attributesOfChildren() returns nil -->
[...]

~~~~~~~~~~~~~~~

I suspect that is also what's taking place with accessibility/non-data-table-cell-title-ui-element.html. I've not fully debugged it to prove that beyond a shadow of a doubt, but the attached patch seems to solve it along with the above two tests.

This is just my opinion, but I don't think webkit_accessible_ref_child is the place for assertions like this because anyone

  * these tests
  * badly-behaved ATs
  * me typing acc.getChildAtIndex(10000000) in Accerciser's iPython console

can easily attempt to ref a child at a bogus index. As such, I think that in this particular case, it makes more sense to do some sanity checking.

As for accessibility/aria-controls-with-tabs.html: Something else is going on there. My two main, totally-unproven theories right now are the hierarchy:

  * Root ain't what the test thinks it is (perhaps its the object of ATK_ROLE_DOCUMENT_FRAME), or
  * We've got "ignored" objects sneaking in

I'll dig some more.

[Joanmarie Diggs]

Created attachment 42363 [details]
Get the correct Gtk+ object before attempting to turn it into an AtkObject

Funny thing about totally-unproven theories: There's a high probability of you're being wrong. :-) I was wrong: Test is fine, hierarchy (at least in this particular instance) is fine. Getting the rootElement, on the other hand....

This solves it for me.

[Xan Lopez]

Comment on attachment 42363 [details]
Get the correct Gtk+ object before attempting to turn it into an AtkObject

Yeah, seems the other ports do pretty much the same, so r+!

[WebKit Commit Bot]

Comment on attachment 42363 [details]
Get the correct Gtk+ object before attempting to turn it into an AtkObject

Clearing flags on attachment: 42363

Committed r50445: <http://trac.webkit.org/changeset/50445>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jan Alonzo]

Still one more patch for review.

[Jan Alonzo]

Comment on attachment 42359 [details]
Replace assertions with sanity checks

> +    AccessibilityObject::AccessibilityChildrenVector children = coreObject->children();
> +    if (index < 0 || index >= children.size())
> +        return 0;
>  
>      AccessibilityObject* coreChild = coreObject->children().at(index).get();

It would be nice if we can just reuse children here instead of calling to coreObject again.

r- because it needs one more round.

[Joanmarie Diggs]

Created attachment 42426 [details]
Replace assertions with sanity checks - take 2

(In reply to comment #7)
> (From update of attachment 42359 [details])
> > +    AccessibilityObject::AccessibilityChildrenVector children = coreObject->children();
> > +    if (index < 0 || index >= children.size())
> > +        return 0;
> >  
> >      AccessibilityObject* coreChild = coreObject->children().at(index).get();
> 
> It would be nice if we can just reuse children here instead of calling to
> coreObject again.

Yup. Done. Thanks for the review!

[Jan Alonzo]

Comment on attachment 42426 [details]
Replace assertions with sanity checks - take 2

Thanks!

[WebKit Commit Bot]

Comment on attachment 42426 [details]
Replace assertions with sanity checks - take 2

Clearing flags on attachment: 42426

Committed r50506: <http://trac.webkit.org/changeset/50506>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.