Title: [290833] trunk/Source/WebCore
- Revision
- 290833
- Author
- [email protected]
- Date
- 2022-03-04 08:51:37 -0800 (Fri, 04 Mar 2022)
Log Message
AX ITM: Updating m_pendingLoadingProgress can cause deadlock on AXIsolatedTree::m_changeLogLock
https://bugs.webkit.org/show_bug.cgi?id=237402
Reviewed by Chris Fleizach.
AXIsolatedTree::m_pendingLoadingProgress is currently guarded by
AXIsolatedTree::m_changeLogLock. Because loading can happen at any time,
deadlocks can happen in this sequence:
1. AXIsolatedTree::updateLoadingProgress is called on the main thread while
the secondary thread holds the lock
2. The secondary thread is holding the lock to service an AX request, and said
AX request does something to call into the main thread (e.g. AXLOGs an isolated object,
which causes a dispatch to the main thread as part of AXIsolatedObject::outerHTML).
3. Deadlock
This patch fixes this by making m_loadingProgress threadsafe
via std::atomic<double> and removing m_pendingLoadingProgress.
This patch also removes an unnecessary acquisition of m_changeLogLock
in AXIsolatedTree::focusedNode(). This function is only called on the
secondary-thread, and only accesses secondary-thread safe functions
and member variables (nodeForID, m_focusedNodeID), so we don't need the lock.
* accessibility/isolatedtree/AXIsolatedTree.cpp:
(WebCore::AXIsolatedTree::focusedNode):
Removed unnecessary m_changeLogLock acquisition.
(WebCore::AXIsolatedTree::updateLoadingProgress):
Update m_loadingProgress directly instead of the now deleted intermediary
m_pendingLoadingProgress.
(WebCore::AXIsolatedTree::applyPendingChanges):
* accessibility/isolatedtree/AXIsolatedTree.h:
Remove m_pendingLoadingProgress.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (290832 => 290833)
--- trunk/Source/WebCore/ChangeLog 2022-03-04 15:49:34 UTC (rev 290832)
+++ trunk/Source/WebCore/ChangeLog 2022-03-04 16:51:37 UTC (rev 290833)
@@ -1,3 +1,39 @@
+2022-03-04 Tyler Wilcock <[email protected]>
+
+ AX ITM: Updating m_pendingLoadingProgress can cause deadlock on AXIsolatedTree::m_changeLogLock
+ https://bugs.webkit.org/show_bug.cgi?id=237402
+
+ Reviewed by Chris Fleizach.
+
+ AXIsolatedTree::m_pendingLoadingProgress is currently guarded by
+ AXIsolatedTree::m_changeLogLock. Because loading can happen at any time,
+ deadlocks can happen in this sequence:
+
+ 1. AXIsolatedTree::updateLoadingProgress is called on the main thread while
+ the secondary thread holds the lock
+ 2. The secondary thread is holding the lock to service an AX request, and said
+ AX request does something to call into the main thread (e.g. AXLOGs an isolated object,
+ which causes a dispatch to the main thread as part of AXIsolatedObject::outerHTML).
+ 3. Deadlock
+
+ This patch fixes this by making m_loadingProgress threadsafe
+ via std::atomic<double> and removing m_pendingLoadingProgress.
+
+ This patch also removes an unnecessary acquisition of m_changeLogLock
+ in AXIsolatedTree::focusedNode(). This function is only called on the
+ secondary-thread, and only accesses secondary-thread safe functions
+ and member variables (nodeForID, m_focusedNodeID), so we don't need the lock.
+
+ * accessibility/isolatedtree/AXIsolatedTree.cpp:
+ (WebCore::AXIsolatedTree::focusedNode):
+ Removed unnecessary m_changeLogLock acquisition.
+ (WebCore::AXIsolatedTree::updateLoadingProgress):
+ Update m_loadingProgress directly instead of the now deleted intermediary
+ m_pendingLoadingProgress.
+ (WebCore::AXIsolatedTree::applyPendingChanges):
+ * accessibility/isolatedtree/AXIsolatedTree.h:
+ Remove m_pendingLoadingProgress.
+
2022-03-04 Rob Buis <[email protected]>
Top layers should not be moved
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp (290832 => 290833)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2022-03-04 15:49:34 UTC (rev 290832)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp 2022-03-04 16:51:37 UTC (rev 290833)
@@ -423,9 +423,9 @@
RefPtr<AXIsolatedObject> AXIsolatedTree::focusedNode()
{
AXTRACE("AXIsolatedTree::focusedNode");
+ RELEASE_ASSERT(!isMainThread());
// Apply pending changes in case focus has changed and hasn't been updated.
applyPendingChanges();
- Locker locker { m_changeLogLock };
AXLOG(makeString("focusedNodeID ", m_focusedNodeID.loggingString()));
AXLOG("focused node:");
AXLOG(nodeForID(m_focusedNodeID));
@@ -467,11 +467,10 @@
void AXIsolatedTree::updateLoadingProgress(double newProgressValue)
{
AXTRACE("AXIsolatedTree::updateLoadingProgress");
- AXLOG(makeString("Queueing loading progress update to ", newProgressValue, " for treeID ", treeID()));
+ AXLOG(makeString("Updating loading progress to ", newProgressValue, " for treeID ", treeID()));
ASSERT(isMainThread());
- Locker locker { m_changeLogLock };
- m_pendingLoadingProgress = newProgressValue;
+ m_loadingProgress = newProgressValue;
}
void AXIsolatedTree::removeNode(const AXCoreObject& axObject)
@@ -533,8 +532,6 @@
Locker locker { m_changeLogLock };
- m_loadingProgress = m_pendingLoadingProgress;
-
if (m_pendingFocusedNodeID != m_focusedNodeID) {
AXLOG(makeString("focusedNodeID ", m_focusedNodeID.loggingString(), " pendingFocusedNodeID ", m_pendingFocusedNodeID.loggingString()));
Modified: trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h (290832 => 290833)
--- trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2022-03-04 15:49:34 UTC (rev 290832)
+++ trunk/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h 2022-03-04 16:51:37 UTC (rev 290833)
@@ -419,8 +419,7 @@
Vector<std::pair<AXID, Vector<AXID>>> m_pendingChildrenUpdates WTF_GUARDED_BY_LOCK(m_changeLogLock);
AXID m_pendingFocusedNodeID WTF_GUARDED_BY_LOCK(m_changeLogLock);
AXID m_focusedNodeID;
- double m_pendingLoadingProgress WTF_GUARDED_BY_LOCK(m_changeLogLock) { 0 };
- double m_loadingProgress { 0 };
+ std::atomic<double> m_loadingProgress { 0 };
Lock m_changeLogLock;
};
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
