AXIsolatedTree::updateChildren should not call nodeForID.
https://bugs.webkit.org/show_bug.cgi?id=212794
Reviewed by Chris Fleizach.
AXIsolatedTree::updateChildren is executed on the main thread and
therfore should not call nodeForID. Since it requires the children IDs
for the isolated object whose children are being updated, we need to
store those children IDs outside the isolated object. For this reason
we added the m_nodeMap member variable which will maintain a map between
object ID and its children IDs.
In addition, since retrieving the root node happens very often and
required also a call to nodeForID, we now store a pointer to the root node instead of its ID, so there is no need to look it up in the reading map.
* accessibility/AXLogger.cpp:
(WebCore::operator<<):
* accessibility/isolatedtree/AXIsolatedObject.cpp:
(WebCore::AXIsolatedObject::setChildrenIDs):
(WebCore::AXIsolatedObject::appendChild): Renamed setChildrenIDs.
* accessibility/isolatedtree/AXIsolatedObject.h:
* accessibility/isolatedtree/AXIsolatedTree.cpp:
(WebCore::AXIsolatedTree::clear):
(WebCore::AXIsolatedTree::create):
(WebCore::AXIsolatedTree::removeTreeForPageID):
(WebCore::AXIsolatedTree::nodeForID const):
(WebCore::AXIsolatedTree::generateSubtree):
(WebCore::AXIsolatedTree::createSubtree):
(WebCore::AXIsolatedTree::updateChildren):
(WebCore::AXIsolatedTree::rootNode):
(WebCore::AXIsolatedTree::setRootNode):
(WebCore::AXIsolatedTree::removeSubtree):
(WebCore::AXIsolatedTree::applyPendingChanges):
(WebCore::AXIsolatedTree::nodeInTreeForID): Deleted, not used.
(WebCore::AXIsolatedTree::setRootNodeID): Renamed setRootNode.
* accessibility/isolatedtree/AXIsolatedTree.h:
* accessibility/mac/WebAccessibilityObjectWrapperBase.mm:
(-[WebAccessibilityObjectWrapperBase attachIsolatedObject:]):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@262626 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 5b3f006..c4891a5 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,43 @@
+2020-06-05 Andres Gonzalez <andresg_22@apple.com>
+
+ AXIsolatedTree::updateChildren should not call nodeForID.
+ https://bugs.webkit.org/show_bug.cgi?id=212794
+
+ Reviewed by Chris Fleizach.
+
+ AXIsolatedTree::updateChildren is executed on the main thread and
+ therfore should not call nodeForID. Since it requires the children IDs
+ for the isolated object whose children are being updated, we need to
+ store those children IDs outside the isolated object. For this reason
+ we added the m_nodeMap member variable which will maintain a map between
+ object ID and its children IDs.
+ In addition, since retrieving the root node happens very often and
+ required also a call to nodeForID, we now store a pointer to the root node instead of its ID, so there is no need to look it up in the reading map.
+
+ * accessibility/AXLogger.cpp:
+ (WebCore::operator<<):
+ * accessibility/isolatedtree/AXIsolatedObject.cpp:
+ (WebCore::AXIsolatedObject::setChildrenIDs):
+ (WebCore::AXIsolatedObject::appendChild): Renamed setChildrenIDs.
+ * accessibility/isolatedtree/AXIsolatedObject.h:
+ * accessibility/isolatedtree/AXIsolatedTree.cpp:
+ (WebCore::AXIsolatedTree::clear):
+ (WebCore::AXIsolatedTree::create):
+ (WebCore::AXIsolatedTree::removeTreeForPageID):
+ (WebCore::AXIsolatedTree::nodeForID const):
+ (WebCore::AXIsolatedTree::generateSubtree):
+ (WebCore::AXIsolatedTree::createSubtree):
+ (WebCore::AXIsolatedTree::updateChildren):
+ (WebCore::AXIsolatedTree::rootNode):
+ (WebCore::AXIsolatedTree::setRootNode):
+ (WebCore::AXIsolatedTree::removeSubtree):
+ (WebCore::AXIsolatedTree::applyPendingChanges):
+ (WebCore::AXIsolatedTree::nodeInTreeForID): Deleted, not used.
+ (WebCore::AXIsolatedTree::setRootNodeID): Renamed setRootNode.
+ * accessibility/isolatedtree/AXIsolatedTree.h:
+ * accessibility/mac/WebAccessibilityObjectWrapperBase.mm:
+ (-[WebAccessibilityObjectWrapperBase attachIsolatedObject:]):
+
2020-06-05 Youenn Fablet <youenn@apple.com>
[Cocoa] Use AVAssetWriterDelegate to implement MediaRecorder
diff --git a/Source/WebCore/accessibility/AXLogger.cpp b/Source/WebCore/accessibility/AXLogger.cpp
index 2f353e4..c92e184 100644
--- a/Source/WebCore/accessibility/AXLogger.cpp
+++ b/Source/WebCore/accessibility/AXLogger.cpp
@@ -29,6 +29,9 @@
#include "config.h"
#include "AXLogger.h"
+#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
+#include "AXIsolatedObject.h"
+#endif
#include "AXObjectCache.h"
#include "Logging.h"
#include <wtf/text/TextStream.h>
@@ -732,7 +735,7 @@
{
TextStream::GroupScope groupScope(stream);
stream << "treeID " << tree.treeID();
- stream.dumpProperty("rootNodeID", tree.m_rootNodeID);
+ stream.dumpProperty("rootNodeID", tree.rootNode()->objectID());
stream.dumpProperty("focusedNodeID", tree.m_focusedNodeID);
AXLogger::add(stream, tree.rootNode(), true);
return stream;
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
index b64d839..848cf5f 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
@@ -456,10 +456,10 @@
m_attributeMap.set(propertyName, value);
}
-void AXIsolatedObject::appendChild(AXID axID)
+void AXIsolatedObject::setChildrenIDs(Vector<AXID>&& childrenIDs)
{
ASSERT(isMainThread());
- m_childrenIDs.append(axID);
+ m_childrenIDs = childrenIDs;
}
void AXIsolatedObject::setParent(AXID parent)
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
index 2bc9928..c494c4e 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
@@ -58,7 +58,7 @@
bool isDetached() const override;
void setParent(AXID);
- void appendChild(AXID);
+ void setChildrenIDs(Vector<AXID>&&);
private:
void detachRemoteParts(AccessibilityDetachmentType) override;
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index c000d34..def2641 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -78,6 +78,18 @@
AXTRACE("AXIsolatedTree::~AXIsolatedTree");
}
+void AXIsolatedTree::clear()
+{
+ AXTRACE("AXIsolatedTree::clear");
+ ASSERT(isMainThread());
+
+ 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");
@@ -85,12 +97,6 @@
return adoptRef(*new AXIsolatedTree());
}
-RefPtr<AXIsolatedObject> AXIsolatedTree::nodeInTreeForID(AXIsolatedTreeID treeID, AXID axID)
-{
- AXTRACE("AXIsolatedTree::nodeInTreeForID");
- return treeForID(treeID)->nodeForID(axID);
-}
-
RefPtr<AXIsolatedTree> AXIsolatedTree::treeForID(AXIsolatedTreeID treeID)
{
AXTRACE("AXIsolatedTree::treeForID");
@@ -117,12 +123,7 @@
if (auto optionalTree = treePageCache().take(pageID)) {
auto& tree { *optionalTree };
-
- LockHolder treeLocker { tree->m_changeLogLock };
- tree->m_pendingSubtreeRemovals.append(tree->m_rootNodeID);
- tree->setAXObjectCache(nullptr);
- treeLocker.unlockEarly();
-
+ tree->clear();
treeIDCache().remove(tree->treeID());
}
}
@@ -139,6 +140,10 @@
RefPtr<AXIsolatedObject> AXIsolatedTree::nodeForID(AXID axID) const
{
+ // 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());
return axID != InvalidAXID ? m_readerThreadNodeMap.get(axID) : nullptr;
}
@@ -166,7 +171,7 @@
appendNodeChanges(nodeChanges);
if (parentID == InvalidAXID)
- setRootNodeID(object->objectID());
+ setRootNode(object.ptr());
// FIXME: else attach the newly created subtree to its parent.
}
@@ -174,6 +179,7 @@
{
AXTRACE("AXIsolatedTree::createSubtree");
ASSERT(isMainThread());
+
auto object = AXIsolatedObject::create(axObject, m_treeID, parentID);
if (attachWrapper) {
object->attachPlatformWrapper(axObject.wrapper());
@@ -185,10 +191,13 @@
nodeChanges.append(NodeChange(object, axObject.wrapper()));
}
+ Vector<AXID> childrenIDs;
for (const auto& axChild : axObject.children()) {
- auto child = createSubtree(*axChild, object->objectID(), attachWrapper, nodeChanges);
- object->appendChild(child->objectID());
+ auto child = createSubtree(*axChild, axObject.objectID(), attachWrapper, nodeChanges);
+ childrenIDs.append(child->objectID());
}
+ m_nodeMap.set(object->objectID(), childrenIDs);
+ object->setChildrenIDs(WTFMove(childrenIDs));
return object;
}
@@ -235,24 +244,27 @@
if (!axObject.document() || !axObject.document()->hasLivingRenderTree())
return;
- applyPendingChanges();
- LockHolder locker { m_changeLogLock };
-
// updateChildren may be called as the result of a children changed
// notification for an axObject that has no associated isolated object.
// An example of this is when an empty element such as a <canvas> or <div>
// is added a new child. So find the closest ancestor of axObject that has
// an associated isolated object and update its children.
- RefPtr<AXIsolatedObject> object;
- auto* axAncestor = Accessibility::findAncestor(axObject, true, [&object, this] (const AXCoreObject& ancestor) {
- return object = nodeForID(ancestor.objectID());
+ auto iterator = m_nodeMap.end();
+ auto* axAncestor = Accessibility::findAncestor(axObject, true, [&iterator, this] (const AXCoreObject& ancestor) {
+ auto it = m_nodeMap.find(ancestor.objectID());
+ if (it != m_nodeMap.end()) {
+ iterator = it;
+ return true;
+ }
+ return false;
});
- ASSERT(object && object->objectID() != InvalidAXID);
- if (!axAncestor || !object)
+ ASSERT(axAncestor && iterator != m_nodeMap.end());
+ if (!axAncestor || iterator == m_nodeMap.end())
return; // nothing to update.
- auto removals = object->m_childrenIDs;
- locker.unlockEarly();
+ // iterator is pointing to the m_nodeMap entry corresponding to axAncestor->objectID().
+ ASSERT(iterator->key == axAncestor->objectID());
+ auto removals = iterator->value;
const auto& axChildren = axAncestor->children();
auto axChildrenIDs = axAncestor->childrenIDs();
@@ -265,7 +277,7 @@
// This is a new child, add it to the tree.
AXLOG("Adding a new child for:");
AXLOG(axChildren[i]);
- generateSubtree(*axChildren[i], object->objectID(), true);
+ generateSubtree(*axChildren[i], axAncestor->objectID(), true);
}
}
@@ -274,10 +286,11 @@
for (const AXID& childID : removals)
removeSubtree(childID);
+ // Lastly, make the children IDs of the isolated object to be the same as the AXObject's.
+ m_nodeMap.set(axAncestor->objectID(), axChildrenIDs);
{
- // Lastly, make the children IDs of the isolated object to be the same as the AXObject's.
LockHolder locker { m_changeLogLock };
- object->m_childrenIDs = axChildrenIDs;
+ m_pendingChildrenUpdates.append(std::make_pair(axAncestor->objectID(), axChildrenIDs));
}
}
@@ -296,18 +309,18 @@
RefPtr<AXIsolatedObject> AXIsolatedTree::rootNode()
{
AXTRACE("AXIsolatedTree::rootNode");
- // Apply pending changes in case the root node is in the pending changes.
- applyPendingChanges();
LockHolder locker { m_changeLogLock };
- return nodeForID(m_rootNodeID);
+ return m_rootNode;
}
-void AXIsolatedTree::setRootNodeID(AXID axID)
+void AXIsolatedTree::setRootNode(AXIsolatedObject* root)
{
- AXTRACE("AXIsolatedTree::setRootNodeID");
+ AXTRACE("AXIsolatedTree::setRootNode");
ASSERT(isMainThread());
ASSERT(m_changeLogLock.isLocked());
- m_rootNodeID = axID;
+ ASSERT(root);
+
+ m_rootNode = root;
}
void AXIsolatedTree::setFocusedNodeID(AXID axID)
@@ -332,6 +345,18 @@
{
AXTRACE("AXIsolatedTree::removeSubtree");
AXLOG(makeString("Removing subtree for axID ", axID));
+ ASSERT(isMainThread());
+
+ Vector<AXID> removals = { axID };
+ while (removals.size()) {
+ AXID axID = removals.takeLast();
+ auto it = m_nodeMap.find(axID);
+ if (it != m_nodeMap.end()) {
+ removals.appendVector(it->value);
+ m_nodeMap.remove(axID);
+ }
+ }
+
LockHolder locker { m_changeLogLock };
m_pendingSubtreeRemovals.append(axID);
}
@@ -402,10 +427,18 @@
ASSERT_UNUSED(addResult, addResult.iterator->value->wrapper());
// The reference count of the just added IsolatedObject must be 2
// because it is referenced by m_readerThreadNodeMap and m_pendingAppends.
- // When m_pendingAppends is cleared, the object will be held only by m_readerThreadNodeMap.
- ASSERT_UNUSED(addResult, addResult.iterator->value->refCount() == 2);
+ // When m_pendingAppends is cleared, the object will be held only by m_readerThreadNodeMap. The exception is the root node whose reference count is 3.
+ ASSERT_UNUSED(addResult, addResult.iterator->value->refCount() == 2
+ || (addResult.iterator->value.ptr() == m_rootNode.get() && m_rootNode->refCount() == 3));
}
m_pendingAppends.clear();
+
+ for (auto& update : m_pendingChildrenUpdates) {
+ AXLOG(makeString("updating children for axID ", update.first));
+ if (auto object = nodeForID(update.first))
+ object->setChildrenIDs(WTFMove(update.second));
+ }
+ m_pendingChildrenUpdates.clear();
}
} // namespace WebCore
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
index 513461e..36b87df 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
@@ -63,7 +63,6 @@
RefPtr<AXIsolatedObject> rootNode();
RefPtr<AXIsolatedObject> focusedNode();
RefPtr<AXIsolatedObject> nodeForID(AXID) const;
- static RefPtr<AXIsolatedObject> nodeInTreeForID(AXIsolatedTreeID, AXID);
Vector<RefPtr<AXCoreObject>> objectsForIDs(Vector<AXID>) const;
struct NodeChange {
@@ -86,7 +85,7 @@
// Both setRootNodeID and setFocusedNodeID are called during the generation
// of the IsolatedTree.
// Focused node updates in AXObjectCache use setFocusNodeID.
- void setRootNodeID(AXID);
+ void setRootNode(AXIsolatedObject*);
void setFocusedNodeID(AXID);
// Called on AX thread from WebAccessibilityObjectWrapper methods.
@@ -97,6 +96,7 @@
private:
AXIsolatedTree();
+ void clear();
static HashMap<AXIsolatedTreeID, Ref<AXIsolatedTree>>& treeIDCache();
static HashMap<PageIdentifier, Ref<AXIsolatedTree>>& treePageCache();
@@ -108,18 +108,21 @@
AXObjectCache* m_axObjectCache { nullptr };
- // Only access on AX thread requesting data.
+ // Only accessed on main thread.
+ HashMap<AXID, Vector<AXID>> m_nodeMap;
+ // Only accessed on AX thread requesting data.
HashMap<AXID, Ref<AXIsolatedObject>> m_readerThreadNodeMap;
// Written to by main thread under lock, accessed and applied by AX thread.
+ RefPtr<AXIsolatedObject> m_rootNode;
Vector<NodeChange> m_pendingAppends; // Nodes to be added to the tree and platform-wrapped.
Vector<AXID> m_pendingNodeRemovals; // Nodes to be removed from the tree.
Vector<AXID> m_pendingSubtreeRemovals; // Nodes whose subtrees are to be removed from the tree.
+ Vector<std::pair<AXID, Vector<AXID>>> m_pendingChildrenUpdates;
AXID m_pendingFocusedNodeID { InvalidAXID };
Lock m_changeLogLock;
AXIsolatedTreeID m_treeID;
- AXID m_rootNodeID { InvalidAXID };
AXID m_focusedNodeID { InvalidAXID };
};
diff --git a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm
index 22c9096..e4c1bad 100644
--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm
+++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm
@@ -306,8 +306,6 @@
- (void)attachIsolatedObject:(AXCoreObject*)isolatedObject
{
ASSERT(isolatedObject && (_identifier == InvalidAXID || _identifier == isolatedObject->objectID()));
- ASSERT(m_axObject);
- ASSERT(isolatedObject->objectID() == m_axObject->objectID());
m_isolatedObject = isolatedObject;
if (_identifier == InvalidAXID)
_identifier = m_isolatedObject->objectID();
