Modified: trunk/Source/WebCore/ChangeLog (292966 => 292967)
--- trunk/Source/WebCore/ChangeLog 2022-04-18 20:25:01 UTC (rev 292966)
+++ trunk/Source/WebCore/ChangeLog 2022-04-18 20:29:40 UTC (rev 292967)
@@ -1,3 +1,81 @@
+2022-04-18 Tyler Wilcock <tyle...@apple.com>
+
+ Node changes created by AXIsolatedTree::updateNode are overwritten when performed during AXIsolatedTree::collectNodeChangesForSubtree
+ https://bugs.webkit.org/show_bug.cgi?id=239398
+
+ Reviewed by Andres Gonzalez.
+
+ We currently do the wrong thing in this scenario:
+
+ 1. A dynamic page change causes an element to be included on the page, so we
+ create a node change for it and its subtree by calling collectNodeChangesForSubtree.
+ This causes a call to children() on the live object.
+
+ 2. In the process of (or after) updating the children of the live object, we perform some
+ operation that triggers AXIsolatedTree::updateNode on said object. AccessibilityRenderObject::updateRoleAfterChildrenCreation
+ would be an example of this if it were properly coded to update the isolated tree if
+ the object's role changes (I'll address that in a separate patch). updateNode results
+ in a node change with the correct properties being added to m_pendingAppends.
+
+ 3. collectNodeChangesForSubtree (started in step 1) finishes, and queues a node change
+ for the same object, but with the wrong properties (because it was created before step 2).
+ Because it comes after the node change added in step 2 to m_pendingAppends, it wins,
+ and we add an object with the wrong properties to the tree.
+
+ This patch fixes this with a new AXIsolatedTree::m_unresolvedPendingAppends member
+ variable. This is a HashMap with an AXID key, representing the object to create a node
+ change for. Now, both collectNodeChangesForSubtree and updateNode will add an entry
+ to this HashMap, allowing it to be resolved it (i.e. by creating the node change) at a later time.
+
+ This is both more correct (as it prevents any ordering bugs entirely), and more
+ efficient, because we now never create multiple node changes for the same object.
+
+ This patch also necessitated other code cleanup items:
+ - m_unresolvedPendingAppends allows us to remove the `idsBeingChanged` parameter
+ we passed to several functions since this member variable does the same thing.
+
+ - m_pendingNodeRemovals has been deleted. The only thing adding to this was
+ AXIsolatedTree::updateNode, and it did so unnecessarily, since we already handle
+ the case where we are replacing an existing object when proessing m_pendingAppends.
+
+ This patch also fixes an issue found by Andres Gonzalez in https://bugs.webkit.org/show_bug.cgi?id=239402.
+ AXIsolatedTree::updateNode should not immediately attach the wrapper to the new object on the main thread
+ since it could be in use on the AX thread. To make this more clear, the `attachWrapper` parameter has been
+ changed from a bool to an enum called AttachWrapper with values `OnMainThread` and `OnAXThread`.
+
+ Fixes six tests in isolated tree mode:
+ - accessibility/aria-labelledby-overrides-label.html
+ - accessibility/aria-role-on-label.html
+ - accessibility/mac/label-element-all-text-string-value.html
+ - accessibility/mac/label-element-with-hidden-control.html
+ - accessibility/mac/label-element-all-text-string-value.html
+ - accessibility/mac/slider-allows-title-ui-element.html
+
+ * accessibility/AXObjectCache.cpp:
+ (WebCore::AXObjectCache::treeData):
+ * accessibility/isolatedtree/AXIsolatedTree.cpp:
+ (WebCore::AXIsolatedTree::create):
+ (WebCore::AXIsolatedTree::generateSubtree):
+ (WebCore::AXIsolatedTree::nodeChangeForObject):
+ (WebCore::AXIsolatedTree::queueRemovals):
+ Added. Allows you to queue objects for removal from contexts where you
+ don't already hold a lock.
+ (WebCore::AXIsolatedTree::queueRemovalsLocked):
+ Added. Allows you to queue objects for removal from contexts where you
+ already hold a lock.
+ (WebCore::AXIsolatedTree::queueRemovalsAndUnresolvedChanges):
+ Added. Replaces AXIsolatedTree::queueChangesAndRemovals.
+ (WebCore::AXIsolatedTree::collectNodeChangesForSubtree):
+ (WebCore::AXIsolatedTree::updateNode):
+ (WebCore::AXIsolatedTree::updateChildren):
+ (WebCore::AXIsolatedTree::removeNode):
+ (WebCore::AXIsolatedTree::removeSubtreeFromNodeMap):
+ (WebCore::AXIsolatedTree::applyPendingChanges):
+ Change local variable name from object to existingObject as this makes
+ the code much more clear.
+ * accessibility/isolatedtree/AXIsolatedTree.h:
+ (WebCore::AXIsolatedTree::queueChangesAndRemovals): Deleted.
+
2022-04-18 Chris Dumez <cdu...@apple.com>
Optimize nodeHasRole()
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp (292966 => 292967)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2022-04-18 20:25:01 UTC (rev 292966)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2022-04-18 20:29:40 UTC (rev 292967)
@@ -106,7 +106,7 @@
// For this, we need the root and focused objects of the AXObject tree.
auto* axRoot = axObjectCache->getOrCreate(axObjectCache->document().view());
if (axRoot)
- tree->generateSubtree(*axRoot, nullptr, true);
+ tree->generateSubtree(*axRoot, nullptr);
auto* axFocus = axObjectCache->focusedObjectForPage(axObjectCache->document().page());
if (axFocus)
tree->setFocusedNodeID(axFocus->objectID());
@@ -169,7 +169,7 @@
return result;
}
-void AXIsolatedTree::generateSubtree(AXCoreObject& axObject, AXCoreObject* axParent, bool attachWrapper)
+void AXIsolatedTree::generateSubtree(AXCoreObject& axObject, AXCoreObject* axParent)
{
AXTRACE("AXIsolatedTree::generateSubtree"_s);
ASSERT(isMainThread());
@@ -178,9 +178,8 @@
return;
AXID parentID = axParent ? axParent->objectID() : AXID();
- Vector<NodeChange> changes;
- collectNodeChangesForSubtree(axObject, parentID, attachWrapper, changes);
- queueChangesAndRemovals(changes);
+ collectNodeChangesForSubtree(axObject, parentID);
+ queueRemovalsAndUnresolvedChanges({ });
}
AXID AXIsolatedTree::parentIDForObject(AXCoreObject& axObject, AXID assumedParentID)
@@ -203,7 +202,7 @@
return assumedParentID;
}
-AXIsolatedTree::NodeChange AXIsolatedTree::nodeChangeForObject(AXCoreObject& axObject, AXID parentID, bool attachWrapper)
+AXIsolatedTree::NodeChange AXIsolatedTree::nodeChangeForObject(AXCoreObject& axObject, AXID parentID, AttachWrapper attachWrapper)
{
ASSERT(isMainThread());
@@ -217,7 +216,7 @@
}
ASSERT(axObject.wrapper());
- if (attachWrapper)
+ if (attachWrapper == AttachWrapper::OnMainThread)
object->attachPlatformWrapper(axObject.wrapper());
else {
// Set the wrapper in the NodeChange so that it is set on the AX thread.
@@ -255,32 +254,55 @@
m_pendingChildrenUpdates.append({ objectID, WTFMove(childrenIDs) });
}
-void AXIsolatedTree::queueChangesAndRemovals(const Vector<NodeChange>& changes, const Vector<AXID>& subtreeRemovals)
+void AXIsolatedTree::queueRemovals(const Vector<AXID>& subtreeRemovals)
{
ASSERT(isMainThread());
Locker locker { m_changeLogLock };
+ queueRemovalsLocked(subtreeRemovals);
+}
+void AXIsolatedTree::queueRemovalsLocked(const Vector<AXID>& subtreeRemovals)
+{
+ ASSERT(isMainThread());
+ ASSERT(m_changeLogLock.isLocked());
+
for (const auto& axID : subtreeRemovals)
m_pendingSubtreeRemovals.append(axID);
+}
- for (const auto& change : changes)
- queueChange(change);
+void AXIsolatedTree::queueRemovalsAndUnresolvedChanges(const Vector<AXID>& subtreeRemovals)
+{
+ ASSERT(isMainThread());
+
+ Vector<NodeChange> resolvedAppends;
+ if (!m_unresolvedPendingAppends.isEmpty()) {
+ if (auto* cache = axObjectCache()) {
+ resolvedAppends.reserveInitialCapacity(m_unresolvedPendingAppends.size());
+ for (const auto& unresolvedAppend : m_unresolvedPendingAppends) {
+ if (auto* axObject = cache->objectFromAXID(unresolvedAppend.key))
+ resolvedAppends.uncheckedAppend(nodeChangeForObject(*axObject, unresolvedAppend.value.first, unresolvedAppend.value.second));
+ }
+ m_unresolvedPendingAppends.clear();
+ }
+ }
+
+ Locker locker { m_changeLogLock };
+ for (const auto& resolvedAppend : resolvedAppends)
+ queueChange(resolvedAppend);
+ queueRemovalsLocked(subtreeRemovals);
}
-void AXIsolatedTree::collectNodeChangesForSubtree(AXCoreObject& axObject, AXID parentID, bool attachWrapper, Vector<NodeChange>& changes, HashSet<AXID>* idsBeingChanged)
+void AXIsolatedTree::collectNodeChangesForSubtree(AXCoreObject& axObject, AXID parentID)
{
AXTRACE("AXIsolatedTree::collectNodeChangesForSubtree"_s);
ASSERT(isMainThread());
+ SetForScope collectingNodeChanges(m_isCollectingNodeChanges, true);
+ m_unresolvedPendingAppends.set(axObject.objectID(), std::make_pair(parentID, AttachWrapper::OnMainThread));
- auto nodeChange = nodeChangeForObject(axObject, parentID, attachWrapper);
- if (idsBeingChanged)
- idsBeingChanged->add(nodeChange.isolatedObject->objectID());
- changes.append(WTFMove(nodeChange));
-
auto axChildrenCopy = axObject.children();
auto axChildrenIDs = axChildrenCopy.map([&](auto& axChild) {
- collectNodeChangesForSubtree(*axChild, axObject.objectID(), attachWrapper, changes, idsBeingChanged);
+ collectNodeChangesForSubtree(*axChild, axObject.objectID());
return axChild->objectID();
});
m_nodeMap.set(axObject.objectID(), ParentChildrenIDs { parentID, WTFMove(axChildrenIDs) });
@@ -292,15 +314,20 @@
AXLOG(&axObject);
ASSERT(isMainThread());
- AXID axID = axObject.objectID();
auto* axParent = axObject.parentObjectUnignored();
AXID parentID = axParent ? axParent->objectID() : AXID();
-
- auto change = nodeChangeForObject(axObject, parentID, true);
-
- // Remove the old object and set the new one to be updated on the AX thread.
+ // If we update a node as the result of some side effect while collecting node changes (e.g. a role change from
+ // AccessibilityRenderObject::updateRoleAfterChildrenCreation), queue the append up to be resolved with the rest
+ // of the collected changes. This prevents us from creating two node changes for the same object.
+ if (m_isCollectingNodeChanges) {
+ m_unresolvedPendingAppends.set(axObject.objectID(), std::make_pair(WTFMove(parentID), AttachWrapper::OnAXThread));
+ return;
+ }
+ // Otherwise, resolve the change immediately and queue it up.
+ // In both cases, we can't attach the wrapper immediately on the main thread, since the wrapper could be in use
+ // on the AX thread (because this function updates an existing node).
+ auto change = nodeChangeForObject(axObject, parentID, AttachWrapper::OnAXThread);
Locker locker { m_changeLogLock };
- m_pendingNodeRemovals.append({ axID, AccessibilityDetachmentType::ElementChanged });
queueChange(change);
}
@@ -462,8 +489,6 @@
const auto& newChildren = axAncestor->children();
auto newChildrenIDs = axAncestor->childrenIDs(false);
- Vector<NodeChange> changes;
- HashSet<AXID> idsBeingChanged;
for (size_t i = 0; i < newChildren.size(); ++i) {
ASSERT(newChildren[i]->objectID() == newChildrenIDs[i]);
ASSERT(newChildrenIDs[i].isValid());
@@ -474,7 +499,7 @@
// This is a new child, add it to the tree.
AXLOG(makeString("AXID ", axAncestor->objectID().loggingString(), " gaining new subtree, starting at ID ", newChildren[i]->objectID().loggingString(), ":"));
AXLOG(newChildren[i]);
- collectNodeChangesForSubtree(*newChildren[i], axAncestor->objectID(), true, changes, &idsBeingChanged);
+ collectNodeChangesForSubtree(*newChildren[i], axAncestor->objectID());
}
}
m_nodeMap.set(axAncestor->objectID(), ParentChildrenIDs { oldIDs.parentID, WTFMove(newChildrenIDs) });
@@ -487,9 +512,9 @@
// 1. Object 123 is slated to be a child of this object (i.e. in newChildren), and we collect node changes for it.
// 2. Object 123 is currently a member of a subtree of some other object in oldChildrenIDs.
// 3. Thus, we don't want to delete Object 123 from the nodemap, instead allowing it to be moved.
- removeSubtreeFromNodeMap(axID, axAncestor, idsBeingChanged);
+ removeSubtreeFromNodeMap(axID, axAncestor);
}
- queueChangesAndRemovals(changes, oldChildrenIDs);
+ queueRemovalsAndUnresolvedChanges(oldChildrenIDs);
// Also queue updates for properties that derive from children().
updateRelatedProperties(*axAncestor);
@@ -554,11 +579,12 @@
AXLOG(makeString("objectID ", axObject.objectID().loggingString()));
ASSERT(isMainThread());
+ m_unresolvedPendingAppends.remove(axObject.objectID());
removeSubtreeFromNodeMap(axObject.objectID(), axObject.parentObjectUnignored());
- queueChangesAndRemovals({ }, { axObject.objectID() });
+ queueRemovals({ axObject.objectID() });
}
-void AXIsolatedTree::removeSubtreeFromNodeMap(AXID objectID, AXCoreObject* axParent, const HashSet<AXID>& idsToKeep)
+void AXIsolatedTree::removeSubtreeFromNodeMap(AXID objectID, AXCoreObject* axParent)
{
AXTRACE("AXIsolatedTree::removeSubtreeFromNodeMap"_s);
AXLOG(makeString("Removing subtree for objectID ", objectID.loggingString()));
@@ -578,7 +604,7 @@
Vector<AXID> removals = { objectID };
while (removals.size()) {
AXID axID = removals.takeLast();
- if (!axID.isValid() || idsToKeep.contains(axID))
+ if (!axID.isValid() || m_unresolvedPendingAppends.contains(axID))
continue;
auto it = m_nodeMap.find(axID);
@@ -619,15 +645,6 @@
m_focusedNodeID = m_pendingFocusedNodeID;
}
- while (m_pendingNodeRemovals.size()) {
- auto removal = m_pendingNodeRemovals.takeLast();
- AXLOG(makeString("removing axID ", removal.first.loggingString()));
- if (auto object = nodeForID(removal.first)) {
- object->detach(removal.second);
- m_readerThreadNodeMap.remove(removal.first);
- }
- }
-
while (m_pendingSubtreeRemovals.size()) {
auto axID = m_pendingSubtreeRemovals.takeLast();
AXLOG(makeString("removing subtree axID ", axID.loggingString()));
@@ -648,13 +665,13 @@
if (!wrapper)
continue;
- if (auto object = m_readerThreadNodeMap.get(axID)) {
- if (object != &item.isolatedObject.get()
- && object->wrapper() == wrapper.get()) {
+ if (auto existingObject = m_readerThreadNodeMap.get(axID)) {
+ if (existingObject != &item.isolatedObject.get()
+ && existingObject->wrapper() == wrapper.get()) {
// The new IsolatedObject is a replacement for an existing object
- // as the result of an update. Thus detach the wrapper from the
- // existing object and attach it to the new one.
- object->detachWrapper(AccessibilityDetachmentType::ElementChanged);
+ // as the result of an update. Thus detach the existing object
+ // and attach the wrapper to the new one.
+ existingObject->detach(AccessibilityDetachmentType::ElementChanged);
item.isolatedObject->attachPlatformWrapper(wrapper.get());
}
m_readerThreadNodeMap.remove(axID);
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h (292966 => 292967)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2022-04-18 20:25:01 UTC (rev 292966)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2022-04-18 20:29:40 UTC (rev 292967)
@@ -349,7 +349,7 @@
#endif
};
- void generateSubtree(AXCoreObject&, AXCoreObject*, bool attachWrapper);
+ void generateSubtree(AXCoreObject&, AXCoreObject*);
void updateNode(AXCoreObject&);
void updateChildren(AXCoreObject&);
void updateNodeProperty(AXCoreObject&, AXPropertyName);
@@ -363,7 +363,7 @@
// Removes the corresponding isolated object and all descendants from the m_nodeMap and queues their removal from the tree.
void removeNode(const AXCoreObject&);
// Removes the given node and all its descendants from m_nodeMap.
- void removeSubtreeFromNodeMap(AXID axID, AXCoreObject*, const HashSet<AXID>& = { });
+ void removeSubtreeFromNodeMap(AXID axID, AXCoreObject*);
// Both setRootNodeID and setFocusedNodeID are called during the generation
// of the IsolatedTree.
@@ -388,10 +388,13 @@
// Methods in this block are called on the main thread.
// Computes the parent ID of the given object, which is generally the "assumed" parent ID (but not always, like in the case of tables).
AXID parentIDForObject(AXCoreObject&, AXID assumedParentID);
- NodeChange nodeChangeForObject(AXCoreObject&, AXID parentID, bool attachWrapper = true);
- void collectNodeChangesForSubtree(AXCoreObject&, AXID parentID, bool attachWrapper, Vector<NodeChange>&, HashSet<AXID>* = nullptr);
+ enum class AttachWrapper : bool { OnMainThread, OnAXThread };
+ NodeChange nodeChangeForObject(AXCoreObject&, AXID parentID, AttachWrapper = AttachWrapper::OnMainThread);
+ void collectNodeChangesForSubtree(AXCoreObject&, AXID parentID);
void queueChange(const NodeChange&) WTF_REQUIRES_LOCK(m_changeLogLock);
- void queueChangesAndRemovals(const Vector<NodeChange>&, const Vector<AXID>& = { });
+ void queueRemovals(const Vector<AXID>&);
+ void queueRemovalsLocked(const Vector<AXID>&) WTF_REQUIRES_LOCK(m_changeLogLock);
+ void queueRemovalsAndUnresolvedChanges(const Vector<AXID>&);
AXIsolatedTreeID m_treeID;
AXObjectCache* m_axObjectCache { nullptr };
@@ -408,6 +411,13 @@
// its ObjectID to its ParentChildrenIDs struct.
HashMap<AXID, ParentChildrenIDs> m_nodeMap;
+ // Only accessed on the main thread.
+ // The key is the ID of the object that will be resolved into an m_pendingAppends NodeChange.
+ // The value represents the parent ID of the object, and whether the wrapper should be attached on the main thread or the AX thread.
+ HashMap<AXID, std::pair<AXID, AttachWrapper>> m_unresolvedPendingAppends;
+ // Only accessed on the main thread.
+ bool m_isCollectingNodeChanges { false };
+
// Only accessed on AX thread requesting data.
HashMap<AXID, Ref<AXIsolatedObject>> m_readerThreadNodeMap;
@@ -415,8 +425,6 @@
RefPtr<AXIsolatedObject> m_rootNode WTF_GUARDED_BY_LOCK(m_changeLogLock);
Vector<NodeChange> m_pendingAppends WTF_GUARDED_BY_LOCK(m_changeLogLock); // Nodes to be added to the tree and platform-wrapped.
Vector<AXPropertyChange> m_pendingPropertyChanges WTF_GUARDED_BY_LOCK(m_changeLogLock);
- // Nodes to be removed from the tree (and the AccessibilityDetachmentType reason for the removal).
- Vector<std::pair<AXID, AccessibilityDetachmentType>> m_pendingNodeRemovals WTF_GUARDED_BY_LOCK(m_changeLogLock);
Vector<AXID> m_pendingSubtreeRemovals WTF_GUARDED_BY_LOCK(m_changeLogLock); // Nodes whose subtrees are to be removed from the tree.
Vector<std::pair<AXID, Vector<AXID>>> m_pendingChildrenUpdates WTF_GUARDED_BY_LOCK(m_changeLogLock);
AXID m_pendingFocusedNodeID WTF_GUARDED_BY_LOCK(m_changeLogLock);
