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...
<rdar://problem/95058458>
rdar://problem/94605574
Created attachment 460217 [details] Patch
Created attachment 460221 [details] Patch
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?
Created attachment 460232 [details] Patch
(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.
Created attachment 460233 [details] Patch
(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.
Created attachment 460236 [details] Patch
(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.
Created attachment 460258 [details] Patch
Created attachment 460296 [details] Patch
'Tyler Wilcock' is not a reviewer, blocking PR #None
Created attachment 460297 [details] Patch
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].