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
<rdar://problem/119532658>
rdar://116133758
Created attachment 468993 [details] Patch
Created attachment 468995 [details] Patch
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?
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.
(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);
(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!
Created attachment 468998 [details] Patch
(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...
Created attachment 468999 [details] Patch
(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!
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?
(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?
(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.
Created attachment 469018 [details] Patch
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].