Diff
Modified: trunk/Source/WebCore/ChangeLog (270237 => 270238)
--- trunk/Source/WebCore/ChangeLog 2020-11-30 12:49:33 UTC (rev 270237)
+++ trunk/Source/WebCore/ChangeLog 2020-11-30 14:11:02 UTC (rev 270238)
@@ -1,3 +1,55 @@
+2020-11-30 Andres Gonzalez <[email protected]>
+
+ AXIsolatedTree::m_axObjectCache should not be accessed on the secondary AX thread.
+ https://bugs.webkit.org/show_bug.cgi?id=219223
+
+ Reviewed by Chris Fleizach.
+
+ AXIsolatedTree::m_axObjectCache was being set on the main thread and
+ also accessed on the AX thread in the nodeForID and applyPendingChanges
+ methods. This problem is fixed with this change by adding the member
+ variable m_usedOnAXThread that is initialized on the main thread and only
+ used on the AX thread. This means also a performance optimization since
+ nodeForID and applyPendingChanges are called very frequently.
+ In addition, the change to AXIsolatedTree::create fixes a possible race
+ condition between the time the newly created tree was added to the map
+ of trees on the main thread, and the time that the tree is ready to be
+ used on the AX thread. Now the newly created tree is not added to the
+ trees map until it is fully functional.
+
+ * accessibility/AXObjectCache.cpp:
+ (WebCore::AXObjectCache::getOrCreateIsolatedTree const):
+ (WebCore::AXObjectCache::generateIsolatedTree): Deleted, the creation
+ and initialization of the isolated tree is now done in
+ AXIsolatedTree::create.
+
+ * accessibility/AXObjectCache.h:
+ (WebCore::AXObjectCache::pageID const): Getter for the m_pageID member
+ variable.
+
+ * accessibility/isolatedtree/AXIsolatedObject.cpp:
+ (WebCore::AXIsolatedObject::AXIsolatedObject): Now takes a pointer t the
+ tree instead of a tree ID.
+ (WebCore::AXIsolatedObject::create): Same as above.
+ (WebCore::AXIsolatedObject::associatedAXObject const):
+ (WebCore::AXIsolatedObject::setSelectedChildren): Check for nullity of
+ the AXObjectCache.
+ * accessibility/isolatedtree/AXIsolatedObject.h:
+ * accessibility/isolatedtree/AXIsolatedTree.cpp:
+ (WebCore::AXIsolatedTree::AXIsolatedTree): Takes an AXObjectCache.
+ (WebCore::AXIsolatedTree::clear):
+ (WebCore::AXIsolatedTree::create): Takes an AXObjectCache.
+ (WebCore::AXIsolatedTree::nodeForID const):
+ (WebCore::AXIsolatedTree::createSubtree):
+ (WebCore::AXIsolatedTree::updateNode):
+ (WebCore::AXIsolatedTree::applyPendingChanges):
+ (WebCore::AXIsolatedTree::createTreeForPageID): Deleted, became
+ AXIsolatedTree::create.
+ * accessibility/isolatedtree/AXIsolatedTree.h:
+ (WebCore::AXIsolatedTree::axObjectCache const):
+ (WebCore::AXIsolatedTree::setAXObjectCache): Deleted, can be set only at
+ construction time.
+
2020-11-30 Xabier Rodriguez Calvar <[email protected]>
[GStreamer] Media player does not properly inhibit, uninhibit sleep
Modified: trunk/Source/WebCore/accessibility/AXObjectCache.cpp (270237 => 270238)
--- trunk/Source/WebCore/accessibility/AXObjectCache.cpp 2020-11-30 12:49:33 UTC (rev 270237)
+++ trunk/Source/WebCore/accessibility/AXObjectCache.cpp 2020-11-30 14:11:02 UTC (rev 270238)
@@ -790,7 +790,7 @@
auto tree = AXIsolatedTree::treeForPageID(*m_pageID);
if (!tree) {
tree = Accessibility::retrieveValueFromMainThread<RefPtr<AXIsolatedTree>>([this] () -> RefPtr<AXIsolatedTree> {
- return generateIsolatedTree(*m_pageID, m_document);
+ return AXIsolatedTree::create(const_cast<AXObjectCache*>(this));
});
AXObjectCache::initializeSecondaryAXThread();
}
@@ -3162,31 +3162,6 @@
}
#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
-Ref<AXIsolatedTree> AXObjectCache::generateIsolatedTree(PageIdentifier pageID, Document& document)
-{
- AXTRACE("AXObjectCache::generateIsolatedTree");
- RELEASE_ASSERT(isMainThread());
-
- RefPtr<AXIsolatedTree> tree(AXIsolatedTree::createTreeForPageID(pageID));
-
- // Set the root and focused objects in the isolated tree. For that, we need
- // the root and the focused object in the AXObject tree.
- auto* axObjectCache = document.axObjectCache();
- if (!axObjectCache)
- return makeRef(*tree);
- tree->setAXObjectCache(axObjectCache);
-
- auto* axRoot = axObjectCache->getOrCreate(document.view());
- if (axRoot)
- tree->generateSubtree(*axRoot, nullptr, true);
-
- auto* axFocus = axObjectCache->focusedObjectForPage(document.page());
- if (axFocus)
- tree->setFocusedNodeID(axFocus->objectID());
-
- return makeRef(*tree);
-}
-
void AXObjectCache::updateIsolatedTree(AXCoreObject& object, AXNotification notification)
{
AXTRACE("AXObjectCache::updateIsolatedTree");
Modified: trunk/Source/WebCore/accessibility/AXObjectCache.h (270237 => 270238)
--- trunk/Source/WebCore/accessibility/AXObjectCache.h 2020-11-30 12:49:33 UTC (rev 270237)
+++ trunk/Source/WebCore/accessibility/AXObjectCache.h 2020-11-30 14:11:02 UTC (rev 270238)
@@ -352,6 +352,7 @@
AXComputedObjectAttributeCache* computedObjectAttributeCache() { return m_computedObjectAttributeCache.get(); }
Document& document() const { return m_document; }
+ Optional<PageIdentifier> pageID() const { return m_pageID; }
#if PLATFORM(MAC)
static void setShouldRepostNotificationsForTests(bool value);
@@ -372,7 +373,6 @@
AXCoreObject* isolatedTreeRootObject();
AXCoreObject* isolatedTreeFocusedObject();
void setIsolatedTreeFocusedObject(Node*);
- static Ref<AXIsolatedTree> generateIsolatedTree(PageIdentifier, Document&);
RefPtr<AXIsolatedTree> getOrCreateIsolatedTree() const;
void updateIsolatedTree(AXCoreObject&, AXNotification);
void updateIsolatedTree(AXCoreObject*, AXNotification);
@@ -432,7 +432,6 @@
private:
AccessibilityObject* rootWebArea();
-
static AccessibilityObject* focusedImageMapUIElement(HTMLAreaElement*);
AXID getAXID(AccessibilityObject*);
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp (270237 => 270238)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp 2020-11-30 12:49:33 UTC (rev 270237)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp 2020-11-30 14:11:02 UTC (rev 270238)
@@ -36,14 +36,12 @@
namespace WebCore {
-AXIsolatedObject::AXIsolatedObject(AXCoreObject& object, AXIsolatedTreeID treeID, AXID parentID)
- : m_treeID(treeID)
+AXIsolatedObject::AXIsolatedObject(AXCoreObject& object, AXIsolatedTree* tree, AXID parentID)
+ : m_cachedTree(tree)
, m_parentID(parentID)
, m_id(object.objectID())
{
ASSERT(isMainThread());
- if (auto tree = AXIsolatedTree::treeForID(m_treeID))
- m_cachedTree = tree;
if (m_id != InvalidAXID)
initializeAttributeData(object, parentID == InvalidAXID);
else {
@@ -52,9 +50,9 @@
}
}
-Ref<AXIsolatedObject> AXIsolatedObject::create(AXCoreObject& object, AXIsolatedTreeID treeID, AXID parentID)
+Ref<AXIsolatedObject> AXIsolatedObject::create(AXCoreObject& object, AXIsolatedTree* tree, AXID parentID)
{
- return adoptRef(*new AXIsolatedObject(object, treeID, parentID));
+ return adoptRef(*new AXIsolatedObject(object, tree, parentID));
}
AXIsolatedObject::~AXIsolatedObject()
@@ -405,6 +403,14 @@
initializePlatformProperties(object);
}
+AXCoreObject* AXIsolatedObject::associatedAXObject() const
+{
+ ASSERT(isMainThread());
+
+ auto* axObjectCache = this->axObjectCache();
+ return axObjectCache && m_id != InvalidAXID ? axObjectCache->objectFromAXID(m_id) : nullptr;
+}
+
void AXIsolatedObject::setMathscripts(AXPropertyName propertyName, AXCoreObject& object)
{
AccessibilityMathMultiscriptPairs pairs;
@@ -511,10 +517,12 @@
return;
}
- ASSERT(axObjectCache());
+ auto* axObjectCache = this->axObjectCache();
+ if (!axObjectCache)
+ return;
auto axIDs = tree()->idsForObjects(selectedChildren);
- auto axSelectedChildren = axObjectCache()->objectsForIDs(axIDs);
+ auto axSelectedChildren = axObjectCache->objectsForIDs(axIDs);
object->setSelectedChildren(axSelectedChildren);
});
}
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h (270237 => 270238)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h 2020-11-30 12:49:33 UTC (rev 270237)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h 2020-11-30 14:11:02 UTC (rev 270238)
@@ -47,7 +47,7 @@
class AXIsolatedObject final : public AXCoreObject {
friend class AXIsolatedTree;
public:
- static Ref<AXIsolatedObject> create(AXCoreObject&, AXIsolatedTreeID, AXID parentID);
+ static Ref<AXIsolatedObject> create(AXCoreObject&, AXIsolatedTree*, AXID parentID);
~AXIsolatedObject();
void setObjectID(AXID id) override { m_id = id; }
@@ -58,14 +58,6 @@
bool isDetached() const override;
void setParent(AXID);
- void setChildrenIDs(Vector<AXID>&& childrenIDs)
- {
- // FIXME: The following ASSERT should be met but it is commented out at the
- // moment because of <rdar://problem/63985646> After calling _AXUIElementUseSecondaryAXThread(true),
- // still receives client request on main thread.
- // ASSERT(axObjectCache()->canUseSecondaryAXThread() ? !isMainThread() : isMainThread());
- m_childrenIDs = childrenIDs;
- }
private:
void detachRemoteParts(AccessibilityDetachmentType) override;
@@ -73,19 +65,14 @@
AXID parent() const { return m_parentID; }
- AXIsolatedTreeID treeIdentifier() const { return m_treeID; }
AXIsolatedTree* tree() const { return m_cachedTree.get(); }
AXIsolatedObject() = default;
- AXIsolatedObject(AXCoreObject&, AXIsolatedTreeID, AXID parentID);
+ AXIsolatedObject(AXCoreObject&, AXIsolatedTree*, AXID parentID);
bool isAXIsolatedObjectInstance() const override { return true; }
void initializeAttributeData(AXCoreObject&, bool isRoot);
void initializePlatformProperties(const AXCoreObject&);
- AXCoreObject* associatedAXObject() const
- {
- ASSERT(isMainThread());
- return m_id != InvalidAXID ? axObjectCache()->objectFromAXID(m_id) : nullptr;
- }
+ AXCoreObject* associatedAXObject() const;
void setProperty(AXPropertyName, AXPropertyValueVariant&&, bool shouldRemove = false);
void setObjectProperty(AXPropertyName, AXCoreObject*);
@@ -650,7 +637,6 @@
String innerHTML() const override;
String outerHTML() const override;
- AXIsolatedTreeID m_treeID;
RefPtr<AXIsolatedTree> m_cachedTree;
AXID m_parentID { InvalidAXID };
AXID m_id { InvalidAXID };
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp (270237 => 270238)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2020-11-30 12:49:33 UTC (rev 270237)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2020-11-30 14:11:02 UTC (rev 270238)
@@ -55,10 +55,13 @@
return map;
}
-AXIsolatedTree::AXIsolatedTree()
+AXIsolatedTree::AXIsolatedTree(AXObjectCache* axObjectCache)
: m_treeID(newTreeID())
+ , m_axObjectCache(axObjectCache)
+ , m_usedOnAXThread(axObjectCache->canUseSecondaryAXThread())
{
AXTRACE("AXIsolatedTree::AXIsolatedTree");
+ ASSERT(isMainThread());
}
AXIsolatedTree::~AXIsolatedTree()
@@ -70,21 +73,14 @@
{
AXTRACE("AXIsolatedTree::clear");
ASSERT(isMainThread());
+ m_axObjectCache = nullptr;
LockHolder locker { m_changeLogLock };
m_pendingSubtreeRemovals.append(m_rootNode->objectID());
m_rootNode = nullptr;
m_nodeMap.clear();
- m_axObjectCache = nullptr;
}
-Ref<AXIsolatedTree> AXIsolatedTree::create()
-{
- AXTRACE("AXIsolatedTree::create");
- ASSERT(isMainThread());
- return adoptRef(*new AXIsolatedTree());
-}
-
RefPtr<AXIsolatedTree> AXIsolatedTree::treeForID(AXIsolatedTreeID treeID)
{
AXTRACE("AXIsolatedTree::treeForID");
@@ -91,16 +87,32 @@
return treeIDCache().get(treeID);
}
-Ref<AXIsolatedTree> AXIsolatedTree::createTreeForPageID(PageIdentifier pageID)
+Ref<AXIsolatedTree> AXIsolatedTree::create(AXObjectCache* axObjectCache)
{
- AXTRACE("AXIsolatedTree::createTreeForPageID");
+ AXTRACE("AXIsolatedTree::create");
+ ASSERT(isMainThread());
+ ASSERT(axObjectCache && axObjectCache->pageID());
+
+ auto tree = adoptRef(*new AXIsolatedTree(axObjectCache));
+
+ // Generate the nodes of the tree and set its root and focused objects.
+ // For this, we need the root and focused objects of the AXObject tree.
+ auto* axRoot = axObjectCache->getOrCreate(axObjectCache->document().view());
+ if (axRoot)
+ tree->generateSubtree(*axRoot, nullptr, true);
+ auto* axFocus = axObjectCache->focusedObjectForPage(axObjectCache->document().page());
+ if (axFocus)
+ tree->setFocusedNodeID(axFocus->objectID());
+
+ // Now that the tree is ready to take client requests, add it to the tree
+ // maps so that it can be found.
+ auto pageID = axObjectCache->pageID();
LockHolder locker(s_cacheLock);
- ASSERT(!treePageCache().contains(pageID));
+ ASSERT(!treePageCache().contains(*pageID));
+ treePageCache().set(*pageID, tree.copyRef());
+ treeIDCache().set(tree->treeID(), tree.copyRef());
- auto newTree = AXIsolatedTree::create();
- treePageCache().set(pageID, newTree.copyRef());
- treeIDCache().set(newTree->treeID(), newTree.copyRef());
- return newTree;
+ return tree;
}
void AXIsolatedTree::removeTreeForPageID(PageIdentifier pageID)
@@ -128,7 +140,7 @@
RefPtr<AXIsolatedObject> AXIsolatedTree::nodeForID(AXID axID) const
{
// In isolated tree mode 2, only access m_readerThreadNodeMap on the AX thread.
- if (axObjectCache()->canUseSecondaryAXThread() && isMainThread())
+ if (m_usedOnAXThread && isMainThread())
return nullptr;
return axID != InvalidAXID ? m_readerThreadNodeMap.get(axID) : nullptr;
@@ -187,7 +199,7 @@
AXTRACE("AXIsolatedTree::createSubtree");
ASSERT(isMainThread());
- auto object = AXIsolatedObject::create(axObject, m_treeID, parentID);
+ auto object = AXIsolatedObject::create(axObject, this, parentID);
if (object->objectID() == InvalidAXID) {
// Either the axObject has an invalid ID or something else went terribly wrong. Don't bother doing anything else.
ASSERT_NOT_REACHED();
@@ -229,7 +241,7 @@
auto* axParent = axObject.parentObject();
AXID parentID = axParent ? axParent->objectID() : InvalidAXID;
- auto newObject = AXIsolatedObject::create(axObject, m_treeID, parentID);
+ auto newObject = AXIsolatedObject::create(axObject, this, parentID);
newObject->m_childrenIDs = axObject.childrenIDs();
LockHolder locker { m_changeLogLock };
@@ -419,7 +431,7 @@
AXTRACE("AXIsolatedTree::applyPendingChanges");
// In isolated tree mode 2, only apply pending changes on the AX thread.
- if (axObjectCache()->canUseSecondaryAXThread() && isMainThread())
+ if (m_usedOnAXThread && isMainThread())
return;
LockHolder locker { m_changeLogLock };
@@ -487,7 +499,7 @@
for (auto& update : m_pendingChildrenUpdates) {
AXLOG(makeString("updating children for axID ", update.first));
if (auto object = nodeForID(update.first))
- object->setChildrenIDs(WTFMove(update.second));
+ object->m_childrenIDs = WTFMove(update.second);
}
m_pendingChildrenUpdates.clear();
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h (270237 => 270238)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2020-11-30 12:49:33 UTC (rev 270237)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2020-11-30 14:11:02 UTC (rev 270238)
@@ -322,16 +322,14 @@
WTF_MAKE_NONCOPYABLE(AXIsolatedTree); WTF_MAKE_FAST_ALLOCATED;
friend WTF::TextStream& operator<<(WTF::TextStream&, AXIsolatedTree&);
public:
- static Ref<AXIsolatedTree> create();
+ static Ref<AXIsolatedTree> create(AXObjectCache*);
virtual ~AXIsolatedTree();
- static Ref<AXIsolatedTree> createTreeForPageID(PageIdentifier);
static void removeTreeForPageID(PageIdentifier);
static RefPtr<AXIsolatedTree> treeForPageID(PageIdentifier);
static RefPtr<AXIsolatedTree> treeForID(AXIsolatedTreeID);
- AXObjectCache* axObjectCache() const { return m_axObjectCache; }
- void setAXObjectCache(AXObjectCache* axObjectCache) { m_axObjectCache = axObjectCache; }
+ AXObjectCache* axObjectCache() const;
RefPtr<AXIsolatedObject> rootNode();
RefPtr<AXIsolatedObject> focusedNode();
@@ -368,7 +366,7 @@
AXIsolatedTreeID treeID() const { return m_treeID; }
private:
- AXIsolatedTree();
+ AXIsolatedTree(AXObjectCache*);
void clear();
static HashMap<AXIsolatedTreeID, Ref<AXIsolatedTree>>& treeIDCache();
@@ -381,6 +379,7 @@
AXIsolatedTreeID m_treeID;
AXObjectCache* m_axObjectCache { nullptr };
+ bool m_usedOnAXThread { true };
// Only accessed on main thread.
HashMap<AXID, Vector<AXID>> m_nodeMap;
@@ -399,6 +398,12 @@
Lock m_changeLogLock;
};
+inline AXObjectCache* AXIsolatedTree::axObjectCache() const
+{
+ ASSERT(isMainThread());
+ return m_axObjectCache;
+}
+
} // namespace WebCore
#endif
