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 <vmiklos@collabora.com>
Tested-by: Jenkins

2 files changed, 104 insertions, 3 deletions

diff --git a/sw/qa/core/doc/doc.cxx b/sw/qa/core/doc/doc.cxx
index 90e69ddc7f73..8087ad456105 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>
@@ -327,6 +329,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 d38542922b52..c19d182c3f7a 100644
--- a/sw/source/core/doc/docbm.cxx
+++ b/sw/source/core/doc/docbm.cxx
@@ -1285,13 +1285,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