RESOLVED FIXED 241735
AX: AXIsolatedTree::updateChildren sometimes fails to update isolated subtrees when the given live object is ignored
https://bugs.webkit.org/show_bug.cgi?id=241735
Summary AX: AXIsolatedTree::updateChildren sometimes fails to update isolated subtree...
Tyler Wilcock
Reported 2022-06-17 17:26:12 PDT
This causes us to fail to expose webpage content.
Attachments
Patch (3.78 KB, patch)
2022-06-17 17:37 PDT, Tyler Wilcock
no flags
Patch (5.85 KB, patch)
2022-06-21 13:06 PDT, Tyler Wilcock
no flags
Patch (7.58 KB, patch)
2022-06-21 19:45 PDT, Tyler Wilcock
no flags
Patch (7.60 KB, patch)
2022-06-22 16:24 PDT, Tyler Wilcock
ews-feeder: commit-queue-
Patch (7.61 KB, patch)
2022-06-22 22:53 PDT, Tyler Wilcock
no flags
Shah117363?. (967 bytes, application/xml)
2022-08-14 00:00 PDT, Wmwm
no flags
Radar WebKit Bug Importer
Comment 1 2022-06-17 17:26:21 PDT
Tyler Wilcock
Comment 2 2022-06-17 17:37:19 PDT
chris fleizach
Comment 3 2022-06-17 18:42:01 PDT
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
Comment 4 2022-06-20 12:33:43 PDT
(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
Comment 5 2022-06-21 13:06:59 PDT
Tyler Wilcock
Comment 6 2022-06-21 13:11:59 PDT
> + 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
Comment 7 2022-06-21 14:33:41 PDT
(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
Comment 8 2022-06-21 19:45:34 PDT
Tyler Wilcock
Comment 9 2022-06-22 12:11:03 PDT
Tyler Wilcock
Comment 10 2022-06-22 16:24:18 PDT
Tyler Wilcock
Comment 11 2022-06-22 22:53:05 PDT
EWS
Comment 12 2022-06-23 09:44:15 PDT
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
Comment 13 2022-08-14 00:00:43 PDT
Created attachment 461606 [details] Shah117363?.
Note You need to log in before you can comment on or make changes to this bug.