Title: [258034] trunk/Source/WebCore
- Revision
- 258034
- Author
- [email protected]
- Date
- 2020-03-06 15:30:29 -0800 (Fri, 06 Mar 2020)
Log Message
Crash accessing AXIsolatedObject::m_childrenIDS from removeSubtree on the main thread.
https://bugs.webkit.org/show_bug.cgi?id=208728
Reviewed by Chris Fleizach.
AXIsolatedTree::removeSubtree was accessing AXIsolatedObject::m_childrenIDs
on the main thread to remove all descendants recursively. But the lock
had to be unlocked and locked again on each iteration, creating problems
if the secondary thread modifies the children in between iterations.
The solution in this patch is to eliminate removeSubtree, and make
removeNode and applyPendingChanges to remove all descendants.
* accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::updateIsolatedTree):
* accessibility/isolatedtree/AXIsolatedTree.cpp:
(WebCore::AXIsolatedTree::appendNodeChanges): Renamed local vars to make it clearer.
(WebCore::AXIsolatedTree::applyPendingChanges):
(WebCore::AXIsolatedTree::removeSubtree): Deleted.
* accessibility/isolatedtree/AXIsolatedTree.h:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (258033 => 258034)
--- trunk/Source/WebCore/ChangeLog 2020-03-06 23:26:35 UTC (rev 258033)
+++ trunk/Source/WebCore/ChangeLog 2020-03-06 23:30:29 UTC (rev 258034)
@@ -1,3 +1,25 @@
+2020-03-06 Andres Gonzalez <[email protected]>
+
+ Crash accessing AXIsolatedObject::m_childrenIDS from removeSubtree on the main thread.
+ https://bugs.webkit.org/show_bug.cgi?id=208728
+
+ Reviewed by Chris Fleizach.
+
+ AXIsolatedTree::removeSubtree was accessing AXIsolatedObject::m_childrenIDs
+ on the main thread to remove all descendants recursively. But the lock
+ had to be unlocked and locked again on each iteration, creating problems
+ if the secondary thread modifies the children in between iterations.
+ The solution in this patch is to eliminate removeSubtree, and make
+ removeNode and applyPendingChanges to remove all descendants.
+
+ * accessibility/AXObjectCache.cpp:
+ (WebCore::AXObjectCache::updateIsolatedTree):
+ * accessibility/isolatedtree/AXIsolatedTree.cpp:
+ (WebCore::AXIsolatedTree::appendNodeChanges): Renamed local vars to make it clearer.
+ (WebCore::AXIsolatedTree::applyPendingChanges):
+ (WebCore::AXIsolatedTree::removeSubtree): Deleted.
+ * accessibility/isolatedtree/AXIsolatedTree.h:
+
2020-03-06 Yusuke Suzuki <[email protected]>
JSDOMIterator classes should be in IsoSubspace
Modified: trunk/Source/WebCore/accessibility/AXObjectCache.cpp (258033 => 258034)
--- trunk/Source/WebCore/accessibility/AXObjectCache.cpp 2020-03-06 23:26:35 UTC (rev 258033)
+++ trunk/Source/WebCore/accessibility/AXObjectCache.cpp 2020-03-06 23:30:29 UTC (rev 258034)
@@ -3147,7 +3147,7 @@
case AXChildrenChanged:
case AXSelectedTextChanged:
case AXValueChanged: {
- tree->removeSubtree(object->objectID());
+ tree->removeNode(object->objectID());
auto* parent = object->parentObject();
AXID parentID = parent ? parent->objectID() : InvalidAXID;
Vector<AXIsolatedTree::NodeChange> nodeChanges;
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp (258033 => 258034)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2020-03-06 23:26:35 UTC (rev 258033)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2020-03-06 23:30:29 UTC (rev 258034)
@@ -198,28 +198,13 @@
m_pendingRemovals.append(axID);
}
-void AXIsolatedTree::removeSubtree(AXID axID)
+void AXIsolatedTree::appendNodeChanges(const Vector<NodeChange>& changes)
{
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);
+ for (const auto& nodeChange : changes)
+ m_pendingAppends.append(nodeChange);
}
-void AXIsolatedTree::appendNodeChanges(const Vector<NodeChange>& log)
-{
- LockHolder locker { m_changeLogLock };
- for (const auto& node : log)
- m_pendingAppends.append(node);
-}
-
void AXIsolatedTree::applyPendingChanges()
{
RELEASE_ASSERT(!isMainThread());
@@ -227,15 +212,16 @@
m_focusedNodeID = m_pendingFocusedNodeID;
- for (const auto& axID : m_pendingRemovals) {
+ while (m_pendingRemovals.size()) {
+ auto axID = m_pendingRemovals.takeLast();
if (axID == InvalidAXID)
continue;
- if (auto object = nodeForID(axID))
+ if (auto object = nodeForID(axID)) {
object->detach(AccessibilityDetachmentType::ElementDestroyed);
- m_readerThreadNodeMap.remove(axID);
+ m_pendingRemovals.appendVector(object->m_childrenIDs);
+ }
}
- m_pendingRemovals.clear();
for (const auto& item : m_pendingAppends) {
AXID axID = item.m_isolatedObject->objectID();
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h (258033 => 258034)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2020-03-06 23:26:35 UTC (rev 258033)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2020-03-06 23:30:29 UTC (rev 258034)
@@ -71,8 +71,8 @@
// Call on main thread
void appendNodeChanges(const Vector<NodeChange>&);
+ // Removes the given node and all its descendants.
void removeNode(AXID);
- void removeSubtree(AXID);
// Both setRootNode and setFocusedNode must be called only during the
// generation of the IsolatedTree.
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
