vcl/win/dtrans/WinClipboard.cxx | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
New commits:
commit 0aaf48d7b3213aba969be24f90d1b9cb39411a3c
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Tue Nov 5 15:58:07 2024 +0500
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Wed Nov 13 21:37:33 2024 +0500
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>
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();
}
}
commit 3cce721d1a93e78d857fd4a8e41301a99844e638
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Sun Nov 3 22:49:28 2024 +0500
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Wed Nov 13 21:37:33 2024 +0500
Related: tdf#163730 Avoid potential deadlock
Similar to commit 43e5118496ae0c9b8f81a54574874eda7d439dbb (Related:
tdf#163730 Avoid deadlock, 2024-11-03). I haven't seen this scenario
myself, but seems likely to be possible.
Change-Id: Ie6bb69e7ebe12a69e4dabee9103de32611235807
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/175971
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
(cherry picked from commit 3015db08c9a8a1b29af1018044955c905c9015aa)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/175920
Reviewed-by: Xisco Fauli <xiscofa...@libreoffice.org>
diff --git a/vcl/win/dtrans/WinClipboard.cxx b/vcl/win/dtrans/WinClipboard.cxx
index 5760155beaa1..3603cf55f5d9 100644
--- a/vcl/win/dtrans/WinClipboard.cxx
+++ b/vcl/win/dtrans/WinClipboard.cxx
@@ -172,6 +172,9 @@ 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)
@@ -180,7 +183,8 @@ void SAL_CALL CWinClipboard::setContents(
IDataObjectPtr pIDataObj;
- m_foreignContent.clear();
+ prev_foreignContent = std::move(m_foreignContent); // clear
m_foreignContent
+ assert(!m_foreignContent.is());
m_pCurrentOwnClipContent = nullptr;
if (xTransferable.is())
@@ -284,7 +288,7 @@ 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
+ // CWinClipboard::onReleaseDataObject in another thread of this process
synchronously
css::uno::Reference<css::datatransfer::XTransferable> old_foreignContent;
{
std::unique_lock aGuard(m_aMutex);
commit 41875ef8c667d77260f238b9acf9513523b15153
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Sun Nov 3 00:10:17 2024 +0500
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Wed Nov 13 21:37:33 2024 +0500
Related: tdf#163730 Avoid deadlock
Seen locally, with main thread querying clipboard state:
vclplug_winlo.dll!std::unique_lock<std::mutex>::unique_lock<std::mutex>(std::mutex
& _Mtx) Line 145
vclplug_winlo.dll!CWinClipboard::getContents() Line 109
vcllo.dll!TransferableDataHelper::CreateFromClipboard(const
com::sun::star::uno::Reference<com::sun::star::datatransfer::clipboard::XClipboard>
& rClipboard) Line 2162
vcllo.dll!TransferableDataHelper::CreateFromSystemClipboard(vcl::Window *
pWindow) Line 2188
swlo.dll!SwBaseShell::StateClpbrd(SfxItemSet & rSet) Line 602
swlo.dll!SfxStubSwBaseShellStateClpbrd(SfxShell * pShell, SfxItemSet &
rSet) Line 2220
sfxlo.dll!SfxDispatcher::FillState_(const SfxSlotServer & rSvr,
SfxItemSet & rState, const SfxSlot * pRealSlot) Line 1726
sfxlo.dll!SfxBindings::Update_Impl(SfxStateCache & rCache) Line 267
sfxlo.dll!SfxBindings::NextJob_Impl(const Timer * pTimer) Line 1280
sfxlo.dll!SfxBindings::NextJob(Timer * pTimer) Line 1225
sfxlo.dll!SfxBindings::LinkStubNextJob(void * instance, Timer * data)
Line 1220
vcllo.dll!Link<Timer *,void>::Call(Timer * data) Line 111
vcllo.dll!Timer::Invoke() Line 75
vcllo.dll!Scheduler::CallbackTaskScheduling() Line 509
vcllo.dll!SalTimer::CallCallback() Line 53
vclplug_winlo.dll!WinSalTimer::ImplHandleElapsedTimer() Line 169
vclplug_winlo.dll!ImplSalYield(bool bWait, bool bHandleAllCurrentEvents)
Line 525
vclplug_winlo.dll!WinSalInstance::DoYield(bool bWait, bool
bHandleAllCurrentEvents) Line 581
vcllo.dll!ImplYield(bool i_bWait, bool i_bAllEvents) Line 385
vcllo.dll!Application::Yield() Line 473
vcCMtaOleClipboard::runllo.dll!Application::Execute() Line 361
sofficeapp.dll!desktop::Desktop::Main() Line 1679
CMtaOleClipboard::clipboardChangeNotifier thread holding CWinClipboard's
mutex in handleClipboardContentChanged, and waiting for the destruction
of IDataObject released from m_foreignContent (which was redirected to
CMtaOleClipboard::run thread):
vclplug_winlo.dll!sal::systools::COMReference<IDataObject>::release(IDataObject
* ptr) Line 235
vclplug_winlo.dll!sal::systools::COMReference<IDataObject>::~COMReference<IDataObject>()
Line 163
vclplug_winlo.dll!CAPNDataObject::~CAPNDataObject() Line 97
vclplug_winlo.dll!CAPNDataObject::`scalar deleting destructor'(unsigned
int)
vclplug_winlo.dll!CAPNDataObject::Release() Line 137
vclplug_winlo.dll!sal::systools::COMReference<IDataObject>::release(IDataObject
* ptr) Line 235
vclplug_winlo.dll!sal::systools::COMReference<IDataObject>::~COMReference<IDataObject>()
Line 163
vclplug_winlo.dll!CDOTransferable::~CDOTransferable()
vclplug_winlo.dll!CDOTransferable::`scalar deleting destructor'(unsigned
int)
cppuhelper3MSC.dll!cppu::OWeakObject::release() Line 230
vclplug_winlo.dll!cppu::WeakImplHelper<com::sun::star::datatransfer::XTransferable>::release()
Line 115
vclplug_winlo.dll!com::sun::star::uno::Reference<com::sun::star::datatransfer::XTransferable>::clear()
Line 234
vclplug_winlo.dll!CWinClipboard::handleClipboardContentChanged() Line 291
vclplug_winlo.dll!CWinClipboard::onClipboardContentChanged() Line 385
vclplug_winlo.dll!CMtaOleClipboard::clipboardChangedNotifierThreadProc(void *
pParam) Line 721
and CMtaOleClipboard::run thread waiting for CWinClipboard's mutex
in CWinClipboard::onReleaseDataObject:
vclplug_winlo.dll!std::_Mutex_base::lock() Line 52
vclplug_winlo.dll!std::unique_lock<std::mutex>::unique_lock<std::mutex>(std::mutex
& _Mtx) Line 145
vclplug_winlo.dll!CWinClipboard::onReleaseDataObject(CXNotifyingDataObject &
theCaller) Line 362
vclplug_winlo.dll!CXNotifyingDataObject::Release() Line 75
ole32.dll!CClipDataObject::Release() Line 960
combase.dll!...
rpcrt4.dll!Invoke()
rpcrt4.dll!Ndr64StubWorker(void *,void *,struct _RPC_MESSAGE *,struct
_MIDL_SERVER_INFO_ *,long (*const *)(void),struct _MIDL_SYNTAX_INFO *,unsigned
long *)
rpcrt4.dll!NdrStubCall3()
combase.dll!...
user32.dll!UserCallWinProcCheckWow(struct _ACTIVATION_CONTEXT *,__int64
(*)(struct tagWND *,unsigned int,unsigned __int64,__int64),struct HWND__ *,enum
_WM_VALUE,unsigned __int64,__int64,void *,int)
user32.dll!DispatchMessageWorker()
vclplug_winlo.dll!CMtaOleClipboard::run() Line 655
vclplug_winlo.dll!CMtaOleClipboard::oleThreadProc(void * pParam) Line 673
Caused by changes in commit 2e0664015255ffc0f76a11a9cb254564b34de496
(tdf#148647: make sure to update own content on Win clipboard change,
2024-07-14).
Change-Id: I26d35726f3d3f650a2db2ac63709ed820a60fc4e
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/175956
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
Tested-by: Jenkins
(cherry picked from commit 9f53d40fd19b22fe1bdbf64e8ce751cf53f4f517)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/175919
Reviewed-by: Xisco Fauli <xiscofa...@libreoffice.org>
diff --git a/vcl/win/dtrans/WinClipboard.cxx b/vcl/win/dtrans/WinClipboard.cxx
index be0266addd18..5760155beaa1 100644
--- a/vcl/win/dtrans/WinClipboard.cxx
+++ b/vcl/win/dtrans/WinClipboard.cxx
@@ -283,33 +283,39 @@ void SAL_CALL CWinClipboard::removeClipboardListener(
void CWinClipboard::handleClipboardContentChanged()
{
- std::unique_lock aGuard(m_aMutex);
- if (m_bDisposed)
- return;
+ // The object must be destroyed only outside of the mutex lock, because it
may call
+ // CWinClipboard::onReleaseDataObject in another thread of this process
+ css::uno::Reference<css::datatransfer::XTransferable> old_foreignContent;
+ {
+ std::unique_lock aGuard(m_aMutex);
+ if (m_bDisposed)
+ return;
- m_foreignContent.clear();
- // 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);
+ 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);
- 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();
+ }
}
}
