Bug 55883

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

(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.

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).

(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...

Created attachment 87332 [details] Patch proposal + Layout test As promised, here you have the patch for this.

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

(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. :-)

(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 :-)