From b14f8b9dfa52fca0c64a88dc041365eb3fd0b385 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Caol=C3=A1n=20McNamara?= Date: Wed, 10 Jun 2026 10:41:33 +0000 Subject: [PATCH] stop the fill-bitmap link listening to its host's SvtBroadcaster MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's annoyingly difficult to remove the links from the broadcaster, drop that approach and do it a more direct and less reentrant way. Get ~SwFormat and SwContentNode::ImplDestroy to report the death to the tracker instead. A flag on SwDoc keeps the common no-links case in those destructors to a single test. During ~SwDoc the hosts stay silent: the links administration manager can already be unreachable through the half-destroyed document, and ~FillBitmapLinkTracker releases whatever is left without touching the dead hosts. SwContentNode::ImplDestroy skips disconnected nodes, the stack-built working copies like SwTextNode::Update's SwContentNodeTmp that have no nodes-array and no SwDoc. A node deleted for real still reaches its SwDoc, since SwNodes::RemoveNode deletes nodes while they are still in the array. Signed-off-by: Caolán McNamara Change-Id: Ibbf15e4e17a7a08aa029c9d5df8f40b7a4a13e04 Reviewed-on: https://gerrit.collaboraoffice.com/c/online/+/4282 Reviewed-by: Miklos Vajna Tested-by: Jenkins CPCI (cherry picked from commit 38ac5c0e6c7baf4de27498e5428d6279c753c97e) Reviewed-on: https://gerrit.libreoffice.org/c/core/+/208603 Tested-by: Jenkins Reviewed-by: Xisco Fauli --- sw/inc/IDocumentLinksAdministration.hxx | 6 +- sw/inc/doc.hxx | 5 ++ .../extras/tiledrendering/tiledrendering2.cxx | 63 +++++++++++++++++++ sw/source/core/attr/format.cxx | 5 ++ .../DocumentLinksAdministrationManager.cxx | 2 + sw/source/core/doc/FillBitmapLinkTracker.cxx | 41 +++--------- sw/source/core/doc/docnew.cxx | 1 + sw/source/core/docnode/node.cxx | 9 +++ sw/source/core/inc/FillBitmapLinkTracker.hxx | 19 +++--- 9 files changed, 104 insertions(+), 47 deletions(-) diff --git a/sw/inc/IDocumentLinksAdministration.hxx b/sw/inc/IDocumentLinksAdministration.hxx index f05db9b83e36..39c28645ad38 100644 --- a/sw/inc/IDocumentLinksAdministration.hxx +++ b/sw/inc/IDocumentLinksAdministration.hxx @@ -71,9 +71,9 @@ namespace sfx2 { class SvLinkSource; class LinkManager; } // Called by SwFormat::SwClientNotify / SwContentNode::SwClientNotify // when XATTR_FILLBITMAP changes in the host's own attribute set, and - // from the registered SvBaseLink itself on host-Dying. rNewURL is the - // deferred origin URL of the new item, or empty if the item is absent, - // already resolved, or the host is being destroyed. + // by the host's destructor. rNewURL is the deferred origin URL of the + // new item, or empty if the item is absent, already resolved, or the + // host is being destroyed. virtual void onFillBitmapURLChanged(SwFormat& rFormat, std::u16string_view rNewURL) = 0; virtual void onFillBitmapURLChanged(SwContentNode& rNode, std::u16string_view rNewURL) = 0; diff --git a/sw/inc/doc.hxx b/sw/inc/doc.hxx index 684902315065..1131acf12ca5 100644 --- a/sw/inc/doc.hxx +++ b/sw/inc/doc.hxx @@ -333,6 +333,7 @@ private: bool mbInWriterfilterImport : 1; //< TRUE: writerfilter import (DOCX,RTF) bool mbUpdateTOX : 1; //< TRUE: After loading document, update TOX. bool mbInLoadAsynchron : 1; //< TRUE: Document is in the process of being loaded asynchronously. + bool mbHasFillBitmapLinks : 1; //< TRUE: the fill-bitmap link tracker has registered links. bool mbIsAutoFormatRedline : 1; //< TRUE: Redlines are recorded by Autoformat. bool mbOLEPrtNotifyPending : 1; /**< TRUE: Printer has changed. At creation of View notification of OLE-Objects PrtOLENotify() is required. */ @@ -1429,6 +1430,10 @@ public: bool IsInLoadAsynchron() const { return mbInLoadAsynchron; } void SetInLoadAsynchron( bool bFlag ) { mbInLoadAsynchron = bFlag; } + // True while the fill-bitmap link tracker holds at least one link. + bool HasFillBitmapLinks() const { return mbHasFillBitmapLinks; } + void SetHasFillBitmapLinks( bool bFlag ) { mbHasFillBitmapLinks = bFlag; } + // For Drag&Move: (e.g. allow "moving" of RefMarks) bool IsCopyIsMove() const { return mbCopyIsMove; } void SetCopyIsMove( bool bFlag ) { mbCopyIsMove = bFlag; } diff --git a/sw/qa/extras/tiledrendering/tiledrendering2.cxx b/sw/qa/extras/tiledrendering/tiledrendering2.cxx index 5fe2f9fdcd53..79d043cc6884 100644 --- a/sw/qa/extras/tiledrendering/tiledrendering2.cxx +++ b/sw/qa/extras/tiledrendering/tiledrendering2.cxx @@ -32,7 +32,9 @@ #include #include +#include #include +#include #include #include #include @@ -41,6 +43,7 @@ #include #include #include +#include #include #include #include @@ -938,6 +941,66 @@ CPPUNIT_TEST_FIXTURE(SwTiledRenderingTest, testParagraphStyleBackgroundImageRemo *pDevice, tools::Rectangle(Point(0, 0), pWrtShell->GetLayout()->getFrameArea().SSize())); } +CPPUNIT_TEST_FIXTURE(SwTiledRenderingTest, testParagraphStyleDeleteReleasesBackgroundImageLink) +{ + // Deleting the paragraph style that carries the remote background must + // drop the style's entry from the link manager, so Edit > Links no + // longer offers a link whose target style is gone. + comphelper::LibreOfficeKit::setActive(false); + + uno::Sequence aParams = { + comphelper::makePropertyValue(u"UpdateDocMode"_ustr, + sal_Int16(css::document::UpdateDocMode::NO_UPDATE)), + }; + loadFromFile(u"paragraph-style-background-link.fodt", aParams); + + SwDocShell* pDocShell = getSwDocShell(); + sfx2::LinkManager& rLinkMgr + = pDocShell->GetDoc()->getIDocumentLinksAdministration().GetLinkManager(); + CPPUNIT_ASSERT_MESSAGE("paragraph-style background image link should be registered", + !rLinkMgr.GetLinks().empty()); + + uno::Reference xSupplier(mxComponent, uno::UNO_QUERY); + uno::Reference xParaStyles( + xSupplier->getStyleFamilies()->getByName(u"ParagraphStyles"_ustr), uno::UNO_QUERY); + xParaStyles->removeByName(u"LinkBackground"_ustr); + + CPPUNIT_ASSERT_MESSAGE("deleting the style should release its link", + rLinkMgr.GetLinks().empty()); +} + +CPPUNIT_TEST_FIXTURE(SwTiledRenderingTest, testParagraphDeleteReleasesBackgroundImageLink) +{ + // Joining away a paragraph with its own remote background must drop that + // paragraph's entry from the link manager once the undo history no + // longer keeps the deleted paragraph alive. + comphelper::LibreOfficeKit::setActive(false); + + uno::Sequence aParams = { + comphelper::makePropertyValue(u"UpdateDocMode"_ustr, + sal_Int16(css::document::UpdateDocMode::NO_UPDATE)), + }; + loadFromFile(u"two-paragraph-background-links.fodt", aParams); + + SwDocShell* pDocShell = getSwDocShell(); + sfx2::LinkManager& rLinkMgr + = pDocShell->GetDoc()->getIDocumentLinksAdministration().GetLinkManager(); + CPPUNIT_ASSERT_EQUAL(size_t(2), rLinkMgr.GetLinks().size()); + + // forward-delete at the end of the first paragraph joins the second + // paragraph into it + SwWrtShell* pWrtShell = pDocShell->GetWrtShell(); + pWrtShell->SttEndDoc(/*bStt=*/true); + pWrtShell->EndPara(); + pWrtShell->DelRight(); + + // the deleted paragraph may survive in the undo history and keep its + // link until the history lets go of it + pDocShell->GetDoc()->GetIDocumentUndoRedo().DelAllUndoObj(); + + CPPUNIT_ASSERT_EQUAL(size_t(1), rLinkMgr.GetLinks().size()); +} + CPPUNIT_TEST_FIXTURE(SwTiledRenderingTest, testBackgroundImageLinkSurvivesUpdate) { // Resolving a paragraph's remote background must write the graphic back diff --git a/sw/source/core/attr/format.cxx b/sw/source/core/attr/format.cxx index 27bd2396dbad..49f9d19938e0 100644 --- a/sw/source/core/attr/format.cxx +++ b/sw/source/core/attr/format.cxx @@ -212,6 +212,11 @@ void SwFormat::Destr() SwFormat::~SwFormat() { + // release any fill-bitmap link that has this format as its target + SwDoc& rDoc = GetDoc(); + if (rDoc.HasFillBitmapLinks() && !rDoc.IsInDtor()) + rDoc.getIDocumentLinksAdministration().onFillBitmapURLChanged(*this, u""); + Destr(); } diff --git a/sw/source/core/doc/DocumentLinksAdministrationManager.cxx b/sw/source/core/doc/DocumentLinksAdministrationManager.cxx index bdc42b3a0505..afd9e25c8bc9 100644 --- a/sw/source/core/doc/DocumentLinksAdministrationManager.cxx +++ b/sw/source/core/doc/DocumentLinksAdministrationManager.cxx @@ -424,12 +424,14 @@ void DocumentLinksAdministrationManager::onFillBitmapURLChanged(SwFormat& rForma std::u16string_view rNewURL) { m_pFillBitmapLinkTracker->onFillBitmapURLChanged(rFormat, rNewURL); + m_rDoc.SetHasFillBitmapLinks(m_pFillBitmapLinkTracker->hasLinks()); } void DocumentLinksAdministrationManager::onFillBitmapURLChanged(SwContentNode& rNode, std::u16string_view rNewURL) { m_pFillBitmapLinkTracker->onFillBitmapURLChanged(rNode, rNewURL); + m_rDoc.SetHasFillBitmapLinks(m_pFillBitmapLinkTracker->hasLinks()); } DocumentLinksAdministrationManager::~DocumentLinksAdministrationManager() diff --git a/sw/source/core/doc/FillBitmapLinkTracker.cxx b/sw/source/core/doc/FillBitmapLinkTracker.cxx index 74fd1ae5c781..55569a72b071 100644 --- a/sw/source/core/doc/FillBitmapLinkTracker.cxx +++ b/sw/source/core/doc/FillBitmapLinkTracker.cxx @@ -15,7 +15,6 @@ #include #include #include -#include #include #include #include @@ -42,15 +41,10 @@ template <> struct HostOps }; // One link per XFillBitmapItem-owner that currently has a deferred -// XFillBitmapItem. The link listens to the XFillBitmapItem-owner for -// SfxHintId::Dying and asks the tracker to drop it, so links never outlive -// their host. -// -// When the remote graphic resolves, DataChanged writes the fetched bitmap back -// via setAttr; XFillBitmapItem-owner fires SwClientNotify, which calls back -// through onFillBitmapURLChanged with an empty URL and the tracker removes us. -template -class SwHostFillBitmapLink final : public sfx2::SvBaseLink, public SvtListener +// XFillBitmapItem. The owner's destructor reports its death to the tracker, +// which releases the link while m_rHost is still valid, so the link itself +// observes nothing and m_rHost cannot dangle. +template class SwHostFillBitmapLink final : public sfx2::SvBaseLink { sw::FillBitmapLinkTracker& m_rTracker; Host& m_rHost; @@ -64,7 +58,6 @@ public: , m_rHost(rHost) , m_rLinkMgr(rLinkMgr) { - StartListening(rHost.GetNotifier()); } UpdateResult DataChanged(const OUString& rMimeType, const css::uno::Any& rValue) override @@ -84,21 +77,14 @@ public: // Write the resolved graphic back to this one host. The write // re-enters onFillBitmapURLChanged with a now-resolved item, so tell // the tracker to ignore that notification for this host and keep us - // registered (like the svx tracker's resolved links), so our - // LinkManager entry survives SvBaseLink::Update. + // registered (like the svx tracker's resolved links). Removing us + // here instead would free the link inside SvBaseLink::Update, which + // still reads its members after DataChanged returns. m_rTracker.setUpdatingHost(&m_rHost); HostOps::setAttr(m_rHost, XFillBitmapItem(aName, aGrf)); m_rTracker.clearUpdatingHost(); return SUCCESS; } - - void Notify(const SfxHint& rHint) override - { - if (rHint.GetId() != SfxHintId::Dying) - return; - tools::SvRef xSelf(this); - m_rTracker.onFillBitmapURLChanged(m_rHost, OUString()); - } }; } @@ -117,14 +103,6 @@ FillBitmapLinkTracker::~FillBitmapLinkTracker() for (const auto& rEntry : m_aNodeLinks) m_rLinkMgr.Remove(rEntry.second.get()); m_aNodeLinks.clear(); - drainPendingReleases(); -} - -void FillBitmapLinkTracker::drainPendingReleases() -{ - // Release through a local so the member is already empty if a freed link parks another: - std::vector> aReleasing; - aReleasing.swap(m_aPendingRelease); } template @@ -135,15 +113,10 @@ void FillBitmapLinkTracker::onURLChangedImpl(std::mapsecond.get()); - // Park rather than drop, since inside rHost's Dying broadcast the link must outlive the - // BroadcasterDying call still to come and so cannot be freed here: - m_aPendingRelease.push_back(std::move(it->second)); rMap.erase(it); } if (rNewURL.empty()) diff --git a/sw/source/core/doc/docnew.cxx b/sw/source/core/doc/docnew.cxx index f20fca5baaff..ee86cb42c96c 100644 --- a/sw/source/core/doc/docnew.cxx +++ b/sw/source/core/doc/docnew.cxx @@ -271,6 +271,7 @@ SwDoc::SwDoc() mbInWriterfilterImport(false), mbUpdateTOX(false), mbInLoadAsynchron(false), + mbHasFillBitmapLinks(false), mbIsAutoFormatRedline(false), mbOLEPrtNotifyPending(false), mbAllOLENotify(false), diff --git a/sw/source/core/docnode/node.cxx b/sw/source/core/docnode/node.cxx index 3548e6cfa827..3a6214cafd87 100644 --- a/sw/source/core/docnode/node.cxx +++ b/sw/source/core/docnode/node.cxx @@ -1094,6 +1094,15 @@ SwContentNode::SwContentNode( const SwNode& rWhere, const SwNodeType nNdType, void SwContentNode::ImplDestroy() { + // release any fill-bitmap link that has this node as its target, + // disconnected working copies have no SwDoc and no links + if (!IsDisconnected()) + { + SwDoc& rDoc = GetDoc(); + if (rDoc.HasFillBitmapLinks() && !rDoc.IsInDtor()) + rDoc.getIDocumentLinksAdministration().onFillBitmapURLChanged(*this, u""); + } + // The base class SwClient of SwFrame excludes itself from the dependency list! // Thus, we need to delete all Frames in the dependency list. if (!IsTextNode()) // see ~SwTextNode diff --git a/sw/source/core/inc/FillBitmapLinkTracker.hxx b/sw/source/core/inc/FillBitmapLinkTracker.hxx index f7cd961d0668..8fb784234e9a 100644 --- a/sw/source/core/inc/FillBitmapLinkTracker.hxx +++ b/sw/source/core/inc/FillBitmapLinkTracker.hxx @@ -14,7 +14,6 @@ #include #include #include -#include class SwContentNode; class SwFormat; @@ -32,8 +31,10 @@ namespace sw // the deferred item is set. // // When the graphic resolves, DataChanged calls SetFormatAttr / SetAttr on the -// host with the fetched graphic. Each link also listens to its host's notifier -// and self-deregisters on Dying so a link never outlives its host. +// host with the fetched graphic. A host destroyed while the document lives +// reports its death here with an empty URL, so a link never outlives its +// host. During ~SwDoc the hosts stay silent and the tracker destructor +// releases the remaining links instead. class FillBitmapLinkTracker final { public: @@ -42,12 +43,14 @@ public: // Called by SwFormat::SwClientNotify / SwContentNode::SwClientNotify // when XATTR_FILLBITMAP changes in the host's own attribute set, and - // from the link itself on host-Dying to drop the stale entry. rNewURL - // is the deferred origin URL of the new item, or empty when the item - // is absent, already resolved, or the host is going away. + // by the host's destructor to drop the stale entry. rNewURL is the + // deferred origin URL of the new item, or empty when the item is + // absent, already resolved, or the host is being destroyed. void onFillBitmapURLChanged(SwFormat& rFormat, std::u16string_view rNewURL); void onFillBitmapURLChanged(SwContentNode& rNode, std::u16string_view rNewURL); + bool hasLinks() const { return !m_aLinks.empty() || !m_aNodeLinks.empty(); } + // Suppress the onFillBitmapURLChanged that a link's own write-back of the // resolved graphic would otherwise raise for its host. The host is held by // its own type so a write-back is matched only against the same host type. @@ -59,8 +62,6 @@ private: void onURLChangedImpl(std::map>& rMap, Host& rHost, std::u16string_view rNewURL); - void drainPendingReleases(); - template bool isUpdatingHost(Host* pHost) const { auto const* ppHost = std::get_if(&m_aUpdatingHost); @@ -72,8 +73,6 @@ private: std::variant m_aUpdatingHost; std::map> m_aLinks; std::map> m_aNodeLinks; - // Links pulled from the maps but not yet freed, since removal may run inside a Dying broadcast: - std::vector> m_aPendingRelease; }; }