drawinglayer/source/primitive2d/BufferedDecompositionFlusher.cxx | 47 +++++++--- include/drawinglayer/primitive2d/BufferedDecompositionFlusher.hxx | 11 +- include/drawinglayer/primitive2d/baseprimitive2d.hxx | 2 3 files changed, 44 insertions(+), 16 deletions(-)
New commits: commit f2e8dc899d3d6b6ebaf4ffd4c93c154b25d88f76 Author: Caolán McNamara <[email protected]> AuthorDate: Sun Oct 19 21:13:41 2025 +0100 Commit: Caolán McNamara <[email protected]> CommitDate: Thu Oct 23 15:46:19 2025 +0200 prevent BufferedDecomposition[Group]Primitive2D from being resurrected The BufferedDecompositionFlusher thread is waiting for the SolarMutex and has accumulated a bunch of rtl::References to Primitive2Ds in aRemoved[1|2] created from direct pointers to Primitive2Ds during the earlier phase. Thread 6 (Thread 0x7fd2e27fe6c0 (LWP 3729389)): #0 0x00007fd313d3012b in () at /lib/x86_64-linux-gnu/libc.so.6 #1 0x00007fd313d364da in pthread_mutex_lock () at /lib/x86_64-linux-gnu/libc.so.6 #2 0x00007fd3140a75f1 in osl_acquireMutex(oslMutex) (pMutex=0x556f87223470) at sal/osl/unx/mutex.cxx:93 nRet = 32723 #3 0x00007fd30e02be53 in osl::Mutex::acquire() (this=0x556f87223968) at include/osl/mutex.hxx:63 pInst = 0x556f87223830 __PRETTY_FUNCTION__ = "virtual void SvpSalYieldMutex::doAcquire(sal_uInt32)" #4 SvpSalYieldMutex::doAcquire(unsigned int) (this=0x556f87223960, nLockCount=1) at vcl/headless/svpinst.cxx:376 pInst = 0x556f87223830 __PRETTY_FUNCTION__ = "virtual void SvpSalYieldMutex::doAcquire(sal_uInt32)" #5 0x00007fd312bcd4fc in comphelper::SolarMutex::acquire(unsigned int) (this=<optimized out>, nLockCount=nLockCount@entry=1) at include/comphelper/solarmutex.hxx:86 __PRETTY_FUNCTION__ = "void comphelper::SolarMutex::acquire(sal_uInt32)" #6 0x00007fd312bcd6bd in osl::Guard<comphelper::SolarMutex>::Guard(comphelper::SolarMutex*) (this=<optimized out>, pT_=<optimized out>) at include/osl/mutex.hxx:137 #7 0x00007fd312bccd53 in drawinglayer::primitive2d::BufferedDecompositionFlusher::run() (this=0x7fd2d4159230) at drawinglayer/source/primitive2d/BufferedDecompositionFlusher.cxx:154 aGuard = {pT = 0x556f87223960} aNow = {__d = {__r = 64556205175663173}} aRemoved1 = std::__debug::vector of length 1155, capacity 2048 = The active thread 1 asserts that the reference count of a SimpleReferenceObject isn't zero during dtor. Thread 1 (Thread 0x7fd2e2fff6c0 (LWP 3726667)): #21 0x00007fd313c2760a in salhelper::SimpleReferenceObject::~SimpleReferenceObject() (this=this@entry=0x7fd2d62f51f0, __in_chrg=<optimized out>) at salhelper/source/simplereferenceobject.cxx:29 #22 0x00007fd312bcbc57 in drawinglayer::primitive2d::BasePrimitive2D::~BasePrimitive2D() (this=this@entry=0x7fd2d62f51f0, __in_chrg=<optimized out>) at drawinglayer/source/primitive2d/baseprimitive2d.cxx:34 #23 0x00007fd312bcc368 in drawinglayer::primitive2d::BufferedDecompositionPrimitive2D::~BufferedDecompositionPrimitive2D() (this=this@entry=0x7fd2d62f51f0, __in_chrg=<optimized out>) at drawinglayer/source/primitive2d/BufferedDecompositionPrimitive2D.cxx:69 #24 0x00007fd310300136 in drawinglayer::primitive2d::SdrGrafPrimitive2D::~SdrGrafPrimitive2D() (this=0x7fd2d62f51f0, __in_chrg=<optimized out>) at svx/inc/sdr/primitive2d/sdrgrafprimitive2d.hxx:29 #25 0x00007fd310300141 in drawinglayer::primitive2d::SdrGrafPrimitive2D::~SdrGrafPrimitive2D() (this=0x7fd2d62f51f0, __in_chrg=<optimized out>) at svx/inc/sdr/primitive2d/sdrgrafprimitive2d.hxx:29 #26 0x00007fd312bcc5d3 in salhelper::SimpleReferenceObject::release() (this=<optimized out>) at include/salhelper/simplereferenceobject.hxx:83 #27 0x00007fd312bd1e63 in rtl::Reference<drawinglayer::primitive2d::BasePrimitive2D>::~Reference() (this=0x7fd2d62f4b10, __in_chrg=<optimized out>) at include/rtl/ref.hxx:126 #28 std::destroy_at<rtl::Reference<drawinglayer::primitive2d::BasePrimitive2D> >(rtl::Reference<drawinglayer::primitive2d::BasePrimitive2D>*) (__location=0x7fd2d62f4b10) at /usr/include/c++/12/bits/stl_construct.h:88 #29 std::_Destroy<rtl::Reference<drawinglayer::primitive2d::BasePrimitive2D> >(rtl::Reference<drawinglayer::primitive2d::BasePrimitive2D>*) (__pointer=0x7fd2d62f4b10) at /usr/include/c++/12/bits/stl_construct.h:149 So, what if a BufferedDecompositionPrimitive2D was registered with BufferedDecompositionFlusher, so a direct pointer to it exists. On Thread 0 the BufferedDecompositionPrimitive2D ref count hits 0, dtoring starts. Meanwhile Thread 6 merrily spins its loop, creates a rtl::Reference from the BufferedDecompositionPrimitive2D, ref count goes back up to 1. BufferedDecompositionPrimitive2D::dtor gets around to unregistering itself from BufferedDecompositionFlusher, but that doesn't matter because the rtl::Reference to the primitive already exists in aRemoved, dtor hits assert that refcount isn't 0 Change-Id: Iac0a03bb7cbadf949ba1ac00d69cf15cc2505e18 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/192674 Tested-by: Jenkins Tested-by: Caolán McNamara <[email protected]> Reviewed-by: Caolán McNamara <[email protected]> diff --git a/drawinglayer/source/primitive2d/BufferedDecompositionFlusher.cxx b/drawinglayer/source/primitive2d/BufferedDecompositionFlusher.cxx index 2d0e4a3834a5..fed3b509178f 100644 --- a/drawinglayer/source/primitive2d/BufferedDecompositionFlusher.cxx +++ b/drawinglayer/source/primitive2d/BufferedDecompositionFlusher.cxx @@ -90,28 +90,36 @@ void BufferedDecompositionFlusher::updateImpl(const BufferedDecompositionPrimiti { std::unique_lock l(maMutex); if (!mbShutdown) - maRegistered1.insert(const_cast<BufferedDecompositionPrimitive2D*>(p)); + { + unotools::WeakReference<BufferedDecompositionPrimitive2D> xRef( + const_cast<BufferedDecompositionPrimitive2D*>(p)); + maRegistered1.insert({ p, std::move(xRef) }); + } } void BufferedDecompositionFlusher::updateImpl(const BufferedDecompositionGroupPrimitive2D* p) { std::unique_lock l(maMutex); if (!mbShutdown) - maRegistered2.insert(const_cast<BufferedDecompositionGroupPrimitive2D*>(p)); + { + unotools::WeakReference<BufferedDecompositionGroupPrimitive2D> xRef( + const_cast<BufferedDecompositionGroupPrimitive2D*>(p)); + maRegistered2.insert({ p, std::move(xRef) }); + } } void BufferedDecompositionFlusher::removeImpl(const BufferedDecompositionPrimitive2D* p) { std::unique_lock l(maMutex); if (!mbShutdown) - maRegistered1.erase(const_cast<BufferedDecompositionPrimitive2D*>(p)); + maRegistered1.erase(p); } void BufferedDecompositionFlusher::removeImpl(const BufferedDecompositionGroupPrimitive2D* p) { std::unique_lock l(maMutex); if (!mbShutdown) - maRegistered2.erase(const_cast<BufferedDecompositionGroupPrimitive2D*>(p)); + maRegistered2.erase(p); } void SAL_CALL BufferedDecompositionFlusher::run() @@ -129,9 +137,12 @@ void SAL_CALL BufferedDecompositionFlusher::run() break; for (auto it = maRegistered1.begin(); it != maRegistered1.end();) { - if (aNow - (*it)->maLastAccess.load() > std::chrono::seconds(10)) + rtl::Reference<BufferedDecompositionPrimitive2D> xPrimitive = it->second.get(); + if (!xPrimitive) + it = maRegistered1.erase(it); + else if (aNow - xPrimitive->maLastAccess.load() > std::chrono::seconds(10)) { - aRemoved1.push_back(*it); + aRemoved1.push_back(std::move(xPrimitive)); it = maRegistered1.erase(it); } else @@ -139,9 +150,12 @@ void SAL_CALL BufferedDecompositionFlusher::run() } for (auto it = maRegistered2.begin(); it != maRegistered2.end();) { - if (aNow - (*it)->maLastAccess.load() > std::chrono::seconds(10)) + rtl::Reference<BufferedDecompositionGroupPrimitive2D> xPrimitive = it->second.get(); + if (!xPrimitive) + it = maRegistered2.erase(it); + else if (aNow - xPrimitive->maLastAccess.load() > std::chrono::seconds(10)) { - aRemoved2.push_back(*it); + aRemoved2.push_back(std::move(xPrimitive)); it = maRegistered2.erase(it); } else @@ -153,10 +167,19 @@ void SAL_CALL BufferedDecompositionFlusher::run() // some parts of skia do not take kindly to being accessed from multiple threads osl::Guard<comphelper::SolarMutex> aGuard(comphelper::SolarMutex::get()); - for (const auto& r : aRemoved1) - r->setBuffered2DDecomposition(nullptr); - for (const auto& r : aRemoved2) - r->setBuffered2DDecomposition(Primitive2DContainer{}); + for (const auto& xPrim : aRemoved1) + { + xPrim->setBuffered2DDecomposition(nullptr); + } + for (const auto& xPrim : aRemoved2) + { + xPrim->setBuffered2DDecomposition(Primitive2DContainer{}); + } + + // Clear these while under the SolarMutex, just in case we are the sole surviving reference, + // and we might trigger destruction of related vcl resources. + aRemoved1.clear(); + aRemoved2.clear(); } wait(TimeValue(2, 0)); diff --git a/include/drawinglayer/primitive2d/BufferedDecompositionFlusher.hxx b/include/drawinglayer/primitive2d/BufferedDecompositionFlusher.hxx index 2206cc600dc0..8b4699d3dce0 100644 --- a/include/drawinglayer/primitive2d/BufferedDecompositionFlusher.hxx +++ b/include/drawinglayer/primitive2d/BufferedDecompositionFlusher.hxx @@ -22,7 +22,8 @@ #include <drawinglayer/primitive2d/BufferedDecompositionGroupPrimitive2D.hxx> #include <drawinglayer/primitive2d/BufferedDecompositionPrimitive2D.hxx> #include <osl/thread.hxx> -#include <unordered_set> +#include <unotools/weakref.hxx> +#include <unordered_map> namespace drawinglayer::primitive2d { @@ -49,8 +50,12 @@ private: void removeImpl(const BufferedDecompositionGroupPrimitive2D*); // Explicitly not using rtl::Reference because they are removed from here when they destruct. - std::unordered_set<BufferedDecompositionPrimitive2D*> maRegistered1; - std::unordered_set<BufferedDecompositionGroupPrimitive2D*> maRegistered2; + std::unordered_map<const BufferedDecompositionPrimitive2D*, + unotools::WeakReference<BufferedDecompositionPrimitive2D>> + maRegistered1; + std::unordered_map<const BufferedDecompositionGroupPrimitive2D*, + unotools::WeakReference<BufferedDecompositionGroupPrimitive2D>> + maRegistered2; std::mutex maMutex; bool mbShutdown{ false }; }; diff --git a/include/drawinglayer/primitive2d/baseprimitive2d.hxx b/include/drawinglayer/primitive2d/baseprimitive2d.hxx index b53fd73d7711..0937c57a1e72 100644 --- a/include/drawinglayer/primitive2d/baseprimitive2d.hxx +++ b/include/drawinglayer/primitive2d/baseprimitive2d.hxx @@ -118,7 +118,7 @@ namespace drawinglayer::primitive2d for view-independent primitives which are defined by not using ViewInformation2D in their get2DDecomposition/getB2DRange implementations. */ -class DRAWINGLAYERCORE_DLLPUBLIC BasePrimitive2D : public salhelper::SimpleReferenceObject +class DRAWINGLAYERCORE_DLLPUBLIC BasePrimitive2D : public cppu::OWeakObject { bool mbVisible = true;
