sw/qa/core/doc/doc.cxx | 86 +++++++++++++++++++++++++++++++++++++++++++
sw/source/core/doc/docbm.cxx | 21 +++++++++-
2 files changed, 104 insertions(+), 3 deletions(-)
New commits:
commit f9a3adc186b31bac198d944a03a7b53dd926da23
Author: Miklos Vajna <vmik...@collabora.com>
AuthorDate: Tue Oct 18 09:22:29 2022 +0200
Commit: Michael Stahl <michael.st...@allotropia.de>
CommitDate: Tue Oct 18 11:31:12 2022 +0200
sw: fix crash on deleting a bookmark with active selection listeners
The document had a registered selection listener that created UNO
cursors (bookmarks) in its selectionChanged() callback. Once an UNO API
user tried to delete a bookmark, Writer crashed.
What happens is that sw::mark::MarkManager::deleteMark() created an
iterator pointing to an element of m_vAllMarks, then called
sw::mark::Bookmark::DeregisterFromDoc(), which indirectly calls
SwXTextView::NotifySelChanged(), which invokes UNO selection listeners,
which are allowed to create UNO cursors, which modify m_vAllMarks,
invalidating the iterator.
Fix the problem by first creating a non-const iterator based on ppMark,
then conditionally invoke DeregisterFromDoc(). If DeregisterFromDoc() is
called, then update the iterator based on pMark, so it is valid by the
time we erase the bookmark from m_vAllMarks.
This was not the problem when erasing the same mark from m_vBookmarks,
as in that case the iterator creation and the erase() both happen before
the de-registration.
Change-Id: Iaae95ec9c3038e8ee3b84408094844d0ff678213
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/141489
Reviewed-by: Miklos Vajna <vmik...@collabora.com>
Tested-by: Jenkins
(cherry picked from commit e9861d8e0138028c17e93fec277125e7752439bd)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/141405
Reviewed-by: Michael Stahl <michael.st...@allotropia.de>
diff --git a/sw/qa/core/doc/doc.cxx b/sw/qa/core/doc/doc.cxx
index c13e7a8d6a55..a9db78ba4871 100644
--- a/sw/qa/core/doc/doc.cxx
+++ b/sw/qa/core/doc/doc.cxx
@@ -9,6 +9,8 @@
#include <swmodeltestbase.hxx>
+#include <com/sun/star/view/XSelectionSupplier.hpp>
+
#include <comphelper/classids.hxx>
#include <tools/globname.hxx>
#include <svtools/embedhlp.hxx>
@@ -285,6 +287,90 @@ CPPUNIT_TEST_FIXTURE(SwCoreDocTest, testLinkedStyleDelete)
xStorable->storeAsURL(maTempFile.GetURL(), aArgs);
}
+namespace
+{
+/// This selection listener calls getAnchor() on selection change, which
creates UNO cursors and is
+/// invoked in the middle of a bookmark deletion.
+struct SelectionChangeListener : public
cppu::WeakImplHelper<view::XSelectionChangeListener>
+{
+ uno::Reference<container::XNameAccess> m_xBookmarks;
+ std::vector<uno::Reference<text::XTextRange>> m_aAnchors;
+
+public:
+ SelectionChangeListener(const uno::Reference<container::XNameAccess>&
xBookmarks);
+ // view::XSelectionChangeListener
+ void SAL_CALL selectionChanged(const lang::EventObject& rEvent) override;
+
+ // lang::XEventListener
+ void SAL_CALL disposing(const lang::EventObject& rSource) override;
+};
+}
+
+SelectionChangeListener::SelectionChangeListener(
+ const uno::Reference<container::XNameAccess>& xBookmarks)
+ : m_xBookmarks(xBookmarks)
+{
+}
+
+void SelectionChangeListener::selectionChanged(const lang::EventObject&
/*rEvent*/)
+{
+ uno::Sequence<OUString> aElementNames = m_xBookmarks->getElementNames();
+ for (const auto& rName : aElementNames)
+ {
+ uno::Reference<text::XTextContent>
xTextContent(m_xBookmarks->getByName(rName),
+ uno::UNO_QUERY);
+ m_aAnchors.push_back(xTextContent->getAnchor());
+ }
+}
+
+void SelectionChangeListener::disposing(const lang::EventObject& /*rSource*/)
{}
+
+CPPUNIT_TEST_FIXTURE(SwCoreDocTest, testBookmarkDeleteListeners)
+{
+ // Given a document with 2 bookmarks:
+ createSwDoc();
+ uno::Reference<text::XTextDocument> xTextDocument(mxComponent,
uno::UNO_QUERY);
+ uno::Reference<text::XText> xText = xTextDocument->getText();
+ uno::Reference<text::XTextCursor> xCursor = xText->createTextCursor();
+ {
+ xText->insertString(xCursor, "test", /*bAbsorb=*/false);
+ xCursor->gotoStart(/*bExpand=*/false);
+ xCursor->gotoEnd(/*bExpand=*/true);
+ uno::Reference<lang::XMultiServiceFactory> xFactory(mxComponent,
uno::UNO_QUERY);
+ uno::Reference<text::XTextContent> xBookmark(
+ xFactory->createInstance("com.sun.star.text.Bookmark"),
uno::UNO_QUERY);
+ uno::Reference<container::XNamed> xBookmarkNamed(xBookmark,
uno::UNO_QUERY);
+ xBookmarkNamed->setName("mybookmark");
+ xText->insertTextContent(xCursor, xBookmark, /*bAbsorb=*/true);
+ }
+ {
+ xCursor->gotoEnd(/*bExpand=*/false);
+ xText->insertString(xCursor, "test2", /*bAbsorb=*/false);
+ xCursor->goLeft(4, /*bExpand=*/true);
+ uno::Reference<lang::XMultiServiceFactory> xFactory(mxComponent,
uno::UNO_QUERY);
+ uno::Reference<text::XTextContent> xBookmark(
+ xFactory->createInstance("com.sun.star.text.Bookmark"),
uno::UNO_QUERY);
+ uno::Reference<container::XNamed> xBookmarkNamed(xBookmark,
uno::UNO_QUERY);
+ xBookmarkNamed->setName("mybookmark2");
+ xText->insertTextContent(xCursor, xBookmark, /*bAbsorb=*/true);
+ }
+ uno::Reference<text::XBookmarksSupplier> xBookmarksSupplier(mxComponent,
uno::UNO_QUERY);
+ uno::Reference<container::XNameAccess> xBookmarks =
xBookmarksSupplier->getBookmarks();
+
+ // When registering a selection listener that creates uno marks:
+ uno::Reference<frame::XModel> xModel(mxComponent, uno::UNO_QUERY);
+ uno::Reference<view::XSelectionSupplier>
xController(xModel->getCurrentController(),
+ uno::UNO_QUERY);
+ xController->addSelectionChangeListener(new
SelectionChangeListener(xBookmarks));
+
+ // Then make sure that deleting a bookmark doesn't crash:
+ uno::Reference<lang::XComponent>
xBookmark(xBookmarks->getByName("mybookmark2"),
+ uno::UNO_QUERY);
+ // Without the accompanying fix in place, this test would have crashed, an
invalidated iterator
+ // was used with erase().
+ xBookmark->dispose();
+}
+
CPPUNIT_PLUGIN_IMPLEMENT();
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sw/source/core/doc/docbm.cxx b/sw/source/core/doc/docbm.cxx
index 1b283437ab6f..629cb4ccf633 100644
--- a/sw/source/core/doc/docbm.cxx
+++ b/sw/source/core/doc/docbm.cxx
@@ -1284,13 +1284,28 @@ namespace sw::mark
// no special marks container
break;
}
- DdeBookmark* const pDdeBookmark = dynamic_cast<DdeBookmark*>(pMark);
- if (pDdeBookmark)
- pDdeBookmark->DeregisterFromDoc(m_rDoc);
//Effective STL Item 27, get a non-const iterator aI at the same
//position as const iterator ppMark was
auto aI = m_vAllMarks.begin();
std::advance(aI, std::distance<container_t::const_iterator>(aI,
ppMark.get()));
+ DdeBookmark* const pDdeBookmark = dynamic_cast<DdeBookmark*>(pMark);
+ if (pDdeBookmark)
+ {
+ pDdeBookmark->DeregisterFromDoc(m_rDoc);
+
+ // Update aI, possibly a selection listener invalidated the
iterators of m_vAllMarks.
+ auto [it, endIt] = equal_range(m_vAllMarks.begin(),
m_vAllMarks.end(),
+ pMark->GetMarkStart(),
CompareIMarkStartsBefore());
+ for (; it != endIt; ++it)
+ {
+ if (*it == pMark)
+ {
+ aI = m_vAllMarks.begin();
+ std::advance(aI,
std::distance<container_t::const_iterator>(aI, it));
+ break;
+ }
+ }
+ }
m_vAllMarks.erase(aI);
// If we don't have a lazy deleter
