Summary: | AX: In AXObjectCache, use WeakHashSet::forEach iteration and clean up stale hashset entries | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tyler Wilcock <tyler_w> | ||||||
Component: | Accessibility | Assignee: | 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
Tyler Wilcock
2022-01-07 08:33:38 PST
Created attachment 448600 [details]
Patch
(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? Created attachment 448607 [details]
Patch
(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. 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]. |