embeddedobj/source/inc/oleembobj.hxx | 4 - embeddedobj/source/msole/oleembed.cxx | 71 ++++++++++-------------- embeddedobj/source/msole/olemisc.cxx | 92 ++++++++++++++++---------------- embeddedobj/source/msole/olepersist.cxx | 33 +++++------ 4 files changed, 96 insertions(+), 104 deletions(-)
New commits: commit 0332a5d47470eb14d4107e3797af6aabd2ad25ff Author: Mike Kaganski <[email protected]> AuthorDate: Thu Aug 22 12:44:50 2024 +0500 Commit: Mike Kaganski <[email protected]> CommitDate: Fri Aug 30 16:04:50 2024 +0500 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 e513c57085ce..0e18a168f00e 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 9a5d41abfabc..af0c74a5af6a 100644 --- a/embeddedobj/source/msole/oleembed.cxx +++ b/embeddedobj/source/msole/oleembed.cxx @@ -478,9 +478,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 { @@ -495,14 +493,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 ) { @@ -517,19 +513,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 ) @@ -541,30 +535,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 @@ -577,8 +574,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; } } @@ -854,9 +850,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 { @@ -866,11 +860,13 @@ void SAL_CALL OleEmbeddedObject::doVerb( sal_Int32 nVerbID ) // ==== the STAMPIT related solution ============================= m_aVerbExecutionController.StartControlExecution(); - { - ClearedMutexArea 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(); @@ -886,9 +882,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; } @@ -1151,10 +1145,7 @@ sal_Int64 SAL_CALL OleEmbeddedObject::getStatus( sal_Int64 nResult = m_nStatus; else if ( m_pOleComponent ) { - { - ClearedMutexArea 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 e2af0872f452..db661c0fb385 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 f2f945d0ec24..fe53f5a35c36 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( "OnSaveAsDone"); + MakeEventListenerNotification_Impl( "OnSaveAsDone", 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( "OnVisAreaChanged"); + MakeEventListenerNotification_Impl( "OnVisAreaChanged", 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( "OnSaveDone"); + MakeEventListenerNotification_Impl( "OnSaveDone", 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( "OnVisAreaChanged"); + MakeEventListenerNotification_Impl( "OnVisAreaChanged", aGuard); } commit 702e8a469ea846f50c104a8522640ba94303c9b6 Author: Mike Kaganski <[email protected]> AuthorDate: Thu Aug 22 09:27:34 2024 +0500 Commit: Mike Kaganski <[email protected]> CommitDate: Thu Aug 29 15:45:02 2024 +0500 Make OleEmbeddedObject::GetRidOfComponent safer when releasing lock Thanks Michael Stahl for the useful advise - see https://gerrit.libreoffice.org/c/core/+/171996/comment/4dc20148_5ec861ad/ Change-Id: I16e1876a81a52f291ede49572f012c15d4cd9a6d Reviewed-on: https://gerrit.libreoffice.org/c/core/+/172233 Tested-by: Jenkins Reviewed-by: Mike Kaganski <[email protected]> diff --git a/embeddedobj/source/msole/olemisc.cxx b/embeddedobj/source/msole/olemisc.cxx index ae4a292c37ce..e2af0872f452 100644 --- a/embeddedobj/source/msole/olemisc.cxx +++ b/embeddedobj/source/msole/olemisc.cxx @@ -230,23 +230,26 @@ void OleEmbeddedObject::GetRidOfComponent(osl::ResettableMutexGuard* guard) SaveObject_Impl(); m_pOleComponent->removeCloseListener( m_xClosePreventer ); + // When releasing the guard below, avoid a case when two threads are doing the same; + // store the reference on stack and clear m_pOleComponent in advance + rtl::Reference<OleComponent> pOleComponent(std::move(m_pOleComponent)); try { std::optional<osl::ResettableMutexGuardScopedReleaser> oReleaser; if (guard) oReleaser.emplace(*guard); - m_pOleComponent->close( false ); + pOleComponent->close(false); } catch( const uno::Exception& ) { + m_pOleComponent = std::move(pOleComponent); // TODO: there should be a special listener to wait for component closing // and to notify object, may be object itself can be such a listener m_pOleComponent->addCloseListener( m_xClosePreventer ); throw; } - m_pOleComponent->disconnectEmbeddedObject(); - m_pOleComponent.clear(); + pOleComponent->disconnectEmbeddedObject(); } #else (void)guard;
