Bug 236988

Summary: AX: AccessibilityObject::ariaTreeRows can crash in a deep hierarchy due to recursive descent
Product: WebKit Reporter: Tyler Wilcock <tyler_w>
Component: AccessibilityAssignee: Tyler Wilcock <tyler_w>
Status: NEW ---    
Severity: Normal CC: aakash_jain, aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch ews-feeder: commit-queue-

Description Tyler Wilcock 2022-02-21 12:01:39 PST 
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.
Comment 1 Radar WebKit Bug Importer 2022-02-21 12:01:55 PST 
<rdar://problem/89249539>
Comment 2 Tyler Wilcock 2022-02-21 12:13:16 PST 
Created attachment 452753 [details]
Patch
Comment 3 Tyler Wilcock 2022-02-21 12:27:31 PST 
Created attachment 452756 [details]
Patch
Comment 4 Andres Gonzalez 2022-02-21 12:44:02 PST 
(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?
Comment 5 Aakash Jain 2022-02-25 06:39:53 PST 
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
Comment 6 chris fleizach 2022-02-25 09:08:20 PST 
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?
Comment 7 Andres Gonzalez 2022-02-25 09:30:23 PST 
(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.