Bug 256644

<rdar://problem/109205791>

Created attachment 466317 [details]
Patch

Comment on attachment 466317 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:310
> +        setProperty(AXPropertyName::ScreenRelativePosition, object.screenRelativePosition());

Is it OK to cache this without including code to update it? The screen relative position can change when someone moves the window, for example.

Created attachment 466324 [details]
Patch

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 466317 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466317&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:310
> > +        setProperty(AXPropertyName::ScreenRelativePosition, object.screenRelativePosition());
> 
> Is it OK to cache this without including code to update it? The screen
> relative position can change when someone moves the window, for example.

Addressed the issue by caching only for the empty, temporary isolated tree.

Comment on attachment 466324 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:343
> +    auto* cache = axObjectCache();
> +    if (!cache)
> +        return { };

Is it possible to get in a scenario where some AXIsolatedTree always returns a null axObjectCache() but still has things added to m_unresolvedPendingAppends, meaning we would never clear it? Before this patch, we always cleared m_unresolvedPendingAppends even if there was no associated cache for some reason.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:348
> +        if (auto* axObject = cache->objectForID(unresolvedAppend.key)) {

This existed before your patch, but I think this should be RefPtr instead of auto* because we pass this object to nodeChangeForObject, which likely can do things that would cause this object to be `AXObjectCache::remove(AXID)` from the cache (deleting the cache's ref to this object).

Do you agree this could be possible?

(In reply to Tyler Wilcock from comment #6)
> Comment on attachment 466324 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466324&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:343
> > +    auto* cache = axObjectCache();
> > +    if (!cache)
> > +        return { };
> 
> Is it possible to get in a scenario where some AXIsolatedTree always returns
> a null axObjectCache() but still has things added to
> m_unresolvedPendingAppends, meaning we would never clear it? Before this
> patch, we always cleared m_unresolvedPendingAppends even if there was no
> associated cache for some reason.

No, before we were clearing this var only if there was a cache, which is fine:

-        if (auto* cache = axObjectCache()) {
-            resolvedAppends.reserveInitialCapacity(m_unresolvedPendingAppends.size());
-            for (const auto& unresolvedAppend : m_unresolvedPendingAppends) {
-                if (auto* axObject = cache->objectForID(unresolvedAppend.key)) {
-                    if (auto nodeChange = nodeChangeForObject(*axObject, unresolvedAppend.value))
-                        resolvedAppends.uncheckedAppend(WTFMove(*nodeChange));
-                }
-            }
-            m_unresolvedPendingAppends.clear();
Plus that if the cache is null, this isolated tree must be about to die, so it really doesn't matter whether this var is cleared or not.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:348
> > +        if (auto* axObject = cache->objectForID(unresolvedAppend.key)) {
> 
> This existed before your patch, but I think this should be RefPtr instead of
> auto* because we pass this object to nodeChangeForObject, which likely can
> do things that would cause this object to be `AXObjectCache::remove(AXID)`
> from the cache (deleting the cache's ref to this object).
> 
> Do you agree this could be possible?

nodeChangeForObject takes a Ref so it is holding its passed object already. I guess we haven't decided yet who is supposed to keep objects alive, the caller or the callee.

Committed 264008@main (225ae0791abf): <https://commits.webkit.org/264008@main>

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