Bug 236988 – AX: AccessibilityObject::ariaTreeRows can crash in a deep hierarchy due to recursive descent

• Status: NEW
• Component: Accessibility

[Tyler Wilcock]

AccessibilityObject::ariaTreeRows calls itself recursively on its child objects all the way down to the leaves. Although WebKit tree depth is limited to 512 levels (https://github.com/WebKit/WebKit/blob/2077b50205f4d8f943b88e233302b52c8b4699af/Source/WebCore/page/SettingsBase.h#L72#L73), this can still sometimes cause a stack overflow.

ITM is especially vulnerable to this, as currently we call ariaTreeRows on every single isolated object we create.

[Radar WebKit Bug Importer]

<rdar://problem/89249539>

[Tyler Wilcock]

Created attachment 452753 [details]
Patch

[Tyler Wilcock]

Created attachment 452756 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Created attachment 452756 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityObject.cpp

+    // This method descends recursively, so avoid stack overflow by bailing early if we're >450 levels away from where we started.
+    if (ancestors.size() > 450)

Can we add the comment here about the arbitrary choice of 450 and a FIXME for making this non-recursive?

Does the test fail/crash without this change?

[Aakash Jain]

I cancelled https://ews-build.webkit.org/#/builders/68/builds/9128 to speed up ios-wk2 queue. It finished running layout-tests and indicated accessibility/deep-aria-tree-rows-crash.html failure

[chris fleizach]

Comment on attachment 452756 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1905
>      

The code for going through aria owns and descendants looks identical. Can we merge those blocks?
450 seems arbitrarily large. Can we cap this to 10 and still be sufficient?

[Andres Gonzalez]

(In reply to chris fleizach from comment #6)
> Comment on attachment 452756 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452756&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1905
> >      
> 
> The code for going through aria owns and descendants looks identical. Can we
> merge those blocks?
> 450 seems arbitrarily large. Can we cap this to 10 and still be sufficient?

I think we agreed that this change is not needed, so this can be closed.