Bug 234961

Summary: AX: In AXObjectCache, use WeakHashSet::forEach iteration and clean up stale hashset entries
Product: WebKit Reporter: Tyler Wilcock <tyler_w>
Component: AccessibilityAssignee: Tyler Wilcock <tyler_w>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Tyler Wilcock 2022-01-07 08:33:38 PST
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.
Comment 1 Radar WebKit Bug Importer 2022-01-07 08:33:51 PST
<rdar://problem/87253115>
Comment 2 Tyler Wilcock 2022-01-07 08:42:37 PST
Created attachment 448600 [details]
Patch
Comment 3 Andres Gonzalez 2022-01-07 09:24:59 PST
(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?
Comment 4 Tyler Wilcock 2022-01-07 10:02:41 PST
Created attachment 448607 [details]
Patch
Comment 5 Tyler Wilcock 2022-01-07 10:04:25 PST
(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.
Comment 6 EWS 2022-01-08 08:47:45 PST
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].