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.kaganski@collabora.com>
Tested-by: Jenkins

1 files changed, 48 insertions, 33 deletions

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();
     }
 }