Bug 234961 – AX: In AXObjectCache, use WeakHashSet::forEach iteration and clean up stale hashset entries

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

Quoting WebKit's documentation for WeakHashSets:

https://github.com/WebKit/WebKit/blob/main/Introduction.md#weakhashset

> Because WeakHashSet does not get notified when the referenced object is deleted, the users / owners of WeakHashSet are still responsible for deleting the relevant entries from the set. Otherwise, WeakHashSet will hold onto WeakPtrImpl until computeSize is called or rehashing happens.

AXObjectCache does not always do this book-keeping for the WeakHashSets it holds.

[Radar WebKit Bug Importer]

<rdar://problem/87253115>

[Tyler Wilcock]

Created attachment 448600 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #2)
> Created attachment 448600 [details]
> Patch

--- a/Source/WebCore/ChangeLog
+++ a/Source/WebCore/ChangeLog

+        AX: Improve WeakHashSet hygienics in AXObjectCache

Would prefer a more concrete, yet concise, title stating the problem or what the change is doing: e.g., Use WeakHashSet::forEach to iterate over AXObjectCache members.

+        AXObjectCache owns four WeakHashSets:
+          - m_deferredRecomputeIsIgnoredList
+          - m_deferredSelectedChildredChangedList
+          - m_deferredModalChangedList
+          - m_deferredMenuListChange
+

You probably don't need to enumerate all member variables in the ChangeLog.

+        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.
+
Do we need to remove from the sets in AXObjectCache::remove? Since these are WeakPtr, I think they should be set to null, and thus the check on the loop would be enough, right?

--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ a/Source/WebCore/accessibility/AXObjectCache.cpp

+        m_deferredRecomputeIsIgnoredList.remove(downcast<Element>(node));
+        m_deferredSelectedChildredChangedList.remove(downcast<Element>(node));
+        m_deferredModalChangedList.remove(downcast<Element>(node));
+        m_deferredMenuListChange.remove(downcast<Element>(node));

Do we need to do this?

[Tyler Wilcock]

Created attachment 448607 [details]
Patch

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #3)
> (In reply to Tyler Wilcock from comment #2)
> > Created attachment 448600 [details]
> > Patch
> 
> --- a/Source/WebCore/ChangeLog
> +++ a/Source/WebCore/ChangeLog
> 
> +        AX: Improve WeakHashSet hygienics in AXObjectCache
> 
> Would prefer a more concrete, yet concise, title stating the problem or what
> the change is doing: e.g., Use WeakHashSet::forEach to iterate over
> AXObjectCache members.
Fixed.

> +        AXObjectCache owns four WeakHashSets:
> +          - m_deferredRecomputeIsIgnoredList
> +          - m_deferredSelectedChildredChangedList
> +          - m_deferredModalChangedList
> +          - m_deferredMenuListChange
> +
> 
> You probably don't need to enumerate all member variables in the ChangeLog.
Fixed.

> +        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.
> +
> Do we need to remove from the sets in AXObjectCache::remove? Since these are
> WeakPtr, I think they should be set to null, and thus the check on the loop
> would be enough, right?
> 
> --- a/Source/WebCore/accessibility/AXObjectCache.cpp
> +++ a/Source/WebCore/accessibility/AXObjectCache.cpp
> 
> +        m_deferredRecomputeIsIgnoredList.remove(downcast<Element>(node));
> +       
> m_deferredSelectedChildredChangedList.remove(downcast<Element>(node));
> +        m_deferredModalChangedList.remove(downcast<Element>(node));
> +        m_deferredMenuListChange.remove(downcast<Element>(node));
> 
> Do we need to do this?
We do need these removes, as WeakHashSets aren't informed when the backing objects go away.

[EWS]

Committed r287811 (245863@main): <https://commits.webkit.org/245863@main>

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