Bug 266268 – AX: Serve relatedObjectIdsFor off of the main thread

• Component: Accessibility
• Status: RESOLVED FIXED

[Joshua Hoffman]

AXIsolatedTRee::relatedObjectIdsFor can currently hit the main thread. We should cache this value appropriately to always serve it on the AX thread instead. 

rdar://116133758

[Radar WebKit Bug Importer]

<rdar://problem/119532658>

[Joshua Hoffman]

rdar://116133758

[Joshua Hoffman]

Created attachment 468993 [details]
Patch

[Joshua Hoffman]

Created attachment 468995 [details]
Patch

[chris fleizach]

Comment on attachment 468995 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=468995&action=review

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1317
> +        setRelations(cache->relations());

Do we need to set this variable to false after processing?

[Joshua Hoffman]

Comment on attachment 468995 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=468995&action=review

>> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:1317
>> +        setRelations(cache->relations());
> 
> Do we need to set this variable to false after processing?

Right now I set it to false above in AXIsolatedTree::applyPendingChanges, when we actually set m_relations. But, this does make me think we might need two flags: 
(1) m_relationsNeedUpdate, which determines whether we should copy from the cache into m_pendingRelations.
(2) m_relationsNeedProcessing to decide whether we should copy m_pendingRelations to m_relations.

Splitting these are important because we could have some timing issue where processQueuedNodeUpdates() is called after applyPendingChanges. In that case, we wouldn't set m_pendingRelations because m_relationsNeedUpdate is false. I will make that update.

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #4)
> Created attachment 468995 [details]
> Patch

diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index c1b5872cff5b..ab962911373d 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -4821,6 +4821,7 @@ void AXObjectCache::relationsNeedUpdate(bool needUpdate)
     if (m_relationsNeedUpdate) {
         if (auto tree = AXIsolatedTree::treeForPageID(m_pageID))
             tree->relationsNeedUpdate(true);
+        startUpdateTreeSnapshotTimer();
     }
 #endif
 }
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 17ff6d748058..ba6fffc6b6aa 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -160,6 +160,8 @@ Ref<AXIsolatedTree> AXIsolatedTree::create(AXObjectCache& axObjectCache)

     tree->updateLoadingProgress(axObjectCache.loadingProgress());

+    tree->setRelations(axObjectCache.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);
     return tree;
@@ -975,6 +977,15 @@ void AXIsolatedTree::setFocusedNodeID(AXID axID)
     m_pendingFocusedNodeID = axID;
 }

+void AXIsolatedTree::setRelations(const HashMap<AXID, AXRelations>& relations)
+{
+    AXTRACE("AXIsolatedTree::setRelations"_s);
+    ASSERT(isMainThread());
+
+    Locker locker { m_changeLogLock };
+    m_pendingRelations = relations;
+}
+
 AXTextMarkerRange AXIsolatedTree::selectedTextMarkerRange()
 {
     AXTRACE("AXIsolatedTree::selectedTextMarkerRange"_s);
@@ -1087,15 +1098,6 @@ std::optional<ListHashSet<AXID>> AXIsolatedTree::relatedObjectIDsFor(const AXIso
 {
     ASSERT(!isMainThread());

-    if (m_relationsNeedUpdate) {
-        m_relations = Accessibility::retrieveValueFromMainThread<HashMap<AXID, AXRelations>>([this] () -> HashMap<AXID, AXRelations> {
-            if (auto* cache = axObjectCache())
-                return cache->relations();
-            return { };
-        });
-        m_relationsNeedUpdate = false;
-    }
-
     auto relationsIterator = m_relations.find(object.objectID());
     if (relationsIterator == m_relations.end())
         return std::nullopt;
@@ -1214,6 +1216,11 @@ void AXIsolatedTree::applyPendingChanges()
         }
     }
     m_pendingPropertyChanges.clear();
+
+    if (m_relationsNeedUpdate) {
+        m_relations = m_pendingRelations;
+        m_relationsNeedUpdate = false;
+    }
 }

 AXTreePtr findAXTree(Function<bool(AXTreePtr)>&& match)
@@ -1306,6 +1313,9 @@ void AXIsolatedTree::processQueuedNodeUpdates()
     }
     m_needsPropertyUpdates.clear();

+    if (m_relationsNeedUpdate)
+        setRelations(cache->relations());
+
     queueRemovalsAndUnresolvedChanges({ });
 }

diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
index 8396a3d29fdb..6c8735df5206 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
@@ -355,6 +355,7 @@ public:
     // Relationships between objects.
     std::optional<ListHashSet<AXID>> relatedObjectIDsFor(const AXIsolatedObject&, AXRelationType);
     void relationsNeedUpdate(bool needUpdate) { m_relationsNeedUpdate = needUpdate; }
+    void setRelations(const HashMap<AXID, AXRelations>&);

     // Called on AX thread from WebAccessibilityObjectWrapper methods.
     // During layout tests, it is called on the main thread.
@@ -469,6 +470,8 @@ private:
     // Set to false on the AX thread by relatedObjectIDsFor.
     std::atomic<bool> m_relationsNeedUpdate { true };

+    HashMap<AXID, AXRelations> m_pendingRelations WTF_GUARDED_BY_LOCK(m_changeLogLock);
+

AG: we are already caching relations in AXIsolatedTree::m_relations. What this patch is trying to do is to update the AXIsolatedTree relations not when they are requested but in AXIsolatedTree::processQueuedNodeUpdates(). At that point you can simply copy the AXObjectCache::m_relations to AXIsolatedTree::m_relations so the above m_pendingRelations and most of the other changes are unnecessary.

     Lock m_changeLogLock;
     AXTextMarkerRange m_selectedTextMarkerRange WTF_GUARDED_BY_LOCK(m_changeLogLock);

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #7)
> (In reply to Joshua Hoffman from comment #4)
> > Created attachment 468995 [details]
> > Patch
> AG: we are already caching relations in AXIsolatedTree::m_relations. What
> this patch is trying to do is to update the AXIsolatedTree relations not
> when they are requested but in AXIsolatedTree::processQueuedNodeUpdates().
> At that point you can simply copy the AXObjectCache::m_relations to
> AXIsolatedTree::m_relations so the above m_pendingRelations and most of the
> other changes are unnecessary.

That makes sense—I can update m_relations to hold a lock so we can update it in AXIsolatedTree::processQueuedNodeUpdates and remove the intermediary m_pendingRelations. Thank you!

[Joshua Hoffman]

Created attachment 468998 [details]
Patch

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #9)
> Created attachment 468998 [details]
> Patch

Looks good. The commit message needs update to reflect what the changes is about, updating the Isolated tree relations on the processNodeUpdates instead of on the relatedObjectsFor...

[Joshua Hoffman]

Created attachment 468999 [details]
Patch

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #10)
> (In reply to Joshua Hoffman from comment #9)
> > Created attachment 468998 [details]
> > Patch
> 
> Looks good. The commit message needs update to reflect what the changes is
> about, updating the Isolated tree relations on the processNodeUpdates
> instead of on the relatedObjectsFor...

Updated!

[Tyler Wilcock]

Comment on attachment 468999 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=468999&action=review

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:469
>      std::atomic<bool> m_relationsNeedUpdate { true };

Do we use this member variable anymore after this patch?

[Tyler Wilcock]

(In reply to Tyler Wilcock from comment #13)
> Comment on attachment 468999 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=468999&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:469
> >      std::atomic<bool> m_relationsNeedUpdate { true };
> 
> Do we use this member variable anymore after this patch?
Oh, maybe we do...but is it only accessed from the main-thread now? If so can we make it a plain bool rather than an atomic bool?

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #14)
> (In reply to Tyler Wilcock from comment #13)
> > Comment on attachment 468999 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=468999&action=review
> > 
> > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:469
> > >      std::atomic<bool> m_relationsNeedUpdate { true };
> > 
> > Do we use this member variable anymore after this patch?
> Oh, maybe we do...but is it only accessed from the main-thread now? If so
> can we make it a plain bool rather than an atomic bool?

Yeah, it's main thread only now. I will go ahead and change its type.

[Joshua Hoffman]

Created attachment 469018 [details]
Patch

[EWS]

Committed 272054@main (260dbd54ddc9): <https://commits.webkit.org/272054@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 469018 [details].