Bug 241735 – AX: AXIsolatedTree::updateChildren sometimes fails to update isolated subtrees when the given live object is ignored

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

This causes us to fail to expose webpage content.

[Radar WebKit Bug Importer]

<rdar://problem/95424726>

[Tyler Wilcock]

Created attachment 460323 [details]
Patch

[chris fleizach]

Comment on attachment 460323 [details]
Patch

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

> COMMIT_MESSAGE:12
> +  in-isolated tree ancestor, and update the children of that

Un isolated?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:533
> +                AXLOG(liveChild);

Do we still need this logging?

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #2)
> Created attachment 460323 [details]
> Patch

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp

-#ifndef NDEBUG
     if (axAncestor != &axObject) {
         AXLOG(makeString("Original object with ID ", axObject.objectID().loggingString(), " wasn't in the isolated tree, so instead updating the closest in-isolated-tree ancestor:"));
         AXLOG(axAncestor);
+        for (auto& child : axObject.children()) {
+            auto* liveChild = dynamicDowncast<AccessibilityObject>(child.get());
+            if (liveChild && !liveChild->childrenInitialized() && m_nodeMap.contains(liveChild->objectID())) {
+                AXLOG(makeString(
+                    "Child ID ", liveChild->objectID().loggingString(), " of original object ID ", axObject.objectID().loggingString(), " was found in the isolated tree with uninitialized live children. Updating its isolated children."
+                ));
+                AXLOG(liveChild);
+                updateChildren(*liveChild);

When we hit this condition, and then continue execution into the following for loop, what is preventing from generating the subtree rooted at liveChild twice in :

    for (size_t i = 0; i < newChildren.size(); ++i) {
...
            collectNodeChangesForSubtree(*newChildren[i]);
...
    }

The assumption here is that the ChildrenChangedNotification is triggered because of one or more new children, namely the children of axObject such as liveChild->childrenInitialized() is false. But it wouldn't cover the case of an axObject's child being removed.

[Tyler Wilcock]

Created attachment 460379 [details]
Patch

[Tyler Wilcock]

> +                AXLOG(liveChild);
> +                updateChildren(*liveChild);
> 
> When we hit this condition, and then continue execution into the following
> for loop, what is preventing from generating the subtree rooted at liveChild
> twice in :
> 
>     for (size_t i = 0; i < newChildren.size(); ++i) {
> ...
>             collectNodeChangesForSubtree(*newChildren[i]);
> ...
>     }
Great catch! I've fixed this by adding a new `ResolveNodeChanges` parameter to `updateChildren` so we don't resolve the node changes (which is the most expensive part) in these sub / recursive calls, instead doing it only once in the "main" updateChildren call.

> The assumption here is that the ChildrenChangedNotification is triggered
> because of one or more new children, namely the children of axObject such as
> liveChild->childrenInitialized() is false. But it wouldn't cover the case of
> an axObject's child being removed.
Also a good point. Upon investigating, I think we're fine, since the only time we remove children without fulling clearing them out and setting m_childrenInitialized = false is in AccessibilityScrollView::removeChildScrollbar.

> COMMIT_MESSAGE:12
> +  in-isolated tree ancestor, and update the children of that
> Un isolated?
"in-isolated tree" was intended. I changed this to "in-isolated-tree" to indicate these words are all linked. Maybe there's a better way to say this though.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:533
> +                AXLOG(liveChild);
> Do we still need this logging?
I removed AXLOG(liveChild) but kept the other added log. I find this type of logging really helpful when debugging tree correctness issues, and I think the other one shouldn't be too noisy (but we can remove later if it is).

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #6)
> > +                AXLOG(liveChild);
> > +                updateChildren(*liveChild);
> > 
> > When we hit this condition, and then continue execution into the following
> > for loop, what is preventing from generating the subtree rooted at liveChild
> > twice in :
> > 
> >     for (size_t i = 0; i < newChildren.size(); ++i) {
> > ...
> >             collectNodeChangesForSubtree(*newChildren[i]);
> > ...
> >     }
> Great catch! I've fixed this by adding a new `ResolveNodeChanges` parameter
> to `updateChildren` so we don't resolve the node changes (which is the most
> expensive part) in these sub / recursive calls, instead doing it only once
> in the "main" updateChildren call.

Couldn't we instead just return after the new loop since at that point we already processed the ChildrenChanged notification for the given target? I.e.:

     if (axAncestor != &axObject) {
         AXLOG(makeString("Original object with ID ", axObject.objectID().loggingString(), " wasn't in the isolated tree, so instead updating the closest in-isolated-tree ancestor:"));
         AXLOG(axAncestor);
         bool childUpdated = false;
+        for (auto& child : axObject.children()) {
+            auto* liveChild = dynamicDowncast<AccessibilityObject>(child.get());
+            if (liveChild && !liveChild->childrenInitialized() && m_nodeMap.contains(liveChild->objectID())) {
+                AXLOG(makeString(
+                    "Child ID ", liveChild->objectID().loggingString(), " of original object ID ", axObject.objectID().loggingString(), " was found in the isolated tree with uninitialized live children. Updating its isolated children."
+                ));
+                updateChildren(*liveChild);
                 childUpdated = true;
+            }
+        }
         if (childUpdated)
             return;
     }

In other words, if we update downstream, we may not need to update from upstream.

Do we need to update the isolated object corresponding to liveChild? updateChildren only updates the children but not the target.

If the new child is such that !m_nodeMap.contains(liveChild->objectID()), then we wouldn't update it. Do we need that check in the above if statement?

[Tyler Wilcock]

Created attachment 460401 [details]
Patch

[Tyler Wilcock]

rdar://95344512

[Tyler Wilcock]

Created attachment 460434 [details]
Patch

[Tyler Wilcock]

Created attachment 460439 [details]
Patch

[EWS]

Committed r295779 (251784@main): <https://commits.webkit.org/251784@main>

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

[Wmwm]

Created attachment 461606 [details]
Shah117363?.