Bug 251223

<rdar://problem/104710054>

Created attachment 464679 [details] Patch

Created attachment 464681 [details] Patch

Comment on attachment 464681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=464681&action=review > COMMIT_MESSAGE:8 > +In order to have a single AXTreeStore that keeps both WeakPtr<AXObjectCache> and ThreadSafeWeakPtr<AXIsolatedTree>, we added the variant AXTreePtr. Note that this avoids having a ThreadSafeWeakPtr<AXObjectCache> which would be an overkill since AXObjectCache is only used on the main thread. Typo — "which would be an overkill" should be "which would be overkill" > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:780 > - if (auto tree = AXTreeStore::treeForID(treeID())) { > + if (auto* tree = AXTreeStore::isolatedTreeForID(treeID())) { > // At this point, there should only be two references left to this tree -- one in the map, > // and the `protectedThis` above. > - ASSERT(tree->refCount() == 2, "Unexpected refcount before attempting to destroy isolated tree: %d", tree->refCount()); > + ASSERT(tree->strongRefCount() == 2, "Unexpected refcount before attempting to destroy isolated tree: %d", tree->strongRefCount()); Correct me if I'm wrong, but with this patch we can now make a ThreadSafeWeakPtr<AXIsolatedTree>. That means we can change AXIsolatedObject::m_cachedTree to be a ThreadSafeWeakPtr<AXIsolatedTree> and remove this whole block (and therefore not add a new strongRefCount method).

Comment on attachment 464681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=464681&action=review > Source/WTF/wtf/ThreadSafeWeakPtr.h:104 > + size_t strongRefCount() const I don't think we should add this. The value returned is immediately possibly incorrect. If we do add it, it should be inside ASSERT_ENABLED so we only use it for assertions.

Created attachment 464691 [details] Patch

(In reply to Tyler Wilcock from comment #5) > Comment on attachment 464681 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=464681&action=review > > > COMMIT_MESSAGE:8 > > +In order to have a single AXTreeStore that keeps both WeakPtr<AXObjectCache> and ThreadSafeWeakPtr<AXIsolatedTree>, we added the variant AXTreePtr. Note that this avoids having a ThreadSafeWeakPtr<AXObjectCache> which would be an overkill since AXObjectCache is only used on the main thread. > > Typo — "which would be an overkill" should be "which would be overkill" Rephrased, hopefully it is clearer and correct. > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:780 > > - if (auto tree = AXTreeStore::treeForID(treeID())) { > > + if (auto* tree = AXTreeStore::isolatedTreeForID(treeID())) { > > // At this point, there should only be two references left to this tree -- one in the map, > > // and the `protectedThis` above. > > - ASSERT(tree->refCount() == 2, "Unexpected refcount before attempting to destroy isolated tree: %d", tree->refCount()); > > + ASSERT(tree->strongRefCount() == 2, "Unexpected refcount before attempting to destroy isolated tree: %d", tree->strongRefCount()); > > Correct me if I'm wrong, but with this patch we can now make a > ThreadSafeWeakPtr<AXIsolatedTree>. That means we can change > AXIsolatedObject::m_cachedTree to be a ThreadSafeWeakPtr<AXIsolatedTree> and > remove this whole block (and therefore not add a new strongRefCount method). Upon further investigation and discussion, we agreed in removing the ASSERT to check the reference count, and thus the need to add strongRefCount(), but not changing the strong reference to the Isolated tree in the object.

Comment on attachment 464691 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=464691&action=review > Source/WebCore/accessibility/AXTreeStore.h:136 > + [] (ThreadSafeWeakPtr<AXIsolatedTree>& typedTree) -> AXIsolatedTree* { return typedTree.get().get(); }, This needs to return a RefPtr<AXIsolatedTree> otherwise it might immediately be deleted by another thread.

Created attachment 464694 [details] Patch

(In reply to Alex Christensen from comment #9) > Comment on attachment 464691 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=464691&action=review > > > Source/WebCore/accessibility/AXTreeStore.h:136 > > + [] (ThreadSafeWeakPtr<AXIsolatedTree>& typedTree) -> AXIsolatedTree* { return typedTree.get().get(); }, > > This needs to return a RefPtr<AXIsolatedTree> otherwise it might immediately > be deleted by another thread. Fixed. Thanks.

Comment on attachment 464694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=464694&action=review > Source/WebCore/accessibility/AXTreeStore.h:76 > + [&] (WeakPtr<AXObjectCache>& typedTree) { This looks unnecessary. > Source/WebCore/accessibility/AXTreeStore.h:85 > + [&] (ThreadSafeWeakPtr<AXIsolatedTree>& typedTree) { ditto > Source/WebCore/accessibility/AXTreeStore.h:94 > + [] (auto&) { ditto

Close!

Created attachment 464704 [details] Patch

(In reply to Alex Christensen from comment #13) > Close! (In reply to Alex Christensen from comment #12) > Comment on attachment 464694 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=464694&action=review > > > Source/WebCore/accessibility/AXTreeStore.h:76 > > + [&] (WeakPtr<AXObjectCache>& typedTree) { > > This looks unnecessary. Fixed. > > > Source/WebCore/accessibility/AXTreeStore.h:85 > > + [&] (ThreadSafeWeakPtr<AXIsolatedTree>& typedTree) { > > ditto Fixed. > > > Source/WebCore/accessibility/AXTreeStore.h:94 > > + [] (auto&) { > > ditto Fixed. Thanks!

Comment on attachment 464704 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=464704&action=review > Source/WebCore/accessibility/AXTreeStore.h:79 > + }, This comma needs to be inside ENABLE(ACCESSIBILITY_ISOLATED_TREE)

Created attachment 464740 [details] Patch

Committed 259536@main (8f8d02ddc94e): <https://commits.webkit.org/259536@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 464740 [details].