8000 AX: WebCore::AXIsolatedTree should use WTF::ThreadSafeRefCountedAndCa… · WebKit/WebKit@8f8d02d · GitHub
[go: up one dir, main page]
Skip to content

Commit 8f8d02d

Browse files
AndresGonzalezAppleAndresGonzalezApple
committed
AX: WebCore::AXIsolatedTree should use WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<>.
https://bugs.webkit.org/show_bug.cgi?id=251223 rdar://104512153 Reviewed by Alex Christensen. AXIsolatedTree now derives from ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr instead of ThreadSafeRefCounted and CanMakeWeakPtr, which was a problem since WeakPtr is not thread safe. In order to have a single AXTreeStore that keeps both WeakPtr<AXObjectCache> and ThreadSafeWeakPtr<AXIsolatedTree>, we added the variant AXTreePtr. Note that WeakPtr<AXObjectCache> does not have to be thread safe since AXObjectCache is only used on the main thread. Canonical link: https://commits.webkit.org/259536@main
1 parent 91955d0 commit 8f8d02d

File tree

5 files changed

+67
-25
lines changed

5 files changed

+67
-25
lines changed

‎Source/WebCore/accessibility/AXObjectCache.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ AXObjectCache::AXObjectCache(Document& document)
246246
loadingProgress = 1;
247247
m_loadingProgress = loadingProgress;
248248

249-
AXTreeStore::add(m_id, this);
249+
AXTreeStore::add(m_id, WeakPtr { this });
250250
}
251251

252252
AXObjectCache::~AXObjectCache()

‎Source/WebCore/accessibility/AXTextMarker.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ AXTextMarker::operator VisiblePosition() const
108108
{
109109
ASSERT(isMainThread());
110110

111-
auto* cache = AXTreeStore<AXObjectCache>::treeForID(treeID());
111+
auto* cache = AXTreeStore<AXObjectCache>::axObjectCacheForID(treeID());
112112
return cache ? cache->visiblePositionForTextMarkerData(m_data) : VisiblePosition();
113113
}
114114

@@ -123,7 +123,7 @@ AXTextMarker::operator CharacterOffset() const
123123
// When we are at a line wrap and the VisiblePosition is upstream, it means the text marker is at the end of the previous line.
124124
// We use the previous CharacterOffset so that it will match the Range.
125125
if (m_data.affinity == Affinity::Upstream) {
126-
if (auto cache = AXTreeStore<AXObjectCache>::treeForID(m_data.axTreeID()))
126+
if (auto* cache = AXTreeStore<AXObjectCache>::axObjectCacheForID(m_data.axTreeID()))
127127
return cache->previousCharacterOffset(result, false);
128128
}
129129
return result;
@@ -134,7 +134,7 @@ AXCoreObject* AXTextMarker::object() const
134134
if (isNull())
135135
return nullptr;
136136

137-
auto tree = AXTreeStore<AXObjectCache>::treeForID(treeID());
137+
auto* tree = AXTreeStore<AXObjectCache>::axObjectCacheForID(treeID());
138138
return tree ? tree->objectForID(objectID()) : nullptr;
139139
}
140140

‎Source/WebCore/accessibility/AXTreeStore.h‎

Lines changed: 60 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,38 +25,65 @@
2525
#pragma once
2626

2727
#include "AccessibilityObjectInterface.h"
28+
#include <variant>
2829
#include <wtf/HashMap.h>
2930
#include <wtf/Lock.h>
3031
#include <wtf/NeverDestroyed.h>
32+
#include <wtf/ThreadSafeWeakPtr.h>
3133
#include <wtf/WeakPtr.h>
3234

3335
namespace WebCore {
3436

37+
#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
38+
class AXIsolatedTree;
39+
#endif
40+
class AXObjectCache;
41+
42+
using AXTreePtr = std::variant<WeakPtr<AXObjectCache>
43+
#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
44+
, ThreadSafeWeakPtr<AXIsolatedTree>
45+
#endif
46+
>;
47+
3548
template<typename T>
3649
class AXTreeStore {
3750
WTF_MAKE_NONCOPYABLE(AXTreeStore);
3851
WTF_MAKE_FAST_ALLOCATED;
3952
public:
4053
AXID treeID() const { return m_id; }
41-
static T* treeForID(AXID);
54+
static AXObjectCache* axObjectCacheForID(AXID);
55+
#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
56+
static RefPtr<AXIsolatedTree> isolatedTreeForID(AXID);
57+
#endif
58+
4259
protected:
4360
AXTreeStore() = default;
44-
static void add(AXID, WeakPtr<T>);
61+
static void add(AXID, const AXTreePtr&);
4562
static void remove(AXID);
4663
static bool contains(AXID);
4764

4865
static AXID generateNewID();
4966
const AXID m_id { generateNewID() };
5067
static Lock s_storeLock;
5168
private:
52-
static HashMap<AXID, WeakPtr<T>>& map() WTF_REQUIRES_LOCK(s_storeLock);
69+
static HashMap<AXID, AXTreePtr>& map() WTF_REQUIRES_LOCK(s_storeLock);
5370
};
5471

5572
template<typename T>
56-
inline void AXTreeStore<T>::add(AXID axID, WeakPtr<T> t)
73+
inline void AXTreeStore<T>::add(AXID axID, const AXTreePtr& tree)
5774
{
58-
Locker locker { s_storeLock };
59-
map().add(axID, t);
75+
switchOn(tree,
76+
[&] (const WeakPtr<AXObjectCache>& typedTree) {
77+
Locker locker { s_storeLock };
78+
map().add(axID, typedTree);
79+
}
80+
#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
81+
, [&] (const ThreadSafeWeakPtr<AXIsolatedTree>& typedTree) {
82+
Locker locker { s_storeLock };
83+
map().add(axID, typedTree.get());
84+
}
85+
#endif
86+
);
6087
}
6188

6289
template<typename T>
@@ -74,16 +101,39 @@ inline bool AXTreeStore<T>::contains(AXID axID)
74101
}
75102

76103
template<typename T>
77-
inline T* AXTreeStore<T>::treeForID(AXID axID)
104+
inline AXObjectCache* AXTreeStore<T>::axObjectCacheForID(AXID axID)
105+
{
106+
Locker locker { s_storeLock };
107+
auto tree = map().get(axID);
108+
return switchOn(tree,
109+
[] (WeakPtr<AXObjectCache>& typedTree) -> AXObjectCache* { return typedTree.get(); },
110+
[] (auto&) -> AXObjectCache* {
111+
ASSERT_NOT_REACHED();
112+
return nullptr;
113+
}
114+
);
115+
}
116+
117+
#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
118+
template<typename T>
119+
inline RefPtr<AXIsolatedTree> AXTreeStore<T>::isolatedTreeForID(AXID axID)
78120
{
79121
Locker locker { s_storeLock };
80-
return map().get(axID).get();
122+
auto tree = map().get(axID);
123+
return switchOn(tree,
124+
[] (ThreadSafeWeakPtr<AXIsolatedTree>& typedTree) -> RefPtr<AXIsolatedTree> { return typedTree.get(); },
125+
[] (auto&) -> RefPtr<AXIsolatedTree> {
126+
ASSERT_NOT_REACHED();
127+
return nullptr;
128+
}
129+
);
81130
}
131+
#endif
82132

83133
template<typename T>
84-
inline HashMap<AXID, WeakPtr<T>>& AXTreeStore<T>::map()
134+
inline HashMap<AXID, AXTreePtr>& AXTreeStore<T>::map()
85135
{
86-
static NeverDestroyed<HashMap<AXID, WeakPtr<T>>> map;
136+
static NeverDestroyed<HashMap<AXID, AXTreePtr>> map;
87137
return map;
88138
}
89139

‎Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ Ref<AXIsolatedTree> AXIsolatedTree::create(AXObjectCache* axObjectCache)
9191

9292
// Now that the tree is ready to take client requests, add it to the tree
9393
// maps so that it can be found.
94-
AXTreeStore::add(tree->treeID(), tree.copyRef());
94+
AXTreeStore::add(tree->treeID(), tree.ptr());
9595
auto pageID = axObjectCache->pageID();
9696
Locker locker { s_storeLock };
9797
ASSERT(!treePageCache().contains(*pageID));
@@ -772,15 +772,7 @@ void AXIsolatedTree::applyPendingChanges()
772772
// We don't need to bother clearing out any other non-cycle-causing member variables as they
773773
// will be cleaned up automatically when the tree is destroyed.
774774

775-
#ifndef NDEBUG
776775
ASSERT(AXTreeStore::contains(treeID()));
777-
if (auto tree = AXTreeStore::treeForID(treeID())) {
778-
// At this point, there should only be two references left to this tree -- one in the map,
779-
// and the `protectedThis` above.
780-
ASSERT(tree->refCount() == 2, "Unexpected refcount before attempting to destroy isolated tree: %d", tree->refCount());
781-
}
782-
#endif
783-
784776
AXTreeStore::remove(treeID());
785777
return;
786778
}

‎Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
#include <wtf/Lock.h>
3636
#include <wtf/RefPtr.h>
3737
#include <wtf/ThreadSafeRefCounted.h>
38+
#include <wtf/ThreadSafeWeakPtr.h>
3839

3940
namespace WTF {
4041
class TextStream;
@@ -273,8 +274,7 @@ struct AXPropertyChange {
273274
AXPropertyMap properties; // Changed properties.
274275
};
275276

276-
class AXIsolatedTree : public ThreadSafeRefCounted<AXIsolatedTree>
277-
, public CanMakeWeakPtr<AXIsolatedTree>
277+
class AXIsolatedTree : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<AXIsolatedTree>
278278
, public AXTreeStore<AXIsolatedTree> {
279279
WTF_MAKE_NONCOPYABLE(AXIsolatedTree);
280280
WTF_MAKE_FAST_ALLOCATED;

0 commit comments

Comments
 (0)
0