Changeset 287811 in webkit

Timestamp:

Jan 8, 2022, 8:46:39 AM (5 years ago)

Author:

Tyler Wilcock

Message:

AX: Improve WeakHashSet hygienics in AXObjectCache
​https://bugs.webkit.org/show_bug.cgi?id=234961

Reviewed by Andres Gonzalez.

AXObjectCache owns four WeakHashSets. WeakHashSets are not notified
when the objects they hold are deleted[1], so we should be cleaning
them up in AXObjectCache::remove(Node&).

This patch also replaces range-based for loop iteration over these
WeakHashSets with WeakHashSet::forEach, which inherently checks that
each item is valid (non-null and contained in the hashset) before
using the item.

[1]: ​https://github.com/WebKit/WebKit/blob/main/Introduction.md#weakhashset

• accessibility/AXObjectCache.cpp:

(WebCore::AXObjectCache::remove):
Delete the removed node from AXObjectCache's owned WeakHashSets.
(WebCore::filterWeakHashSetForRemoval): Added.
(WebCore::AXObjectCache::prepareForDocumentDestruction):
Use filterWeakHashSetForRemoval to remove soon-to-be-deleted nodes
from AXObjectCache's owned WeakHashSets.
(WebCore::AXObjectCache::performDeferredCacheUpdate):
Use WeakHashSet::forEach instead of range-based for loops.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• accessibility/AXObjectCache.cpp (modified) (5 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-01-08  Tyler Wilcock  <tyler_w@apple.com>
+
+        AX: Improve WeakHashSet hygienics in AXObjectCache
+        https://bugs.webkit.org/show_bug.cgi?id=234961
+
+        Reviewed by Andres Gonzalez.
+
+        AXObjectCache owns four WeakHashSets. WeakHashSets are not notified
+        when the objects they hold are deleted[1], so we should be cleaning
+        them up in AXObjectCache::remove(Node&).
+
+        This patch also replaces range-based for loop iteration over these
+        WeakHashSets with WeakHashSet::forEach, which inherently checks that
+        each item is valid (non-null and contained in the hashset) before
+        using the item.
+
+        [1]: https://github.com/WebKit/WebKit/blob/main/Introduction.md#weakhashset
+
+        * accessibility/AXObjectCache.cpp:
+        (WebCore::AXObjectCache::remove):
+        Delete the removed node from AXObjectCache's owned WeakHashSets.
+        (WebCore::filterWeakHashSetForRemoval): Added.
+        (WebCore::AXObjectCache::prepareForDocumentDestruction):
+        Use filterWeakHashSetForRemoval to remove soon-to-be-deleted nodes
+        from AXObjectCache's owned WeakHashSets.
+        (WebCore::AXObjectCache::performDeferredCacheUpdate):
+        Use WeakHashSet::forEach instead of range-based for loops.
+
 2022-01-08  Tyler Wilcock  <tyler_w@apple.com>
 

trunk/Source/WebCore/accessibility/AXObjectCache.cpp

@@ -864 +864 @@
         m_deferredAttributeChange.remove(downcast<Element>(&node));
         m_modalElementsSet.remove(downcast<Element>(&node));
+        m_deferredRecomputeIsIgnoredList.remove(downcast<Element>(node));
+        m_deferredSelectedChildredChangedList.remove(downcast<Element>(node));
+        m_deferredModalChangedList.remove(downcast<Element>(node));
+        m_deferredMenuListChange.remove(downcast<Element>(node));
     }
     m_deferredChildrenChangedNodeList.remove(&node);
@@ -3162 +3166 @@
 }
 
+static void filterWeakHashSetForRemoval(WeakHashSet<Element>& weakHashSet, const Document& document, HashSet<Ref<Node>>& nodesToRemove)
+{
+    weakHashSet.forEach([&] (auto& element) {
+        conditionallyAddNodeToFilterList(&element, document, nodesToRemove);
+    });
+}
+
 void AXObjectCache::prepareForDocumentDestruction(const Document& document)
 {
@@ -3169 +3180 @@
     filterListForRemoval(m_deferredTextChangedList, document, nodesToRemove);
     filterListForRemoval(m_deferredChildrenChangedNodeList, document, nodesToRemove);
+    filterWeakHashSetForRemoval(m_deferredRecomputeIsIgnoredList, document, nodesToRemove);
+    filterWeakHashSetForRemoval(m_deferredSelectedChildredChangedList, document, nodesToRemove);
+    filterWeakHashSetForRemoval(m_deferredModalChangedList, document, nodesToRemove);
+    filterWeakHashSetForRemoval(m_deferredMenuListChange, document, nodesToRemove);
     filterMapForRemoval(m_deferredTextFormControlValue, document, nodesToRemove);
     filterMapForRemoval(m_deferredAttributeChange, document, nodesToRemove);
@@ -3219 +3234 @@
     m_deferredTextChangedList.clear();
 
-    for (auto& element : m_deferredRecomputeIsIgnoredList) {
+    m_deferredRecomputeIsIgnoredList.forEach([this] (auto& element) {
         if (auto* renderer = element.renderer())
             recomputeIsIgnored(renderer);
-    }
+    });
     m_deferredRecomputeIsIgnoredList.clear();
-    
-    for (auto& selectElement : m_deferredSelectedChildredChangedList)
+
+    m_deferredSelectedChildredChangedList.forEach([this] (auto& selectElement) {
         selectedChildrenChanged(&selectElement);
+    });
     m_deferredSelectedChildredChangedList.clear();
 
@@ -3247 +3263 @@
     m_deferredFocusedNodeChange.clear();
 
-    for (auto& deferredModalChangedElement : m_deferredModalChangedList)
+    m_deferredModalChangedList.forEach([this] (auto& deferredModalChangedElement) {
         handleModalChange(deferredModalChangedElement);
+    });
     m_deferredModalChangedList.clear();
 
-    for (auto& deferredMenuListChangeElement : m_deferredMenuListChange)
+    m_deferredMenuListChange.forEach([this] (auto& deferredMenuListChangeElement) {
         postNotification(&deferredMenuListChangeElement, AXObjectCache::AXMenuListValueChanged);
+    });
     m_deferredMenuListChange.clear();