mir/libreoffice
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Make OleEmbeddedObject locking stricter

Browse Source 
This is an attempt to reduce areas that execute with mutex unlocked
because the called code may deadlock. Also, this copies objects on
stack before unlocking, using lambdas' own variables.

I hope that it somewhat improves reliability. OTOH, this runs more
code with lock active, has a potential of new deadlocks, so risky.

Change-Id: I558b7f5f18f63622fed3a9c3978327d0d76fe90d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/172237
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>
This commit is contained in:
Mike Kaganski
2024-08-22 12:44:50 +05:00
parent b572bf6a71
commit 6084a2904d
4 changed files with 91 additions and 102 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
embeddedobj/source/inc/oleembobj.hxx
View File

@@ -219,9 +219,9 @@ protected:
#ifdef _WIN32 #ifdef _WIN32
void SwitchComponentToRunningState_Impl(osl::ResettableMutexGuard& guard); void SwitchComponentToRunningState_Impl(osl::ResettableMutexGuard& guard);
#endif #endif
void MakeEventListenerNotification_Impl( const OUString& aEventName ); void MakeEventListenerNotification_Impl( const OUString& aEventName, osl::ResettableMutexGuard& guard );
#ifdef _WIN32 #ifdef _WIN32
void StateChangeNotification_Impl( bool bBeforeChange, sal_Int32 nOldState, sal_Int32 nNewState ); void StateChangeNotification_Impl( bool bBeforeChange, sal_Int32 nOldState, sal_Int32 nNewState, osl::ResettableMutexGuard& guard );
css::uno::Reference< css::io::XOutputStream > GetStreamForSaving(); css::uno::Reference< css::io::XOutputStream > GetStreamForSaving();

71
embeddedobj/source/msole/oleembed.cxx
View File

@@ -481,9 +481,7 @@ void SAL_CALL OleEmbeddedObject::changeState( sal_Int32 nNewState )
// will behave after activation // will behave after activation
sal_Int32 nOldState = m_nObjectState; sal_Int32 nOldState = m_nObjectState;
aGuard.clear(); StateChangeNotification_Impl( true, nOldState, nNewState, aGuard );
StateChangeNotification_Impl( true, nOldState, nNewState );
aGuard.reset();
try try
{ {
@@ -498,14 +496,12 @@ void SAL_CALL OleEmbeddedObject::changeState( sal_Int32 nNewState )
// the loaded state must be set before, because of notifications! // the loaded state must be set before, because of notifications!
m_nObjectState = nNewState; m_nObjectState = nNewState;
aGuard.clear();
{ {
VerbExecutionControllerGuard aVerbGuard( m_aVerbExecutionController ); VerbExecutionControllerGuard aVerbGuard( m_aVerbExecutionController );
m_pOleComponent->CloseObject(); ExecUnlocked([p = m_pOleComponent] { p->CloseObject(); }, aGuard);
} }
StateChangeNotification_Impl( false, nOldState, m_nObjectState ); StateChangeNotification_Impl( false, nOldState, m_nObjectState, aGuard );
aGuard.reset();
} }
else if ( nNewState == embed::EmbedStates::RUNNING || nNewState == embed::EmbedStates::ACTIVE ) else if ( nNewState == embed::EmbedStates::RUNNING || nNewState == embed::EmbedStates::ACTIVE )
{ {
@@ -520,19 +516,17 @@ void SAL_CALL OleEmbeddedObject::changeState( sal_Int32 nNewState )
SwitchComponentToRunningState_Impl(aGuard); SwitchComponentToRunningState_Impl(aGuard);
m_nObjectState = embed::EmbedStates::RUNNING; m_nObjectState = embed::EmbedStates::RUNNING;
aGuard.clear(); StateChangeNotification_Impl( false, nOldState, m_nObjectState, aGuard );
StateChangeNotification_Impl( false, nOldState, m_nObjectState );
aGuard.reset();
if ( m_pOleComponent && m_bHasSizeToSet ) if ( m_pOleComponent && m_bHasSizeToSet )
{ {
aGuard.clear();
try { try {
m_pOleComponent->SetExtent( m_aSizeToSet, m_nAspectToSet ); ExecUnlocked([p = m_pOleComponent, s = m_aSizeToSet,
a = m_nAspectToSet]() { p->SetExtent(s, a); },
aGuard);
m_bHasSizeToSet = false; m_bHasSizeToSet = false;
} }
catch( const uno::Exception& ) {} catch( const uno::Exception& ) {}
aGuard.reset();
} }
if ( m_nObjectState == nNewState ) if ( m_nObjectState == nNewState )
@@ -544,30 +538,33 @@ void SAL_CALL OleEmbeddedObject::changeState( sal_Int32 nNewState )
if ( m_nObjectState == embed::EmbedStates::RUNNING && nNewState == embed::EmbedStates::ACTIVE ) if ( m_nObjectState == embed::EmbedStates::RUNNING && nNewState == embed::EmbedStates::ACTIVE )
{ {
// execute OPEN verb, if object does not reach active state it is an object's problem // execute OPEN verb, if object does not reach active state it is an object's problem
aGuard.clear(); ExecUnlocked([p = m_pOleComponent]()
m_pOleComponent->ExecuteVerb( embed::EmbedVerbs::MS_OLEVERB_OPEN ); { p->ExecuteVerb(embed::EmbedVerbs::MS_OLEVERB_OPEN); },
aGuard.reset(); aGuard);
// some objects do not allow to set the size even in running state // some objects do not allow to set the size even in running state
if ( m_pOleComponent && m_bHasSizeToSet ) if ( m_pOleComponent && m_bHasSizeToSet )
{ {
aGuard.clear();
try { try {
m_pOleComponent->SetExtent( m_aSizeToSet, m_nAspectToSet ); ExecUnlocked([p = m_pOleComponent, s = m_aSizeToSet,
a = m_nAspectToSet]() { p->SetExtent(s, a); },
aGuard);
m_bHasSizeToSet = false; m_bHasSizeToSet = false;
} }
catch( uno::Exception& ) {} catch( uno::Exception& ) {}
aGuard.reset();
} }
m_nObjectState = nNewState; m_nObjectState = nNewState;
} }
else if ( m_nObjectState == embed::EmbedStates::ACTIVE && nNewState == embed::EmbedStates::RUNNING ) else if ( m_nObjectState == embed::EmbedStates::ACTIVE && nNewState == embed::EmbedStates::RUNNING )
{ {
aGuard.clear(); ExecUnlocked(
m_pOleComponent->CloseObject(); [p = m_pOleComponent]()
m_pOleComponent->RunObject(); // Should not fail, the object already was active {
aGuard.reset(); p->CloseObject();
p->RunObject(); // Should not fail, the object already was active
},
aGuard);
m_nObjectState = nNewState; m_nObjectState = nNewState;
} }
else else
@@ -580,8 +577,7 @@ void SAL_CALL OleEmbeddedObject::changeState( sal_Int32 nNewState )
} }
catch( uno::Exception& ) catch( uno::Exception& )
{ {
aGuard.clear(); StateChangeNotification_Impl( false, nOldState, m_nObjectState, aGuard );
StateChangeNotification_Impl( false, nOldState, m_nObjectState );
throw; throw;
} }
} }
@@ -857,9 +853,7 @@ void SAL_CALL OleEmbeddedObject::doVerb( sal_Int32 nVerbID )
{ {
// if the target object is in loaded state // if the target object is in loaded state
// it must be switched to running state to execute verb // it must be switched to running state to execute verb
aGuard.clear(); ExecUnlocked([this]() { changeState(embed::EmbedStates::RUNNING); }, aGuard);
changeState( embed::EmbedStates::RUNNING );
aGuard.reset();
} }
try { try {
@@ -869,11 +863,13 @@ void SAL_CALL OleEmbeddedObject::doVerb( sal_Int32 nVerbID )
// ==== the STAMPIT related solution ============================= // ==== the STAMPIT related solution =============================
m_aVerbExecutionController.StartControlExecution(); m_aVerbExecutionController.StartControlExecution();
{ ExecUnlocked(
osl::ResettableMutexGuardScopedReleaser clearedMutex(aGuard); [nVerbID, p = m_pOleComponent, name = m_aContainerName]()
m_pOleComponent->ExecuteVerb(nVerbID); {
m_pOleComponent->SetHostName(m_aContainerName); p->ExecuteVerb(nVerbID);
} p->SetHostName(name);
},
aGuard);
// ==== the STAMPIT related solution ============================= // ==== the STAMPIT related solution =============================
bool bModifiedOnExecution = m_aVerbExecutionController.EndControlExecution_WasModified(); bool bModifiedOnExecution = m_aVerbExecutionController.EndControlExecution_WasModified();
@@ -889,9 +885,7 @@ void SAL_CALL OleEmbeddedObject::doVerb( sal_Int32 nVerbID )
// ==== the STAMPIT related solution ============================= // ==== the STAMPIT related solution =============================
m_aVerbExecutionController.EndControlExecution_WasModified(); m_aVerbExecutionController.EndControlExecution_WasModified();
StateChangeNotification_Impl( false, nOldState, m_nObjectState, aGuard );
aGuard.clear();
StateChangeNotification_Impl( false, nOldState, m_nObjectState );
throw; throw;
} }
@@ -1154,10 +1148,7 @@ sal_Int64 SAL_CALL OleEmbeddedObject::getStatus( sal_Int64
nResult = m_nStatus; nResult = m_nStatus;
else if ( m_pOleComponent ) else if ( m_pOleComponent )
{ {
{ m_nStatus = ExecUnlocked([p = m_pOleComponent, nAspect] { return p->GetMiscStatus(nAspect); }, aGuard);
osl::ResettableMutexGuardScopedReleaser clearedMutex(aGuard);
m_nStatus = m_pOleComponent->GetMiscStatus(nAspect);
}
m_nStatusAspect = nAspect; m_nStatusAspect = nAspect;
m_bGotStatus = true; m_bGotStatus = true;
nResult = m_nStatus; nResult = m_nStatus;

85
embeddedobj/source/msole/olemisc.cxx
View File

@@ -154,7 +154,8 @@ OleEmbeddedObject::~OleEmbeddedObject()
} }
void OleEmbeddedObject::MakeEventListenerNotification_Impl( const OUString& aEventName ) void OleEmbeddedObject::MakeEventListenerNotification_Impl( const OUString& aEventName,
osl::ResettableMutexGuard& guard )
{ {
if ( !m_pInterfaceContainer ) if ( !m_pInterfaceContainer )
return; return;
@@ -165,59 +166,59 @@ void OleEmbeddedObject::MakeEventListenerNotification_Impl( const OUString& aEve
if ( pContainer == nullptr ) if ( pContainer == nullptr )
return; return;
document::EventObject aEvent( static_cast< ::cppu::OWeakObject* >( this ), aEventName ); auto proc = [&guard, aEvent = document::EventObject(getXWeak(), aEventName)](
comphelper::OInterfaceIteratorHelper2 pIterator(*pContainer); const uno::Reference<document::XEventListener>& xListener)
while (pIterator.hasMoreElements())
{ {
try try
{ {
static_cast<document::XEventListener*>(pIterator.next())->notifyEvent( aEvent ); osl::ResettableMutexGuardScopedReleaser area(guard);
xListener->notifyEvent(aEvent);
} }
catch( const uno::RuntimeException& ) catch (const lang::DisposedException&)
{
throw; // forEach handles this
}
catch (const uno::RuntimeException&)
{ {
} }
} };
pContainer->forEach<document::XEventListener>(proc);
} }
#ifdef _WIN32 #ifdef _WIN32
void OleEmbeddedObject::StateChangeNotification_Impl( bool bBeforeChange, sal_Int32 nOldState, sal_Int32 nNewState ) void OleEmbeddedObject::StateChangeNotification_Impl( bool bBeforeChange, sal_Int32 nOldState, sal_Int32 nNewState,
osl::ResettableMutexGuard& guard )
{ {
if ( m_pInterfaceContainer ) if (!m_pInterfaceContainer)
{ return;
comphelper::OInterfaceContainerHelper2* pContainer = m_pInterfaceContainer->getContainer(
cppu::UnoType<embed::XStateChangeListener>::get());
if ( pContainer != nullptr )
{
lang::EventObject aSource( static_cast< ::cppu::OWeakObject* >( this ) );
comphelper::OInterfaceIteratorHelper2 pIterator(*pContainer);
while (pIterator.hasMoreElements()) comphelper::OInterfaceContainerHelper2* pContainer = m_pInterfaceContainer->getContainer(
{ cppu::UnoType<embed::XStateChangeListener>::get());
if ( bBeforeChange ) if (!pContainer)
{ return;
try
{ auto proc
static_cast<embed::XStateChangeListener*>(pIterator.next())->changingState( aSource, nOldState, nNewState ); = [bBeforeChange, nOldState, nNewState, &guard, aSource = lang::EventObject(getXWeak())](
} const uno::Reference<embed::XStateChangeListener>& xListener)
catch( const uno::Exception& ) {
{ try
// even if the listener complains ignore it for now {
} osl::ResettableMutexGuardScopedReleaser area(guard);
} if (bBeforeChange)
else xListener->changingState(aSource, nOldState, nNewState);
{ else
try xListener->stateChanged(aSource, nOldState, nNewState);
{
static_cast<embed::XStateChangeListener*>(pIterator.next())->stateChanged( aSource, nOldState, nNewState );
}
catch( const uno::Exception& )
{
// if anything happened it is problem of listener, ignore it
}
}
}
} }
} catch (const lang::DisposedException&)
{
throw; // forEach handles this
}
catch (const uno::Exception&)
{
// even if the listener complains ignore it for now
}
};
pContainer->forEach<embed::XStateChangeListener>(proc);
} }
#endif #endif

33
embeddedobj/source/msole/olepersist.cxx
View File

@@ -822,7 +822,7 @@ bool OleEmbeddedObject::SaveObject_Impl()
bool OleEmbeddedObject::OnShowWindow_Impl( bool bShow ) bool OleEmbeddedObject::OnShowWindow_Impl( bool bShow )
{ {
osl::ClearableMutexGuard aGuard(m_aMutex); osl::ResettableMutexGuard aGuard(m_aMutex);
bool bResult = false; bool bResult = false;
@@ -838,21 +838,19 @@ bool OleEmbeddedObject::OnShowWindow_Impl( bool bShow )
m_nObjectState = embed::EmbedStates::ACTIVE; m_nObjectState = embed::EmbedStates::ACTIVE;
m_aVerbExecutionController.ObjectIsActive(); m_aVerbExecutionController.ObjectIsActive();
aGuard.clear(); StateChangeNotification_Impl( false, nOldState, m_nObjectState, aGuard );
StateChangeNotification_Impl( false, nOldState, m_nObjectState );
} }
else if ( !bShow && m_nObjectState == embed::EmbedStates::ACTIVE ) else if ( !bShow && m_nObjectState == embed::EmbedStates::ACTIVE )
{ {
m_nObjectState = embed::EmbedStates::RUNNING; m_nObjectState = embed::EmbedStates::RUNNING;
aGuard.clear(); StateChangeNotification_Impl( false, nOldState, m_nObjectState, aGuard );
StateChangeNotification_Impl( false, nOldState, m_nObjectState );
} }
if ( m_xClientSite.is() ) if ( m_xClientSite.is() )
{ {
try try
{ {
m_xClientSite->visibilityChanged( bShow ); ExecUnlocked([p = m_xClientSite, bShow] { p->visibilityChanged(bShow); }, aGuard);
bResult = true; bResult = true;
} }
catch( const uno::Exception& ) catch( const uno::Exception& )
@@ -873,6 +871,7 @@ void OleEmbeddedObject::OnIconChanged_Impl()
void OleEmbeddedObject::OnViewChanged_Impl() void OleEmbeddedObject::OnViewChanged_Impl()
{ {
osl::ResettableMutexGuard aGuard(m_aMutex);
if ( m_bDisposed ) if ( m_bDisposed )
throw lang::DisposedException(); throw lang::DisposedException();
@@ -896,7 +895,7 @@ void OleEmbeddedObject::OnViewChanged_Impl()
// The view is changed while the object is in running state, save the new object // The view is changed while the object is in running state, save the new object
m_xCachedVisualRepresentation.clear(); m_xCachedVisualRepresentation.clear();
SaveObject_Impl(); SaveObject_Impl();
MakeEventListenerNotification_Impl( "OnVisAreaChanged" ); MakeEventListenerNotification_Impl( "OnVisAreaChanged", aGuard );
} }
} }
@@ -904,6 +903,7 @@ void OleEmbeddedObject::OnViewChanged_Impl()
void OleEmbeddedObject::OnClosed_Impl() void OleEmbeddedObject::OnClosed_Impl()
{ {
osl::ResettableMutexGuard aGuard(m_aMutex);
if ( m_bDisposed ) if ( m_bDisposed )
throw lang::DisposedException(); throw lang::DisposedException();
@@ -911,7 +911,7 @@ void OleEmbeddedObject::OnClosed_Impl()
{ {
sal_Int32 nOldState = m_nObjectState; sal_Int32 nOldState = m_nObjectState;
m_nObjectState = embed::EmbedStates::LOADED; m_nObjectState = embed::EmbedStates::LOADED;
StateChangeNotification_Impl( false, nOldState, m_nObjectState ); StateChangeNotification_Impl( false, nOldState, m_nObjectState, aGuard );
} }
} }
@@ -1114,7 +1114,7 @@ void OleEmbeddedObject::StoreToLocation_Impl(
#ifdef _WIN32 #ifdef _WIN32
// if the object was NOT modified after storing it can be just copied // if the object was NOT modified after storing it can be just copied
// as if it was in loaded state // as if it was in loaded state
|| (m_pOleComponent && !ExecUnlocked([this] { return m_pOleComponent->IsDirty(); }, rGuard)) || (m_pOleComponent && !ExecUnlocked([p = m_pOleComponent] { return p->IsDirty(); }, rGuard))
#endif #endif
) )
{ {
@@ -1537,7 +1537,7 @@ void SAL_CALL OleEmbeddedObject::saveCompleted( sal_Bool bUseNew )
} }
// end wrapping related part ==================== // end wrapping related part ====================
osl::ClearableMutexGuard aGuard(m_aMutex); osl::ResettableMutexGuard aGuard(m_aMutex);
if ( m_bDisposed ) if ( m_bDisposed )
throw lang::DisposedException(); // TODO throw lang::DisposedException(); // TODO
@@ -1606,16 +1606,15 @@ void SAL_CALL OleEmbeddedObject::saveCompleted( sal_Bool bUseNew )
{} {}
} }
aGuard.clear();
if ( bUseNew ) if ( bUseNew )
{ {
MakeEventListenerNotification_Impl( u"OnSaveAsDone"_ustr); MakeEventListenerNotification_Impl( u"OnSaveAsDone"_ustr, aGuard);
// the object can be changed only on windows // the object can be changed only on windows
// the notification should be done only if the object is not in loaded state // the notification should be done only if the object is not in loaded state
if ( m_pOleComponent && m_nUpdateMode == embed::EmbedUpdateModes::ALWAYS_UPDATE && !bStoreLoaded ) if ( m_pOleComponent && m_nUpdateMode == embed::EmbedUpdateModes::ALWAYS_UPDATE && !bStoreLoaded )
{ {
MakeEventListenerNotification_Impl( u"OnVisAreaChanged"_ustr); MakeEventListenerNotification_Impl( u"OnVisAreaChanged"_ustr, aGuard);
} }
} }
} }
@@ -1721,7 +1720,7 @@ void SAL_CALL OleEmbeddedObject::storeOwn()
bool bStoreLoaded = true; bool bStoreLoaded = true;
#ifdef _WIN32 #ifdef _WIN32
if ( m_nObjectState != embed::EmbedStates::LOADED && m_pOleComponent && ExecUnlocked([this] { return m_pOleComponent->IsDirty(); }, aGuard) ) if ( m_nObjectState != embed::EmbedStates::LOADED && m_pOleComponent && ExecUnlocked([p = m_pOleComponent] { return p->IsDirty(); }, aGuard) )
{ {
bStoreLoaded = false; bStoreLoaded = false;
@@ -1782,14 +1781,12 @@ void SAL_CALL OleEmbeddedObject::storeOwn()
{} {}
} }
aGuard.clear(); MakeEventListenerNotification_Impl( u"OnSaveDone"_ustr, aGuard);
MakeEventListenerNotification_Impl( u"OnSaveDone"_ustr);
// the object can be changed only on Windows // the object can be changed only on Windows
// the notification should be done only if the object is not in loaded state // the notification should be done only if the object is not in loaded state
if ( m_pOleComponent && m_nUpdateMode == embed::EmbedUpdateModes::ALWAYS_UPDATE && !bStoreLoaded ) if ( m_pOleComponent && m_nUpdateMode == embed::EmbedUpdateModes::ALWAYS_UPDATE && !bStoreLoaded )
MakeEventListenerNotification_Impl( u"OnVisAreaChanged"_ustr); MakeEventListenerNotification_Impl( u"OnVisAreaChanged"_ustr, aGuard);
} }