AXIsolatedObject uses WeakPtr<AXIsolatedTree> in a different thread than it was created on
<rdar://problem/100178376>
Created attachment 462476 [details] Patch
Comment on attachment 462476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462476&action=review > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:770 > + m_rootNode = nullptr; Do we also need to clear out m_cachedTree somewhere
(In reply to Tyler Wilcock from comment #2) > Created attachment 462476 [details] > Patch --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp @@ -764,6 +764,14 @@ void AXIsolatedTree::applyPendingChanges() for (const auto& object : m_readerThreadNodeMap.values()) object->detach(AccessibilityDetachmentType::CacheDestroyed); + // Because each AXIsolatedObject holds a RefPtr to this tree, clear out any member variable + // that holds an AXIsolatedObject so the ref-cycle is broken and this tree can be destroyed. + m_readerThreadNodeMap.clear(); + m_rootNode = nullptr; + m_pendingAppends.clear(); + // We don't need to bother clearing out any other non-cycle-causing member variables as they + // will be cleaned up automatically when the tree is destroyed. + Locker locker { s_cacheLock }; #ifndef NDEBUG ASSERT(treeIDCache().contains(treeID())); Can we move this to an AXIsolatedTree::clear() function?
(In reply to Andres Gonzalez from comment #4) > (In reply to Tyler Wilcock from comment #2) > > Created attachment 462476 [details] > > Patch > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp > @@ -764,6 +764,14 @@ void AXIsolatedTree::applyPendingChanges() > for (const auto& object : m_readerThreadNodeMap.values()) > object->detach(AccessibilityDetachmentType::CacheDestroyed); > > + // Because each AXIsolatedObject holds a RefPtr to this tree, clear > out any member variable > + // that holds an AXIsolatedObject so the ref-cycle is broken and > this tree can be destroyed. > + m_readerThreadNodeMap.clear(); > + m_rootNode = nullptr; > + m_pendingAppends.clear(); > + // We don't need to bother clearing out any other non-cycle-causing > member variables as they > + // will be cleaned up automatically when the tree is destroyed. > + > Locker locker { s_cacheLock }; > #ifndef NDEBUG > ASSERT(treeIDCache().contains(treeID())); > > Can we move this to an AXIsolatedTree::clear() function? I tried this out locally, but didn't feel like an improvement. 1. This behavior is not a general purpose "clear", which sounds like it would clear all member variables. This only clears specific ones (which contain refs to the tree). So it seems like the name should be something like "clearBeforeDestruction". 2. This function would only be called from one place, and making the reader have to go somewhere else to find what this function does, out of context from the rest of the destruction code, feels less good. But I don't mind if you'd prefer it this way -- let me know. > Do we also need to clear out m_cachedTree somewhere I don't think so — m_cachedTree is on AXIsolatedObject, so by clearing m_readerThreadNodeMap, m_rootNode, and m_pendingAppends, we destroy the isolated objects within them, inherently causing their RefPtr<AXIsolatedTree> m_cachedTree to also be cleaned up.
Committed 254708@main (ace2a410b6ff): <https://commits.webkit.org/254708@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 462476 [details].