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
<rdar://problem/89726179>
Created attachment 453689 [details] Patch
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
Created attachment 453691 [details] Patch
(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.
> - 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.
Created attachment 453773 [details] Patch
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].