Bug 244724 – AX: Cache accessibilityIsIgnored before in AXIsolatedTree::generateSubtree and AXIsolatedTree::updateChildren

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

[Tyler Wilcock]

We do a lot of repeated live tree reads in these codepaths, so we should make sure we are caching is-ignored for better performance.

[Radar WebKit Bug Importer]

<rdar://problem/99501850>

[Tyler Wilcock]

Created attachment 462104 [details]
Patch

[chris fleizach]

Comment on attachment 462104 [details]
Patch

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

> COMMIT_MESSAGE:12
> +This patch also AXAttributeCacheEnabler to not disable the AXComputedObjectAttributeCache

also modifies(?)

> Source/WebCore/accessibility/AXObjectCache.h:605
> +    bool m_wasAlreadyCachingAttributes { false };

wasAlready feel redundant to just having "m_cachingAttributes"... or maybe something shorter "m_inAttributeCaching"

[Andres Gonzalez]

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

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

+    bool m_wasAlreadyCachingAttributes { false };

Why do we need this extra flag?

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -205,6 +205,8 @@ void AXIsolatedTree::generateSubtree(AXCoreObject& axObject)
     if (!axObject.objectID().isValid())
         return;

+    // We're about to a lot of read-only work, so start the attribute cache.

"to a lot" -> "to do a lot"

What do you mean by "read-only work"? Not read-only for this isolated tree.

@@ -533,6 +535,9 @@ void AXIsolatedTree::updateChildren(AccessibilityObject& axObject, ResolveNodeCh
     if (!axObject.document() || !axObject.document()->hasLivingRenderTree())
         return;

+    // We're about to a lot of read-only work, so start the attribute cache.

Dito.

[Tyler Wilcock]

Created attachment 466469 [details]
Patch

[Tyler Wilcock]

(In reply to chris fleizach from comment #3)
> Comment on attachment 462104 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=462104&action=review
> 
> > COMMIT_MESSAGE:12
> > +This patch also AXAttributeCacheEnabler to not disable the AXComputedObjectAttributeCache
> 
> also modifies(?)
> 
> > Source/WebCore/accessibility/AXObjectCache.h:605
> > +    bool m_wasAlreadyCachingAttributes { false };
> 
> wasAlready feel redundant to just having "m_cachingAttributes"... or maybe
> something shorter "m_inAttributeCaching"
The intent I'm trying to capture is whether AXObjectCache::computedObjectAttributeCache existed before this AXAttributeCacheEnabler was created (i.e. we were already caching attributes). If so, we need to set this flag so we don't destroy it when this AXAttributeCacheEnabler is destroyed.

I feel like m_inAttributeCaching or m_cachingAttributes don't capture that as well as m_wasAlreadyCachingAttributes...but I don't feel too strongly about it.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #4)
> (In reply to Tyler Wilcock from comment #2)
> > Created attachment 462104 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AXObjectCache.h
> +++ a/Source/WebCore/accessibility/AXObjectCache.h
> 
> +    bool m_wasAlreadyCachingAttributes { false };
> 
> Why do we need this extra flag?
I tried to describe this in the commit message, but let me know if that needs some more clarity.

[Andres Gonzalez]

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

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp

+    // We're about to a lot of read-only work, so start the attribute cache.

about to a lot -> about to do a lot ?

Is traversing the live tree always read-only? I think there are cases where getting the children for an object, for instance, can change the role of that object.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #6)
> (In reply to chris fleizach from comment #3)
> > Comment on attachment 462104 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=462104&action=review
> > 
> > > COMMIT_MESSAGE:12
> > > +This patch also AXAttributeCacheEnabler to not disable the AXComputedObjectAttributeCache
> > 
> > also modifies(?)
> > 
> > > Source/WebCore/accessibility/AXObjectCache.h:605
> > > +    bool m_wasAlreadyCachingAttributes { false };
> > 
> > wasAlready feel redundant to just having "m_cachingAttributes"... or maybe
> > something shorter "m_inAttributeCaching"
> The intent I'm trying to capture is whether
> AXObjectCache::computedObjectAttributeCache existed before this
> AXAttributeCacheEnabler was created (i.e. we were already caching
> attributes). If so, we need to set this flag so we don't destroy it when
> this AXAttributeCacheEnabler is destroyed.
> 
> I feel like m_inAttributeCaching or m_cachingAttributes don't capture that
> as well as m_wasAlreadyCachingAttributes...but I don't feel too strongly
> about it.

Agree that it should be a shorter name. 

Where is this flag turned off? generateSubtree and updateChildren will turn it on but not off, i.e., once it is on, it will be on until something else turns it off. I believe this whole mechanism is quite weak and bug prone.

[Tyler Wilcock]

Created attachment 466483 [details]
Patch

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #9)
> (In reply to Tyler Wilcock from comment #6)
> > (In reply to chris fleizach from comment #3)
> > > Comment on attachment 462104 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=462104&action=review
> > > 
> > > > COMMIT_MESSAGE:12
> > > > +This patch also AXAttributeCacheEnabler to not disable the AXComputedObjectAttributeCache
> > > 
> > > also modifies(?)
> > > 
> > > > Source/WebCore/accessibility/AXObjectCache.h:605
> > > > +    bool m_wasAlreadyCachingAttributes { false };
> > > 
> > > wasAlready feel redundant to just having "m_cachingAttributes"... or maybe
> > > something shorter "m_inAttributeCaching"
> > The intent I'm trying to capture is whether
> > AXObjectCache::computedObjectAttributeCache existed before this
> > AXAttributeCacheEnabler was created (i.e. we were already caching
> > attributes). If so, we need to set this flag so we don't destroy it when
> > this AXAttributeCacheEnabler is destroyed.
> > 
> > I feel like m_inAttributeCaching or m_cachingAttributes don't capture that
> > as well as m_wasAlreadyCachingAttributes...but I don't feel too strongly
> > about it.
> 
> Agree that it should be a shorter name. 
> 
> Where is this flag turned off? generateSubtree and updateChildren will turn
> it on but not off, i.e., once it is on, it will be on until something else
> turns it off. I believe this whole mechanism is quite weak and bug prone.
Re-capping our conversation off Bugzilla for posterity:

The cache is turned off when the highest-most-stackframe AXAttributeCacheEnabler is destroyed, or when we post any notification. We should be safe from this causing bugs because we unconditionally bust the cache in every known way the tree mutates such that a different is-ignored value would be computed.

[Tyler Wilcock]

Also made the name of the flag shorter.

[EWS]

Committed 264524@main (9128760a1c6a): <https://commits.webkit.org/264524@main>

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