When updating a subtree of the IsolatedTree, first remove the entire subtree, not...
authorandresg_22@apple.com <andresg_22@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Feb 2020 19:22:33 +0000 (19:22 +0000)
committerandresg_22@apple.com <andresg_22@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Feb 2020 19:22:33 +0000 (19:22 +0000)
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:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@256630 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/accessibility/AXObjectCache.cpp
Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h

index c1ca216..ba410fa 100644 (file)
@@ -1,3 +1,33 @@
+2020-02-14  Andres Gonzalez  <andresg_22@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  <graouts@webkit.org>
 
         [Web Animations] Style changes due to Web Animations should not trigger CSS Transitions 
index 4343980..3fde38a 100644 (file)
@@ -3075,9 +3075,15 @@ void AXObjectCache::performDeferredCacheUpdate()
 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 @@ void AXObjectCache::updateIsolatedTree(AXCoreObject* object, AXNotification noti
     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;
index 197df45..ce3bed4 100644 (file)
@@ -48,7 +48,10 @@ Ref<AXIsolatedObject> AXIsolatedObject::create(AXCoreObject& object, AXIsolatedT
     return adoptRef(*new AXIsolatedObject(object, treeID, parentID));
 }
 
-AXIsolatedObject::~AXIsolatedObject() = default;
+AXIsolatedObject::~AXIsolatedObject()
+{
+    ASSERT(!wrapper());
+}
 
 void AXIsolatedObject::initializeAttributeData(AXCoreObject& object, bool isRoot)
 {
index 71c1c2c..ab815be 100644 (file)
@@ -46,6 +46,7 @@ namespace WebCore {
 class AXIsolatedTree;
 
 class AXIsolatedObject final : public AXCoreObject {
+    friend class AXIsolatedTree;
 public:
     static Ref<AXIsolatedObject> create(AXCoreObject&, AXIsolatedTreeID, AXID parentID);
     ~AXIsolatedObject();
index a3090d2..1141567 100644 (file)
@@ -42,14 +42,14 @@ static unsigned newTreeID()
     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::setFocusedNodeID(AXID axID)
 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 @@ void AXIsolatedTree::applyPendingChanges()
 
     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();
 }
index 00187d0..bb8f0d3 100644 (file)
@@ -64,13 +64,14 @@ public:
     struct NodeChange {
         Ref<AXIsolatedObject> m_isolatedObject;
         AccessibilityObjectWrapper* m_wrapper;
-        NodeChange(const Ref<AXIsolatedObject>&, AccessibilityObjectWrapper*);
+        NodeChange(AXIsolatedObject&, AccessibilityObjectWrapper*);
         NodeChange(const NodeChange&);
     };
 
     // Call on main thread
     void appendNodeChanges(const Vector<NodeChange>&);
     void removeNode(AXID);
+    void removeSubtree(AXID);
 
     void setRootNode(Ref<AXIsolatedObject>&);
     void setFocusedNodeID(AXID);