embeddedobj/source/inc/oleembobj.hxx | 4 - embeddedobj/source/msole/oleembed.cxx | 71 +++++++++++---------------- embeddedobj/source/msole/olemisc.cxx | 83 ++++++++++++++++---------------- embeddedobj/source/msole/olepersist.cxx | 33 +++++------- 4 files changed, 90 insertions(+), 101 deletions(-)
New commits: commit 6084a2904d1cb38c47874e260ce0b7ccc7899154 Author: Mike Kaganski <[email protected]> AuthorDate: Thu Aug 22 12:44:50 2024 +0500 Commit: Mike Kaganski <[email protected]> CommitDate: Thu Aug 22 12:06:43 2024 +0200 Make OleEmbeddedObject locking stricter 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 <[email protected]> diff --git a/embeddedobj/source/inc/oleembobj.hxx b/embeddedobj/source/inc/oleembobj.hxx index 62fea5d35702..28fe88b02b38 100644 --- a/embeddedobj/source/inc/oleembobj.hxx +++ b/embeddedobj/source/inc/oleembobj.hxx @@ -219,9 +219,9 @@ protected: #ifdef _WIN32 void SwitchComponentToRunningState_Impl(osl::ResettableMutexGuard& guard); #endif - void MakeEventListenerNotification_Impl( const OUString& aEventName ); + void MakeEventListenerNotification_Impl( const OUString& aEventName, osl::ResettableMutexGuard& guard ); #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(); diff --git a/embeddedobj/source/msole/oleembed.cxx b/embeddedobj/source/msole/oleembed.cxx index f61d8056fdeb..09eac6e5e1b7 100644 --- a/embeddedobj/source/msole/oleembed.cxx +++ b/embeddedobj/source/msole/oleembed.cxx @@ -481,9 +481,7 @@ void SAL_CALL OleEmbeddedObject::changeState( sal_Int32 nNewState ) // will behave after activation sal_Int32 nOldState = m_nObjectState; - aGuard.clear(); - StateChangeNotification_Impl( true, nOldState, nNewState ); - aGuard.reset(); + StateChangeNotification_Impl( true, nOldState, nNewState, aGuard ); try { @@ -498,14 +496,12 @@ void SAL_CALL OleEmbeddedObject::changeState( sal_Int32 nNewState ) // the loaded state must be set before, because of notifications! m_nObjectState = nNewState; - aGuard.clear(); { VerbExecutionControllerGuard aVerbGuard( m_aVerbExecutionController ); - m_pOleComponent->CloseObject(); + ExecUnlocked([p = m_pOleComponent] { p->CloseObject(); }, aGuard); } - StateChangeNotification_Impl( false, nOldState, m_nObjectState ); - aGuard.reset(); + StateChangeNotification_Impl( false, nOldState, m_nObjectState, aGuard ); } 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); m_nObjectState = embed::EmbedStates::RUNNING; - aGuard.clear(); - StateChangeNotification_Impl( false, nOldState, m_nObjectState ); - aGuard.reset(); + StateChangeNotification_Impl( false, nOldState, m_nObjectState, aGuard ); if ( m_pOleComponent && m_bHasSizeToSet ) { - aGuard.clear(); 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; } catch( const uno::Exception& ) {} - aGuard.reset(); } 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 ) { // execute OPEN verb, if object does not reach active state it is an object's problem - aGuard.clear(); - m_pOleComponent->ExecuteVerb( embed::EmbedVerbs::MS_OLEVERB_OPEN ); - aGuard.reset(); + ExecUnlocked([p = m_pOleComponent]() + { p->ExecuteVerb(embed::EmbedVerbs::MS_OLEVERB_OPEN); }, + aGuard); // some objects do not allow to set the size even in running state if ( m_pOleComponent && m_bHasSizeToSet ) { - aGuard.clear(); 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; } catch( uno::Exception& ) {} - aGuard.reset(); } m_nObjectState = nNewState; } else if ( m_nObjectState == embed::EmbedStates::ACTIVE && nNewState == embed::EmbedStates::RUNNING ) { - aGuard.clear(); - m_pOleComponent->CloseObject(); - m_pOleComponent->RunObject(); // Should not fail, the object already was active - aGuard.reset(); + ExecUnlocked( + [p = m_pOleComponent]() + { + p->CloseObject(); + p->RunObject(); // Should not fail, the object already was active + }, + aGuard); m_nObjectState = nNewState; } else @@ -580,8 +577,7 @@ void SAL_CALL OleEmbeddedObject::changeState( sal_Int32 nNewState ) } catch( uno::Exception& ) { - aGuard.clear(); - StateChangeNotification_Impl( false, nOldState, m_nObjectState ); + StateChangeNotification_Impl( false, nOldState, m_nObjectState, aGuard ); throw; } } @@ -857,9 +853,7 @@ void SAL_CALL OleEmbeddedObject::doVerb( sal_Int32 nVerbID ) { // if the target object is in loaded state // it must be switched to running state to execute verb - aGuard.clear(); - changeState( embed::EmbedStates::RUNNING ); - aGuard.reset(); + ExecUnlocked([this]() { changeState(embed::EmbedStates::RUNNING); }, aGuard); } try { @@ -869,11 +863,13 @@ void SAL_CALL OleEmbeddedObject::doVerb( sal_Int32 nVerbID ) // ==== the STAMPIT related solution ============================= m_aVerbExecutionController.StartControlExecution(); - { - osl::ResettableMutexGuardScopedReleaser clearedMutex(aGuard); - m_pOleComponent->ExecuteVerb(nVerbID); - m_pOleComponent->SetHostName(m_aContainerName); - } + ExecUnlocked( + [nVerbID, p = m_pOleComponent, name = m_aContainerName]() + { + p->ExecuteVerb(nVerbID); + p->SetHostName(name); + }, + aGuard); // ==== the STAMPIT related solution ============================= bool bModifiedOnExecution = m_aVerbExecutionController.EndControlExecution_WasModified(); @@ -889,9 +885,7 @@ void SAL_CALL OleEmbeddedObject::doVerb( sal_Int32 nVerbID ) // ==== the STAMPIT related solution ============================= m_aVerbExecutionController.EndControlExecution_WasModified(); - - aGuard.clear(); - StateChangeNotification_Impl( false, nOldState, m_nObjectState ); + StateChangeNotification_Impl( false, nOldState, m_nObjectState, aGuard ); throw; } @@ -1154,10 +1148,7 @@ sal_Int64 SAL_CALL OleEmbeddedObject::getStatus( sal_Int64 nResult = m_nStatus; else if ( m_pOleComponent ) { - { - osl::ResettableMutexGuardScopedReleaser clearedMutex(aGuard); - m_nStatus = m_pOleComponent->GetMiscStatus(nAspect); - } + m_nStatus = ExecUnlocked([p = m_pOleComponent, nAspect] { return p->GetMiscStatus(nAspect); }, aGuard); m_nStatusAspect = nAspect; m_bGotStatus = true; nResult = m_nStatus; diff --git a/embeddedobj/source/msole/olemisc.cxx b/embeddedobj/source/msole/olemisc.cxx index 582ae49de56f..e3159e99fd0f 100644 --- a/embeddedobj/source/msole/olemisc.cxx +++ b/embeddedobj/source/msole/olemisc.cxx @@ -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 ) return; @@ -165,59 +166,59 @@ void OleEmbeddedObject::MakeEventListenerNotification_Impl( const OUString& aEve if ( pContainer == nullptr ) return; - document::EventObject aEvent( static_cast< ::cppu::OWeakObject* >( this ), aEventName ); - comphelper::OInterfaceIteratorHelper2 pIterator(*pContainer); - while (pIterator.hasMoreElements()) + auto proc = [&guard, aEvent = document::EventObject(getXWeak(), aEventName)]( + const uno::Reference<document::XEventListener>& xListener) { 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 -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) + return; + + auto proc + = [bBeforeChange, nOldState, nNewState, &guard, aSource = lang::EventObject(getXWeak())]( + const uno::Reference<embed::XStateChangeListener>& xListener) { - comphelper::OInterfaceContainerHelper2* pContainer = m_pInterfaceContainer->getContainer( - cppu::UnoType<embed::XStateChangeListener>::get()); - if ( pContainer != nullptr ) + try { - lang::EventObject aSource( static_cast< ::cppu::OWeakObject* >( this ) ); - comphelper::OInterfaceIteratorHelper2 pIterator(*pContainer); - - while (pIterator.hasMoreElements()) - { - if ( bBeforeChange ) - { - try - { - static_cast<embed::XStateChangeListener*>(pIterator.next())->changingState( aSource, nOldState, nNewState ); - } - catch( const uno::Exception& ) - { - // even if the listener complains ignore it for now - } - } - else - { - try - { - static_cast<embed::XStateChangeListener*>(pIterator.next())->stateChanged( aSource, nOldState, nNewState ); - } - catch( const uno::Exception& ) - { - // if anything happened it is problem of listener, ignore it - } - } - } + osl::ResettableMutexGuardScopedReleaser area(guard); + if (bBeforeChange) + xListener->changingState(aSource, nOldState, nNewState); + else + xListener->stateChanged(aSource, nOldState, nNewState); } - } + 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 diff --git a/embeddedobj/source/msole/olepersist.cxx b/embeddedobj/source/msole/olepersist.cxx index 65811c31a331..1ddc02b27c8b 100644 --- a/embeddedobj/source/msole/olepersist.cxx +++ b/embeddedobj/source/msole/olepersist.cxx @@ -822,7 +822,7 @@ bool OleEmbeddedObject::SaveObject_Impl() bool OleEmbeddedObject::OnShowWindow_Impl( bool bShow ) { - osl::ClearableMutexGuard aGuard(m_aMutex); + osl::ResettableMutexGuard aGuard(m_aMutex); bool bResult = false; @@ -838,21 +838,19 @@ bool OleEmbeddedObject::OnShowWindow_Impl( bool bShow ) m_nObjectState = embed::EmbedStates::ACTIVE; m_aVerbExecutionController.ObjectIsActive(); - aGuard.clear(); - StateChangeNotification_Impl( false, nOldState, m_nObjectState ); + StateChangeNotification_Impl( false, nOldState, m_nObjectState, aGuard ); } else if ( !bShow && m_nObjectState == embed::EmbedStates::ACTIVE ) { m_nObjectState = embed::EmbedStates::RUNNING; - aGuard.clear(); - StateChangeNotification_Impl( false, nOldState, m_nObjectState ); + StateChangeNotification_Impl( false, nOldState, m_nObjectState, aGuard ); } if ( m_xClientSite.is() ) { try { - m_xClientSite->visibilityChanged( bShow ); + ExecUnlocked([p = m_xClientSite, bShow] { p->visibilityChanged(bShow); }, aGuard); bResult = true; } catch( const uno::Exception& ) @@ -873,6 +871,7 @@ void OleEmbeddedObject::OnIconChanged_Impl() void OleEmbeddedObject::OnViewChanged_Impl() { + osl::ResettableMutexGuard aGuard(m_aMutex); if ( m_bDisposed ) 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 m_xCachedVisualRepresentation.clear(); SaveObject_Impl(); - MakeEventListenerNotification_Impl( "OnVisAreaChanged" ); + MakeEventListenerNotification_Impl( "OnVisAreaChanged", aGuard ); } } @@ -904,6 +903,7 @@ void OleEmbeddedObject::OnViewChanged_Impl() void OleEmbeddedObject::OnClosed_Impl() { + osl::ResettableMutexGuard aGuard(m_aMutex); if ( m_bDisposed ) throw lang::DisposedException(); @@ -911,7 +911,7 @@ void OleEmbeddedObject::OnClosed_Impl() { sal_Int32 nOldState = m_nObjectState; 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 // if the object was NOT modified after storing it can be just copied // 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 ) { @@ -1537,7 +1537,7 @@ void SAL_CALL OleEmbeddedObject::saveCompleted( sal_Bool bUseNew ) } // end wrapping related part ==================== - osl::ClearableMutexGuard aGuard(m_aMutex); + osl::ResettableMutexGuard aGuard(m_aMutex); if ( m_bDisposed ) throw lang::DisposedException(); // TODO @@ -1606,16 +1606,15 @@ void SAL_CALL OleEmbeddedObject::saveCompleted( sal_Bool bUseNew ) {} } - aGuard.clear(); if ( bUseNew ) { - MakeEventListenerNotification_Impl( u"OnSaveAsDone"_ustr); + MakeEventListenerNotification_Impl( u"OnSaveAsDone"_ustr, aGuard); // the object can be changed only on windows // the notification should be done only if the object is not in loaded state 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; #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; @@ -1782,14 +1781,12 @@ void SAL_CALL OleEmbeddedObject::storeOwn() {} } - aGuard.clear(); - - MakeEventListenerNotification_Impl( u"OnSaveDone"_ustr); + MakeEventListenerNotification_Impl( u"OnSaveDone"_ustr, aGuard); // the object can be changed only on Windows // the notification should be done only if the object is not in loaded state if ( m_pOleComponent && m_nUpdateMode == embed::EmbedUpdateModes::ALWAYS_UPDATE && !bStoreLoaded ) - MakeEventListenerNotification_Impl( u"OnVisAreaChanged"_ustr); + MakeEventListenerNotification_Impl( u"OnVisAreaChanged"_ustr, aGuard); }
