In AXIsolatedTree::updateChildren, we have this: const auto& axChildren = axAncestor->children(); auto axChildrenIDs = axAncestor->childrenIDs(); Because the current version of AXCoreObject::childrenIDs always updates the underlying children if necessary, these two variables could get out of sync if childrenIDs actually performs an update after we already got children().
<rdar://problem/87791765>
Created attachment 449519 [details] Patch
(In reply to Tyler Wilcock from comment #2) > Created attachment 449519 [details] > Patch No need to add a bool param to ChildrenIDs since it is never used with value true. Keeping it simple, we can say that childrenIDs returns the IDs of the current children.
(In reply to Andres Gonzalez from comment #3) > (In reply to Tyler Wilcock from comment #2) > > Created attachment 449519 [details] > > Patch > > No need to add a bool param to ChildrenIDs since it is never used with value > true. Keeping it simple, we can say that childrenIDs returns the IDs of the > current children. Well, it's used in places outside this patch in ways that do expect childrenIDs to update if necessary. For example, this: void AXIsolatedTree::updateNode(AXCoreObject& axObject) { ...truncated... auto newObject = AXIsolatedObject::create(axObject, this, parentID); newObject->m_childrenIDs = axObject.childrenIDs(); ...truncated... } I did try to make childrenIDs be non-updating always, but that causes ~10 additional ITM failures. So I opted to give childrenIDs() the same bool API that children() has and fix the definitely wrong usage of it in AXIsolatedTree::updateChildren.
(In reply to Tyler Wilcock from comment #4) > (In reply to Andres Gonzalez from comment #3) > > (In reply to Tyler Wilcock from comment #2) > > > Created attachment 449519 [details] > > > Patch > > > > No need to add a bool param to ChildrenIDs since it is never used with value > > true. Keeping it simple, we can say that childrenIDs returns the IDs of the > > current children. > Well, it's used in places outside this patch in ways that do expect > childrenIDs to update if necessary. For example, this: > > void AXIsolatedTree::updateNode(AXCoreObject& axObject) > { > ...truncated... > auto newObject = AXIsolatedObject::create(axObject, this, parentID); > newObject->m_childrenIDs = axObject.childrenIDs(); > ...truncated... > } > > I did try to make childrenIDs be non-updating always, but that causes ~10 > additional ITM failures. So I opted to give childrenIDs() the same bool API > that children() has and fix the definitely wrong usage of it in > AXIsolatedTree::updateChildren. OK, thanks.
Thank you both for the review!
Committed r288313 (246229@main): <https://commits.webkit.org/246229@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449519 [details].
Comment on attachment 449519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449519&action=review > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1612 > + auto& kids = children(updateChildrenIfNecessary); Itβs more elegant to use Vector::map instead of a loop and easier to get the vector size allocation right; something to consider in future. > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1614 > + childrenIDs.reserveCapacity(kids.size()); This should use reserveInitialCapacity, slightly more efficient. > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1615 > + for (const auto& child : kids) This can just use auto&, no need for the const. > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1616 > childrenIDs.append(child->objectID()); This can use the slightly more efficient uncheckedAppend because of the reserveCapacity above.