AX ITM: Updating m_pendingLoadingProgress can cause deadlock on AXIsolatedTree::m_changeLogLock

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

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

[Radar WebKit Bug Importer]

<rdar://problem/89726179>

[Tyler Wilcock]

Created attachment 453689 [details]
Patch

[chris fleizach]

Comment on attachment 453689 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453689&action=review

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:534
> +

I’m still worried about this logging happening inside the lock

[Tyler Wilcock]

Created attachment 453691 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #4)
> Created attachment 453691 [details]
> Patch

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp

+    ASSERT(!isMainThread());
     AXTRACE("AXIsolatedTree::focusedNode");

AXTRACE is the first line in all functions, by convention, so that when you look at the logs, the { } encompasses the scope of that function as accurate as possible.

-    double m_pendingLoadingProgress WTF_GUARDED_BY_LOCK(m_changeLogLock) { 0 };
+    std::atomic<double> m_pendingLoadingProgress { 0 };

If you are going to make the loading progress atomic, make m_loadingProgress and get rid of m_pendingLoadingProgress, no need to keep a pending variable. std::atomic<double> guaranties that access to that variable is thread safe.

[Tyler Wilcock]

> -    double m_pendingLoadingProgress WTF_GUARDED_BY_LOCK(m_changeLogLock) {
> 0 };
> +    std::atomic<double> m_pendingLoadingProgress { 0 };
> 
> If you are going to make the loading progress atomic, make m_loadingProgress
> and get rid of m_pendingLoadingProgress, no need to keep a pending variable.
> std::atomic<double> guaranties that access to that variable is thread safe.
The thought here was that continuing to keep them separate will be more performant since any reads of m_loadingProgress will not be atomic.

[Tyler Wilcock]

Created attachment 453773 [details]
Patch

[EWS]

Committed r290833 (248069@main): <https://commits.webkit.org/248069@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453773 [details].