Bug 55883 – [Gtk] Consistent crash from Google/ARIA combobox click

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

Steps to reproduce:

1. view translate.google.com
2. click on the left-most combo box (source lang)

I reliably segfault: http://pastebin.com/VZHNginK

[Martin Robinson]

Do you need to have particular GTK+ modules enabled to see this crash?

[Joanmarie Diggs]

(In reply to comment #1)
> Do you need to have particular GTK+ modules enabled to see this crash?

A11y support.

$ env | grep GTK
GTK_MODULES=canberra-gtk-module:gail:atk-bridge

~~~~~~~~~

(In reply to a discussion Mario and I had via IRC)

I've looked at regular ol' combo boxes and confirmed that they are not spitting up. As discussed, 25531 is not for ARIA; 30796 is. Changing blockers.

[Mario Sanchez Prada]

It looks like the problem is an infinite loop cause by the call to firstChild() in the following snipped of code:

  AccessibilityObjectInclusion AccessibilityObject::accessibilityPlatformIncludesObject() const
  {
      [...]

      if (isGroup()) {
          // When a list item is made up entirely of children (e.g. paragraphs)
          // the list item gets ignored. We need it.
          if (parent->isList())
              return IncludeObject;

          // We expect the parent of a table cell to be a table.
          AccessibilityObject* child = firstChild();
          if (child && child->roleValue() == CellRole)
              return IgnoreObject;
      }
      [...]
  }


This snippet is normally not troublesome, but when the 'first child' is something with an ARIA role established to either 'option' or 'menuitem' (as it happens in this case), at some point during the execution of firstChild(), the following code will be executed:

  AccessibilityRole AccessibilityRenderObject::determineAriaRoleAttribute() const
  {
      [...]
      // selects and listboxes both have options as child roles, but they map to different roles within WebCore
      if (equalIgnoringCase(ariaRole, "option")) {
          if (parentObjectUnignored()->ariaRoleAttribute() == MenuRole)
              return MenuItemRole;
          if (parentObjectUnignored()->ariaRoleAttribute() == ListBoxRole)
              return ListBoxOptionRole;
      }
      // an aria "menuitem" may map to MenuButton or MenuItem depending on its parent
      if (equalIgnoringCase(ariaRole, "menuitem")) {
          if (parentObjectUnignored()->ariaRoleAttribute() == GroupRole)
              return MenuButtonRole;
          if (parentObjectUnignored()->ariaRoleAttribute() == MenuRole)
              return MenuItemRole;
      }
    
      return UnknownRole;
  }

As you can see parentObjectUnignored() is executed before determining the actual ARIA role for that first child, and this happens before returning the instance when it's the first time it's accessed (while creating the AX object through AXObjectCache), resulting in calling again to accessibilityPlatformIncludesObject() over the same instance as done at the beginning, hence starting a new iteration of our beloved infinite loop :P

Perhaps a way to get this fixed would be to try to avoid calling to firstChild() in accessibilityPlatformIncludesObject() as a rule of thumb, or just removing completely this code:

   [...]
   // We expect the parent of a table cell to be a table.
   AccessibilityObject* child = firstChild();
   if (child && child->roleValue() == CellRole)
       return IgnoreObject;
   [...]

... and then trying to get some equivalent code for that in case we think it's really needed after all (which is something I currently have doubts about, since just removing that fixed the bug and doesn't get any unit/layout test failing).

[Mario Sanchez Prada]

(In reply to comment #3)
> [...]
> Perhaps a way to get this fixed would be to try to avoid calling to 
> firstChild() in accessibilityPlatformIncludesObject() as a rule of thumb, or 
> just removing completely this code:
> 
>    [...]
>    // We expect the parent of a table cell to be a table.
>    AccessibilityObject* child = firstChild();
>    if (child && child->roleValue() == CellRole)
>        return IgnoreObject;
>    [...]
> 
> ... and then trying to get some equivalent code for that in case we think it's 
> really needed after all (which is something I currently have doubts about, 
> since just removing that fixed the bug and doesn't get any unit/layout test 
> failing).

I've just done some investigation and found out that these lines of code were introduced along with SVN revision 55623 (http://trac.webkit.org/changeset/55623), where a new layout test was also added: http://trac.webkit.org/changeset/55623

I've executed that test after completely removing those lines and still succeeds, as well as the rest of the non-skipped a11y tests, hence I think perhaps that code is no longer needed so it could be safely removed.

Expect a patch in a while, then...

[Mario Sanchez Prada]

Created attachment 87332 [details]
Patch proposal + Layout test

As promised, here you have the patch for this.

[Mario Sanchez Prada]

Committed r82296: <http://trac.webkit.org/changeset/82296>

[Joanmarie Diggs]

(In reply to comment #4)
> (In reply to comment #3)
> 
> I've executed that test after completely removing those lines and still succeeds, as well as the rest of the non-skipped a11y tests, hence I think perhaps that code is no longer needed so it could be safely removed.

Just for the record, I looked at some tables with the latest from master, and I'm not seeing any table weirdness like I was back in the days of that patch. So I believe you are correct. :-)

[Mario Sanchez Prada]

(In reply to comment #7)
> (In reply to comment #4)
> > (In reply to comment #3)
> > 
> > I've executed that test after completely removing those lines and still succeeds, as well as the rest of the non-skipped a11y tests, hence I think perhaps that code is no longer needed so it could be safely removed.
> 
> Just for the record, I looked at some tables with the latest from master, and I'm not seeing any table weirdness like I was back in the days of that patch. So I believe you are correct. :-)

That's great news! Thanks for sharing, Joanie :-)