241571 – AX ITM: Crash in com.apple.WebKit.WebContent at Recursion :: com.apple.WebCore: WebCore::AXIsolatedTree::collectNodeChangesForSubtree

RESOLVED FIXED 241571

AX ITM: Crash in com.apple.WebKit.WebContent at Recursion :: com.apple.WebCore: WebCore::AXIsolatedTree::collectNodeChangesForSubtree

https://bugs.webkit.org/show_bug.cgi?id=241571

Summary AX ITM: Crash in com.apple.WebKit.WebContent at Recursion :: com.apple.WebCor...

Andres Gonzalez

Reported 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...

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
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2022-06-13 15:01:15 PDT

<rdar://problem/95058458>

Andres Gonzalez

Comment 2 2022-06-13 15:03:16 PDT

rdar://problem/94605574

Andres Gonzalez

Comment 3 2022-06-13 15:40:06 PDT

Created attachment 460217 [details] Patch

Andres Gonzalez

Comment 4 2022-06-13 17:46:00 PDT

Created attachment 460221 [details] Patch

Tyler Wilcock

Comment 5 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?

Andres Gonzalez

Comment 6 2022-06-14 08:32:12 PDT

Created attachment 460232 [details] Patch

Andres Gonzalez

Comment 7 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.

Andres Gonzalez

Comment 8 2022-06-14 09:13:05 PDT

Created attachment 460233 [details] Patch

Andres Gonzalez

Comment 9 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.

Andres Gonzalez

Comment 10 2022-06-14 11:14:34 PDT

Created attachment 460236 [details] Patch

Andres Gonzalez

Comment 11 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.

Andres Gonzalez

Comment 12 2022-06-15 12:08:36 PDT

Created attachment 460258 [details] Patch

Andres Gonzalez

Comment 13 2022-06-17 07:18:17 PDT

Created attachment 460296 [details] Patch

EWS

Comment 14 2022-06-17 07:46:23 PDT

'Tyler Wilcock' is not a reviewer, blocking PR #None

Andres Gonzalez

Comment 15 2022-06-17 07:51:46 PDT

Created attachment 460297 [details] Patch

EWS

Comment 16 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].

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component Accessibility

Assignee

Andres Gonzalez

Reported

2022-06-13 15:01 PDT

Modified

2022-06-17 08:45 PDT History

CC List

11 users Show

URL

Keywords InRadar

Depends on

Blocks