Bug 272803 – AX: Move objectsForIDs back off of the main thread

• Component: Accessibility
• Status: RESOLVED FIXED

[Joshua Hoffman]

272323@main was reverted due to a timing issue with addUnconnectedNode. This should be re-landed with the appropriate changes.

[Radar WebKit Bug Importer]

<rdar://problem/126600212>

[Joshua Hoffman]

Created attachment 470963 [details]
Patch

[Tyler Wilcock]

Comment on attachment 470963 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:4142
> +        if (auto tree = AXIsolatedTree::treeForPageID(m_pageID))
> +            tree->addUnconnectedNode(object);

Rather than retrieving the tree every iteration, can we do it just once above the forEach?

> LayoutTests/accessibility/create-unconnected-node-during-layout-crash.html:34
> +        await sleep(500)

500ms seems really excessive. Can we shorten this and still trigger the crash if the implementation is wrong?

[Joshua Hoffman]

Comment on attachment 470963 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:4142
>> +            tree->addUnconnectedNode(object);
> 
> Rather than retrieving the tree every iteration, can we do it just once above the forEach?

Yep, I'll move this outside

>> LayoutTests/accessibility/create-unconnected-node-during-layout-crash.html:34
>> +        await sleep(500)
> 
> 500ms seems really excessive. Can we shorten this and still trigger the crash if the implementation is wrong?

Just tested and we can make it 10s and it will still crash, so I will change this to that.

[Joshua Hoffman]

Created attachment 470967 [details]
Patch

[Tyler Wilcock]

Comment on attachment 470967 [details]
Patch

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

Looks good to me, but maybe we can wait for Andres to take a pass too since he's the expert in the relations code.

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:504
>      if (object.accessibilityIsIgnored()) {
> -        if (auto tree = AXIsolatedTree::treeForPageID(pageID))
> -            tree->addUnconnectedNode(object);
> +        cache.deferAddUnconnectedNode(object);
>      }

Curly-braces can be removed here since this is now only one line

[Joshua Hoffman]

Created attachment 470968 [details]
Patch

[Joshua Hoffman]

Created attachment 470977 [details]
Patch

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index 0e14dc92f247..defe4cb18b9d 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -1398,6 +1398,14 @@ void AXObjectCache::deferNodeAddedOrRemoved(Node* node)
         m_performCacheUpdateTimer.startOneShot(0_s);
 }

+void AXObjectCache::deferAddUnconnectedNode(AccessibilityObject& axObject)
+{
+    m_deferredUnconnectedNodeList.add(axObject);
+
+    if (!m_performCacheUpdateTimer.isActive())
+        m_performCacheUpdateTimer.startOneShot(0_s);
+}
+
 void AXObjectCache::childrenChanged(Node* node, Node* changedChild)
 {
     childrenChanged(get(node));
@@ -4127,6 +4135,15 @@ void AXObjectCache::performDeferredCacheUpdate(ForceLayout forceLayout)
     AXLOGDeferredCollection("ChildrenChangedList"_s, m_deferredChildrenChangedList);
     handleAllDeferredChildrenChanged();

+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+    AXLOGDeferredCollection("UnconnectedNodeList"_s, m_deferredUnconnectedNodeList);
+    if (auto tree = AXIsolatedTree::treeForPageID(m_pageID)) {
+        m_deferredUnconnectedNodeList.forEach([tree] (auto& object) {

AG: [&tree]

+            tree->addUnconnectedNode(object);
+        });
+    }
+#endif
+
     AXLOGDeferredCollection("RecomputeTableCellSlotsList"_s, m_deferredRecomputeTableCellSlotsList);
     m_deferredRecomputeTableCellSlotsList.forEach([this] (auto& axTable) {
         handleRecomputeCellSlots(axTable);
@@ -4922,6 +4939,15 @@ bool AXObjectCache::addRelation(AccessibilityObject* origin, AccessibilityObject
         // If the IDs are still in the m_objects map, the objects should be still alive.
         if (auto symmetric = symmetricRelation(relationType); symmetric != AXRelationType::None)
             addRelation(target, origin, symmetric, AddSymmetricRelation::No);
+
+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+        if (auto tree = AXIsolatedTree::treeForPageID(m_pageID)) {
+            if (origin && origin->accessibilityIsIgnored())
+                deferAddUnconnectedNode(*origin);
+            if (target && target->accessibilityIsIgnored())
+                deferAddUnconnectedNode(*target);
+        }
+#endif
     }

     return true;

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/AXObjectCache.h b/Source/WebCore/accessibility/AXObjectCache.h
index efec912b8dde..bc062cfe8091 100644
--- a/Source/WebCore/accessibility/AXObjectCache.h
+++ b/Source/WebCore/accessibility/AXObjectCache.h
@@ -379,6 +379,7 @@ public:
         , Vector<std::pair<Node*, Node*>>
         , WeakHashSet<Element, WeakPtrImplWithEventTargetData>
         , WeakHashSet<HTMLTableElement, WeakPtrImplWithEventTargetData>
+        , WeakHashSet<AccessibilityObject>
         , WeakHashSet<AccessibilityTable>
         , WeakHashSet<AccessibilityTableCell>
         , WeakListHashSet<Node, WeakPtrImplWithEventTargetData>
@@ -387,6 +388,7 @@ public:
     void deferModalChange(Element*);
     void deferMenuListValueChange(Element*);
     void deferNodeAddedOrRemoved(Node*);
+    void deferAddUnconnectedNode(AccessibilityObject&);

AG: this can be private, right?

     void handleScrolledToAnchor(const Node* anchorNode);
     void onScrollbarUpdate(ScrollView*);
     void onRemoteFrameInitialized(AXRemoteFrame&);
@@ -803,6 +805,7 @@ private:
     WeakHashMap<Element, String, WeakPtrImplWithEventTargetData> m_deferredTextFormControlValue;
     Vector<AttributeChange> m_deferredAttributeChange;
     std::optional<std::pair<WeakPtr<Node, WeakPtrImplWithEventTargetData>, WeakPtr<Node, WeakPtrImplWithEventTargetData>>> m_deferredFocusedNodeChange;
+    WeakHashSet<AccessibilityObject> m_deferredUnconnectedNodeList;

 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
     Timer m_buildIsolatedTreeTimer;

[Andres Gonzalez]

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

diff --git a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
index 2d4d86e29238..1cf53a7a3ab3 100644
--- a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
+++ b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
@@ -487,7 +487,7 @@ void AXObjectCache::postPlatformAnnouncementNotification(const String& message)
 }

 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
-static void createIsolatedObjectIfNeeded(AccessibilityObject& object, std::optional<PageIdentifier> pageID)
+static void createIsolatedObjectIfNeeded(AccessibilityObject& object, AXObjectCache& cache)

AG: instead of passing the cache explicitly as a param, this would be cleaner as a class member function.

 {
     // The wrapper associated with a published notification may not have an isolated object yet.
     // This should only happen when the live object is ignored, meaning we will never create an isolated object for it.
@@ -499,10 +499,8 @@ static void createIsolatedObjectIfNeeded(AccessibilityObject& object, std::optio
     if (!wrapper || [wrapper hasIsolatedObject])
         return;

-    if (object.accessibilityIsIgnored()) {
-        if (auto tree = AXIsolatedTree::treeForPageID(pageID))
-            tree->addUnconnectedNode(object);
-    }
+    if (object.accessibilityIsIgnored())
+        cache.deferAddUnconnectedNode(object);
 }
 #endif

@@ -601,7 +599,7 @@ void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject*
     if (id wrapper = object->wrapper()) {
         [userInfo setObject:wrapper forKey:NSAccessibilityTextChangeElement];
 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
-        createIsolatedObjectIfNeeded(*object, m_pageID);
+        createIsolatedObjectIfNeeded(*object, *this);
 #endif
     }

@@ -653,7 +651,7 @@ void AXObjectCache::postTextStateChangePlatformNotification(AccessibilityObject*
     postTextReplacementPlatformNotification(object, AXTextEditTypeUnknown, emptyString(), type, text, position);
 }

-static void postUserInfoForChanges(AccessibilityObject& rootWebArea, AccessibilityObject& object, NSMutableArray *changes, std::optional<PageIdentifier> pageID)
+static void postUserInfoForChanges(AccessibilityObject& rootWebArea, AccessibilityObject& object, NSMutableArray *changes, AXObjectCache& cache)
 {
     auto userInfo = adoptNS([[NSMutableDictionary alloc] initWithCapacity:4]);
     [userInfo setObject:@(platformChangeTypeForWebCoreChangeType(AXTextStateChangeTypeEdit)) forKey:NSAccessibilityTextStateChangeTypeKey];
@@ -663,9 +661,9 @@ static void postUserInfoForChanges(AccessibilityObject& rootWebArea, Accessibili
     if (id wrapper = object.wrapper()) {
         [userInfo setObject:wrapper forKey:NSAccessibilityTextChangeElement];
 #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
-        createIsolatedObjectIfNeeded(object, pageID);
+        createIsolatedObjectIfNeeded(object, cache);
 #else
-        UNUSED_PARAM(pageID);
+        UNUSED_PARAM(cache);
 #endif
     }

@@ -694,7 +692,7 @@ void AXObjectCache::postTextReplacementPlatformNotification(AccessibilityObject*
         [changes addObject:change];

     if (RefPtr root = rootWebArea())
-        postUserInfoForChanges(*root, *object, changes.get(), m_pageID);
+        postUserInfoForChanges(*root, *object, changes.get(), *this);
 }

 void AXObjectCache::postTextReplacementPlatformNotificationForTextControl(AccessibilityObject* object, const String& deletedText, const String& insertedText, HTMLTextFormControlElement& textControl)
@@ -717,7 +715,7 @@ void AXObjectCache::postTextReplacementPlatformNotificationForTextControl(Access
         [changes addObject:change];

     if (RefPtr root = rootWebArea())
-        postUserInfoForChanges(*root, *object, changes.get(), m_pageID);
+        postUserInfoForChanges(*root, *object, changes.get(), *this);
 }

 void AXObjectCache::frameLoadingEventPlatformNotification(AccessibilityObject* axFrameObject, AXLoadingEvent loadingEvent)
@@ -1022,9 +1020,9 @@ void AXObjectCache::onSelectedTextChanged(const VisiblePositionRange& selection)
                 tree->setSelectedTextMarkerRange({ });
             else {
                 if (auto* startObject = get(startPosition.anchorNode()))
-                    createIsolatedObjectIfNeeded(*startObject, m_pageID);
+                    createIsolatedObjectIfNeeded(*startObject, *this);
                 if (auto* endObject = get(endPosition.anchorNode()))
-                    createIsolatedObjectIfNeeded(*endObject, m_pageID);
+                    createIsolatedObjectIfNeeded(*endObject, *this);

                 tree->setSelectedTextMarkerRange({ selection });
             }

[Joshua Hoffman]

Created attachment 471055 [details]
Patch

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #11)
> (In reply to Joshua Hoffman from comment #8)
> > Created attachment 470977 [details]
> > Patch
> 
> diff --git a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
> b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
> index 2d4d86e29238..1cf53a7a3ab3 100644
> --- a/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
> +++ b/Source/WebCore/accessibility/mac/AXObjectCacheMac.mm
> @@ -487,7 +487,7 @@ void
> AXObjectCache::postPlatformAnnouncementNotification(const String& message)
>  }
> 
>  #if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> -static void createIsolatedObjectIfNeeded(AccessibilityObject& object,
> std::optional<PageIdentifier> pageID)
> +static void createIsolatedObjectIfNeeded(AccessibilityObject& object,
> AXObjectCache& cache)
> 
> AG: instead of passing the cache explicitly as a param, this would be
> cleaner as a class member function.



Updated this to be a member function, and was able to make deferAddUnconnectedNode private as a result.

[EWS]

Committed 277946@main (44ced5953d9e): <https://commits.webkit.org/277946@main>

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