ODFstudio/odfstudio
2
0
Fork 0
Code Issues Packages Activity

stop the fill-bitmap link listening to its host's SvtBroadcaster

Browse Source 
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 <caolan.mcnamara@collabora.com>
Change-Id: Ibbf15e4e17a7a08aa029c9d5df8f40b7a4a13e04
Reviewed-on: https://gerrit.collaboraoffice.com/c/online/+/4282
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>
Tested-by: Jenkins CPCI <releng@collaboraoffice.com>
(cherry picked from commit 38ac5c0e6c7baf4de27498e5428d6279c753c97e)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/208603
Tested-by: Jenkins
Reviewed-by: Xisco Fauli <xiscofauli@libreoffice.org>
This commit is contained in:
Caolán McNamara
2026-07-25 15:26:55 +02:00
committed by Xisco Fauli
parent a59f6ff190
commit b14f8b9dfa
9 changed files with 104 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files
sw/inc/IDocumentLinksAdministration.hxx
+3 -3
View File
@@ -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;
sw/inc/doc.hxx
+5
View File
@@ -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; }
sw/qa/extras/tiledrendering/tiledrendering2.cxx
+63
View File
@@ -32,7 +32,9 @@
#include <vcl/virdev.hxx>
#include <unotxdoc.hxx>
#include <com/sun/star/container/XNameContainer.hpp>
#include <com/sun/star/document/UpdateDocMode.hpp>
#include <com/sun/star/style/XStyleFamiliesSupplier.hpp>
#include <sfx2/linkmgr.hxx>
#include <svx/fillbitmaplink.hxx>
#include <svx/xbtmpit.hxx>
@@ -41,6 +43,7 @@
#include <view.hxx>
#include <IDocumentLayoutAccess.hxx>
#include <IDocumentLinksAdministration.hxx>
#include <IDocumentUndoRedo.hxx>
#include <rootfrm.hxx>
#include <pagefrm.hxx>
#include <docsh.hxx>
@@ -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<beans::PropertyValue> 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<style::XStyleFamiliesSupplier> xSupplier(mxComponent, uno::UNO_QUERY);
uno::Reference<container::XNameContainer> 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<beans::PropertyValue> 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
sw/source/core/attr/format.cxx
+5
View File
@@ -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();
}
sw/source/core/doc/DocumentLinksAdministrationManager.cxx
+2
View File
@@ -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()
sw/source/core/doc/FillBitmapLinkTracker.cxx
+7 -34
View File
@@ -15,7 +15,6 @@
#include <sfx2/linkmgr.hxx>
#include <sfx2/lnkbase.hxx>
#include <sot/formats.hxx>
#include <svl/listener.hxx>
#include <svx/xbtmpit.hxx>
#include <svx/xdef.hxx>
#include <vcl/graph.hxx>
@@ -42,15 +41,10 @@ template <> struct HostOps<SwContentNode>
};
// 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 <typename Host>
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 <typename Host> 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<Host>::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<SvBaseLink> 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<tools::SvRef<sfx2::SvBaseLink>> aReleasing;
aReleasing.swap(m_aPendingRelease);
}
template <typename Host>
@@ -135,15 +113,10 @@ void FillBitmapLinkTracker::onURLChangedImpl(std::map<Host*, tools::SvRef<sfx2::
if (isUpdatingHost(&rHost))
return;
drainPendingReleases();
auto it = rMap.find(&rHost);
if (it != rMap.end())
{
m_rLinkMgr.Remove(it->second.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())
sw/source/core/doc/docnew.cxx
+1
View File
@@ -271,6 +271,7 @@ SwDoc::SwDoc()
mbInWriterfilterImport(false),
mbUpdateTOX(false),
mbInLoadAsynchron(false),
mbHasFillBitmapLinks(false),
mbIsAutoFormatRedline(false),
mbOLEPrtNotifyPending(false),
mbAllOLENotify(false),
sw/source/core/docnode/node.cxx
+9
View File
@@ -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
sw/source/core/inc/FillBitmapLinkTracker.hxx
+9 -10
View File
@@ -14,7 +14,6 @@
#include <map>
#include <string_view>
#include <variant>
#include <vector>
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<Host*, tools::SvRef<sfx2::SvBaseLink>>& rMap, Host& rHost,
std::u16string_view rNewURL);
void drainPendingReleases();
template <typename Host> bool isUpdatingHost(Host* pHost) const
{
auto const* ppHost = std::get_if<Host*>(&m_aUpdatingHost);
@@ -72,8 +73,6 @@ private:
std::variant<std::monostate, SwFormat*, SwContentNode*> m_aUpdatingHost;
std::map<SwFormat*, tools::SvRef<sfx2::SvBaseLink>> m_aLinks;
std::map<SwContentNode*, tools::SvRef<sfx2::SvBaseLink>> m_aNodeLinks;
// Links pulled from the maps but not yet freed, since removal may run inside a Dying broadcast:
std::vector<tools::SvRef<sfx2::SvBaseLink>> m_aPendingRelease;
};
}