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.
<rdar://problem/89249539>
Created attachment 452753 [details] Patch
Created attachment 452756 [details] Patch
(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?
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 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?
(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.