embeddedobj/source/inc/oleembobj.hxx | 6 +++++- embeddedobj/source/msole/olepersist.cxx | 13 +++++++------ embeddedobj/source/msole/olevisual.cxx | 17 +++++++++++++---- 3 files changed, 25 insertions(+), 11 deletions(-)
New commits: commit 7122fcf862ce970c230f21ce4c47469cae4fa4f0 Author: Mike Kaganski <[email protected]> AuthorDate: Mon Oct 14 15:14:26 2024 +0500 Commit: Mike Kaganski <[email protected]> CommitDate: Mon Oct 14 21:18:05 2024 +0200 Avoid deadlock because of recursive lock As seen in the following call stacks: Main thread sal3!osl_acquireMutex+0x49 [libre-office-git-repo\sal\osl\w32\mutex.cxx @ 65] emboleobj!osl::Mutex::acquire+0x9 [libre-office-git-repo\include\osl\mutex.hxx @ 63] emboleobj!osl::Guard<osl::Mutex>::{ctor}+0x9 [libre-office-git-repo\include\osl\mutex.hxx @ 144] emboleobj!OleEmbeddedObject::getCurrentState+0x68 [libre-office-git-repombeddedobj\source\msole\oleembed.cxx @ 643] sfxlo!SfxInPlaceClient::IsObjectUIActive+0x23 [libre-office-git-repo\sfx2\source iew\ipclient.cxx @ 847] sfxlo!SfxViewShell::GetUIActiveClient+0x5b [libre-office-git-repo\sfx2\source iew iewsh.cxx @ 2521] sfxlo!SfxDispatcher::FindServer_+0x282 [libre-office-git-repo\sfx2\source sfxlo!SfxDispatcher::GetShellAndSlot_Impl+0x2f [libre-office-git-repo\sfx2\source sfxlo!SfxDispatcher::QueryState+0x5b [libre-office-git-repo\sfx2\source swlo!SwView::CheckReadonlyState+0x64 [libre-office-git-repo\sw\source\uibase\uiview iew.cxx @ 627] swlo!SwView::TimeoutHdl+0x76 [libre-office-git-repo\sw\source\uibase\uiview iew.cxx @ 608] vcllo!Scheduler::CallbackTaskScheduling+0x130e [libre-office-git-repo cl\sourcepp\scheduler.cxx @ 511] vclplug_winlo!WinSalTimer::ImplHandleElapsedTimer+0x2e [libre-office-git-repo cl\winpp\saltimer.cxx @ 167] vclplug_winlo!ImplSalYield+0x14f [libre-office-git-repo cl\winpp\salinst.cxx @ 525] vclplug_winlo!WinSalInstance::DoYield+0xad [libre-office-git-repo cl\winpp\salinst.cxx @ 581] vcllo!ImplYield+0x367 [libre-office-git-repo cl\sourcepp\svapp.cxx @ 393] vcllo!Application::Execute+0xfa [libre-office-git-repo cl\sourcepp\svapp.cxx @ 369] sofficeapp!desktop::Desktop::Main+0x173f [libre-office-git-repo\desktop\sourcepppp.cxx @ 1605] vcllo!ImplSVMain+0xda [libre-office-git-repo cl\sourcepp\svmain.cxx @ 229] sofficeapp!soffice_main+0xf3 [libre-office-git-repo\desktop\sourcepp\sofficemain.cxx @ 94] Request thread sal3!osl_acquireMutex+0x49 [libre-office-git-repo\sal\osl\w32\mutex.cxx @ 65] vclplug_winlo!osl::Mutex::acquire+0xa [libre-office-git-repo\include\osl\mutex.hxx @ 63] vclplug_winlo!SalYieldMutex::doAcquire+0x91 [libre-office-git-repo cl\winpp\salinst.cxx @ 148] emboleobj!SolarMutexReleaser::{dtor}+0xc [libre-office-git-repo\include cl\svapp.hxx @ 1425] emboleobj!OleComponent::GetExtent+0x102 [libre-office-git-repombeddedobj\source\msole\olecomponent.cxx @ 1134] emboleobj!OleEmbeddedObject::getVisualAreaSize+0x23f [libre-office-git-repombeddedobj\source\msole\olevisual.cxx @ 227] emboleobj!OleEmbeddedObject::saveCompleted+0x406 [libre-office-git-repombeddedobj\source\msole\olepersist.cxx @ 1603] comphelper!comphelper::EmbeddedObjectContainer::StoreEmbeddedObject+0x49b [libre-office-git-repo comphelper!comphelper::EmbeddedObjectContainer::InsertEmbeddedObject+0x6f [libre-office-git-repo comphelper!comphelper::EmbeddedObjectContainer::RemoveEmbeddedObject+0x388 [libre-office-git-repo comphelper!comphelper::EmbeddedObjectContainer::RemoveEmbeddedObject+0x46 [libre-office-git-repo swlo!SwOLENode::SavePersistentData+0x20b [libre-office-git-repo\sw\source swlo!SwNodes::ChgNode+0x411 [libre-office-git-repo\sw\source swlo!SwNodes::MoveNodes+0x18e6 [libre-office-git-repo\sw\source swlo!SwUndoSaveContent::MoveToUndoNds+0x1c1 [libre-office-git-repo\sw\source swlo!SwUndoSaveSection::SaveSection+0x559 [libre-office-git-repo\sw\source swlo!SwUndoSaveSection::SaveSection+0x55 [libre-office-git-repo\sw\source swlo!SwUndoFlyBase::DelFly+0x127 [libre-office-git-repo\sw\source swlo!SwUndoDelLayFormat::SwUndoDelLayFormat+0x64 [libre-o ffice-git-repo\sw\source swlo!std::make_unique+0x22 [C:\Program Files (x86)\Microsoft Visual Studio�9\BuildTools\VC\Tools\MSVC .29.30133\include\memory @ 3382] swlo!sw::DocumentLayoutManager::DelLayoutFormat+0x283 [libre-office-git-repo\sw\source swlo!SwTextNode::DestroyAttr+0x82 [libre-office-git-repo\sw\source swlo!SwTextNode::EraseText+0x18f [libre-office-git-repo\sw\source swlo!SwTextNode::DeleteAttributes+0x115 [libre-office-git-repo\sw\source swlo!SwXFrame::dispose+0xdb [libre-office-git-repo\sw\source mscx_uno!`anonymous namespace'::cpp_call+0x710 [libre-office-git-reporidges\source mscx_uno!unoInterfaceProxyDispatch+0x2fa [libre-office-git-reporidges\source binaryurplo!binaryurp::IncomingRequest::execute_throw+0x635 [libre-office-git-repoinaryurp\source\incomingrequest.cxx @ 239] binaryurplo!binaryurp::IncomingRequest::execute+0xbf [libre-office-git-repoinaryurp\source\incomingrequest.cxx @ 79] binaryurplo!request+0x1c [libre-office-git-repoinaryurp\source eader.cxx @ 84] cppu3!cppu_threadpool::JobQueue::enter+0x21e [libre-office-git-repo cppu3!cppu_threadpool::ORequestThread::run+0xa0 [libre-office-git-repo As seen, the request thread has OleEmbeddedObject::saveCompleted and OleEmbeddedObject::getVisualAreaSize on the stack; both acquire OleEmbeddedObject's mutex; but only getVisualAreaSize releases it for the call of OleComponent::GetExtent. The latter releases solar mutex, at which time, the main thread (locking the solar mutex) attempts to call OleEmbeddedObject::getCurrentState, which needs the object's mutex, which is still locked in the request thread. Avoid this, by passing the lock object to the implementation of getVisualAreaSize from saveCompleted. Change-Id: Ie44a20a8c89000e0e951e024c2bfde93cf2cc3f6 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/174891 Tested-by: Jenkins Reviewed-by: Mike Kaganski <[email protected]> diff --git a/embeddedobj/source/inc/oleembobj.hxx b/embeddedobj/source/inc/oleembobj.hxx index 28fe88b02b38..d193ad8c9def 100644 --- a/embeddedobj/source/inc/oleembobj.hxx +++ b/embeddedobj/source/inc/oleembobj.hxx @@ -258,7 +258,8 @@ protected: /// @throws css::uno::Exception void InsertVisualCache_Impl( const css::uno::Reference< css::io::XStream >& xTargetStream, - const css::uno::Reference< css::io::XStream >& xCachedVisualRepresentation ); + const css::uno::Reference< css::io::XStream >& xCachedVisualRepresentation, + osl::ResettableMutexGuard& rGuard); /// @throws css::uno::Exception void RemoveVisualCache_Impl( const css::uno::Reference< css::io::XStream >& xTargetStream ); @@ -453,6 +454,9 @@ public: OUString SAL_CALL getImplementationName() override; sal_Bool SAL_CALL supportsService( const OUString& ServiceName ) override; css::uno::Sequence< OUString > SAL_CALL getSupportedServiceNames() override; + +private: + css::awt::Size getVisualAreaSize_impl(sal_Int64 nAspect, osl::ResettableMutexGuard& guard); }; namespace diff --git a/embeddedobj/source/msole/olepersist.cxx b/embeddedobj/source/msole/olepersist.cxx index 1ddc02b27c8b..9a9a54a7eb8b 100644 --- a/embeddedobj/source/msole/olepersist.cxx +++ b/embeddedobj/source/msole/olepersist.cxx @@ -353,7 +353,8 @@ uno::Reference< io::XStream > OleEmbeddedObject::TryToGetAcceptableFormat_Impl( void OleEmbeddedObject::InsertVisualCache_Impl( const uno::Reference< io::XStream >& xTargetStream, - const uno::Reference< io::XStream >& xCachedVisualRepresentation ) + const uno::Reference< io::XStream >& xCachedVisualRepresentation, + osl::ResettableMutexGuard& rGuard ) { OSL_ENSURE( xTargetStream.is() && xCachedVisualRepresentation.is(), "Invalid arguments!" ); @@ -433,7 +434,7 @@ void OleEmbeddedObject::InsertVisualCache_Impl( const uno::Reference< io::XStrea xTempOutStream->writeBytes( aData ); // get the size - awt::Size aSize = getVisualAreaSize( embed::Aspects::MSOLE_CONTENT ); + awt::Size aSize = getVisualAreaSize_impl(embed::Aspects::MSOLE_CONTENT, rGuard); sal_Int32 nIndex = 0; // write width @@ -1223,7 +1224,7 @@ void OleEmbeddedObject::StoreToLocation_Impl( } } - InsertVisualCache_Impl( xTargetStream, xCachedVisualRepresentation ); + InsertVisualCache_Impl(xTargetStream, xCachedVisualRepresentation, rGuard); } else { @@ -1600,7 +1601,7 @@ void SAL_CALL OleEmbeddedObject::saveCompleted( sal_Bool bUseNew ) { // the call will cache the size in case of success // probably it might need to be done earlier, while the object is in active state - getVisualAreaSize( embed::Aspects::MSOLE_CONTENT ); + getVisualAreaSize_impl(embed::Aspects::MSOLE_CONTENT, aGuard); } catch( const uno::Exception& ) {} @@ -1750,7 +1751,7 @@ void SAL_CALL OleEmbeddedObject::storeOwn() { // the m_xCachedVisualRepresentation must be set or it should be already stored if ( m_xCachedVisualRepresentation.is() ) - InsertVisualCache_Impl( m_xObjectStream, m_xCachedVisualRepresentation ); + InsertVisualCache_Impl(m_xObjectStream, m_xCachedVisualRepresentation, aGuard); else { m_xCachedVisualRepresentation = TryToRetrieveCachedVisualRepresentation_Impl( m_xObjectStream, aGuard ); @@ -1775,7 +1776,7 @@ void SAL_CALL OleEmbeddedObject::storeOwn() { // the call will cache the size in case of success // probably it might need to be done earlier, while the object is in active state - getVisualAreaSize( embed::Aspects::MSOLE_CONTENT ); + getVisualAreaSize_impl(embed::Aspects::MSOLE_CONTENT, aGuard); } catch( const uno::Exception& ) {} diff --git a/embeddedobj/source/msole/olevisual.cxx b/embeddedobj/source/msole/olevisual.cxx index 539933278e45..bcee997c189e 100644 --- a/embeddedobj/source/msole/olevisual.cxx +++ b/embeddedobj/source/msole/olevisual.cxx @@ -155,18 +155,19 @@ void SAL_CALL OleEmbeddedObject::setVisualAreaSize( sal_Int64 nAspect, const awt m_nCachedAspect = nAspect; } -awt::Size SAL_CALL OleEmbeddedObject::getVisualAreaSize( sal_Int64 nAspect ) +awt::Size OleEmbeddedObject::getVisualAreaSize_impl(sal_Int64 nAspect, + osl::ResettableMutexGuard& guard) { // begin wrapping related part ==================== uno::Reference< embed::XEmbeddedObject > xWrappedObject = m_xWrappedObject; if ( xWrappedObject.is() ) { + osl::ResettableMutexGuardScopedReleaser area(guard); // the object was converted to OOo embedded object, the current implementation is now only a wrapper return xWrappedObject->getVisualAreaSize( nAspect ); } // end wrapping related part ==================== - ::osl::ResettableMutexGuard aGuard( m_aMutex ); if ( m_bDisposed ) throw lang::DisposedException(); // TODO @@ -197,7 +198,9 @@ awt::Size SAL_CALL OleEmbeddedObject::getVisualAreaSize( sal_Int64 nAspect ) { // there is no internal cache awt::Size aSize; - aGuard.clear(); + + { // => unguarded + osl::ResettableMutexGuardScopedReleaser area(guard); bool bBackToLoaded = false; @@ -277,7 +280,7 @@ awt::Size SAL_CALL OleEmbeddedObject::getVisualAreaSize( sal_Int64 nAspect ) "No size available!", static_cast< ::cppu::OWeakObject* >(this) ); - aGuard.reset(); + } // <= unguarded m_aCachedSize = aSize; m_nCachedAspect = nAspect; @@ -314,6 +317,12 @@ awt::Size SAL_CALL OleEmbeddedObject::getVisualAreaSize( sal_Int64 nAspect ) return aResult; } +awt::Size SAL_CALL OleEmbeddedObject::getVisualAreaSize( sal_Int64 nAspect ) +{ + osl::ResettableMutexGuard aGuard(m_aMutex); + return getVisualAreaSize_impl(nAspect, aGuard); +} + embed::VisualRepresentation SAL_CALL OleEmbeddedObject::getPreferredVisualRepresentation( sal_Int64 nAspect ) { // begin wrapping related part ====================
