AX: Accessing AXIsolatedTree::{m_relations, m_selectedTextMarkerRange} requires m_changeLogLock, which can cause the AX thread to block on the main-thread or vice versa

https://bugs.webkit.org/show_bug.cgi?id=294652
rdar://153703482

Reviewed by Joshua Hoffman.

Ideally, the accessibility thread should only synchronize with the main-thread via AXIsolatedTree::m_changeLogLock
once: at the beginning of serving a request, via AXIsolatedTree::applyPendingChanges(). But prior to this commit,
we would take the lock any time we needed to access AXIsolatedTree::m_relations or AXIsolatedTree::m_selectedTextMarkerRange,
which could potentially be a lot. This can cause the AX thread to block on the main-thread at random times, and vice
versa, which can harm performance.

With this commit, we move caching the selected text marker range and relations map to a similar approach taken for other
synced data structures. We create an m_pendingFoo equivalent, guarded by a lock, and apply that in AXIsolatedTree::applyPendingChanges.
Then we can use the synced data structure safely without a lock off the main-thread.

* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::dirtyIsolatedTreeRelations):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:
(WebCore::AXIsolatedTree::create):
(WebCore::AXIsolatedTree::updateRelations):
(WebCore::AXIsolatedTree::setSelectedTextMarkerRange):
(WebCore::AXIsolatedTree::relatedObjectIDsFor):
(WebCore::AXIsolatedTree::applyPendingChanges):
(WebCore::AXIsolatedTree::selectedTextMarkerRange): Deleted.
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:
(WebCore::AXIsolatedTree::markRelationsDirty):
(WebCore::AXIsolatedTree::selectedTextMarkerRange):
(WebCore::AXIsolatedTree::relationsNeedUpdate): Deleted.

Canonical link: https://commits.webkit.org/296378@main

Author: twilco

Source/WebCore/accessibility/AXObjectCache.cpp

@@ -3290,8 +3290,8 @@ void AXObjectCache::dirtyIsolatedTreeRelations()
     AXTRACE("AXObjectCache::dirtyIsolatedTreeRelations"_s);
 
 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
-    if (auto tree = AXIsolatedTree::treeForPageID(m_pageID))
-        tree->relationsNeedUpdate(true);
+    if (RefPtr tree = AXIsolatedTree::treeForPageID(m_pageID))
+        tree->markRelationsDirty();
     startUpdateTreeSnapshotTimer();
 #endif
 }

Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp

@@ -168,14 +168,13 @@ RefPtr<AXIsolatedTree> AXIsolatedTree::create(AXObjectCache& axObjectCache)
     tree->setInitialSortedNonRootWebAreas(axIDs(axObjectCache.sortedNonRootWebAreas()));
     tree->updateLoadingProgress(axObjectCache.loadingProgress());
 
-    const auto relations = axObjectCache.relations();
-    tree->updateRelations(relations);
-
+    auto relations = axObjectCache.relations();
     for (auto& relatedObjectID : relations.keys()) {
         RefPtr axObject = axObjectCache.objectForID(relatedObjectID);
         if (axObject && axObject->isIgnored())
             tree->addUnconnectedNode(axObject.releaseNonNull());
     }
+    tree->updateRelations(WTFMove(relations));
 
     // Now that the tree is ready to take client requests, add it to the tree maps so that it can be found.
     storeTree(axObjectCache, tree);
@@ -1172,21 +1171,14 @@ void AXIsolatedTree::setFocusedNodeID(std::optional<AXID> axID)
     m_pendingFocusedNodeID = axID;
 }
 
-void AXIsolatedTree::updateRelations(const HashMap<AXID, AXRelations>& relations)
+void AXIsolatedTree::updateRelations(HashMap<AXID, AXRelations>&& relations)
 {
     AXTRACE("AXIsolatedTree::updateRelations"_s);
     ASSERT(isMainThread());
 
-    Locker locker { m_changeLogLock };
-    m_relations = relations;
     m_relationsNeedUpdate = false;
-}
-
-AXTextMarkerRange AXIsolatedTree::selectedTextMarkerRange()
-{
-    AXTRACE("AXIsolatedTree::selectedTextMarkerRange"_s);
     Locker locker { m_changeLogLock };
-    return m_selectedTextMarkerRange;
+    m_pendingRelations = WTFMove(relations);
 }
 
 void AXIsolatedTree::setSelectedTextMarkerRange(AXTextMarkerRange&& range)
@@ -1195,7 +1187,7 @@ void AXIsolatedTree::setSelectedTextMarkerRange(AXTextMarkerRange&& range)
     ASSERT(isMainThread());
 
     Locker locker { m_changeLogLock };
-    m_selectedTextMarkerRange = range;
+    m_pendingSelectedTextMarkerRange = WTFMove(range);
 }
 
 void AXIsolatedTree::updateLoadingProgress(double newProgressValue)
@@ -1286,8 +1278,6 @@ void AXIsolatedTree::removeSubtreeFromNodeMap(std::optional<AXID> objectID, std:
 std::optional<ListHashSet<AXID>> AXIsolatedTree::relatedObjectIDsFor(const AXIsolatedObject& object, AXRelation relation)
 {
     ASSERT(!isMainThread());
-    // FIXME: Taking the lock any time we want to access relations isn't ideal for performance.
-    Locker locker { m_changeLogLock };
 
     auto relationsIterator = m_relations.find(object.objectID());
     if (relationsIterator == m_relations.end())
@@ -1413,6 +1403,12 @@ void AXIsolatedTree::applyPendingChanges()
     if (m_pendingMostRecentlyPaintedText)
         m_mostRecentlyPaintedText = std::exchange(m_pendingMostRecentlyPaintedText, std::nullopt).value();
 
+    if (m_pendingRelations)
+        m_relations = std::exchange(m_pendingRelations, std::nullopt).value();
+
+    if (m_pendingSelectedTextMarkerRange)
+        m_selectedTextMarkerRange = std::exchange(m_pendingSelectedTextMarkerRange, std::nullopt).value();
+
     // Do this at the end because it requires looking up the root node by ID, so doing it at the end
     // ensures all additions to m_readerThreadNodeMap have been made by now.
     applyPendingRootNodeLocked();

Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h

@@ -498,8 +498,8 @@ class AXIsolatedTree : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AX
 
     // Relationships between objects.
     std::optional<ListHashSet<AXID>> relatedObjectIDsFor(const AXIsolatedObject&, AXRelation);
-    void relationsNeedUpdate(bool needUpdate) { m_relationsNeedUpdate = needUpdate; }
-    void updateRelations(const HashMap<AXID, AXRelations>&);
+    void markRelationsDirty() { m_relationsNeedUpdate = true; }
+    void updateRelations(HashMap<AXID, AXRelations>&&);
 
     AXCoreObject::AccessibilityChildrenVector sortedLiveRegions();
     AXCoreObject::AccessibilityChildrenVector sortedNonRootWebAreas();
@@ -517,7 +517,7 @@ class AXIsolatedTree : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AX
     // Use only if the s_storeLock is already held like in findAXTree.
     WEBCORE_EXPORT OptionSet<ActivityState> lockedPageActivityState() const;
 
-    AXTextMarkerRange selectedTextMarkerRange();
+    AXTextMarkerRange selectedTextMarkerRange() { return m_selectedTextMarkerRange; }
     void setSelectedTextMarkerRange(AXTextMarkerRange&&);
 
     void sortedLiveRegionsDidChange(Vector<AXID>);
@@ -641,6 +641,8 @@ class AXIsolatedTree : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AX
     std::optional<Vector<AXID>> m_pendingSortedLiveRegionIDs WTF_GUARDED_BY_LOCK(m_changeLogLock);
     std::optional<Vector<AXID>> m_pendingSortedNonRootWebAreaIDs WTF_GUARDED_BY_LOCK(m_changeLogLock);
     std::optional<HashMap<AXID, LineRange>> m_pendingMostRecentlyPaintedText WTF_GUARDED_BY_LOCK(m_changeLogLock);
+    std::optional<HashMap<AXID, AXRelations>> m_pendingRelations WTF_GUARDED_BY_LOCK(m_changeLogLock);
+    std::optional<AXTextMarkerRange> m_pendingSelectedTextMarkerRange WTF_GUARDED_BY_LOCK(m_changeLogLock);
     Markable<AXID> m_focusedNodeID;
     std::atomic<double> m_loadingProgress { 0 };
     std::atomic<double> m_processingProgress { 1 };
@@ -649,17 +651,15 @@ class AXIsolatedTree : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AX
     Vector<AXID> m_sortedLiveRegionIDs;
     Vector<AXID> m_sortedNonRootWebAreaIDs;
     HashMap<AXID, LineRange> m_mostRecentlyPaintedText;
-
-    // Relationships between objects.
-    HashMap<AXID, AXRelations> m_relations WTF_GUARDED_BY_LOCK(m_changeLogLock);
+    HashMap<AXID, AXRelations> m_relations;
 
     // Set to true by the AXObjectCache and false by AXIsolatedTree.
     // Both are only to be used on the main-thread.
     bool m_relationsNeedUpdate { true };
     bool m_mostRecentlyPaintedTextIsDirty { true };
 
     Lock m_changeLogLock;
-    AXTextMarkerRange m_selectedTextMarkerRange WTF_GUARDED_BY_LOCK(m_changeLogLock);
+    AXTextMarkerRange m_selectedTextMarkerRange;
 
     // Queued node updates used for building a new tree snapshot.
     ListHashSet<AXID> m_needsUpdateChildren;