embeddedobj/source/msole/olecomponent.cxx | 42 ++++++++++-----------------
fpicker/source/win32/VistaFilePickerImpl.cxx | 1
include/vcl/svapp.hxx | 12 ++++++-
sfx2/source/control/bindings.cxx | 1
vcl/source/app/scheduler.cxx | 7 ++--
vcl/source/helper/threadex.cxx | 1
vcl/win/dtrans/MtaOleClipb.cxx | 35 +++++++++++++---------
vcl/win/dtrans/WinClipboard.cxx | 14 +++++----
8 files changed, 62 insertions(+), 51 deletions(-)
New commits:
commit cb9c9d990600baad9e5a63c80ddaa46e98248161
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Thu Mar 21 10:01:36 2024 +0500
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Wed Mar 27 09:36:54 2024 +0500
Downgrade sfx::SfxBindings aAutoTimer to an idle
This is a follow-up to commit f45402ae3f5241b460d9f1dcb04183893e1f91f7
(Fix a spurious JunitTest_sw_unoapi_3 failure, 2024-03-15). As noted
by Stephan, the failure persisted; it was because the update of slots
also accessed the changing document model, and it wasn't prevented by
IdlesLockGuard, because aAutoTimer had a default priority.
Change-Id: Iad8dfadcd35d9611e61e4c011511d6155a343f58
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165090
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
diff --git a/sfx2/source/control/bindings.cxx b/sfx2/source/control/bindings.cxx
index 4ea06231562a..44fbbd10c2f0 100644
--- a/sfx2/source/control/bindings.cxx
+++ b/sfx2/source/control/bindings.cxx
@@ -146,6 +146,7 @@ SfxBindings::SfxBindings()
// all caches are valid (no pending invalidate-job)
// create the list of caches
+ pImpl->aAutoTimer.SetPriority(TaskPriority::HIGH_IDLE);
pImpl->aAutoTimer.SetInvokeHandler( LINK(this, SfxBindings, NextJob) );
}
commit 3bb1590c78ffcf26cfadcc89a3dd3e0bf8be50e3
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Thu Mar 21 10:00:44 2024 +0500
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Wed Mar 27 09:27:14 2024 +0500
Only hold clipboard mutexes to obtain the values, to avoid deadlocks
Change-Id: I36a2f6e444b8a5397b96d3cba475a8d06ea86459
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165089
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
diff --git a/vcl/win/dtrans/MtaOleClipb.cxx b/vcl/win/dtrans/MtaOleClipb.cxx
index 865025057dcc..19a37dc52e19 100644
--- a/vcl/win/dtrans/MtaOleClipb.cxx
+++ b/vcl/win/dtrans/MtaOleClipb.cxx
@@ -713,25 +713,32 @@ DWORD WINAPI
CMtaOleClipboard::clipboardChangedNotifierThreadProc( _In_ LPVOID p
MsgWaitForMultipleObjects(2, pInst->m_hClipboardChangedNotifierEvents,
false, INFINITE,
QS_ALLINPUT | QS_ALLPOSTMESSAGE);
- ClearableMutexGuard aGuard2( pInst->m_ClipboardChangedEventCountMutex
);
-
- if ( pInst->m_ClipboardChangedEventCount > 0 )
+ bool hadEvents;
{
- pInst->m_ClipboardChangedEventCount--;
- if ( 0 == pInst->m_ClipboardChangedEventCount )
- ResetEvent( pInst->m_hClipboardChangedEvent );
-
- aGuard2.clear( );
+ ClearableMutexGuard
aGuard2(pInst->m_ClipboardChangedEventCountMutex);
+ hadEvents = pInst->m_ClipboardChangedEventCount > 0;
+ if (hadEvents)
+ {
+ pInst->m_ClipboardChangedEventCount--;
+ if (0 == pInst->m_ClipboardChangedEventCount)
+ ResetEvent(pInst->m_hClipboardChangedEvent);
+ }
+ }
- // nobody should touch m_pfncClipViewerCallback while we do
- MutexGuard aClipViewerGuard( pInst->m_pfncClipViewerCallbackMutex
);
+ if (hadEvents)
+ {
+ LPFNC_CLIPVIEWER_CALLBACK_t pClipViewerCallback;
+ {
+ // nobody should touch m_pfncClipViewerCallback while we do
+ // but don't hold the mutex while calling the callback itself:
it could deadlock
+ MutexGuard
aClipViewerGuard(pInst->m_pfncClipViewerCallbackMutex);
+ pClipViewerCallback = pInst->m_pfncClipViewerCallback;
+ }
// notify all clipboard listener
- if ( pInst->m_pfncClipViewerCallback )
- pInst->m_pfncClipViewerCallback( );
+ if (pClipViewerCallback)
+ pClipViewerCallback();
}
- else
- aGuard2.clear( );
}
return 0;
diff --git a/vcl/win/dtrans/WinClipboard.cxx b/vcl/win/dtrans/WinClipboard.cxx
index c2fa269dfc5d..33c8023a925c 100644
--- a/vcl/win/dtrans/WinClipboard.cxx
+++ b/vcl/win/dtrans/WinClipboard.cxx
@@ -373,13 +373,17 @@ void CWinClipboard::unregisterClipboardViewer() {
m_MtaOleClipboard.registerClip
void WINAPI CWinClipboard::onClipboardContentChanged()
{
- osl::MutexGuard aGuard(s_aClipboardSingletonMutex);
+ rtl::Reference<CWinClipboard> pWinClipboard;
+ {
+ // Only hold the mutex to obtain a safe reference to the impl, to
avoid deadlock
+ osl::MutexGuard aGuard(s_aClipboardSingletonMutex);
+ pWinClipboard.set(s_pCWinClipbImpl);
+ }
- // reassociation to instance through static member
- if (nullptr != s_pCWinClipbImpl)
+ if (pWinClipboard)
{
- s_pCWinClipbImpl->m_foreignContent.clear();
- s_pCWinClipbImpl->notifyAllClipboardListener();
+ pWinClipboard->m_foreignContent.clear();
+ pWinClipboard->notifyAllClipboardListener();
}
}
commit 1d43c9ad8485ca837767bff87160639c65932e6c
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Tue Mar 26 12:11:13 2024 +0500
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Wed Mar 27 09:05:39 2024 +0500
This assert was wrong
In commit e2bfc34d146806a8f96be0cd2323d716f12cba4e (Reimplement
OleComponentNative_Impl to use IGlobalInterfaceTable, 2024-03-11),
I added the assert in a false assumption that RunObject may only
be called once. Indeed, this is not so. Thus just make sure to not
try to register it second time.
Change-Id: I36fc4f3939bd0061e462659749bba8e4bd3075ba
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165299
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
diff --git a/embeddedobj/source/msole/olecomponent.cxx
b/embeddedobj/source/msole/olecomponent.cxx
index 142d92ddf1c3..ca104a7c3425 100644
--- a/embeddedobj/source/msole/olecomponent.cxx
+++ b/embeddedobj/source/msole/olecomponent.cxx
@@ -200,7 +200,8 @@ private:
template <typename T> void registerInterface(T* pInterface, DWORD& cookie)
{
- assert(cookie == 0); // do not set again
+ if (cookie != 0) // E.g., on subsequent RunObject calls
+ return;
HRESULT hr = m_pGlobalTable->RegisterInterfaceInGlobal(pInterface,
__uuidof(T), &cookie);
SAL_WARN_IF(FAILED(hr), "embeddedobj.ole", "RegisterInterfaceInGlobal
failed");
}
commit 8e47b628cc7a1d6496a5f73748ab5e7b441fdb1d
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Tue Mar 26 19:06:25 2024 +0500
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Tue Mar 26 23:19:44 2024 +0500
Set the condition, even when main thread message queue is empty
Post an empty user event for that.
Change-Id: I9ab4b4374a25eddce8c58d36f58e08bec4a1855c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165352
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
diff --git a/vcl/source/app/scheduler.cxx b/vcl/source/app/scheduler.cxx
index 61b82d21b984..f7075584924b 100644
--- a/vcl/source/app/scheduler.cxx
+++ b/vcl/source/app/scheduler.cxx
@@ -285,6 +285,9 @@ Scheduler::IdlesLockGuard::IdlesLockGuard()
// condition to proceed. Only main thread returning to
Application::Execute guarantees that
// the flag really took effect.
pSVData->m_inExecuteCondtion.reset();
+ // Put an empty event to the application's queue, to make sure that it
loops through the
+ // code that sets the condition, even when there's no other events in
the queue
+ Application::PostUserEvent({});
SolarMutexReleaser releaser;
pSVData->m_inExecuteCondtion.wait();
}
commit 51b7d8314a377249b99643b5174b3a5ca71ffde5
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Mon Mar 25 09:05:00 2024 +0500
Commit: Mike Kaganski <mike.kagan...@collabora.com>
CommitDate: Tue Mar 26 23:19:38 2024 +0500
Relax SolarMutexReleaser precondition to not require SolarMutex lock
As discussed on
https://gerrit.libreoffice.org/c/core/+/164843/2#message-8873d3d119de7206b33bc824f5809b8b1d3d97da,
it is impossible at times to know in advance, if a specific code, that
must not be guarded by SolarMutex (e.g., calling to other threads, which
might need to grab the mutex), will itself be guarded by SolarMutex.
Before this change, a required pre-requisite for SolarMutexReleaser use
was existing lock of SolarMutex; otherwise, an attempt to release it
would call abort(). Thus, in some places we had to grab the mutex prior
to releasing it, and that itself introduced more potential for deadlock.
Now the SolarMutexReleaser is safe to use without the lock, in which
case, it will do nothing.
Change-Id: I8759c2f6ed448598b3be4d6c5109804b5e7523ed
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165262
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
diff --git a/embeddedobj/source/msole/olecomponent.cxx
b/embeddedobj/source/msole/olecomponent.cxx
index da10b9d9d233..142d92ddf1c3 100644
--- a/embeddedobj/source/msole/olecomponent.cxx
+++ b/embeddedobj/source/msole/olecomponent.cxx
@@ -206,15 +206,6 @@ private:
}
};
-namespace
-{
-struct SafeSolarMutexReleaser
-{
- SolarMutexGuard guard; // To make sure we actually hold it prior to release
- SolarMutexReleaser releaser;
-};
-}
-
static DWORD GetAspectFromFlavor( const datatransfer::DataFlavor& aFlavor )
{
if ( aFlavor.MimeType.indexOf( ";Aspect=THUMBNAIL" ) != -1 )
@@ -569,7 +560,7 @@ void OleComponent::RetrieveObjectDataFlavors_Impl()
HRESULT hr;
sal::systools::COMReference< IEnumFORMATETC > pFormatEnum;
{
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
hr = pDataObject->EnumFormatEtc(DATADIR_GET, &pFormatEnum);
}
if ( SUCCEEDED( hr ) && pFormatEnum )
@@ -859,7 +850,7 @@ void OleComponent::InitEmbeddedCopyOfLink(
rtl::Reference<OleComponent> const &
// the object must be already disconnected from the temporary URL
auto pStorage(m_pNativeImpl->CreateNewStorage(getTempURL()));
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
auto
pDataObject(pOleLinkComponentObj.QueryInterface<IDataObject>(sal::systools::COM_QUERY));
if ( pDataObject && SUCCEEDED( OleQueryCreateFromData( pDataObject ) ) )
@@ -987,7 +978,7 @@ void OleComponent::CloseObject()
auto pOleObject(m_pNativeImpl->get<IOleObject>());
if (pOleObject && OleIsRunning(pOleObject))
{
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
HRESULT hr = pOleObject->Close(OLECLOSE_NOSAVE); // must be saved
before
SAL_WARN_IF(FAILED(hr), "embeddedobj.ole", "IOleObject::Close failed");
}
@@ -1005,7 +996,7 @@ uno::Sequence< embed::VerbDescriptor >
OleComponent::GetVerbList()
sal::systools::COMReference< IEnumOLEVERB > pEnum;
HRESULT hr;
{
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
hr = pOleObject->EnumVerbs(&pEnum);
}
if (SUCCEEDED(hr))
@@ -1048,7 +1039,7 @@ void OleComponent::ExecuteVerb( sal_Int32 nVerbID )
if (!pOleObject)
throw embed::WrongStateException(); // TODO
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
// TODO: probably extents should be set here and stored in aRect
// TODO: probably the parent window also should be set
@@ -1065,7 +1056,7 @@ void OleComponent::SetHostName( const OUString&
aEmbDocName )
if (!pOleObject)
throw embed::WrongStateException(); // TODO: the object is in wrong
state
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
pOleObject->SetHostNames(L"app name", o3tl::toW(aEmbDocName.getStr()));
}
@@ -1081,7 +1072,7 @@ void OleComponent::SetExtent( const awt::Size&
aVisAreaSize, sal_Int64 nAspect )
SIZEL aSize = { aVisAreaSize.Width, aVisAreaSize.Height };
HRESULT hr;
{
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
hr = pOleObject->SetExtent(nMSAspect, &aSize);
}
@@ -1117,7 +1108,7 @@ awt::Size OleComponent::GetExtent( sal_Int64 nAspect )
HRESULT hr;
{
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
hr = pDataObject->GetData(&aFormat, &aMedium);
}
@@ -1204,7 +1195,7 @@ awt::Size OleComponent::GetCachedExtent( sal_Int64
nAspect )
HRESULT hr;
{
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
hr = pViewObject2->GetExtent(nMSAspect, -1, nullptr, &aSize);
}
@@ -1235,7 +1226,7 @@ awt::Size OleComponent::GetRecommendedExtent( sal_Int64
nAspect )
SIZEL aSize;
HRESULT hr;
{
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
hr = pOleObject->GetExtent(nMSAspect, &aSize);
}
if ( FAILED( hr ) )
@@ -1256,7 +1247,7 @@ sal_Int64 OleComponent::GetMiscStatus( sal_Int64 nAspect )
DWORD nResult = 0;
{
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
pOleObject->GetMiscStatus(static_cast<DWORD>(nAspect), &nResult);
}
return static_cast<sal_Int64>(nResult); // first 32 bits are for MS flags
@@ -1272,7 +1263,7 @@ uno::Sequence< sal_Int8 > OleComponent::GetCLSID()
GUID aCLSID;
HRESULT hr;
{
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
hr = pOleObject->GetUserClassID(&aCLSID);
}
if ( FAILED( hr ) )
@@ -1295,7 +1286,7 @@ bool OleComponent::IsDirty()
if ( !pPersistStorage )
throw io::IOException(); // TODO
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
HRESULT hr = pPersistStorage->IsDirty();
return ( hr != S_FALSE );
}
@@ -1311,7 +1302,7 @@ void OleComponent::StoreOwnTmpIfNecessary()
if ( !pPersistStorage )
throw io::IOException(); // TODO
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
if ( m_bWorkaroundActive || pPersistStorage->IsDirty() != S_FALSE )
{
@@ -1573,7 +1564,7 @@ uno::Any SAL_CALL OleComponent::getTransferData( const
datatransfer::DataFlavor&
HRESULT hr;
{
- SafeSolarMutexReleaser releaser;
+ SolarMutexReleaser releaser;
hr = pDataObject->GetData(&aFormat, &aMedium);
}
if ( SUCCEEDED( hr ) )
diff --git a/fpicker/source/win32/VistaFilePickerImpl.cxx
b/fpicker/source/win32/VistaFilePickerImpl.cxx
index 741fdadb621a..d1a0cdf8a5a9 100644
--- a/fpicker/source/win32/VistaFilePickerImpl.cxx
+++ b/fpicker/source/win32/VistaFilePickerImpl.cxx
@@ -965,7 +965,6 @@ void VistaFilePickerImpl::impl_sta_ShowDialogModal(Request&
rRequest)
{
// tdf#146007: Make sure we don't hold solar mutex: COM may need to
forward
// the execution to the main thread, and holding solar mutex could
deadlock
- SolarMutexGuard g; // First acquire, to avoid releaser failure
SolarMutexReleaser r;
// show dialog and wait for user decision
hResult = iDialog->Show(m_hParentWindow ? m_hParentWindow
diff --git a/include/vcl/svapp.hxx b/include/vcl/svapp.hxx
index 398358f1bfad..7aef4926f9c2 100644
--- a/include/vcl/svapp.hxx
+++ b/include/vcl/svapp.hxx
@@ -1448,8 +1448,16 @@ class SolarMutexReleaser
{
const sal_uInt32 mnReleased;
public:
- SolarMutexReleaser(): mnReleased(Application::ReleaseSolarMutex()) {}
- ~SolarMutexReleaser() { Application::AcquireSolarMutex( mnReleased ); }
+ SolarMutexReleaser()
+ : mnReleased(
+ Application::GetSolarMutex().IsCurrentThread() ?
Application::ReleaseSolarMutex() : 0)
+ {
+ }
+ ~SolarMutexReleaser()
+ {
+ if (mnReleased)
+ Application::AcquireSolarMutex(mnReleased);
+ }
};
VCL_DLLPUBLIC Application* GetpApp();
diff --git a/vcl/source/app/scheduler.cxx b/vcl/source/app/scheduler.cxx
index c0fc79d4ac5c..61b82d21b984 100644
--- a/vcl/source/app/scheduler.cxx
+++ b/vcl/source/app/scheduler.cxx
@@ -285,9 +285,7 @@ Scheduler::IdlesLockGuard::IdlesLockGuard()
// condition to proceed. Only main thread returning to
Application::Execute guarantees that
// the flag really took effect.
pSVData->m_inExecuteCondtion.reset();
- std::optional<SolarMutexReleaser> releaser;
- if (pSVData->mpDefInst->GetYieldMutex()->IsCurrentThread())
- releaser.emplace();
+ SolarMutexReleaser releaser;
pSVData->m_inExecuteCondtion.wait();
}
}
diff --git a/vcl/source/helper/threadex.cxx b/vcl/source/helper/threadex.cxx
index 1590b91f1167..cbd342bd28df 100644
--- a/vcl/source/helper/threadex.cxx
+++ b/vcl/source/helper/threadex.cxx
@@ -52,7 +52,6 @@ void SolarThreadExecutor::execute()
m_aStart.reset();
m_aFinish.reset();
ImplSVEvent* nEvent = Application::PostUserEvent(LINK(this,
SolarThreadExecutor, worker));
- SolarMutexGuard aGuard;
SolarMutexReleaser aReleaser;
if (m_aStart.wait() == osl::Condition::result_timeout)
{
