- Revision
- 256630
- Author
- andresg...@apple.com
- Date
- 2020-02-14 11:22:33 -0800 (Fri, 14 Feb 2020)
Log Message
When updating a subtree of the IsolatedTree, first remove the entire subtree, not just the subtree root.
https://bugs.webkit.org/show_bug.cgi?id=207759
Reviewed by Chris Fleizach.
When updating an IsolatedTree subtree, we were removing just the root
of the subtree. Added AXIsolatedTree::removeSubtree that is now used in
updateIsolatedTree.
* accessibility/AXObjectCache.cpp:
(WebCore::createIsolatedTreeHierarchy): If the wrapper is attached
during creation, set it to null in the NodeChange so that it is not
re-attached on the AX thread.
(WebCore::AXObjectCache::updateIsolatedTree): removeSubtree instead of
removeNode.
* accessibility/isolatedtree/AXIsolatedObject.cpp:
(WebCore::AXIsolatedObject::~AXIsolatedObject): When an IsolatedObject
is destroyed, it must have been detached from its wrapper.
* accessibility/isolatedtree/AXIsolatedObject.h:
* accessibility/isolatedtree/AXIsolatedTree.cpp:
(WebCore::AXIsolatedTree::NodeChange::NodeChange): Constructors now
take an IsolatedObject instead of a Ref.
(WebCore::AXIsolatedTree::removeNode):
(WebCore::AXIsolatedTree::removeSubtree):
(WebCore::AXIsolatedTree::applyPendingChanges): Attach wrappers only if
not null. The IsolatedObject refCount must be 2 at that point.
* accessibility/isolatedtree/AXIsolatedTree.h:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (256629 => 256630)
--- trunk/Source/WebCore/ChangeLog 2020-02-14 19:09:32 UTC (rev 256629)
+++ trunk/Source/WebCore/ChangeLog 2020-02-14 19:22:33 UTC (rev 256630)
@@ -1,3 +1,33 @@
+2020-02-14 Andres Gonzalez <andresg...@apple.com>
+
+ When updating a subtree of the IsolatedTree, first remove the entire subtree, not just the subtree root.
+ https://bugs.webkit.org/show_bug.cgi?id=207759
+
+ Reviewed by Chris Fleizach.
+
+ When updating an IsolatedTree subtree, we were removing just the root
+ of the subtree. Added AXIsolatedTree::removeSubtree that is now used in
+ updateIsolatedTree.
+
+ * accessibility/AXObjectCache.cpp:
+ (WebCore::createIsolatedTreeHierarchy): If the wrapper is attached
+ during creation, set it to null in the NodeChange so that it is not
+ re-attached on the AX thread.
+ (WebCore::AXObjectCache::updateIsolatedTree): removeSubtree instead of
+ removeNode.
+ * accessibility/isolatedtree/AXIsolatedObject.cpp:
+ (WebCore::AXIsolatedObject::~AXIsolatedObject): When an IsolatedObject
+ is destroyed, it must have been detached from its wrapper.
+ * accessibility/isolatedtree/AXIsolatedObject.h:
+ * accessibility/isolatedtree/AXIsolatedTree.cpp:
+ (WebCore::AXIsolatedTree::NodeChange::NodeChange): Constructors now
+ take an IsolatedObject instead of a Ref.
+ (WebCore::AXIsolatedTree::removeNode):
+ (WebCore::AXIsolatedTree::removeSubtree):
+ (WebCore::AXIsolatedTree::applyPendingChanges): Attach wrappers only if
+ not null. The IsolatedObject refCount must be 2 at that point.
+ * accessibility/isolatedtree/AXIsolatedTree.h:
+
2020-02-14 Antoine Quint <grao...@webkit.org>
[Web Animations] Style changes due to Web Animations should not trigger CSS Transitions
Modified: trunk/Source/WebCore/accessibility/AXObjectCache.cpp (256629 => 256630)
--- trunk/Source/WebCore/accessibility/AXObjectCache.cpp 2020-02-14 19:09:32 UTC (rev 256629)
+++ trunk/Source/WebCore/accessibility/AXObjectCache.cpp 2020-02-14 19:22:33 UTC (rev 256630)
@@ -3075,9 +3075,15 @@
static Ref<AXIsolatedObject> createIsolatedTreeHierarchy(AXCoreObject& object, AXIsolatedTreeID treeID, AXID parentID, bool attachWrapper, Vector<AXIsolatedTree::NodeChange>& nodeChanges)
{
auto isolatedObject = AXIsolatedObject::create(object, treeID, parentID);
- nodeChanges.append(AXIsolatedTree::NodeChange(isolatedObject, object.wrapper()));
- if (attachWrapper)
+ if (attachWrapper) {
isolatedObject->attachPlatformWrapper(object.wrapper());
+ // Since this object has already an attached wrapper, set the wrapper
+ // in the NodeChange to null so that it is not re-attached.
+ nodeChanges.append(AXIsolatedTree::NodeChange(isolatedObject, nullptr));
+ } else {
+ // Set the wrapper in the NodeChange so that it is set on the AX thread.
+ nodeChanges.append(AXIsolatedTree::NodeChange(isolatedObject, object.wrapper()));
+ }
for (const auto& child : object.children()) {
auto staticChild = createIsolatedTreeHierarchy(*child, treeID, isolatedObject->objectID(), attachWrapper, nodeChanges);
@@ -3128,7 +3134,7 @@
case AXCheckedStateChanged:
case AXChildrenChanged:
case AXValueChanged: {
- tree->removeNode(object->objectID());
+ tree->removeSubtree(object->objectID());
auto* parent = object->parentObject();
AXID parentID = parent ? parent->objectID() : InvalidAXID;
Vector<AXIsolatedTree::NodeChange> nodeChanges;
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp (256629 => 256630)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp 2020-02-14 19:09:32 UTC (rev 256629)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp 2020-02-14 19:22:33 UTC (rev 256630)
@@ -48,7 +48,10 @@
return adoptRef(*new AXIsolatedObject(object, treeID, parentID));
}
-AXIsolatedObject::~AXIsolatedObject() = default;
+AXIsolatedObject::~AXIsolatedObject()
+{
+ ASSERT(!wrapper());
+}
void AXIsolatedObject::initializeAttributeData(AXCoreObject& object, bool isRoot)
{
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h (256629 => 256630)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h 2020-02-14 19:09:32 UTC (rev 256629)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h 2020-02-14 19:22:33 UTC (rev 256630)
@@ -46,6 +46,7 @@
class AXIsolatedTree;
class AXIsolatedObject final : public AXCoreObject {
+ friend class AXIsolatedTree;
public:
static Ref<AXIsolatedObject> create(AXCoreObject&, AXIsolatedTreeID, AXID parentID);
~AXIsolatedObject();
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp (256629 => 256630)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2020-02-14 19:09:32 UTC (rev 256629)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2020-02-14 19:22:33 UTC (rev 256630)
@@ -42,14 +42,14 @@
return ++s_currentTreeID;
}
-AXIsolatedTree::NodeChange::NodeChange(const Ref<AXIsolatedObject>& isolatedObject, AccessibilityObjectWrapper* wrapper)
- : m_isolatedObject(isolatedObject.copyRef())
+AXIsolatedTree::NodeChange::NodeChange(AXIsolatedObject& isolatedObject, AccessibilityObjectWrapper* wrapper)
+ : m_isolatedObject(isolatedObject)
, m_wrapper(wrapper)
{
}
AXIsolatedTree::NodeChange::NodeChange(const NodeChange& other)
- : m_isolatedObject(other.m_isolatedObject.copyRef())
+ : m_isolatedObject(other.m_isolatedObject.get())
, m_wrapper(other.m_wrapper)
{
}
@@ -164,9 +164,25 @@
void AXIsolatedTree::removeNode(AXID axID)
{
LockHolder locker { m_changeLogLock };
+ ASSERT(m_readerThreadNodeMap.contains(axID));
m_pendingRemovals.append(axID);
}
+void AXIsolatedTree::removeSubtree(AXID axID)
+{
+ LockHolder locker { m_changeLogLock };
+ m_pendingRemovals.append(axID);
+
+ auto object = nodeForID(axID);
+ if (!object)
+ return;
+ auto childrenIDs(object->m_childrenIDs);
+ locker.unlockEarly();
+
+ for (const auto& childID : childrenIDs)
+ removeSubtree(childID);
+}
+
void AXIsolatedTree::appendNodeChanges(const Vector<NodeChange>& log)
{
LockHolder locker { m_changeLogLock };
@@ -181,17 +197,25 @@
m_focusedNodeID = m_pendingFocusedNodeID;
- for (const auto& item : m_pendingRemovals) {
- if (auto object = nodeForID(item))
+ for (const auto& axID : m_pendingRemovals) {
+ if (auto object = nodeForID(axID))
object->detach(AccessibilityDetachmentType::ElementDestroyed);
- m_readerThreadNodeMap.remove(item);
+ m_readerThreadNodeMap.remove(axID);
}
m_pendingRemovals.clear();
- for (auto& item : m_pendingAppends) {
- ASSERT(item.m_wrapper);
- item.m_isolatedObject->attachPlatformWrapper(item.m_wrapper);
- m_readerThreadNodeMap.add(item.m_isolatedObject->objectID(), item.m_isolatedObject.copyRef());
+ for (const auto& item : m_pendingAppends) {
+ ASSERT(!m_readerThreadNodeMap.contains(item.m_isolatedObject->objectID())
+ || item.m_isolatedObject->objectID() == m_rootNodeID);
+
+ if (item.m_wrapper)
+ item.m_isolatedObject->attachPlatformWrapper(item.m_wrapper);
+
+ m_readerThreadNodeMap.add(item.m_isolatedObject->objectID(), item.m_isolatedObject.get());
+ // 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(m_readerThreadNodeMap.get(item.m_isolatedObject->objectID())->refCount() == 2);
}
m_pendingAppends.clear();
}
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h (256629 => 256630)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2020-02-14 19:09:32 UTC (rev 256629)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2020-02-14 19:22:33 UTC (rev 256630)
@@ -64,7 +64,7 @@
struct NodeChange {
Ref<AXIsolatedObject> m_isolatedObject;
AccessibilityObjectWrapper* m_wrapper;
- NodeChange(const Ref<AXIsolatedObject>&, AccessibilityObjectWrapper*);
+ NodeChange(AXIsolatedObject&, AccessibilityObjectWrapper*);
NodeChange(const NodeChange&);
};
@@ -71,6 +71,7 @@
// Call on main thread
void appendNodeChanges(const Vector<NodeChange>&);
void removeNode(AXID);
+ void removeSubtree(AXID);
void setRootNode(Ref<AXIsolatedObject>&);
void setFocusedNodeID(AXID);
