sw/qa/core/doc/doc.cxx | 86 +++++++++++++++++++++++++++++++++++++++++++
sw/source/core/doc/docbm.cxx | 21 +++++++++-
2 files changed, 104 insertions(+), 3 deletions(-)
New commits:
commit 61c467d0ccd97b8d69d6085e25e5e3f3395f2201
Author: Miklos Vajna <vmik...@collabora.com>
AuthorDate: Tue Oct 18 09:22:29 2022 +0200
Commit: Miklos Vajna <vmik...@collabora.com>
CommitDate: Tue Oct 18 11:56:07 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.
(cherry picked from commit e9861d8e0138028c17e93fec277125e7752439bd)
Conflicts:
sw/qa/core/doc/doc.cxx
sw/source/core/doc/docbm.cxx
Change-Id: Iaae95ec9c3038e8ee3b84408094844d0ff678213
diff --git a/sw/qa/core/doc/doc.cxx b/sw/qa/core/doc/doc.cxx
index 59a73751308d..1cefc11e55c9 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>
@@ -110,6 +112,90 @@ CPPUNIT_TEST_FIXTURE(SwCoreDocTest,
testImageHyperlinkStyle)
CPPUNIT_ASSERT_EQUAL(aExpected, aActual);
}
+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:
+ createDoc();
+ 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 c76e4417c020..f1aa34b36dbf 100644
--- a/sw/source/core/doc/docbm.cxx
+++ b/sw/source/core/doc/docbm.cxx
@@ -1269,13 +1269,28 @@ namespace sw::mark
// no special marks container
break;
}
- DdeBookmark* const pDdeBookmark = dynamic_cast<DdeBookmark*>(pMark);
- if (pDdeBookmark)
- pDdeBookmark->DeregisterFromDoc(m_pDoc);
//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_pDoc);
+
+ // 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
