WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
251223
AX: WebCore::AXIsolatedTree should use WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<>.
https://bugs.webkit.org/show_bug.cgi?id=251223
Summary
AX: WebCore::AXIsolatedTree should use WTF::ThreadSafeRefCountedAndCanMakeThr...
Andres Gonzalez
Reported
2023-01-26 13:05:09 PST
Currently it derives from ThreadSafeRefCounted and CanMakeWeakPtr.
Attachments
Patch
(9.98 KB, patch)
2023-01-26 17:11 PST
,
Andres Gonzalez
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(10.23 KB, patch)
2023-01-26 18:04 PST
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Patch
(9.31 KB, patch)
2023-01-27 14:28 PST
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Patch
(9.34 KB, patch)
2023-01-27 14:55 PST
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Patch
(8.94 KB, patch)
2023-01-28 12:12 PST
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Patch
(8.95 KB, patch)
2023-01-28 19:25 PST
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2023-01-26 13:05:22 PST
<
rdar://problem/104710054
>
Andres Gonzalez
Comment 2
2023-01-26 13:06:07 PST
rdar://104512153
Andres Gonzalez
Comment 3
2023-01-26 17:11:25 PST
Created
attachment 464679
[details]
Patch
Andres Gonzalez
Comment 4
2023-01-26 18:04:15 PST
Created
attachment 464681
[details]
Patch
Tyler Wilcock
Comment 5
2023-01-26 18:17:05 PST
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 6
2023-01-26 19:04:45 PST
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
Comment 7
2023-01-27 14:28:59 PST
Created
attachment 464691
[details]
Patch
Andres Gonzalez
Comment 8
2023-01-27 14:36:34 PST
(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 9
2023-01-27 14:38:17 PST
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
Comment 10
2023-01-27 14:55:03 PST
Created
attachment 464694
[details]
Patch
Andres Gonzalez
Comment 11
2023-01-27 14:56:48 PST
(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 12
2023-01-27 16:15:52 PST
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
Comment 13
2023-01-27 16:16:02 PST
Close!
Andres Gonzalez
Comment 14
2023-01-28 12:12:54 PST
Created
attachment 464704
[details]
Patch
Andres Gonzalez
Comment 15
2023-01-28 12:15:33 PST
(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 16
2023-01-28 15:28:06 PST
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
Comment 17
2023-01-28 19:25:13 PST
Created
attachment 464740
[details]
Patch
EWS
Comment 18
2023-01-29 07:57:53 PST
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]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug