Avoid crashing when the end of an empty markdown list is detected

The markdown parser generates empty lists in some cases when a character
that can be used as a bullet is found on a line by itself.
cbEnterBlock() and cbLeaveBlock() are called symmetrically in such cases.
QStack::pop() on an empty stack triggers an assert, so push and pop need
to be done symmetrically too.  But it's difficult to actually create the
list as soon as the MD_BLOCK_UL or MD_BLOCK_OL callback occurs, without
breaking the case fixed in 7224d0e427d71e559b928c44634839b4791c1416 (and
probably other cases).  That's because QTextCursor::insertList() creates
a list item at the same time as it creates the list itself, and also
inherits block formatting from the previous block.  We now insert empty
lists with empty items whenever the need for that is detected though,
and there's a failsafe to prevent popping in case something still goes
wrong with that logic.  We aren't strict about reproducing the original
markdown when regenerating it via toMarkdown(), but it's getting closer.

Fixes: QTBUG-78870
Change-Id: Ided194ce7aec2710c60dbac42761ee4169ed9b78
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
Reviewed-by: Robert Loehning <robert.loehning@qt.io>

2 files changed, 81 insertions, 6 deletions

diff --git a/src/gui/text/qtextmarkdownimporter.cpp b/src/gui/text/qtextmarkdownimporter.cpp
index 87ade1f973..c2ad1e5612 100644
--- a/src/gui/text/qtextmarkdownimporter.cpp
+++ b/src/gui/text/qtextmarkdownimporter.cpp
@@ -216,6 +216,10 @@ int QTextMarkdownImporter::cbEnterBlock(int blockType, void *det)
         qCDebug(lcMD) << "LI";
     } break;
     case MD_BLOCK_UL: {
+        if (m_needsInsertList) // list nested in an empty list
+            m_listStack.push(m_cursor->insertList(m_listFormat));
+        else
+            m_needsInsertList = true;
         MD_BLOCK_UL_DETAIL *detail = static_cast<MD_BLOCK_UL_DETAIL *>(det);
         m_listFormat = QTextListFormat();
         m_listFormat.setIndent(m_listStack.count() + 1);
@@ -230,17 +234,19 @@ int QTextMarkdownImporter::cbEnterBlock(int blockType, void *det)
             m_listFormat.setStyle(QTextListFormat::ListDisc);
             break;
         }
-        qCDebug(lcMD, "UL %c level %d", detail->mark, m_listStack.count());
-        m_needsInsertList = true;
+        qCDebug(lcMD, "UL %c level %d", detail->mark, m_listStack.count() + 1);
     } break;
     case MD_BLOCK_OL: {
+        if (m_needsInsertList) // list nested in an empty list
+            m_listStack.push(m_cursor->insertList(m_listFormat));
+        else
+            m_needsInsertList = true;
         MD_BLOCK_OL_DETAIL *detail = static_cast<MD_BLOCK_OL_DETAIL *>(det);
         m_listFormat = QTextListFormat();
         m_listFormat.setIndent(m_listStack.count() + 1);
         m_listFormat.setNumberSuffix(QChar::fromLatin1(detail->mark_delimiter));
         m_listFormat.setStyle(QTextListFormat::ListDecimal);
-        qCDebug(lcMD, "OL xx%d level %d", detail->mark_delimiter, m_listStack.count());
-        m_needsInsertList = true;
+        qCDebug(lcMD, "OL xx%d level %d", detail->mark_delimiter, m_listStack.count() + 1);
     } break;
     case MD_BLOCK_TD: {
         MD_BLOCK_TD_DETAIL *detail = static_cast<MD_BLOCK_TD_DETAIL *>(det);
@@ -306,8 +312,14 @@ int QTextMarkdownImporter::cbLeaveBlock(int blockType, void *detail)
         break;
     case MD_BLOCK_UL:
     case MD_BLOCK_OL:
-        qCDebug(lcMD, "list at level %d ended", m_listStack.count());
-        m_listStack.pop();
+        if (Q_UNLIKELY(m_needsInsertList))
+            m_listStack.push(m_cursor->createList(m_listFormat));
+        if (Q_UNLIKELY(m_listStack.isEmpty())) {
+            qCWarning(lcMD, "list ended unexpectedly");
+        } else {
+            qCDebug(lcMD, "list at level %d ended", m_listStack.count());
+            m_listStack.pop();
+        }
         break;
     case MD_BLOCK_TR: {
         // https://github.com/mity/md4c/issues/29
diff --git a/tests/auto/gui/text/qtextmarkdownimporter/tst_qtextmarkdownimporter.cpp b/tests/auto/gui/text/qtextmarkdownimporter/tst_qtextmarkdownimporter.cpp
index 8f51a7a474..1aa1406218 100644
--- a/tests/auto/gui/text/qtextmarkdownimporter/tst_qtextmarkdownimporter.cpp
+++ b/tests/auto/gui/text/qtextmarkdownimporter/tst_qtextmarkdownimporter.cpp
@@ -53,6 +53,8 @@ class tst_QTextMarkdownImporter : public QObject
 private slots:
     void headingBulletsContinuations();
     void thematicBreaks();
+    void lists_data();
+    void lists();
 };
 
 void tst_QTextMarkdownImporter::headingBulletsContinuations()
@@ -159,5 +161,66 @@ void tst_QTextMarkdownImporter::thematicBreaks()
 #endif
 }
 
+void tst_QTextMarkdownImporter::lists_data()
+{
+    QTest::addColumn<QString>("input");
+    QTest::addColumn<int>("expectedItemCount");
+    QTest::addColumn<bool>("expectedEmptyItems");
+    QTest::addColumn<QString>("rewrite");
+
+    // Some of these cases show odd behavior, which is subject to change
+    // as the importer and the writer are tweaked to fix bugs over time.
+    QTest::newRow("dot newline") << ".\n" << 0 << true << ".\n\n";
+    QTest::newRow("number dot newline") << "1.\n" << 1 << true << "";
+    QTest::newRow("star newline") << "*\n" << 1 << true << "";
+    QTest::newRow("hyphen newline") << "-\n" << 1 << true << "";
+    QTest::newRow("hyphen space newline") << "- \n" << 1 << true << "";
+    QTest::newRow("hyphen space letter newline") << "- a\n" << 1 << false << "- a\n";
+    QTest::newRow("hyphen nbsp newline") <<
+        QString::fromUtf8("-\u00A0\n") << 0 << true << "-\u00A0\n\n";
+    QTest::newRow("nested empty lists") << "*\n  *\n  *\n" << 1 << true << "";
+    QTest::newRow("list nested in empty list") << "-\n  * a\n" << 2 << false << "- \n  * a\n";
+    QTest::newRow("lists nested in empty lists")
+            << "-\n  * a\n  * b\n- c\n  *\n    + d\n" << 5 << false
+            << "- \n  * a\n  * b\n- c *\n  + d\n";
+    QTest::newRow("numeric lists nested in empty lists")
+            << "- \n    1.  a\n    2.  b\n- c\n  1.\n       + d\n" << 4 << false
+            << "- \n    1.  a\n    2.  b\n- c 1. + d\n";
+}
+
+void tst_QTextMarkdownImporter::lists()
+{
+    QFETCH(QString, input);
+    QFETCH(int, expectedItemCount);
+    QFETCH(bool, expectedEmptyItems);
+    QFETCH(QString, rewrite);
+
+    QTextDocument doc;
+    doc.setMarkdown(input); // QTBUG-78870 : don't crash
+    QTextFrame::iterator iterator = doc.rootFrame()->begin();
+    QTextFrame *currentFrame = iterator.currentFrame();
+    int i = 0;
+    int itemCount = 0;
+    bool emptyItems = true;
+    while (!iterator.atEnd()) {
+        // There are no child frames
+        QCOMPARE(iterator.currentFrame(), currentFrame);
+        // Check whether the block is text or a horizontal rule
+        QTextBlock block = iterator.currentBlock();
+        if (block.textList()) {
+            ++itemCount;
+            if (!block.text().isEmpty())
+                emptyItems = false;
+        }
+        qCDebug(lcTests, "%d %s%s", i,
+                (block.textList() ? "<li>" : "<p>"), qPrintable(block.text()));
+        ++iterator;
+        ++i;
+    }
+    QCOMPARE(itemCount, expectedItemCount);
+    QCOMPARE(emptyItems, expectedEmptyItems);
+    QCOMPARE(doc.toMarkdown(), rewrite);
+}
+
 QTEST_MAIN(tst_QTextMarkdownImporter)
 #include "tst_qtextmarkdownimporter.moc"