Bug 122572 – AX: Crash at WebCore::accessibleNameForNode when visiting Facebook

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

[chris fleizach]

>  1 com.apple.WebCore              0x10bdb025f WebCore::accessibleNameForNode(WebCore::Node*) + 0x12f (AccessibilityNodeObject.cpp:1806)
   2 com.apple.WebCore              0x10bdb00e0 WebCore::AccessibilityNodeObject::accessibilityDescriptionForElements(WTF::Vector<WebCore::Element*, 0ul, WTF::CrashOnOverflow>&) const + 0x80 (AccessibilityNodeObject.cpp:1825)
   3 com.apple.WebCore              0x10bdb0579 WebCore::AccessibilityNodeObject::ariaLabeledByAttribute() const + 0x49 (AccessibilityNodeObject.cpp:1877)
   4 com.apple.WebCore              0x10bdae4f3 WebCore::AccessibilityNodeObject::ariaLabeledByText(WTF::Vector<WebCore::AccessibilityText, 0ul, WTF::CrashOnOverflow>&) const + 0x43 (AccessibilityNodeObject.cpp:1379)
   5 com.apple.WebCore              0x10bdadf25 WebCore::AccessibilityNodeObject::alternativeText(WTF::Vector<WebCore::AccessibilityText, 0ul, WTF::CrashOnOverflow>&) const + 0xb5 (AccessibilityNodeObject.cpp:1236)
   6 com.apple.WebCore              0x10bdaed03 WebCore::AccessibilityNodeObject::accessibilityText(WTF::Vector<WebCore::AccessibilityText, 0ul, WTF::CrashOnOverflow>&) + 0x43 (AccessibilityNodeObject.cpp:1368)
   7 com.apple.WebCore              0x10d7725fd -[WebAccessibilityObjectWrapperBase accessibilityDescription] + 0xbd (WebAccessibilityObjectWrapperBase.mm:198)
   8 com.apple.WebCore              0x10d77d195 -[WebAccessibilityObjectWrapper accessibilityAttributeValue:] + 0x1625 (WebAccessibilityObjectWrapperMac.mm:2177)
   9 com.apple.AppKit               0x7fff8884feb1 -[NSObject(NSAccessibilityInternal) _accessibilityValueForAttribute:clientError:] + 0xf2
  10 com.apple.AppKit               0x7fff88853f7d 


<rdar://problem/15189506>

[chris fleizach]

Looks like they're probably doing something like

<button aria-labeledby="item">

<div id="item" style="display:none">data</div>

[chris fleizach]

Created attachment 213818 [details]
patch

[Mario Sanchez Prada]

Comment on attachment 213818 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213818&action=review

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1814
> -    String text = node->document().axObjectCache()->getOrCreate(node)->textUnderElement();
> +    // If the node can be turned into an AX object, we can use standard name computation rules.
> +    // If however, the node cannot (because there's no renderer e.g.) fallback to using the basic text underneath.
> +    AccessibilityObject* axObject = node->document().axObjectCache()->getOrCreate(node);
> +    String text;
> +    if (axObject)
> +        text = axObject->textUnderElement();
> +    else if (node->isElementNode())
> +        text = toElement(node)->innerText();
> +    

I wonder if we could fix this issue in a different way, by making AXObjectCache::getOrCreate(Node* node) able to handle this kind of situation, creating a AccessibilityNodeObject that wraps that not rendered node inside.

What do you think?

[chris fleizach]

(In reply to comment #3)
> (From update of attachment 213818 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213818&action=review
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1814
> > -    String text = node->document().axObjectCache()->getOrCreate(node)->textUnderElement();
> > +    // If the node can be turned into an AX object, we can use standard name computation rules.
> > +    // If however, the node cannot (because there's no renderer e.g.) fallback to using the basic text underneath.
> > +    AccessibilityObject* axObject = node->document().axObjectCache()->getOrCreate(node);
> > +    String text;
> > +    if (axObject)
> > +        text = axObject->textUnderElement();
> > +    else if (node->isElementNode())
> > +        text = toElement(node)->innerText();
> > +    
> 
> I wonder if we could fix this issue in a different way, by making AXObjectCache::getOrCreate(Node* node) able to handle this kind of situation, creating a AccessibilityNodeObject that wraps that not rendered node inside.
> 
> What do you think?

Certainly getOrCreate could return an object, but there's a bit of code to ensure we only return a AXNode based object under certain conditions (either in a canvas or aria-hidden=false). Hard for me to say if there's any negative consequence of doing that. It might allow more elements into the tree than we would like...

For example, we definitely don't want to add objects that are just hidden to the tree, as in <div style="display:none;"> sub tree here </div>

So it could be done, but I am less sure of the consequences of doing that right now.

Thoughts

[Mario Sanchez Prada]

Comment on attachment 213818 [details]
patch

(In reply to comment #4)
> > I wonder if we could fix this issue in a different way, by making
> > AXObjectCache::getOrCreate(Node* node) able to handle this kind of situation,
> > creating a AccessibilityNodeObject that wraps that not rendered node inside.
> > 
> > What do you think?
> 
> Certainly getOrCreate could return an object, but there's a bit of code
> to ensure we only return a AXNode based object under certain conditions
> (either in a canvas or aria-hidden=false). Hard for me to say if there's
> any negative consequence of doing that. It might allow more elements
> into the tree than we would like...
> 
Yes, I've seen that code and it's not clear to me either. That's why I ask.

> For example, we definitely don't want to add objects that are just hidden
> to the tree, as in <div style="display:none;"> sub tree here </div>
> 
Agreed.

> So it could be done, but I am less sure of the consequences of doing that
> right now.
> 
Fair enough. Let's give this a try then :)

[WebKit Commit Bot]

Comment on attachment 213818 [details]
patch

Clearing flags on attachment: 213818

Committed r157221: <http://trac.webkit.org/changeset/157221>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.