Bug 268429 – AX: Crash in AXIsolatedTree::removeNode due to update of relations triggering a layout.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

frame #0: WebCore`WTFCrashWithInfo((null)=140, (null)="/Users/twilco/projects/web/OpenSource/Source/WebCore/rendering/updating/RenderTreeBuilder.cpp", (null)="WebCore::RenderTreeBuilder::RenderTreeBuilder(RenderView &)", (null)=136) at Assertions.h:778:5 [opt]
    frame #1: WebCore`WebCore::RenderTreeBuilder::RenderTreeBuilder(WebCore::RenderView&) (.cold.1) at RenderTreeBuilder.cpp:140:5 [opt]
    frame #2: WebCore`WebCore::RenderTreeBuilder::RenderTreeBuilder(this=0x000000016afcd0e8, view=<unavailable>) at RenderTreeBuilder.cpp:140:5 [opt]
    frame #3: WebCore`WebCore::RenderTreeUpdater::RenderTreeUpdater(WebCore::Document&, WebCore::Style::PostResolutionCallbackDisabler&) [inlined] WebCore::RenderTreeUpdater::RenderTreeUpdater(this=0x000000016afcd0b8, document=<unavailable>, (null)=<unavailable>) at RenderTreeUpdater.cpp:82:7 [opt]
    frame #4: WebCore`WebCore::RenderTreeUpdater::RenderTreeUpdater(this=0x000000016afcd0b8, document=<unavailable>, (null)=<unavailable>) at RenderTreeUpdater.cpp:83:1 [opt]
    frame #5: WebCore`WebCore::Document::updateRenderTree(this={ origin = , url = , inMainFrame = Detached, backForwardCacheState = NotInBackForwardCache }, styleUpdate=WebCore::Style::Update @ 0x0000000290b17790) at Document.cpp:2454:31 [opt]
    frame #6: WebCore`WebCore::Document::resolveStyle(this={ origin = , url = , inMainFrame = Detached, backForwardCacheState = NotInBackForwardCache }, type=<unavailable>) at Document.cpp:2553:13 [opt]
    frame #7: WebCore`WebCore::Document::updateStyleIfNeeded(this={ origin = , url = , inMainFrame = Detached, backForwardCacheState = NotInBackForwardCache }) at Document.cpp:2655:5 [opt]
    frame #8: WebCore`WebCore::Document::updateLayout(this={ origin = , url = , inMainFrame = Detached, backForwardCacheState = NotInBackForwardCache }, layoutOptions={ size = 1 }, context=0x0000000000000000) at Document.cpp:2699:13 [opt]
    frame #9: WebCore`WebCore::TextIterator::TextIterator(this=0x000000016afcd6c8, range=0x000000016afcd868, behaviors=<unavailable>) at TextIterator.cpp:357:38 [opt]
    frame #10: WebCore`WebCore::plainText(WebCore::SimpleRange const&, WTF::OptionSet<WebCore::TextIteratorBehavior>, bool) [inlined] WebCore::TextIterator::TextIterator(this=0x000000016afcd6c8, range=0x000000016afcd868, behaviors=<unavailable>) at TextIterator.cpp:356:1 [opt]
    frame #11: WebCore`WebCore::plainText(range=0x000000016afcd868, defaultBehavior=<unavailable>, isDisplayString=false) at TextIterator.cpp:2545:23 [opt]
  * frame #12: WebCore`WebCore::AccessibilityRenderObject::textUnderElement(this=0x00000002906a2c00, mode=AccessibilityTextUnderElementMode @ 0x0000600003fa2ac0) const at AccessibilityRenderObject.cpp:697:24 [opt]
    frame #13: WebCore`WebCore::AccessibilityNodeObject::textUnderElement(this=<unavailable>, mode=AccessibilityTextUnderElementMode @ 0x0000600003fa2b20) const at AccessibilityNodeObject.cpp:2327:35 [opt]
    frame #14: WebCore`WebCore::AccessibilityRenderObject::textUnderElement(this=0x00000002906a2900, mode=AccessibilityTextUnderElementMode @ 0x0000600003f804c0) const at AccessibilityRenderObject.cpp:718:37 [opt]
    frame #15: WebCore`WebCore::AccessibilityNodeObject::textUnderElement(this=<unavailable>, mode=AccessibilityTextUnderElementMode @ 0x0000600003f8e4e0) const at AccessibilityNodeObject.cpp:2327:35 [opt]
    frame #16: WebCore`WebCore::AccessibilityRenderObject::textUnderElement(this=0x00000002906a2a00, mode=AccessibilityTextUnderElementMode @ 0x0000600003f8e4e0) const at AccessibilityRenderObject.cpp:718:37 [opt]
    frame #17: WebCore`WebCore::AccessibilityNodeObject::textUnderElement(this=<unavailable>, mode=AccessibilityTextUnderElementMode @ 0x0000600003f80120) const at AccessibilityNodeObject.cpp:2327:35 [opt]
    frame #18: WebCore`WebCore::AccessibilityRenderObject::textUnderElement(this=0x00000002906a2b00, mode=AccessibilityTextUnderElementMode @ 0x0000600003f8eda0) const at AccessibilityRenderObject.cpp:718:37 [opt]
    frame #19: WebCore`WebCore::AccessibilityNodeObject::textUnderElement(this=<unavailable>, mode=AccessibilityTextUnderElementMode @ 0x0000600003f8dbc0) const at AccessibilityNodeObject.cpp:2327:35 [opt]
    frame #20: WebCore`WebCore::AccessibilityRenderObject::textUnderElement(this=0x0000000163cb2000, mode=AccessibilityTextUnderElementMode @ 0x0000600003f80120) const at AccessibilityRenderObject.cpp:718:37 [opt]
    frame #21: WebCore`WebCore::AccessibilityNodeObject::visibleText(this=0x0000000163cb2000, textOrder={ size = 2, capacity = 16 }) const at AccessibilityNodeObject.cpp:1934:23 [opt]
    frame #22: WebCore`WebCore::AccessibilityNodeObject::accessibilityText(this=0x0000000163cb2000, textOrder={ size = 2, capacity = 16 }) const at AccessibilityNodeObject.cpp:1984:5 [opt]
    frame #23: WebCore`WebCore::AXIsolatedObject::initializeProperties(this=0x00000002941e2680, axObject=<unavailable>) at AXIsolatedObject.cpp:310:12 [opt]
    frame #24: WebCore`WebCore::AXIsolatedObject::AXIsolatedObject(this=0x00000002941e2680, axObject=0x000000016afce100, tree=0x000000010d0f7c20) at AXIsolatedObject.cpp:59:5 [opt]
    frame #25: WebCore`WebCore::AXIsolatedTree::addUnconnectedNode(WTF::Ref<WebCore::AccessibilityObject, WTF::RawPtrTraits<WebCore::AccessibilityObject>>) [inlined] WebCore::AXIsolatedObject::AXIsolatedObject(this=0x00000002941e2680, axObject=0x000000016afce100, tree=0x000000010d0f7c20) at AXIsolatedObject.cpp:49:1 [opt]
    frame #26: WebCore`WebCore::AXIsolatedTree::addUnconnectedNode(WTF::Ref<WebCore::AccessibilityObject, WTF::RawPtrTraits<WebCore::AccessibilityObject>>) [inlined] WebCore::AXIsolatedObject::create(object=0x000000016afce100, tree=0x000000010d0f7c20) at AXIsolatedObject.cpp:64:26 [opt]
    frame #27: WebCore`WebCore::AXIsolatedTree::addUnconnectedNode(this=0x000000010d0f7c20, axObject=Ref<WebCore::AccessibilityObject, WTF::RawPtrTraits<WebCore::AccessibilityObject> > @ 0x000000016afce100) at AXIsolatedTree.cpp:321:19 [opt]
    frame #28: WebCore`WebCore::AXObjectCache::addRelation(this=0x000000010d241e80, origin=<unavailable>, target=0x0000000164df65a0, relationType=<unavailable>, addSymmetricRelation=Yes) at AXObjectCache.cpp:4784:23 [opt]
    frame #29: WebCore`WebCore::AXObjectCache::addRelation(this=<unavailable>, origin=<unavailable>, target=<unavailable>, relationType=<unavailable>) at AXObjectCache.cpp:4699:12 [opt] [artificial]
    frame #30: WebCore`WebCore::AXObjectCache::addRelation(this=0x000000010d241e80, origin=0x00000002906b1b00, attribute=<unavailable>) at AXObjectCache.cpp:4909:25 [opt]
    frame #31: WebCore`WebCore::AXObjectCache::updateRelationsForTree(this=0x000000010d241e80, rootNode={ origin = , url = , inMainFrame = Detached, backForwardCacheState = NotInBackForwardCache }) at AXObjectCache.cpp:4885:13 [opt]
    frame #32: WebCore`WebCore::AXObjectCache::relatedObjectIDsFor(WebCore::AXCoreObject const&, WebCore::AXRelationType) [inlined] WebCore::AXObjectCache::updateRelationsIfNeeded(this=0x000000010d241e80) at AXObjectCache.cpp:4867:5 [opt]
    frame #33: WebCore`WebCore::AXObjectCache::relatedObjectIDsFor(this=0x000000010d241e80, object=0x00000001634c6400, relationType=LabelFor) at AXObjectCache.cpp:5012:5 [opt]
    frame #34: WebCore`WebCore::AccessibilityObject::relatedObjects(this=0x00000001634c6400, relationType=LabelFor) const at AccessibilityObject.cpp:4295:36 [opt]
    frame #35: WebCore`WebCore::AXIsolatedTree::removeNode(WebCore::AccessibilityObject const&) [inlined] WebCore::AXCoreObject::labelForObjects(this=0x00000001634c6400) const at AXCoreObject.h:1024:66 [opt]
    frame #36: WebCore`WebCore::AXIsolatedTree::removeNode(WebCore::AccessibilityObject const&) [inlined] WebCore::AccessibilityObject::isLabel(this=0x00000001634c6400) const at AccessibilityObject.h:129:69 [opt]
    frame #37: WebCore`WebCore::AXIsolatedTree::removeNode(this=0x000000010d0f7c20, axObject=0x00000001634c6400) at AXIsolatedTree.cpp:1021:18 [opt]
    frame #38: WebCore`WebCore::AXObjectCache::remove(this=0x000000010d241e80, axID=WebCore::AXID @ 0x000000016afce3f0) at AXObjectCache.cpp:1076:15 [opt]
    frame #39: WebCore`WebCore::AXObjectCache::remove(this=0x000000010d241e80, renderer=0x0000000154058d80) at AXObjectCache.cpp:1095:5 [opt]
    frame #40: WebCore`WebCore::RenderObject::willBeDestroyed() (.cold.1) at RenderObject.cpp:1779:16 [opt]
    frame #41: WebCore`WebCore::RenderObject::willBeDestroyed() [inlined] WTF::RawPtrTraits<WebCore::Node>::unwrap(ptr=0x0000000154058d98) at RawPtrTraits.h:44:69 [opt]
    frame #42: WebCore`WebCore::RenderObject::willBeDestroyed() [inlined] WTF::CheckedRef<WebCore::Node, WTF::RawPtrTraits<WebCore::Node>>::ptr(this=0x0000000154058d98) const at CheckedRef.h:129:43 [opt]
    frame #43: WebCore`WebCore::RenderObject::willBeDestroyed() [inlined] WTF::CheckedRef<WebCore::Node, WTF::RawPtrTraits<WebCore::Node>>::get(this=0x0000000154058d98) const at CheckedRef.h:130:59 [opt]
    frame #44: WebCore`WebCore::RenderObject::willBeDestroyed() [inlined] WebCore::RenderObject::document(this=0x0000000154058d80) const at RenderObject.h:738:48 [opt]
    frame #45: WebCore`WebCore::RenderObject::willBeDestroyed(this=0x0000000154058d80) at RenderObject.cpp:1778:28 [opt]
    frame #46: WebCore`WebCore::RenderElement::willBeDestroyed(this=0x0000000154058d80) at RenderElement.cpp:1106:19 [opt]
    frame #47: WebCore`WebCore::RenderObject::destroy(this=0x0000000154058d80) at RenderObject.cpp:1834:5 [opt]
    frame #48: WebCore`WebCore::RenderTreeBuilder::destroy(WebCore::RenderObject&, WebCore::RenderTreeBuilder::CanCollapseAnonymousBlock) [inlined] std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>::reset[abi:v160006](this=0x000000016afce508, __p=0x0000000000000000) at unique_ptr.h:297:7 [opt]
    frame #49: WebCore`WebCore::RenderTreeBuilder::destroy(WebCore::RenderObject&, WebCore::RenderTreeBuilder::CanCollapseAnonymousBlock) [inlined] std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>::~unique_ptr[abi:v160006](this=0x000000016afce508) at unique_ptr.h:263:75 [opt]
    frame #50: WebCore`WebCore::RenderTreeBuilder::destroy(WebCore::RenderObject&, WebCore::RenderTreeBuilder::CanCollapseAnonymousBlock) [inlined] std::__1::unique_ptr<WebCore::RenderObject, WebCore::RenderObjectDeleter>::~unique_ptr[abi:v160006](this=0x000000016afce508) at unique_ptr.h:263:73 [opt]
    frame #51: WebCore`WebCore::RenderTreeBuilder::destroy(this=0x000000016afd0bd0, renderer=0x0000000154058d80, canCollapseAnonymousBlock=<unavailable>) at RenderTreeBuilder.cpp:188:1 [opt]
    frame #52: WebCore`WebCore::RenderTreeBuilder::destroyAndCleanUpAnonymousWrappers(this=0x000000016afd0bd0, rendererToDestroy=0x0000000154058d80) at RenderTreeBuilder.cpp:911:5 [opt]
    frame #53: WebCore`WebCore::RenderTreeUpdater::tearDownRenderers(WebCore::Element&, WebCore::RenderTreeUpdater::TeardownType, WebCore::RenderTreeBuilder&) at RenderTreeUpdater.cpp:746:25 [opt]
    frame #54: WebCore`WebCore::RenderTreeUpdater::tearDownRenderers(root=0x000000016c9d9800, teardownType=Full, builder=0x000000016afd0bd0) at RenderTreeUpdater.cpp:759:9 [opt]
    frame #55: WebCore`WebCore::RenderTreeUpdater::tearDownRenderers(root=0x000000016c9d9800) at RenderTreeUpdater.cpp:662:5 [opt]
    frame #56: WebCore`WebCore::ContainerNode::removeBetween(WebCore::Node*, WebCore::Node*, WebCore::Node&) [inlined] WebCore::destroyRenderTreeIfNeeded(child=0x000000016c9d9800) at ContainerNode.cpp:358:9 [opt]
    frame #57: WebCore`WebCore::ContainerNode::removeBetween(this=0x0000000154031e80, previousChild=0x0000000000000000, nextChild=0x000000016cacc800, oldChild=0x000000016c9d9800) at ContainerNode.cpp:707:5 [opt]
    frame #58: WebCore`WebCore::ContainerNode::removeChild(WebCore::Node&) at ContainerNode.cpp:232:9 [opt]
    frame #59: WebCore`WebCore::ContainerNode::removeChild(this=0x0000000154031e80, oldChild=0x000000016c9d9800) at ContainerNode.cpp:682:10 [opt]

[Radar WebKit Bug Importer]

<rdar://problem/121976907>

[Andres Gonzalez]

<rdar://problem/121976907>

[Andres Gonzalez]

Created attachment 469624 [details]
Patch

[Tyler Wilcock]

Comment on attachment 469624 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1031
> +    auto labeledObjectIDs = axObjectCache() ? axObjectCache()->relatedObjectIDsFor(axObject, AXRelationType::LabelFor, AXObjectCache::UpdateRelations::No) : std::nullopt;
> +    if (labeledObjectIDs) {
> +        // Update the labeled objects since axObject is one of their labels and it is being removed.
> +        for (AXID labeledObjectID : *labeledObjectIDs) {
>              // The label/title of an isolated object is computed based on its AccessibilityText propperty, thus update it.
> -            queueNodeUpdate(*labeledObject, { AXPropertyName::AccessibilityText });
> +            queueNodeUpdate(labeledObjectID, { AXPropertyName::AccessibilityText });

If we aren't updating relations here, and they are stale, doesn't that mean we could be making the decision to update or not update the tree based off the wrong information?

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 469624 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=469624&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1031
> > +    auto labeledObjectIDs = axObjectCache() ? axObjectCache()->relatedObjectIDsFor(axObject, AXRelationType::LabelFor, AXObjectCache::UpdateRelations::No) : std::nullopt;
> > +    if (labeledObjectIDs) {
> > +        // Update the labeled objects since axObject is one of their labels and it is being removed.
> > +        for (AXID labeledObjectID : *labeledObjectIDs) {
> >              // The label/title of an isolated object is computed based on its AccessibilityText propperty, thus update it.
> > -            queueNodeUpdate(*labeledObject, { AXPropertyName::AccessibilityText });
> > +            queueNodeUpdate(labeledObjectID, { AXPropertyName::AccessibilityText });
> 
> If we aren't updating relations here, and they are stale, doesn't that mean
> we could be making the decision to update or not update the tree based off
> the wrong information?

No, because the only thing we are doing here is to scheduling an update of a single property of the labeled objects of a label that is gone. We are not marking relations updated or any thing els, so next time things need update they will update.

[Tyler Wilcock]

Comment on attachment 469624 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1026
> +    auto labeledObjectIDs = axObjectCache() ? axObjectCache()->relatedObjectIDsFor(axObject, AXRelationType::LabelFor, AXObjectCache::UpdateRelations::No) : std::nullopt;

If we do decide to go this route rather than changing `addUnconnectedNode` or how it's called, it might be nice to have a comment explaining why we do this rather than just calling labelForObjects(). Moot point if you aren't keeping this.

[Andres Gonzalez]

Created attachment 469675 [details]
Patch

[EWS]

Commit message contains (OOPS!) and no reviewer found, blocking PR #None

[Andres Gonzalez]

Created attachment 469676 [details]
Patch

[EWS]

Committed 274003@main (7858210211c2): <https://commits.webkit.org/274003@main>

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