vcl/win/dtrans/WinClipboard.cxx | 131 ++++++++++++++++++++--------------------
vcl/win/dtrans/WinClipboard.hxx | 11 ++-
2 files changed, 74 insertions(+), 68 deletions(-)
New commits:
commit 71a4864794d600f3a07fc544e4fcb16596840687
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Sun Jul 14 22:01:59 2024 +0500
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Tue Oct 1 15:58:14 2024 +0500
tdf#148647: make sure to update own content on Win clipboard change
Before, only a new call to setContents, or destruction of the data
object referenced from CWinClipboard, would force CWinClipboard to
clean its m_pCurrentClipContent. A data object could be referenced
elsewhere, and so not get destroyed on clipboard change; this kept
the value of m_pCurrentClipContent, and all the following calls to
getContents would still return that stuck own data object.
This change makes CWinClipboard store new own data object pointer
into temporary m_pNewOwnClipContent. In onClipboardContentChanged
notification, the value is moved to m_pCurrentOwnClipContent. This
ensures that following onClipboardContentChanged events will clear
m_pCurrentOwnClipContent, preventing stuck clipboard content.
Change-Id: Idd0a64a820d79bede9deceba8d5800b7fc61e66b
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170459
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
Tested-by: Jenkins
Signed-off-by: Xisco Fauli <xiscofa...@libreoffice.org>
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170462
diff --git a/vcl/win/dtrans/WinClipboard.cxx b/vcl/win/dtrans/WinClipboard.cxx
index 3fa2ba67cdc9..be0266addd18 100644
--- a/vcl/win/dtrans/WinClipboard.cxx
+++ b/vcl/win/dtrans/WinClipboard.cxx
@@ -60,7 +60,6 @@ CWinClipboard::CWinClipboard(const
uno::Reference<uno::XComponentContext>& rxCon
const OUString& aClipboardName)
: m_xContext(rxContext)
, m_itsName(aClipboardName)
- , m_pCurrentClipContent(nullptr)
{
// necessary to reassociate from
// the static callback function
@@ -92,6 +91,12 @@ void CWinClipboard::disposing(std::unique_lock<std::mutex>&
mutex)
// XClipboard
+CXNotifyingDataObject* CWinClipboard::getOwnClipContent() const
+{
+ assert(!m_pCurrentOwnClipContent || !m_pNewOwnClipContent); // Both can be
null, or only one set
+ return m_pCurrentOwnClipContent ? m_pCurrentOwnClipContent :
m_pNewOwnClipContent;
+}
+
// to avoid unnecessary traffic we check first if there is a clipboard
// content which was set via setContent, in this case we don't need
// to query the content from the clipboard, create a new wrapper object
@@ -110,12 +115,12 @@ css::uno::Reference<css::datatransfer::XTransferable>
CWinClipboard::getContents
throw lang::DisposedException("object is already disposed",
static_cast<XClipboardEx*>(this));
- assert(!m_pCurrentClipContent || !m_foreignContent); // Both can be null,
or only one set
+ assert(!getOwnClipContent() || !m_foreignContent); // Both can be null, or
only one set
// use the shortcut or create a transferable from
// system clipboard
- if (nullptr != m_pCurrentClipContent)
- return m_pCurrentClipContent->m_XTransferable;
+ if (auto pOwnClipContent = getOwnClipContent())
+ return pOwnClipContent->m_XTransferable;
// Content cached?
if (m_foreignContent.is())
@@ -176,18 +181,21 @@ void SAL_CALL CWinClipboard::setContents(
IDataObjectPtr pIDataObj;
m_foreignContent.clear();
+ m_pCurrentOwnClipContent = nullptr;
if (xTransferable.is())
{
- m_pCurrentClipContent = new CXNotifyingDataObject(
+ // Store the new object's pointer to temporary m_pNewOwnClipContent,
to be moved to
+ // m_pCurrentOwnClipContent in handleClipboardContentChanged.
+ m_pNewOwnClipContent = new CXNotifyingDataObject(
CDTransObjFactory::createDataObjFromTransferable(m_xContext,
xTransferable),
xTransferable, xClipboardOwner, this);
- pIDataObj = IDataObjectPtr(m_pCurrentClipContent);
+ pIDataObj = IDataObjectPtr(m_pNewOwnClipContent);
}
else
{
- m_pCurrentClipContent = nullptr;
+ m_pNewOwnClipContent = nullptr;
}
m_MtaOleClipboard.setClipboard(pIDataObj.get());
@@ -220,7 +228,7 @@ void SAL_CALL CWinClipboard::flushClipboard()
// It may be possible to move the request to the clipboard STA thread by
saving the
// DataObject and call OleIsCurrentClipboard before flushing.
- if (nullptr != m_pCurrentClipContent)
+ if (getOwnClipContent())
{
aGuard.unlock();
m_MtaOleClipboard.flushClipboard();
@@ -273,13 +281,16 @@ void SAL_CALL CWinClipboard::removeClipboardListener(
maClipboardListeners.removeInterface(aGuard, listener);
}
-void CWinClipboard::clearCacheAndAllClipboardListener()
+void CWinClipboard::handleClipboardContentChanged()
{
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);
if (!maClipboardListeners.getLength(aGuard))
return;
@@ -352,8 +363,8 @@ void
CWinClipboard::onReleaseDataObject(CXNotifyingDataObject* theCaller)
// because an external source must be the clipboardowner now
std::unique_lock aGuard(m_aMutex);
- if (m_pCurrentClipContent == theCaller)
- m_pCurrentClipContent = nullptr;
+ if (getOwnClipContent() == theCaller)
+ m_pCurrentOwnClipContent = m_pNewOwnClipContent = nullptr;
}
void CWinClipboard::registerClipboardViewer()
@@ -373,7 +384,7 @@ void WINAPI CWinClipboard::onClipboardContentChanged()
}
if (pWinClipboard)
- pWinClipboard->clearCacheAndAllClipboardListener();
+ pWinClipboard->handleClipboardContentChanged();
}
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/vcl/win/dtrans/WinClipboard.hxx b/vcl/win/dtrans/WinClipboard.hxx
index fcf7bf089cb6..83d01da65e05 100644
--- a/vcl/win/dtrans/WinClipboard.hxx
+++ b/vcl/win/dtrans/WinClipboard.hxx
@@ -57,12 +57,15 @@ class CWinClipboard final
css::uno::Reference<css::uno::XComponentContext> m_xContext;
const OUString m_itsName;
CMtaOleClipboard m_MtaOleClipboard;
- CXNotifyingDataObject* m_pCurrentClipContent;
+ CXNotifyingDataObject* m_pNewOwnClipContent = nullptr; // until
onClipboardContentChanged
+ CXNotifyingDataObject* m_pCurrentOwnClipContent = nullptr;
com::sun::star::uno::Reference<com::sun::star::datatransfer::XTransferable>
m_foreignContent;
comphelper::OInterfaceContainerHelper4<css::datatransfer::clipboard::XClipboardListener>
maClipboardListeners;
- void clearCacheAndAllClipboardListener();
+ CXNotifyingDataObject* getOwnClipContent() const;
+
+ void handleClipboardContentChanged();
void onReleaseDataObject(CXNotifyingDataObject* theCaller);
void registerClipboardViewer();
commit d9bc08839198df43773e573b27b24a7da7a3b79e
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Mon Jul 15 18:55:50 2024 +0500
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Tue Oct 1 15:58:14 2024 +0500
Release the lock for getClipboard call to avoid deadlock
The call stacks that deadlock in my testing:
Thread A: CMtaOleClipboard::clipboardChangedNotifierThreadProc()
win32u.dll!NtUserMsgWaitForMultipleObjectsEx()
user32.dll!MsgWaitForMultipleObjects()
vclplug_winlo.dll!`anonymous namespace'::Win32Condition::wait(void *
hEvtAbort) Line 92 C++
vclplug_winlo.dll!CMtaOleClipboard::getClipboard(IDataObject * *
ppIDataObject) Line 353 C++
vclplug_winlo.dll!CWinClipboard::getIDataObject() Line 158 C++
vclplug_winlo.dll!CDOTransferable::tryToGetIDataObjectIfAbsent() Line 413
C++
vclplug_winlo.dll!CDOTransferable::getClipboardData(CFormatEtc &
aFormatEtc) Line 426 C++
vclplug_winlo.dll!CDOTransferable::getTransferData(const
com::sun::star::datatransfer::DataFlavor & aFlavor) Line 252 C++
vcllo.dll!TransferableDataHelper::GetAny(const
com::sun::star::datatransfer::DataFlavor & rFlavor, const rtl::OUString &
rDestDoc) Line 1484 C++
vcllo.dll!TransferableDataHelper::GetSequence(const
com::sun::star::datatransfer::DataFlavor & rFlavor, const rtl::OUString &
rDestDoc) Line 2078 C++
vcllo.dll!TransferableDataHelper::InitFormats() Line 1344 C++
vcllo.dll!TransferableDataHelper::TransferableDataHelper(const
com::sun::star::uno::Reference<com::sun::star::datatransfer::XTransferable> &
rxTransferable) Line 1156 C++
svtlo.dll!TransferableClipboardListener::changedContents(const
com::sun::star::datatransfer::clipboard::ClipboardEvent & rEventObject) Line 52
C++
vclplug_winlo.dll!comphelper::OInterfaceContainerHelper4<com::sun::star::datatransfer::clipboard::XClipboardListener>::NotifySingleListener<com::sun::star::datatransfer::clipboard::ClipboardEvent>::operator()(const
com::sun::star::uno::Reference<com::sun::star::datatransfer::clipboard::XClipboardListener>
& listener) Line 275 C++
vclplug_winlo.dll!comphelper::OInterfaceContainerHelper4<com::sun::star::datatransfer::clipboard::XClipboardListener>::forEach<comphelper::OInterfaceContainerHelper4<com::sun::star::datatransfer::clipboard::XClipboardListener>::NotifySingleListener<com::sun::star::datatransfer::clipboard::ClipboardEvent>>(std::unique_lock<std::mutex>
& rGuard, const
comphelper::OInterfaceContainerHelper4<com::sun::star::datatransfer::clipboard::XClipboardListener>::NotifySingleListener<com::sun::star::datatransfer::clipboard::ClipboardEvent>
& func) Line 304 C++
vclplug_winlo.dll!comphelper::OInterfaceContainerHelper4<com::sun::star::datatransfer::clipboard::XClipboardListener>::notifyEach<com::sun::star::datatransfer::clipboard::ClipboardEvent>(std::unique_lock<std::mutex>
& rGuard,
void(com::sun::star::datatransfer::clipboard::XClipboardListener::*)(const
com::sun::star::datatransfer::clipboard::ClipboardEvent &) NotificationMethod,
const com::sun::star::datatransfer::clipboard::ClipboardEvent & Event) Line 327
C++
vclplug_winlo.dll!CWinClipboard::handleClipboardContentChanged() Line 302
C++
vclplug_winlo.dll!CWinClipboard::onClipboardContentChanged() Line 386 C++
vclplug_winlo.dll!CMtaOleClipboard::clipboardChangedNotifierThreadProc(void *
pParam) Line 723 C++
ucrtbased.dll!thread_start<unsigned int (__cdecl*)(void *),1>(void *
const parameter) Line 97 C++
kernel32.dll!BaseThreadInitThunk()
ntdll.dll!RtlUserThreadStart()
Thread B: CMtaOleClipboard::run()
ntdll.dll!NtWaitForAlertByThreadId()
ntdll.dll!RtlAcquireSRWLockExclusive()
msvcp140d.dll!mtx_do_lock(_Mtx_internal_imp_t * mtx, const _timespec64 *
target) Line 93 C++
msvcp140d.dll!_Mtx_lock(_Mtx_internal_imp_t * mtx) Line 165 C++
vclplug_winlo.dll!std::_Mutex_base::lock() Line 56 C++
vclplug_winlo.dll!std::unique_lock<std::mutex>::unique_lock<std::mutex>(std::mutex
& _Mtx) Line 149 C++
vclplug_winlo.dll!CWinClipboard::onReleaseDataObject(CXNotifyingDataObject *
theCaller) Line 363 C++
vclplug_winlo.dll!CXNotifyingDataObject::Release() Line 77 C++
combase.dll!CStdIdentity::ReleaseCtrlUnk::__l5::<lambda>() Line 1408 C++
combase.dll!ObjectMethodExceptionHandlingAction<void
<lambda>(void)>(CStdIdentity::ReleaseCtrlUnk::__l5::void <lambda>(void) action,
ObjectMethodExceptionHandlingInfo * pExceptionHandlingInfo,
ExceptionHandlingResult * pExceptionHandlingResult, void * __formal) Line 135
C++
combase.dll!CStdIdentity::ReleaseCtrlUnk(unsigned long dwDisconnectType)
Line 1411 C++
[Inline Frame]
combase.dll!ObjectLibrary::ReferencedPtr<CObjectContext>::operator int
ObjectLibrary::Details::BoolStruct::*() Line 1313 C++
combase.dll!CStdMarshal::DisconnectWorker_ReleasesLock(unsigned long
dwType, bool logEventIsActive, CObjectContext * explicitServerContext, bool
performCallback, CIDObject * pIDDelayRelease) Line 4783 C++
combase.dll!CStdMarshal::Disconnect(unsigned long dwType) Line 4452 C++
[Inline Frame] combase.dll!CStdMarshal::HandlePendingDisconnect(HRESULT)
Line 4285 C++
combase.dll!CRemoteUnknown::RemReleaseWorker(unsigned short
cInterfaceRefs, tagREMINTERFACEREF * InterfaceRefs, int fTopLevel) Line 1307 C++
rpcrt4.dll!Invoke()
rpcrt4.dll!Ndr64StubWorker()
rpcrt4.dll!NdrStubCall3()
combase.dll!CStdStubBuffer_Invoke(IRpcStubBuffer * This, tagRPCOLEMESSAGE
* prpcmsg, IRpcChannelBuffer * pRpcChannelBuffer) Line 1479 C++
[Inline Frame]
combase.dll!InvokeStubWithExceptionPolicyAndTracing::__l6::<lambda_c9f3956a20c9da92a64affc24fdd69ec>::operator()()
Line 1151 C++
combase.dll!ObjectMethodExceptionHandlingAction<<lambda_c9f3956a20c9da92a64affc24fdd69ec>>(InvokeStubWithExceptionPolicyAndTracing::__l6::<lambda_c9f3956a20c9da92a64affc24fdd69ec>
action, ObjectMethodExceptionHandlingInfo * pExceptionHandlingInfo,
ExceptionHandlingResult * pExceptionHandlingResult, void *) Line 94 C++
[Inline Frame]
combase.dll!InvokeStubWithExceptionPolicyAndTracing(IRpcStubBuffer * pMsg,
tagRPCOLEMESSAGE *) Line 1149 C++
combase.dll!DefaultStubInvoke(bool bIsAsyncBeginMethod, IServerCall *
pServerCall, IRpcChannelBuffer * pChannel, IRpcStubBuffer * pStub, unsigned
long * pdwFault) Line 1218 C++
combase.dll!SyncServerCall::StubInvoke(IRpcChannelBuffer * pChannel,
IRpcStubBuffer * pStub, unsigned long * pdwFault) Line 791 C++
[Inline Frame] combase.dll!StubInvoke(tagRPCOLEMESSAGE * pMsg, const
_GUID &) Line 1483 C++
combase.dll!ServerCall::ContextInvoke(tagIPIDEntry * ipidEntry) Line 1421
C++
[Inline Frame] combase.dll!DefaultInvokeInApartment(ServerCall *) Line
3257 C++
combase.dll!ReentrantSTAInvokeInApartment(ServerCall * serverCall,
tagIPIDEntry * ipidEntry) Line 110 C++
combase.dll!ComInvokeWithLockAndIPID(ServerCall * pServerCall,
tagIPIDEntry * pIPIDEntry) Line 2152 C++
combase.dll!ThreadDispatch(ServerCall * pServerCall) Line 1637 C++
combase.dll!ThreadWndProc(HWND__ * window, unsigned int message, unsigned
__int64 wparam, __int64 params) Line 720 C++
user32.dll!UserCallWinProcCheckWow()
user32.dll!DispatchMessageWorker()
[Inline Frame] combase.dll!CCliModalLoop::MyDispatchMessage(tagMSG *)
Line 2959 C++
combase.dll!CCliModalLoop::PeekRPCAndDDEMessage() Line 2563 C++
combase.dll!CCliModalLoop::BlockFn(void * * ahEvent, unsigned long
cEvents, unsigned long * lpdwSignaled) Line 2055 C++
combase.dll!ModalLoop(CSyncClientCall * pClientCall) Line 169 C++
[Inline Frame] combase.dll!ThreadSendReceive(tagRPCOLEMESSAGE *) Line
7259 C++
[Inline Frame]
combase.dll!CSyncClientCall::SwitchAptAndDispatchCall(tagRPCOLEMESSAGE *
pMessage) Line 5735 C++
combase.dll!CSyncClientCall::SendReceive2(tagRPCOLEMESSAGE * pMessage,
unsigned long * pstatus) Line 5297 C++
combase.dll!SyncClientCallRetryContext::SendReceiveWithRetry(tagRPCOLEMESSAGE *
pMsg, unsigned long * pulStatus, ClientCall * pClientCall, bool *
pbIsAutoRetry) Line 1502 C++
combase.dll!CSyncClientCall::SendReceiveInRetryContext(SyncClientCallRetryContext
* pRetryContext, tagRPCOLEMESSAGE * pMsg, unsigned long * pulStatus) Line 582
C++
combase.dll!ClassicSTAThreadSendReceive(CSyncClientCall * pClientCall,
tagRPCOLEMESSAGE * pMsg, unsigned long * pulStatus) Line 564 C++
combase.dll!CSyncClientCall::SendReceive(tagRPCOLEMESSAGE * pMessage,
unsigned long * pulStatus) Line 787 C++
combase.dll!CClientChannel::SendReceive(tagRPCOLEMESSAGE * pMessage,
unsigned long * pulStatus) Line 659 C++
combase.dll!NdrExtpProxySendReceive(void * pThis, _MIDL_STUB_MESSAGE *
pStubMsg) Line 1989 C++
rpcrt4.dll!NdrpClientCall3()
combase.dll!ObjectStublessClient(void * ParamAddress, __int64 *
FloatRegisters, long Method) Line 366 C++
combase.dll!ObjectStubless() Line 176
combase.dll!CStdMarshal::RemoteAddRef(tagIPIDEntry * pIPIDEntry,
OXIDEntry * pOXIDEntry, unsigned long cStrongNeed, unsigned long cSecureNeed,
int fGiveToCaller) Line 8489 C++
[Inline Frame] combase.dll!CStdMarshal::GetNeededRefs(tagSTDOBJREF *)
Line 3654 C++
combase.dll!CStdMarshal::ConnectCliIPIDEntry(tagSTDOBJREF * pStd,
OXIDEntry * pOXIDEntry, tagIPIDEntry * pEntry) Line 3338 C++
combase.dll!CStdMarshal::MakeCliIPIDEntry(const _GUID & riid,
tagSTDOBJREF * pStd, OXIDEntry * pOXIDEntry, tagIPIDEntry * * ppEntry) Line
3108 C++
combase.dll!CStdMarshal::UnmarshalIPID(const _GUID & riid, tagSTDOBJREF *
pStd, OXIDEntry * pOXIDEntry, void * * ppv) Line 2621 C++
combase.dll!CStdMarshal::UnmarshalObjRef(tagOBJREF & objref, void * *
ppv) Line 2479 C++
combase.dll!UnmarshalSwitch(void * pv) Line 2112 C++
combase.dll!UnmarshalObjRef(tagOBJREF & objref,
EffectiveUnmarshalingPolicy policy, void * * ppv, int fBypassActLock, CBaseCall
* callMarshalingContext, CStdMarshal * * ppStdMarshal) Line 2261 C++
combase.dll!InternalGetWindowPropInterface2(HWND__ * hWnd, unsigned
__int64 dwCookie, int fCallStrongNamedProcesses, const _GUID & iid, void * *
ppv, int * pfLocal) Line 285 C++
ole32.dll!UnmarshalFromEndpointProperty(HWND__ * hWnd, int fDragDrop, int
fCallStrongNamedProcesses, IUnknown * * ppUnk, int * pfLocal) Line 407 C++
ole32.dll!GetInterfaceFromWindowProp(HWND__ * hWnd, const _GUID &
fCallStrongNamedProcesses, int ppunk, IUnknown * *) Line 440 C++
ole32.dll!CClipDataObject::CacheDataPointer() Line 223 C++
ole32.dll!CClipDataObject::QueryInterface(const _GUID & riid, void * *
ppvObj) Line 886 C++
[Inline Frame] combase.dll!IUnknown::QueryInterface(INoMarshal * *) Line
138 C++
combase.dll!CoMarshalInterface(IStream * pStm, const _GUID & riid,
IUnknown * pUnk, unsigned long dwDestCtx, void * pvDestCtx, unsigned long
mshlflags) Line 1133 C++
combase.dll!wCoMarshalInterThreadInterfaceInStream(const _GUID & riid,
IUnknown * pUnk, IStream * * ppStm) Line 1046 C++
combase.dll!CoMarshalInterThreadInterfaceInStream(const _GUID & riid,
IUnknown * pUnk, IStream * * ppStm) Line 709 C++
vclplug_winlo.dll!MarshalIDataObjectInStream(IDataObject * pIDataObject,
IStream * * ppStream) Line 158 C++
vclplug_winlo.dll!CMtaOleClipboard::onGetClipboard(IStream * * ppStream)
Line 465 C++
vclplug_winlo.dll!CMtaOleClipboard::mtaOleReqWndProc(HWND__ * hWnd,
unsigned int uMsg, unsigned __int64 wParam, __int64 lParam) Line 547 C++
user32.dll!UserCallWinProcCheckWow()
user32.dll!DispatchMessageWorker()
vclplug_winlo.dll!CMtaOleClipboard::run() Line 656 C++
vclplug_winlo.dll!CMtaOleClipboard::oleThreadProc(void * pParam) Line 673
C++
ucrtbased.dll!thread_start<unsigned int (__cdecl*)(void *),1>(void *
const parameter) Line 97 C++
kernel32.dll!BaseThreadInitThunk()
ntdll.dll!RtlUserThreadStart()
Thread A locks the mutex, and calls CMtaOleClipboard::getClipboard, which
is synchronous; the call is marshaled to Thread B, which eventually tries to
release a data object, which requires the mutex.
Change-Id: I5ad8847b98ca23ad84c0595a2baad2c13f1e809e
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170504
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
Tested-by: Jenkins
(cherry picked from commit b3922a8d0051a656228f80aeee5360692c9bba2e)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170483
Reviewed-by: Xisco Fauli <xiscofa...@libreoffice.org>
diff --git a/vcl/win/dtrans/WinClipboard.cxx b/vcl/win/dtrans/WinClipboard.cxx
index ccd3c6907078..3fa2ba67cdc9 100644
--- a/vcl/win/dtrans/WinClipboard.cxx
+++ b/vcl/win/dtrans/WinClipboard.cxx
@@ -142,12 +142,13 @@ css::uno::Reference<css::datatransfer::XTransferable>
CWinClipboard::getContents
IDataObjectPtr CWinClipboard::getIDataObject()
{
- std::unique_lock aGuard(m_aMutex);
-
- if (m_bDisposed)
- throw lang::DisposedException("object is already disposed",
- static_cast<XClipboardEx*>(this));
+ {
+ std::unique_lock aGuard(m_aMutex);
+ if (m_bDisposed)
+ throw lang::DisposedException("object is already disposed",
+ static_cast<XClipboardEx*>(this));
+ }
// get the current dataobject from clipboard
IDataObjectPtr pIDataObject;
HRESULT hr = m_MtaOleClipboard.getClipboard(&pIDataObject);
commit d28addd0176c301fcfab2f11b22cf88783aa4963
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Sun Jul 14 10:39:29 2024 +0500
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Tue Oct 1 15:58:14 2024 +0500
Try to simplify Windows clipboard locking
Having three mutexes for one object is a bit too much. It is impossible
to handle it properly. Issues like tdf#148647, that affect many users,
but have no reliable reproducer, hint that possibly there is a deadlock
somewhere in the code that should clear m_pCurrentClipContent, or maybe
something similar.
This drops both local mutexes, and uses inherited m_aMutex everywhere
to guard accessto all the local data. I hope that this this will make
debugging of the problems around the clipboard possible. But it looks
invasive, of course.
Change-Id: I2311b1b4702f766e3e5e0d104f9b2b2999aa53c9
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170450
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
Signed-off-by: Xisco Fauli <xiscofa...@libreoffice.org>
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170461
diff --git a/vcl/win/dtrans/WinClipboard.cxx b/vcl/win/dtrans/WinClipboard.cxx
index 5f46bebecfd2..ccd3c6907078 100644
--- a/vcl/win/dtrans/WinClipboard.cxx
+++ b/vcl/win/dtrans/WinClipboard.cxx
@@ -52,7 +52,7 @@ using namespace com::sun::star;
namespace
{
CWinClipboard* s_pCWinClipbImpl = nullptr;
-osl::Mutex s_aClipboardSingletonMutex;
+std::mutex s_aClipboardSingletonMutex;
}
/*XEventListener,*/
@@ -65,7 +65,7 @@ CWinClipboard::CWinClipboard(const
uno::Reference<uno::XComponentContext>& rxCon
// necessary to reassociate from
// the static callback function
{
- osl::MutexGuard aGuard(s_aClipboardSingletonMutex);
+ std::unique_lock aGuard(s_aClipboardSingletonMutex);
s_pCWinClipbImpl = this;
}
@@ -74,18 +74,14 @@ CWinClipboard::CWinClipboard(const
uno::Reference<uno::XComponentContext>& rxCon
CWinClipboard::~CWinClipboard()
{
- {
- osl::MutexGuard aGuard(s_aClipboardSingletonMutex);
- s_pCWinClipbImpl = nullptr;
- }
-
- unregisterClipboardViewer();
+ assert(m_bDisposed);
+ assert(!s_pCWinClipbImpl);
}
void CWinClipboard::disposing(std::unique_lock<std::mutex>& mutex)
{
{
- osl::MutexGuard aGuard(s_aClipboardSingletonMutex);
+ std::unique_lock aGuard(s_aClipboardSingletonMutex);
s_pCWinClipbImpl = nullptr;
}
@@ -104,27 +100,26 @@ void
CWinClipboard::disposing(std::unique_lock<std::mutex>& mutex)
uno::Reference<datatransfer::XTransferable> SAL_CALL
CWinClipboard::getContents()
{
- osl::MutexGuard aGuard(m_aContentMutex);
+ std::unique_lock aGuard(m_aMutex);
+ return getContents_noLock();
+}
+css::uno::Reference<css::datatransfer::XTransferable>
CWinClipboard::getContents_noLock()
+{
if (m_bDisposed)
throw lang::DisposedException("object is already disposed",
static_cast<XClipboardEx*>(this));
+ assert(!m_pCurrentClipContent || !m_foreignContent); // Both can be null,
or only one set
+
// use the shortcut or create a transferable from
// system clipboard
- {
- osl::MutexGuard aGuard2(m_aContentCacheMutex);
-
- if (nullptr != m_pCurrentClipContent)
- return m_pCurrentClipContent->m_XTransferable;
-
- // Content cached?
- if (m_foreignContent.is())
- return m_foreignContent;
+ if (nullptr != m_pCurrentClipContent)
+ return m_pCurrentClipContent->m_XTransferable;
- // release the mutex, so that the variable may be
- // changed by other threads
- }
+ // Content cached?
+ if (m_foreignContent.is())
+ return m_foreignContent;
uno::Reference<datatransfer::XTransferable> rClipContent;
@@ -138,7 +133,6 @@ uno::Reference<datatransfer::XTransferable> SAL_CALL
CWinClipboard::getContents(
std::vector<sal_uInt32> aFormats(aUINTFormats.begin(),
aUINTFormats.end());
rClipContent = new CDOTransferable(m_xContext, this, aFormats);
- osl::MutexGuard aGuard2(m_aContentCacheMutex);
m_foreignContent = rClipContent;
}
}
@@ -148,7 +142,7 @@ uno::Reference<datatransfer::XTransferable> SAL_CALL
CWinClipboard::getContents(
IDataObjectPtr CWinClipboard::getIDataObject()
{
- osl::MutexGuard aGuard(m_aContentMutex);
+ std::unique_lock aGuard(m_aMutex);
if (m_bDisposed)
throw lang::DisposedException("object is already disposed",
@@ -172,7 +166,7 @@ void SAL_CALL CWinClipboard::setContents(
const uno::Reference<datatransfer::XTransferable>& xTransferable,
const uno::Reference<datatransfer::clipboard::XClipboardOwner>&
xClipboardOwner)
{
- osl::MutexGuard aGuard(m_aContentMutex);
+ std::unique_lock aGuard(m_aMutex);
if (m_bDisposed)
throw lang::DisposedException("object is already disposed",
@@ -180,26 +174,27 @@ void SAL_CALL CWinClipboard::setContents(
IDataObjectPtr pIDataObj;
+ m_foreignContent.clear();
+
if (xTransferable.is())
{
- {
- osl::MutexGuard aGuard2(m_aContentCacheMutex);
-
- m_foreignContent.clear();
-
- m_pCurrentClipContent = new CXNotifyingDataObject(
- CDTransObjFactory::createDataObjFromTransferable(m_xContext,
xTransferable),
- xTransferable, xClipboardOwner, this);
- }
+ m_pCurrentClipContent = new CXNotifyingDataObject(
+ CDTransObjFactory::createDataObjFromTransferable(m_xContext,
xTransferable),
+ xTransferable, xClipboardOwner, this);
pIDataObj = IDataObjectPtr(m_pCurrentClipContent);
}
+ else
+ {
+ m_pCurrentClipContent = nullptr;
+ }
m_MtaOleClipboard.setClipboard(pIDataObj.get());
}
OUString SAL_CALL CWinClipboard::getName()
{
+ std::unique_lock aGuard(m_aMutex);
if (m_bDisposed)
throw lang::DisposedException("object is already disposed",
static_cast<XClipboardEx*>(this));
@@ -211,24 +206,24 @@ OUString SAL_CALL CWinClipboard::getName()
void SAL_CALL CWinClipboard::flushClipboard()
{
- osl::MutexGuard aGuard(m_aContentMutex);
+ std::unique_lock aGuard(m_aMutex);
if (m_bDisposed)
throw lang::DisposedException("object is already disposed",
static_cast<XClipboardEx*>(this));
- // actually it should be ClearableMutexGuard aGuard( m_aContentCacheMutex
);
- // but it does not work since FlushClipboard does a callback and frees
DataObject
- // which results in a deadlock in onReleaseDataObject.
- // FlushClipboard had to be synchron in order to prevent shutdown until all
- // clipboard-formats are rendered.
- // The request is needed to prevent flushing if we are not clipboard owner
(it is
- // not known what happens if we flush but aren't clipboard owner).
+ // FlushClipboard does a callback and frees DataObject, which calls
onReleaseDataObject and
+ // locks mutex. FlushClipboard has to be synchron in order to prevent
shutdown until all
+ // clipboard-formats are rendered. The request is needed to prevent
flushing if we are not
+ // clipboard owner (it is not known what happens if we flush but aren't
clipboard owner).
// It may be possible to move the request to the clipboard STA thread by
saving the
// DataObject and call OleIsCurrentClipboard before flushing.
if (nullptr != m_pCurrentClipContent)
+ {
+ aGuard.unlock();
m_MtaOleClipboard.flushClipboard();
+ }
}
// XClipboardEx
@@ -248,6 +243,7 @@ sal_Int8 SAL_CALL CWinClipboard::getRenderingCapabilities()
void SAL_CALL CWinClipboard::addClipboardListener(
const uno::Reference<datatransfer::clipboard::XClipboardListener>&
listener)
{
+ std::unique_lock aGuard(m_aMutex);
if (m_bDisposed)
throw lang::DisposedException("object is already disposed",
static_cast<XClipboardEx*>(this));
@@ -257,13 +253,13 @@ void SAL_CALL CWinClipboard::addClipboardListener(
throw lang::IllegalArgumentException("empty reference",
static_cast<XClipboardEx*>(this),
1);
- std::unique_lock aGuard(m_aMutex);
maClipboardListeners.addInterface(aGuard, listener);
}
void SAL_CALL CWinClipboard::removeClipboardListener(
const uno::Reference<datatransfer::clipboard::XClipboardListener>&
listener)
{
+ std::unique_lock aGuard(m_aMutex);
if (m_bDisposed)
throw lang::DisposedException("object is already disposed",
static_cast<XClipboardEx*>(this));
@@ -273,25 +269,23 @@ void SAL_CALL CWinClipboard::removeClipboardListener(
throw lang::IllegalArgumentException("empty reference",
static_cast<XClipboardEx*>(this),
1);
- std::unique_lock aGuard(m_aMutex);
maClipboardListeners.removeInterface(aGuard, listener);
}
-void CWinClipboard::notifyAllClipboardListener()
+void CWinClipboard::clearCacheAndAllClipboardListener()
{
- if (m_bDisposed)
- return;
-
std::unique_lock aGuard(m_aMutex);
if (m_bDisposed)
return;
+ m_foreignContent.clear();
+
if (!maClipboardListeners.getLength(aGuard))
return;
try
{
- uno::Reference<datatransfer::XTransferable> rXTransf(getContents());
+ uno::Reference<datatransfer::XTransferable>
rXTransf(getContents_noLock());
datatransfer::clipboard::ClipboardEvent
aClipbEvent(static_cast<XClipboard*>(this),
rXTransf);
maClipboardListeners.notifyEach(
@@ -355,7 +349,7 @@ void
CWinClipboard::onReleaseDataObject(CXNotifyingDataObject* theCaller)
// if the current caller is the one we currently hold, then set it to NULL
// because an external source must be the clipboardowner now
- osl::MutexGuard aGuard(m_aContentCacheMutex);
+ std::unique_lock aGuard(m_aMutex);
if (m_pCurrentClipContent == theCaller)
m_pCurrentClipContent = nullptr;
@@ -373,15 +367,12 @@ void WINAPI CWinClipboard::onClipboardContentChanged()
rtl::Reference<CWinClipboard> pWinClipboard;
{
// Only hold the mutex to obtain a safe reference to the impl, to
avoid deadlock
- osl::MutexGuard aGuard(s_aClipboardSingletonMutex);
+ std::unique_lock aGuard(s_aClipboardSingletonMutex);
pWinClipboard.set(s_pCWinClipbImpl);
}
if (pWinClipboard)
- {
- pWinClipboard->m_foreignContent.clear();
- pWinClipboard->notifyAllClipboardListener();
- }
+ pWinClipboard->clearCacheAndAllClipboardListener();
}
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/vcl/win/dtrans/WinClipboard.hxx b/vcl/win/dtrans/WinClipboard.hxx
index fbaa1b206288..fcf7bf089cb6 100644
--- a/vcl/win/dtrans/WinClipboard.hxx
+++ b/vcl/win/dtrans/WinClipboard.hxx
@@ -59,12 +59,10 @@ class CWinClipboard final
CMtaOleClipboard m_MtaOleClipboard;
CXNotifyingDataObject* m_pCurrentClipContent;
com::sun::star::uno::Reference<com::sun::star::datatransfer::XTransferable>
m_foreignContent;
- osl::Mutex m_aContentMutex;
- osl::Mutex m_aContentCacheMutex;
comphelper::OInterfaceContainerHelper4<css::datatransfer::clipboard::XClipboardListener>
maClipboardListeners;
- void notifyAllClipboardListener();
+ void clearCacheAndAllClipboardListener();
void onReleaseDataObject(CXNotifyingDataObject* theCaller);
void registerClipboardViewer();
@@ -72,6 +70,8 @@ class CWinClipboard final
static void WINAPI onClipboardContentChanged();
+ css::uno::Reference<css::datatransfer::XTransferable> getContents_noLock();
+
public:
CWinClipboard(const css::uno::Reference<css::uno::XComponentContext>&
rxContext,
const OUString& aClipboardName);
