Changeset 259832 in webkit

Timestamp:

Apr 9, 2020, 2:55:53 PM (6 years ago)

Author:

Andres Gonzalez

Message:

Fix for crash in test accessibility/mac/aria-grid-with-strange-hierarchy.html in isolated tree mode.
​https://bugs.webkit.org/show_bug.cgi?id=210295

Reviewed by Chris Fleizach.

Covered by accessibility/mac/aria-grid-with-strange-hierarchy.html.

• When AXIsolatedTree::applyPendingChanges encounters a change for an

already existing object, the existing object is discarded and the new
object replaces it in the nodes map. The existing and new objects must
have the same platform wrapper. Thus the wrapper needs to be detached
from the existing object about to be discarded, and re-attached to the
new object. We were missing the re-attachment, and hence the crash when
the wrapper tries to access its underlying object.

• In addition, moved the LockHolder in a couple of intances to before

AXIsolatedTree::nodeForID, because this method accesses a member
variable used in both threads.

• Added stricter assert checks to catch problems with the management of

objects and wrappers during tree updates.

• accessibility/isolatedtree/AXIsolatedTree.cpp:

(WebCore::AXIsolatedTree::updateNode):
(WebCore::AXIsolatedTree::updateChildren):
(WebCore::AXIsolatedTree::applyPendingChanges):

• accessibility/mac/WebAccessibilityObjectWrapperBase.mm:

(-[WebAccessibilityObjectWrapperBase attachIsolatedObject:]):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• accessibility/isolatedtree/AXIsolatedTree.cpp (modified) (5 diffs)
• accessibility/mac/WebAccessibilityObjectWrapperBase.mm (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-04-09  Andres Gonzalez  <andresg_22@apple.com>
+
+        Fix for crash in test accessibility/mac/aria-grid-with-strange-hierarchy.html in isolated tree mode.
+        https://bugs.webkit.org/show_bug.cgi?id=210295
+
+        Reviewed by Chris Fleizach.
+
+        Covered by accessibility/mac/aria-grid-with-strange-hierarchy.html.
+
+        - When AXIsolatedTree::applyPendingChanges encounters a change for an
+        already existing object, the existing object is discarded and the new
+        object replaces it in the nodes map. The existing and new objects must
+        have the same platform wrapper. Thus the wrapper needs to be detached
+        from the existing object about to be discarded, and re-attached to the
+        new object. We were missing the re-attachment, and hence the crash when
+        the wrapper tries to access its underlying object.
+        - In addition, moved the LockHolder in a couple of intances to before
+        AXIsolatedTree::nodeForID, because this method accesses a member
+        variable used in both threads.
+        - Added stricter assert checks to catch problems with the management of
+        objects and wrappers during tree updates.
+
+        * accessibility/isolatedtree/AXIsolatedTree.cpp:
+        (WebCore::AXIsolatedTree::updateNode):
+        (WebCore::AXIsolatedTree::updateChildren):
+        (WebCore::AXIsolatedTree::applyPendingChanges):
+        * accessibility/mac/WebAccessibilityObjectWrapperBase.mm:
+        (-[WebAccessibilityObjectWrapperBase attachIsolatedObject:]):
+
 2020-04-09  Said Abou-Hallawa  <sabouhallawa@apple.com>
 

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

@@ -187 +187 @@
     AXID parentID = axParent ? axParent->objectID() : InvalidAXID;
 
+    LockHolder locker { m_changeLogLock };
     if (auto object = nodeForID(axID)) {
         ASSERT(object->objectID() == axID);
         auto newObject = AXIsolatedObject::create(axObject, m_treeID, parentID);
 
-        LockHolder locker { m_changeLogLock };
         // The new object should have the same children as the old one.
         newObject->m_childrenIDs = object->m_childrenIDs;
@@ -213 +213 @@
     ASSERT(isMainThread());
     AXID axObjectID = axObject.objectID();
+
+    LockHolder locker { m_changeLogLock };
     auto object = nodeForID(axObjectID);
     if (!object)
         return; // nothing to update.
 
+    auto removals = object->m_childrenIDs;
+    locker.unlockEarly();
+
     const auto& axChildren = axObject.children();
     auto axChildrenIDs = axObject.childrenIDs();
-
-    LockHolder locker { m_changeLogLock };
-    auto removals = object->m_childrenIDs;
-    // Make the children IDs of the isolated object to be the same as the AXObject's.
-    object->m_childrenIDs = axChildrenIDs;
-    locker.unlockEarly();
 
     for (size_t i = 0; i < axChildrenIDs.size(); ++i) {
@@ -232 +231 @@
         else {
             // This is a new child, add it to the tree.
-            generateSubtree(*axChildren[i], axObjectID, false);
+            generateSubtree(*axChildren[i], axObjectID, true);
         }
     }
@@ -240 +239 @@
     for (const AXID& childID : removals)
         removeSubtree(childID);
+
+    {
+        // 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;
+    }
 }
 
@@ -332 +337 @@
 
     for (const auto& item : m_pendingAppends) {
-        ASSERT(item.m_isolatedObject->wrapper() || item.m_wrapper);
+        // 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();
         if (axID == InvalidAXID)
             continue;
 
+        auto& wrapper = item.m_wrapper ? item.m_wrapper : item.m_isolatedObject->wrapper();
+
         if (auto object = m_readerThreadNodeMap.get(axID)) {
             if (object != &item.m_isolatedObject.get()
-                && (object->wrapper() == item.m_wrapper || object->wrapper() == item.m_isolatedObject->wrapper())) {
+                && object->wrapper() == wrapper.get()) {
                 // The new IsolatedObject is a replacement for an existing object
-                // as the result of an update. Thus detach the existing one before
-                // adding the new one.
+                // as the result of an update. Thus detach the wrapper from the
+                // existing object and attach it to the new one.
                 object->detachWrapper(AccessibilityDetachmentType::ElementDestroyed);
+                item.m_isolatedObject->attachPlatformWrapper(wrapper.get());
             }
             m_readerThreadNodeMap.remove(axID);
         }
 
-        if (m_readerThreadNodeMap.add(axID, item.m_isolatedObject.get()) && item.m_wrapper)
-            m_readerThreadNodeMap.get(axID)->attachPlatformWrapper(item.m_wrapper.get());
-
+        if (!item.m_isolatedObject->wrapper()) {
+            // The new object hasn't been attached a wrapper yet, so attach it.
+            item.m_isolatedObject->attachPlatformWrapper(wrapper.get());
+        }
+
+        auto addResult = m_readerThreadNodeMap.add(axID, item.m_isolatedObject.get());
+        // The newly added object must have a wrapper.
+        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(m_readerThreadNodeMap.get(axID)->refCount() == 2);
+        ASSERT_UNUSED(addResult, addResult.iterator->value->refCount() == 2);
     }
     m_pendingAppends.clear();

trunk/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm

@@ -305 +305 @@
 {
     ASSERT(isolatedObject && (_identifier == InvalidAXID || _identifier == isolatedObject->objectID()));
+    ASSERT(m_axObject && isolatedObject->objectID() == m_axObject->objectID());
     m_isolatedObject = isolatedObject;
     if (_identifier == InvalidAXID)