vcl/win/dtrans/WinClipboard.cxx | 81 +++++++++++++++++++++++----------------- 1 file changed, 48 insertions(+), 33 deletions(-)
New commits: commit e1beaf844f5d6fdb0a72cdd70f2bbe29b699fe46 Author: Mike Kaganski <[email protected]> AuthorDate: Tue Nov 5 15:58:07 2024 +0500 Commit: Mike Kaganski <[email protected]> CommitDate: Wed Nov 6 06:22:22 2024 +0100 Related: tdf#163730 Release the object in separate thread Using a local build having both commits 9f53d40fd19b22fe1bdbf64e8ce751cf53f4f517 "Related: tdf#163730 Avoid deadlock", 2024-11-02, and 3015db08c9a8a1b29af1018044955c905c9015aa "Related: tdf#163730 Avoid potential deadlock", 2024-11-03, I got another deadlock (unfortunately, I didn't copy call stacks), where main thread, holding soler mutex, called CWinClipboard::setContents, and that tried to lock solar mutex in CMtaOleClipboard::run thread, which released the m_foreignContent data. This change releases m_foreignContent in a separate thread, which has no dependencies, and so its wait wouldn't block other threads. Change-Id: If4c486e6f3ba9201c4c9c6991992e38929ab3b81 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/176047 Reviewed-by: Mike Kaganski <[email protected]> Tested-by: Jenkins diff --git a/vcl/win/dtrans/WinClipboard.cxx b/vcl/win/dtrans/WinClipboard.cxx index dbaa019f14d8..6fb07aaa6bb4 100644 --- a/vcl/win/dtrans/WinClipboard.cxx +++ b/vcl/win/dtrans/WinClipboard.cxx @@ -54,6 +54,23 @@ namespace { CWinClipboard* s_pCWinClipbImpl = nullptr; std::mutex s_aClipboardSingletonMutex; + +unsigned __stdcall releaseAsyncProc(void* p) +{ + static_cast<css::datatransfer::XTransferable*>(p)->release(); + return 0; +} + +void releaseAsync(css::uno::Reference<css::datatransfer::XTransferable>& ref) +{ + if (!ref) + return; + auto pInterface = ref.get(); + pInterface->acquire(); + ref.clear(); // before starting the thread, to avoid race + if (auto handle = _beginthreadex(nullptr, 0, releaseAsyncProc, pInterface, 0, nullptr)) + CloseHandle(reinterpret_cast<HANDLE>(handle)); +} } /*XEventListener,*/ @@ -173,9 +190,6 @@ void SAL_CALL CWinClipboard::setContents( const uno::Reference<datatransfer::XTransferable>& xTransferable, const uno::Reference<datatransfer::clipboard::XClipboardOwner>& xClipboardOwner) { - // The object must be destroyed only outside of the mutex lock, because it may call - // CWinClipboard::onReleaseDataObject in another thread of this process synchronously - css::uno::Reference<css::datatransfer::XTransferable> prev_foreignContent; std::unique_lock aGuard(m_aMutex); if (m_bDisposed) @@ -184,7 +198,10 @@ void SAL_CALL CWinClipboard::setContents( IDataObjectPtr pIDataObj; - prev_foreignContent = std::move(m_foreignContent); // clear m_foreignContent + // The object must be destroyed only outside of the mutex lock, and in a different thread, + // because it may call CWinClipboard::onReleaseDataObject, or try to lock solar mutex, in + // another thread of this process synchronously + releaseAsync(m_foreignContent); // clear m_foreignContent assert(!m_foreignContent.is()); m_pCurrentOwnClipContent = nullptr; @@ -288,39 +305,37 @@ void SAL_CALL CWinClipboard::removeClipboardListener( void CWinClipboard::handleClipboardContentChanged() { - // The object must be destroyed only outside of the mutex lock, because it may call - // CWinClipboard::onReleaseDataObject in another thread of this process synchronously - css::uno::Reference<css::datatransfer::XTransferable> old_foreignContent; - { - std::unique_lock aGuard(m_aMutex); - if (m_bDisposed) - return; + std::unique_lock aGuard(m_aMutex); + if (m_bDisposed) + return; - old_foreignContent = std::move(m_foreignContent); // clear m_foreignContent - assert(!m_foreignContent.is()); - // If new own content assignment is pending, do it; otherwise, clear it. - // This makes sure that there will be no stuck clipboard content. - m_pCurrentOwnClipContent = std::exchange(m_pNewOwnClipContent, nullptr); + // The object must be destroyed only outside of the mutex lock, and in a different thread, + // because it may call CWinClipboard::onReleaseDataObject, or try to lock solar mutex, in + // another thread of this process synchronously + releaseAsync(m_foreignContent); // clear m_foreignContent + assert(!m_foreignContent.is()); + // If new own content assignment is pending, do it; otherwise, clear it. + // This makes sure that there will be no stuck clipboard content. + m_pCurrentOwnClipContent = std::exchange(m_pNewOwnClipContent, nullptr); - if (!maClipboardListeners.getLength(aGuard)) - return; + if (!maClipboardListeners.getLength(aGuard)) + return; - try - { - uno::Reference<datatransfer::XTransferable> rXTransf(getContents_noLock()); - datatransfer::clipboard::ClipboardEvent aClipbEvent(static_cast<XClipboard*>(this), - rXTransf); - maClipboardListeners.notifyEach( - aGuard, &datatransfer::clipboard::XClipboardListener::changedContents, aClipbEvent); - } - catch (const lang::DisposedException&) - { - OSL_FAIL("Service Manager disposed"); + try + { + uno::Reference<datatransfer::XTransferable> rXTransf(getContents_noLock()); + datatransfer::clipboard::ClipboardEvent aClipbEvent(static_cast<XClipboard*>(this), + rXTransf); + maClipboardListeners.notifyEach( + aGuard, &datatransfer::clipboard::XClipboardListener::changedContents, aClipbEvent); + } + catch (const lang::DisposedException&) + { + OSL_FAIL("Service Manager disposed"); - aGuard.unlock(); - // no further clipboard changed notifications - unregisterClipboardViewer(); - } + aGuard.unlock(); + // no further clipboard changed notifications + unregisterClipboardViewer(); } }
