tdf#109376 sw: fix SwUndoDelete with end pos on SwTableNode crash

Commit 6ff263b837831d46d0c215963b70543a9ea5bd2a added a check in
SwUndoSaveContent::DelContentIndex() to avoid moving the anchor of a
FLY_AT_PARA if its new position would be a table node, because
SwFlyAtContentFrame::Modify() requires a SwTextNode to be the anchor.
However, that doesn't actually avoid moving the anchor - later,
SwNodes::RemoveNode() relocates the anchor to the next node regardless
of type!

It's probably better to just delete the fly in the situation when the
end position is a SwTableNode, which fixes the reported crash.

Unfortunately on Redo, the SwUndoDelete::UndoImpl() does not recreate
the nodes correctly, hence the fly then is inserted on the wrong node,
which later crashes again.

The problem is that due to the table node, a dummy SwTextNode is inserted,
which should be at the end of the range, but ends up at the start due to
an erroneous ++aPos.nNode; - the result is that the fly is inserted on
the dummy node and is immediately deleted again, triggering another
assert.  If there is a dummy node, it also doesn't make sense to call
SplitNode().

Yet another problem is that in SwUndoDelete::UndoImpl(), the frames for
the moved text nodes are not created, because the first node is skipped
with the wrong assumption that it already has frames.

Reportedly this started to crash with commit
e07feb9457f2ffb373ae69b73dda290140e4005f, previously it was just wrong.

Change-Id: I5094638e34c6ed52c836e57691d377b8cd1608f9
Reviewed-on: https://gerrit.libreoffice.org/70683
Tested-by: Jenkins
Reviewed-by: Michael Stahl <Michael.Stahl@cib.de>

3 files changed, 63 insertions, 12 deletions

diff --git a/sw/qa/extras/uiwriter/uiwriter2.cxx b/sw/qa/extras/uiwriter/uiwriter2.cxx
index ddebd9a1a662..f58819aee41c 100644
--- a/sw/qa/extras/uiwriter/uiwriter2.cxx
+++ b/sw/qa/extras/uiwriter/uiwriter2.cxx
@@ -30,6 +30,8 @@
 #include <sortedobjs.hxx>
 #include <anchoredobject.hxx>
 #include <swtypes.hxx>
+#include <itabenum.hxx>
+#include <fmtfsize.hxx>
 #include <fmtornt.hxx>
 #include <xmloff/odffields.hxx>
 #include <txtfrm.hxx>
@@ -49,6 +51,7 @@ public:
     void testTdf47471_paraStyleBackground();
     void testTdf101534();
     void testTdf54819();
+    void testTdf109376();
     void testTdf64242_optimizeTable();
     void testTdf108687_tabstop();
     void testTdf119571();
@@ -79,6 +82,7 @@ public:
     CPPUNIT_TEST(testTdf47471_paraStyleBackground);
     CPPUNIT_TEST(testTdf101534);
     CPPUNIT_TEST(testTdf54819);
+    CPPUNIT_TEST(testTdf109376);
     CPPUNIT_TEST(testTdf64242_optimizeTable);
     CPPUNIT_TEST(testTdf108687_tabstop);
     CPPUNIT_TEST(testTdf119571);
@@ -343,6 +347,53 @@ void SwUiWriterTest2::testTdf54819()
                          getProperty<OUString>(getParagraph(1), "ParaStyleName"));
 }
 
+void SwUiWriterTest2::testTdf109376()
+{
+    SwDoc* pDoc = createDoc();
+    SwWrtShell* pWrtShell = pDoc->GetDocShell()->GetWrtShell();
+    CPPUNIT_ASSERT(pWrtShell);
+    // need 2 paragraphs to get to the bMoveNds case
+    pWrtShell->Insert("foo");
+    pWrtShell->SplitNode();
+    pWrtShell->Insert("bar");
+    pWrtShell->SplitNode();
+    pWrtShell->StartOfSection(false);
+
+    // add AT_PARA fly at 1st to be deleted node
+    SwFormatAnchor anchor(RndStdIds::FLY_AT_PARA);
+    anchor.SetAnchor(pWrtShell->GetCursor()->GetPoint());
+    SfxItemSet flySet(pDoc->GetAttrPool(),
+                      svl::Items<RES_FRM_SIZE, RES_FRM_SIZE, RES_ANCHOR, RES_ANCHOR>{});
+    flySet.Put(anchor);
+    SwFormatFrameSize size(ATT_MIN_SIZE, 1000, 1000);
+    flySet.Put(size); // set a size, else we get 1 char per line...
+    SwFrameFormat const* pFly = pWrtShell->NewFlyFrame(flySet, /*bAnchValid=*/true);
+    CPPUNIT_ASSERT(pFly != nullptr);
+
+    pWrtShell->SttEndDoc(false);
+    SwInsertTableOptions tableOpt(SwInsertTableFlags::DefaultBorder, 0);
+    const SwTable& rTable = pWrtShell->InsertTable(tableOpt, 1, 1);
+
+    pWrtShell->StartOfSection(false);
+    SwPaM pam(*pWrtShell->GetCursor()->GetPoint());
+    pam.SetMark();
+    pam.GetPoint()->nNode = *rTable.GetTableNode();
+    pam.GetPoint()->nContent.Assign(nullptr, 0);
+    pam.Exchange(); // same selection direction as in doc compare...
+
+    // this used to assert/crash with m_pAnchoredFlys mismatch because the
+    // fly was not deleted but its anchor was moved to the SwTableNode
+    pDoc->getIDocumentContentOperations().DeleteRange(pam);
+    CPPUNIT_ASSERT_EQUAL(size_t(0), pWrtShell->GetFlyCount(FLYCNTTYPE_FRM));
+    sw::UndoManager& rUndoManager = pDoc->GetUndoManager();
+    rUndoManager.Undo();
+    CPPUNIT_ASSERT_EQUAL(size_t(1), pWrtShell->GetFlyCount(FLYCNTTYPE_FRM));
+    rUndoManager.Redo();
+    CPPUNIT_ASSERT_EQUAL(size_t(0), pWrtShell->GetFlyCount(FLYCNTTYPE_FRM));
+    rUndoManager.Undo();
+    CPPUNIT_ASSERT_EQUAL(size_t(1), pWrtShell->GetFlyCount(FLYCNTTYPE_FRM));
+}
+
 void SwUiWriterTest2::testTdf64242_optimizeTable()
 {
     SwDoc* pDoc = createDoc("tdf64242_optimizeTable.odt");
diff --git a/sw/source/core/undo/undel.cxx b/sw/source/core/undo/undel.cxx
index 1ec3a95be66e..daa062536d88 100644
--- a/sw/source/core/undo/undel.cxx
+++ b/sw/source/core/undo/undel.cxx
@@ -903,7 +903,7 @@ void SwUndoDelete::UndoImpl(::sw::UndoRedoContext & rContext)
                 pTextNd->RestoreMetadata(m_pMetadataUndoEnd);
             }
         }
-        else if( m_aSttStr && bNodeMove )
+        else if (m_aSttStr && bNodeMove && pInsNd == nullptr)
         {
             SwTextNode * pNd = aPos.nNode.GetNode().GetTextNode();
             if( pNd )
@@ -1114,8 +1114,10 @@ void SwUndoDelete::UndoImpl(::sw::UndoRedoContext & rContext)
     {
         // tdf#121031 if the start node is a text node, it already has a frame;
         // if it's a table, it does not
+        // tdf#109376 exception: end on non-text-node -> start node was inserted
         SwNodeIndex const start(rDoc.GetNodes(), nSttNode +
-            ((m_bDelFullPara || !rDoc.GetNodes()[nSttNode]->IsTextNode()) ? 0 : 1));
+            ((m_bDelFullPara || !rDoc.GetNodes()[nSttNode]->IsTextNode() || pInsNd)
+                 ? 0 : 1));
         // don't include end node in the range: it may have been merged already
         // by the start node, or it may be merged by one of the moved nodes,
         // but if it isn't merged, its current frame(s) should be good...
diff --git a/sw/source/core/undo/undobj.cxx b/sw/source/core/undo/undobj.cxx
index eb909378556b..0d17568e3b09 100644
--- a/sw/source/core/undo/undobj.cxx
+++ b/sw/source/core/undo/undobj.cxx
@@ -987,17 +987,15 @@ void SwUndoSaveContent::DelContentIndex( const SwPosition& rMark,
 
                                 // Moving the anchor?
                                 if( !( DelContentType::CheckNoCntnt & nDelContentType ) &&
-                                    ( rPoint.nNode.GetIndex() == pAPos->nNode.GetIndex() ) )
-                                {
+                                    (rPoint.nNode.GetIndex() == pAPos->nNode.GetIndex())
                                     // Do not try to move the anchor to a table!
-                                    if( rMark.nNode.GetNode().GetTextNode() )
-                                    {
-                                        pHistory->Add( *pFormat );
-                                        SwFormatAnchor aAnch( *pAnchor );
-                                        SwPosition aPos( rMark.nNode );
-                                        aAnch.SetAnchor( &aPos );
-                                        pFormat->SetFormatAttr( aAnch );
-                                    }
+                                    && rMark.nNode.GetNode().IsTextNode())
+                                {
+                                    pHistory->Add( *pFormat );
+                                    SwFormatAnchor aAnch( *pAnchor );
+                                    SwPosition aPos( rMark.nNode );
+                                    aAnch.SetAnchor( &aPos );
+                                    pFormat->SetFormatAttr( aAnch );
                                 }
                                 else
                                 {