Bug 251223 – AX: WebCore::AXIsolatedTree should use WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<>.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

Currently it derives from ThreadSafeRefCounted and CanMakeWeakPtr.

[Radar WebKit Bug Importer]

<rdar://problem/104710054>

[Andres Gonzalez]

rdar://104512153

[Andres Gonzalez]

Created attachment 464679 [details]
Patch

[Andres Gonzalez]

Created attachment 464681 [details]
Patch

[Tyler Wilcock]

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

[Alex Christensen]

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.

[Andres Gonzalez]

Created attachment 464691 [details]
Patch

[Andres Gonzalez]

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

[Alex Christensen]

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.

[Andres Gonzalez]

Created attachment 464694 [details]
Patch

[Andres Gonzalez]

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

[Alex Christensen]

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

[Alex Christensen]

Close!

[Andres Gonzalez]

Created attachment 464704 [details]
Patch

[Andres Gonzalez]

(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!

[Alex Christensen]

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)

[Andres Gonzalez]

Created attachment 464740 [details]
Patch

[EWS]

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