Changeset 261934 in webkit

Timestamp:

May 20, 2020 11:21:10 AM (4 years ago)

Author:

Andres Gonzalez

Message:

Fix for accessibility-node-memory-management.html in isolated tree mode.
​https://bugs.webkit.org/show_bug.cgi?id=212142

Reviewed by Chris Fleizach.

LayoutTests/accessibility/accessibility-node-memory-management.html.

• Fix in applyPendingChanges that was not removing removed nodes from

the nodes map. This was causing that some detached AXIsolatedObjects
were being returned.

• Also handle the case where pending changes can come from a detached

AXObject with invalid ID and no platform wrapper.

• updateChildren better handles the case when the notification target

is not in the isolated tree by walking up the object hierarchy until it
finds an associated object that does have a corresponding isolated object.

• accessibility/isolatedtree/AXIsolatedTree.cpp:

(WebCore::AXIsolatedTree::treeForPageID):
(WebCore::AXIsolatedTree::updateChildren):
(WebCore::AXIsolatedTree::applyPendingChanges):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• accessibility/isolatedtree/AXIsolatedTree.cpp (modified) (6 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-05-20  Andres Gonzalez  <andresg_22@apple.com>
+
+        Fix for accessibility-node-memory-management.html in isolated tree mode.
+        https://bugs.webkit.org/show_bug.cgi?id=212142
+
+        Reviewed by Chris Fleizach.
+
+        LayoutTests/accessibility/accessibility-node-memory-management.html.
+
+        - Fix in applyPendingChanges that was not removing removed nodes from
+        the nodes map. This was causing that some detached AXIsolatedObjects
+        were being returned.
+        - Also handle the case where pending changes can come from a detached
+        AXObject with invalid ID and no platform wrapper.
+        - updateChildren better handles the case when the notification target
+        is not in the isolated tree by walking up the object hierarchy until it
+        finds an associated object that does have a corresponding isolated object.
+
+        * accessibility/isolatedtree/AXIsolatedTree.cpp:
+        (WebCore::AXIsolatedTree::treeForPageID):
+        (WebCore::AXIsolatedTree::updateChildren):
+        (WebCore::AXIsolatedTree::applyPendingChanges):
+
 2020-05-20  Antoine Quint  <graouts@apple.com>
 

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

@@ -130 +130 @@
 RefPtr<AXIsolatedTree> AXIsolatedTree::treeForPageID(PageIdentifier pageID)
 {
-    AXTRACE("AXIsolatedTree::treeForPageID");
     LockHolder locker(s_cacheLock);
 
@@ -238 +237 @@
         return;
 
-    AXID axObjectID = axObject.objectID();
-
     applyPendingChanges();
     LockHolder locker { m_changeLogLock };
-    auto object = nodeForID(axObjectID);
-    if (!object) {
-        AXLOG("No associated isolated object!");
+
+    // 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());
+    });
+    ASSERT(object && object->objectID() != InvalidAXID);
+    if (!axAncestor || !object)
         return; // nothing to update.
-    }
 
     auto removals = object->m_childrenIDs;
     locker.unlockEarly();
 
-    const auto& axChildren = axObject.children();
-    auto axChildrenIDs = axObject.childrenIDs();
+    const auto& axChildren = axAncestor->children();
+    auto axChildrenIDs = axAncestor->childrenIDs();
 
     for (size_t i = 0; i < axChildrenIDs.size(); ++i) {
@@ -262 +267 @@
             AXLOG("Adding a new child for:");
             AXLOG(axChildren[i]);
-            generateSubtree(*axChildren[i], axObjectID, true);
+            generateSubtree(*axChildren[i], object->objectID(), true);
         }
     }
@@ -351 +356 @@
         auto axID = m_pendingNodeRemovals.takeLast();
         AXLOG(makeString("removing axID ", axID));
-        if (axID == InvalidAXID)
-            continue;
-
-        if (auto object = nodeForID(axID))
+        if (auto object = nodeForID(axID)) {
             object->detach(AccessibilityDetachmentType::ElementDestroyed);
+            m_readerThreadNodeMap.remove(axID);
+        }
     }
 
@@ -361 +365 @@
         auto axID = m_pendingSubtreeRemovals.takeLast();
         AXLOG(makeString("removing subtree axID ", axID));
-        if (axID == InvalidAXID)
-            continue;
-
         if (auto object = nodeForID(axID)) {
             object->detach(AccessibilityDetachmentType::ElementDestroyed);
             m_pendingSubtreeRemovals.appendVector(object->m_childrenIDs);
+            m_readerThreadNodeMap.remove(axID);
         }
     }
 
     for (const auto& item : m_pendingAppends) {
-        // Either the new object has a wrapper already attached, or one is passed to be attached, not both.
-        ASSERT((item.m_isolatedObject->wrapper() || item.m_wrapper)
-            && !(item.m_isolatedObject->wrapper() && item.m_wrapper));
         AXID axID = item.m_isolatedObject->objectID();
         AXLOG(makeString("appending axID ", axID));
@@ -380 +379 @@
 
         auto& wrapper = item.m_wrapper ? item.m_wrapper : item.m_isolatedObject->wrapper();
+        if (!wrapper)
+            continue;
 
         if (auto object = m_readerThreadNodeMap.get(axID)) {