WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(5.85 KB, patch)
2022-06-21 13:06 PDT
,
Tyler Wilcock
no flags
Details
Formatted Diff
Diff
Patch
(7.58 KB, patch)
2022-06-21 19:45 PDT
,
Tyler Wilcock
no flags
Details
Formatted Diff
Diff
Patch
(7.60 KB, patch)
2022-06-22 16:24 PDT
,
Tyler Wilcock
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(7.61 KB, patch)
2022-06-22 22:53 PDT
,
Tyler Wilcock
no flags
Details
Formatted Diff
Diff
Shah117363?.
(967 bytes, application/xml)
2022-08-14 00:00 PDT
,
Wmwm
no flags
Details
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2022-06-17 17:26:21 PDT
<
rdar://problem/95424726
>
Tyler Wilcock
Comment 2
2022-06-17 17:37:19 PDT
Created
attachment 460323
[details]
Patch
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
Created
attachment 460379
[details]
Patch
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
Created
attachment 460401
[details]
Patch
Tyler Wilcock
Comment 9
2022-06-22 12:11:03 PDT
rdar://95344512
Tyler Wilcock
Comment 10
2022-06-22 16:24:18 PDT
Created
attachment 460434
[details]
Patch
Tyler Wilcock
Comment 11
2022-06-22 22:53:05 PDT
Created
attachment 460439
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug