Bug 241571 - AX ITM: Crash in com.apple.WebKit.WebContent at Recursion :: com.apple.WebCore: WebCore::AXIsolatedTree::collectNodeChangesForSubtree
Summary: AX ITM: Crash in com.apple.WebKit.WebContent at Recursion :: com.apple.WebCor...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-06-13 15:01 PDT by Andres Gonzalez
Modified: 2022-06-17 08:45 PDT (History)
11 users (show)

See Also:


Attachments
Patch (8.01 KB, patch)
2022-06-13 15:40 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (8.98 KB, patch)
2022-06-13 17:46 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (8.94 KB, patch)
2022-06-14 08:32 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (9.46 KB, patch)
2022-06-14 09:13 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (10.35 KB, patch)
2022-06-14 11:14 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (11.13 KB, patch)
2022-06-15 12:08 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (11.08 KB, patch)
2022-06-17 07:18 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (11.06 KB, patch)
2022-06-17 07:51 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2022-06-13 15:01:07 PDT
3 WebCore::AccessibilityObject::isAXHidden() const
        3 WebCore::AccessibilityObject::defaultObjectInclusion() const
          3 WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const
            3 WebCore::AccessibilityObject::accessibilityIsIgnored() const
              5 WebCore::AccessibilityObject::insertChild(WebCore::AXCoreObject*, unsigned int, WebCore::AccessibilityObject::DescendIfIgnored)
                6 WebCore::AccessibilityScrollView::updateChildrenIfNecessary()
                  6 WebCore::AccessibilityObject::children(bool)
      4 WebCore::AXObjectCache::get(WebCore::Node*)
        4 WebCore::AXObjectCache::getOrCreate(WebCore::Node*)
          5 WebCore::AccessibilityObject::isAXHidden() const
            6 WebCore::AccessibilityObject::defaultObjectInclusion() const
              7 WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const
                7 WebCore::AccessibilityObject::accessibilityIsIgnored() const
                  7 WebCore::AccessibilityRenderObject::parentObjectUnignored() const
             ==> 13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&) <==
                      13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                        13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                          13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                            13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                              13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                  13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                    13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                      13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                        13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                          13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                            13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                              13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                  13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                    13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                      13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                        13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                          13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                            13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                              13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                  13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                    13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                      13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                        13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                          13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                            13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                              13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                  13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                    13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                      13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                        13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                          13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                            13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                              13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                                13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                                  13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                                    13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                                      13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                                        13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                                          13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                                            13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                                              13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                                                13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                                                  13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                                                    13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                                                      13 WebCore::AXIsolatedTree::collectNodeChangesForSubtree(WebCore::AXCoreObject&)
                                                                                                                        truncating...
Comment 1 Radar WebKit Bug Importer 2022-06-13 15:01:15 PDT
<rdar://problem/95058458>
Comment 2 Andres Gonzalez 2022-06-13 15:03:16 PDT
rdar://problem/94605574
Comment 3 Andres Gonzalez 2022-06-13 15:40:06 PDT
Created attachment 460217 [details]
Patch
Comment 4 Andres Gonzalez 2022-06-13 17:46:00 PDT
Created attachment 460221 [details]
Patch
Comment 5 Tyler Wilcock 2022-06-13 19:06:01 PDT
Comment on attachment 460221 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:106
> +    ASSERT(tree->m_maxTreeDepth);

Neither the render tree nor the DOM can be greater than 512 layers deep (i.e. maximumHTMLParserDOMTreeDepth), so would it even be possible for us to have an isolated tree greater than 512 layers? i.e., is this m_maxTreeDepth limit of 512 a change in behavior?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:290
> +        AXLOG("aqui estoy");

Could we change this log to something like "Bailing collectNodeChangesForSubtree for AXID %s because the tree was deeper than %d levels". Did we hit this log in your testing?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:307
> +    axChildrenIDs.shrinkToFit();

The only time this will ever shrink anything is when `!axChild || axChild.get() == &axObject`, which we don't even know is possible (unless you could repro it?). With that in mind, do you think this call to shrinkToFit is necessary?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:323
> +    if (m_collectingAtTreeLevel) {

I feel like this `if` doesn't read as intuitively as before. What if we added a method named `isCollectingNodeChanges` that is `m_collectingAtTreeLevel >= 1`? Or is there some other way to recover the readability?

> LayoutTests/accessibility/deep-tree.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Did this test crash before your patch?
Comment 6 Andres Gonzalez 2022-06-14 08:32:12 PDT
Created attachment 460232 [details]
Patch
Comment 7 Andres Gonzalez 2022-06-14 08:49:35 PDT
(In reply to Tyler Wilcock from comment #5)
> Comment on attachment 460221 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=460221&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:106
> > +    ASSERT(tree->m_maxTreeDepth);
> 
> Neither the render tree nor the DOM can be greater than 512 layers deep
> (i.e. maximumHTMLParserDOMTreeDepth), so would it even be possible for us to
> have an isolated tree greater than 512 layers? i.e., is this m_maxTreeDepth
> limit of 512 a change in behavior?

It is a change in behavior if, as we suspect, children() may be returning an object in the ancestry of parent, thus creating a circular reference. This may be happening because of miss-use of aria-owns or some other bug. This is not the fix for that bug if in fact it exists, but it would prevent a stack overflow.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:290
> > +        AXLOG("aqui estoy");
> 
> Could we change this log to something like "Bailing
> collectNodeChangesForSubtree for AXID %s because the tree was deeper than %d
> levels". Did we hit this log in your testing?

Removed, debugging leftover that forgot to remove.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:307
> > +    axChildrenIDs.shrinkToFit();
> 
> The only time this will ever shrink anything is when `!axChild ||
> axChild.get() == &axObject`, which we don't even know is possible (unless
> you could repro it?). With that in mind, do you think this call to
> shrinkToFit is necessary?

It is possible to hit the above condition, so the resulting vector of IDs may indeed be shorter than the original and hence shrinkable. Vector.map was doing the shrinking before, so I chose to maintain that behavior.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:323
> > +    if (m_collectingAtTreeLevel) {
> 
> I feel like this `if` doesn't read as intuitively as before. What if we
> added a method named `isCollectingNodeChanges` that is
> `m_collectingAtTreeLevel >= 1`? Or is there some other way to recover the
> readability?
> 
> > LayoutTests/accessibility/deep-tree.html:1
> > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> Did this test crash before your patch?

No, unfortunately couldn't get it to crash in LayoutTests. I think it is worth to add this layout test as a bellwether.
Comment 8 Andres Gonzalez 2022-06-14 09:13:05 PDT
Created attachment 460233 [details]
Patch
Comment 9 Andres Gonzalez 2022-06-14 09:16:51 PDT
(In reply to Tyler Wilcock from comment #5)
> Comment on attachment 460221 [details]
> Patch
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:323
> > +    if (m_collectingAtTreeLevel) {
> 
> I feel like this `if` doesn't read as intuitively as before. What if we
> added a method named `isCollectingNodeChanges` that is
> `m_collectingAtTreeLevel >= 1`? Or is there some other way to recover the
> readability?

Done.
Comment 10 Andres Gonzalez 2022-06-14 11:14:34 PDT
Created attachment 460236 [details]
Patch
Comment 11 Andres Gonzalez 2022-06-14 11:18:24 PDT
(In reply to Andres Gonzalez from comment #9)
> (In reply to Tyler Wilcock from comment #5)
> > Comment on attachment 460221 [details]
> > Patch
> > 
> > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:323
> > > +    if (m_collectingAtTreeLevel) {
> > 
> > I feel like this `if` doesn't read as intuitively as before. What if we
> > added a method named `isCollectingNodeChanges` that is
> > `m_collectingAtTreeLevel >= 1`? Or is there some other way to recover the
> > readability?
> 
> Done.

Renamed m_collectingAtTreeLevel -> m_collectingNodeChangesAtTreeLevel to make it more explicit.
Comment 12 Andres Gonzalez 2022-06-15 12:08:36 PDT
Created attachment 460258 [details]
Patch
Comment 13 Andres Gonzalez 2022-06-17 07:18:17 PDT
Created attachment 460296 [details]
Patch
Comment 14 EWS 2022-06-17 07:46:23 PDT
'Tyler Wilcock' is not a reviewer, blocking PR #None
Comment 15 Andres Gonzalez 2022-06-17 07:51:46 PDT
Created attachment 460297 [details]
Patch
Comment 16 EWS 2022-06-17 08:45:32 PDT
Committed r295636 (251641@main): <https://commits.webkit.org/251641@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 460297 [details].