From d900a952eb2267717797c7e91326a0ce3f222063 Mon Sep 17 00:00:00 2001 From: Noel Grandin Date: Wed, 26 Jun 2024 11:57:42 +0200 Subject: [PATCH] reduce number of SvxUnoShape listeners on SdrModel when we have a lot of shapes, the number of listeners attached to SdrModel becomes a serious bottleneck because of the number of broadcast/notify function calls. So (1) make SvxUnoShape listen to the SdrObject which achieves the same end, and reduces the bottleneck on the SdrModel. (2) repurpose the maAllIncarnatedObjects set, which tracks all objects linked to the SdrModel, so we can shut down the associated UNO shapes during ClearModel. There is no other good way of doing this, and if we don't do it, we get various use-after-free issues during shutdown. This takes the load time of a large DOCX with lots of shapes, from 35s to 17s on my machine. Change-Id: I34a6ac35c90de8b7103a7373aafe6e9607cc01c4 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/169612 Reviewed-by: Noel Grandin Tested-by: Jenkins --- include/svx/svdmodel.hxx | 10 +++------- include/svx/unoshape.hxx | 2 +- svx/source/svdraw/svdmodel.cxx | 21 ++++++++++++++++++++- svx/source/svdraw/svdobj.cxx | 12 ------------ svx/source/unodraw/unoshape.cxx | 31 +++++++++---------------------- 5 files changed, 33 insertions(+), 43 deletions(-) diff --git a/include/svx/svdmodel.hxx b/include/svx/svdmodel.hxx index 81ec0927db4c..f029d928aa8a 100644 --- a/include/svx/svdmodel.hxx +++ b/include/svx/svdmodel.hxx @@ -42,11 +42,7 @@ #include #include - -#ifdef DBG_UTIL -// SdrObjectLifetimeWatchDog #include -#endif class OutputDevice; class SdrOutliner; @@ -153,8 +149,9 @@ struct SdrModelImpl; class SVXCORE_DLLPUBLIC SdrModel : public SfxBroadcaster, public tools::WeakBase { -#ifdef DBG_UTIL - // SdrObjectLifetimeWatchDog: + // We need to keep track of all the SdrObjects linked to this SdrModel so that we can + // shut them down cleanly, otherwise we end up with use-after-free issues. + // // Use maAllIncarnatedObjects to keep track of all SdrObjects incarnated using this SdrModel // (what is now possible after the paradigm change that a SdrObject stays at a single SdrModel // for it's whole lifetime). @@ -174,7 +171,6 @@ class SVXCORE_DLLPUBLIC SdrModel : public SfxBroadcaster, public tools::WeakBase friend void impAddIncarnatedSdrObjectToSdrModel(SdrObject& rSdrObject, SdrModel& rSdrModel); friend void impRemoveIncarnatedSdrObjectToSdrModel(SdrObject& rSdrObject, SdrModel& rSdrModel); std::unordered_set< SdrObject* > maAllIncarnatedObjects; -#endif protected: std::vector> maMasterPages; std::vector> maPages; diff --git a/include/svx/unoshape.hxx b/include/svx/unoshape.hxx index a72c3beae1b5..4ae84a2bc74e 100644 --- a/include/svx/unoshape.hxx +++ b/include/svx/unoshape.hxx @@ -226,7 +226,7 @@ public: void setMaster( SvxShapeMaster* pMaster ); // SfxListener - virtual void Notify( SfxBroadcaster& rBC, const SfxHint& rHint ) noexcept override; + virtual void Notify( SfxBroadcaster& rBC, const SfxHint& rHint ) noexcept override final; // XAggregation virtual css::uno::Any SAL_CALL queryAggregation( const css::uno::Type& aType ) override; diff --git a/svx/source/svdraw/svdmodel.cxx b/svx/source/svdraw/svdmodel.cxx index 835689a00726..3c43cf2c81bc 100644 --- a/svx/source/svdraw/svdmodel.cxx +++ b/svx/source/svdraw/svdmodel.cxx @@ -44,6 +44,7 @@ #include #include #include +#include #include #include #include @@ -222,7 +223,6 @@ SdrModel::~SdrModel() implDtorClearModel(); #ifdef DBG_UTIL - // SdrObjectLifetimeWatchDog: if(!maAllIncarnatedObjects.empty()) { SAL_WARN("svx", @@ -580,6 +580,25 @@ void SdrModel::ClearModel(bool bCalledFromDestructor) mbInDestruction = true; } + // Disconnect all SvxShape's from their SdrObjects to prevent the SdrObjects + // from hanging around and causing use-after-free. + // Make a copy because it might modified during InvalidateSdrObject calls. + std::vector> allObjs(maAllIncarnatedObjects.begin(), maAllIncarnatedObjects.end()); + for (const auto & pSdrObj : allObjs) + { + uno::Reference xShape = pSdrObj->getWeakUnoShape().get(); + rtl::Reference pSvxShape = dynamic_cast(xShape.get()); + // calling getWeakUnoShape so we don't accidentally create new UNO shapes + if (pSvxShape) + pSvxShape->InvalidateSdrObject(); + else + { + // because some things like SwXShape don't subclass SvxShape + uno::Reference xComp(xShape, uno::UNO_QUERY); + if (xComp) + xComp->dispose(); + } + } sal_Int32 i; // delete all drawing pages sal_Int32 nCount=GetPageCount(); diff --git a/svx/source/svdraw/svdobj.cxx b/svx/source/svdraw/svdobj.cxx index 6c8330185f0a..1b768aee12d9 100644 --- a/svx/source/svdraw/svdobj.cxx +++ b/svx/source/svdraw/svdobj.cxx @@ -321,8 +321,6 @@ void SdrObject::SetBoundRectDirty() resetOutRectangle(); } -#ifdef DBG_UTIL -// SdrObjectLifetimeWatchDog: void impAddIncarnatedSdrObjectToSdrModel(SdrObject& rSdrObject, SdrModel& rSdrModel) { rSdrModel.maAllIncarnatedObjects.insert(&rSdrObject); @@ -334,7 +332,6 @@ void impRemoveIncarnatedSdrObjectToSdrModel(SdrObject& rSdrObject, SdrModel& rSd assert(false && "SdrObject::~SdrObject: Destructed incarnation of SdrObject not member of this SdrModel (!)"); } } -#endif SdrObject::SdrObject(SdrModel& rSdrModel) : mpFillGeometryDefiningShape(nullptr) @@ -369,10 +366,7 @@ SdrObject::SdrObject(SdrModel& rSdrModel) m_bIs3DObj=false; m_bMarkProt=false; m_bIsUnoObj=false; -#ifdef DBG_UTIL - // SdrObjectLifetimeWatchDog: impAddIncarnatedSdrObjectToSdrModel(*this, getSdrModelFromSdrObject()); -#endif } SdrObject::SdrObject(SdrModel& rSdrModel, SdrObject const & rSource) @@ -440,10 +434,7 @@ SdrObject::SdrObject(SdrModel& rSdrModel, SdrObject const & rSource) m_pGrabBagItem.reset(); if (rSource.m_pGrabBagItem!=nullptr) m_pGrabBagItem.reset(rSource.m_pGrabBagItem->Clone()); -#ifdef DBG_UTIL - // SdrObjectLifetimeWatchDog: impAddIncarnatedSdrObjectToSdrModel(*this, getSdrModelFromSdrObject()); -#endif } SdrObject::~SdrObject() @@ -470,10 +461,7 @@ SdrObject::~SdrObject() m_pGrabBagItem.reset(); mpProperties.reset(); mpViewContact.reset(); -#ifdef DBG_UTIL - // SdrObjectLifetimeWatchDog: impRemoveIncarnatedSdrObjectToSdrModel(*this, getSdrModelFromSdrObject()); -#endif } void SdrObject::acquire() noexcept diff --git a/svx/source/unodraw/unoshape.cxx b/svx/source/unodraw/unoshape.cxx index 5c3807817a6f..b547db4b436a 100644 --- a/svx/source/unodraw/unoshape.cxx +++ b/svx/source/unodraw/unoshape.cxx @@ -192,7 +192,7 @@ SvxShape::~SvxShape() noexcept if ( mxSdrObject ) { - EndListening(mxSdrObject->getSdrModelFromSdrObject()); + mxSdrObject->RemoveListener(*this); mxSdrObject->setUnoShape(nullptr); mxSdrObject.clear(); } @@ -205,7 +205,7 @@ void SvxShape::InvalidateSdrObject() { if(mxSdrObject) { - EndListening(mxSdrObject->getSdrModelFromSdrObject()); + mxSdrObject->RemoveListener(*this); mxSdrObject.clear(); } }; @@ -285,7 +285,7 @@ void SvxShape::impl_construct() { if ( HasSdrObject() ) { - StartListening(GetSdrObject()->getSdrModelFromSdrObject()); + GetSdrObject()->AddListener(*this); impl_initFromSdrObject(); } } @@ -355,14 +355,14 @@ void SvxShape::Create( SdrObject* pNewObj, SvxDrawPage* /*pNewPage*/ ) if( HasSdrObject() ) { - EndListening( GetSdrObject()->getSdrModelFromSdrObject() ); + GetSdrObject()->RemoveListener( *this ); } mxSdrObject = pNewObj; if( HasSdrObject() ) { - StartListening( GetSdrObject()->getSdrModelFromSdrObject() ); + GetSdrObject()->AddListener( *this ); } OSL_ENSURE( !mbIsMultiPropertyCall, "SvxShape::Create: hmm?" ); @@ -929,8 +929,7 @@ void SvxShape::Notify( SfxBroadcaster&, const SfxHint& rHint ) noexcept return; const SdrHint* pSdrHint = static_cast(&rHint); // #i55919# SdrHintKind::ObjectChange is only interesting if it's for this object - if ((pSdrHint->GetKind() != SdrHintKind::ModelCleared) && - (pSdrHint->GetKind() != SdrHintKind::ObjectChange || pSdrHint->GetObject() != mxSdrObject.get() )) + if (pSdrHint->GetKind() != SdrHintKind::ObjectChange || pSdrHint->GetObject() != mxSdrObject.get()) return; // prevent object being deleted from under us @@ -938,24 +937,12 @@ void SvxShape::Notify( SfxBroadcaster&, const SfxHint& rHint ) noexcept uno::Reference< uno::XInterface > xSelf( mxSdrObject->getWeakUnoShape() ); if( !xSelf.is() ) { - EndListening(mxSdrObject->getSdrModelFromSdrObject()); + mxSdrObject->RemoveListener(*this); mxSdrObject.clear(); return; } - if (pSdrHint->GetKind() == SdrHintKind::ObjectChange) - { - updateShapeKind(); - } - else // (pSdrHint->GetKind() == SdrHintKind::ModelCleared) - { - EndListening(mxSdrObject->getSdrModelFromSdrObject()); - mxSdrObject->setUnoShape(nullptr); - mxSdrObject.clear(); - - if(!mpImpl->mbDisposing) - dispose(); - } + updateShapeKind(); } // XShape @@ -1190,7 +1177,7 @@ void SAL_CALL SvxShape::dispose() if (!pObject) return; - EndListening( pObject->getSdrModelFromSdrObject() ); + pObject->RemoveListener( *this ); if ( pObject->IsInserted() && pObject->getSdrPageFromSdrObject() ) {