<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].