vcl/win/dtrans/WinClipboard.cxx | 81 +++++++++++++++++++++++-----------------
1 file changed, 48 insertions(+), 33 deletions(-)
New commits:
commit 29da5e292302c232a5e6f3ec9c2768aaeaa2edd5
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Tue Nov 5 15:58:07 2024 +0500
Commit: Christian Lohmaier <lohmaier+libreoff...@googlemail.com>
CommitDate: Fri Nov 8 11:51:11 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 <mike.kagan...@collabora.com>
Tested-by: Jenkins
(cherry picked from commit e1beaf844f5d6fdb0a72cdd70f2bbe29b699fe46)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/176066
Reviewed-by: Xisco Fauli <xiscofa...@libreoffice.org>
Signed-off-by: Xisco Fauli <xiscofa...@libreoffice.org>
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/176163
Reviewed-by: Christian Lohmaier <lohmaier+libreoff...@googlemail.com>
Tested-by: Christian Lohmaier <lohmaier+libreoff...@googlemail.com>
Reviewed-by: Michael Stahl <michael.st...@allotropia.de>
diff --git a/vcl/win/dtrans/WinClipboard.cxx b/vcl/win/dtrans/WinClipboard.cxx
index 3603cf55f5d9..1fd2a9412346 100644
--- a/vcl/win/dtrans/WinClipboard.cxx
+++ b/vcl/win/dtrans/WinClipboard.cxx
@@ -53,6 +53,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,*/
@@ -172,9 +189,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)
@@ -183,7 +197,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;
@@ -287,39 +304,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();
}
}
