Improve polish loop detection and diagnostics

The existing warning was pretty much useless since it would only warn
after having looped INT_MAX times. In addition, it didn't actually
detect if polish() was called from within updatePolish().

Instead, the counting is changed to be strictly more correct:

The counter is now only increased when polish() is called within the
updatePolish(). It will reset back to 1 if that does not occur.
Effectively, the counter will reflect how many consecutive polish loops
we have processed.

This patch will show diagnostics after having reached 1000 consecutive
polish loops. It will only warn for the next 5 items in order to not be
too verbose...(most likely they will be the same 5 items).

If the counter reaches 100,000, we break out of the loop:
This might be important for e.g. CI runs so that the process can actually
terminate in order to get some useful diagnostics.

Note that the item that calls polish() within updatePolish() doesn't
have to be the same item as updatePolish() was called on. We also want
to track these since there might be several items working in tandem to
create the loop.

With this change it will now give the following output:

  main.qml:10:5: QML Row: possible QQuickItem::polish() loop
  main.qml:10:5: QML Row: Row called polish() inside updatePolish() of Row

(This is when Row called polish() from within its own updatePolish())

Fixes: QTBUG-40220
Task-number: QTBUG-83856
Change-Id: Ib8a7242908082c70d8cf71efbbe1fa148dbfada0
Reviewed-by: Shawn Rutledge <shawn.rutledge@qt.io>
(cherry picked from commit 1c8bce285522e0dcfd13fe6c514f4756d6d6438c)
Reviewed-by: Jan Arve Sæther <jan-arve.saether@qt.io>
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>

1 files changed, 82 insertions, 0 deletions

diff --git a/tests/auto/quick/qquickitem/tst_qquickitem.cpp b/tests/auto/quick/qquickitem/tst_qquickitem.cpp
index 7e132f97b6..61b6902b43 100644
--- a/tests/auto/quick/qquickitem/tst_qquickitem.cpp
+++ b/tests/auto/quick/qquickitem/tst_qquickitem.cpp
@@ -40,6 +40,7 @@
 #include "../../shared/util.h"
 #include "../shared/viewtestutil.h"
 #include <QSignalSpy>
+#include <QtCore/qregularexpression.h>
 
 #ifdef TEST_QTBUG_60123
 #include <QWidget>
@@ -111,10 +112,15 @@ public:
     }
 
     bool wasPolished;
+    int repolishLoopCount = 0;
 
 protected:
     virtual void updatePolish() {
         wasPolished = true;
+        if (repolishLoopCount > 0) {
+            --repolishLoopCount;
+            polish();
+        }
     }
 
 public slots:
@@ -197,6 +203,9 @@ private slots:
     void qtBug60123();
 #endif
 
+    void polishLoopDetection_data();
+    void polishLoopDetection();
+
 private:
 
     enum PaintOrderOp {
@@ -1427,6 +1436,79 @@ void tst_qquickitem::polishOnCompleted()
     QTRY_VERIFY(item->wasPolished);
 }
 
+struct PolishItemSpan {
+    int itemCount;      // Number of items...
+    int repolishCount;  // ...repolishing 'repolishCount' times
+};
+
+/*
+ * For instance, two consecutive spans {99,0} and {1,2000} } instructs to
+ * construct 99 items with no repolish, and 1 item with 2000 repolishes (in that sibling order)
+ */
+typedef QVector<PolishItemSpan> PolishItemSpans;
+
+Q_DECLARE_METATYPE(PolishItemSpan)
+Q_DECLARE_METATYPE(PolishItemSpans)
+
+void tst_qquickitem::polishLoopDetection_data()
+{
+    QTest::addColumn<PolishItemSpans>("listOfItemsToPolish");
+    QTest::addColumn<int>("expectedNumberOfWarnings");
+
+    QTest::newRow("test1.100") <<   PolishItemSpans({ {1, 100} }) << 0;
+    QTest::newRow("test1.1002") <<  PolishItemSpans({ {1, 1002} }) << 3;
+    QTest::newRow("test1.2020") <<  PolishItemSpans({ {1, 2020} }) << 10;
+
+    QTest::newRow("test5.1") <<    PolishItemSpans({ {5, 1} }) << 0;
+    QTest::newRow("test5.10") <<   PolishItemSpans({ {5, 10} }) << 0;
+    QTest::newRow("test5.100") <<  PolishItemSpans({ {5, 100} }) << 0;
+    QTest::newRow("test5.1000") << PolishItemSpans({ {5, 1000} }) << 5;
+
+    QTest::newRow("test1000.1") <<  PolishItemSpans({ {1000,1} }) << 0;
+    QTest::newRow("test2000.1") <<  PolishItemSpans({ {2000,1} }) << 0;
+
+    QTest::newRow("test99.0-1.1100") << PolishItemSpans({ {99,0},{1,1100} }) << 5;
+    QTest::newRow("test98.0-2.1100") << PolishItemSpans({ {98,0},{2,1100} }) << 5+5;
+
+    // reverse the two above
+    QTest::newRow("test1.1100-99.0") << PolishItemSpans({ {1,1100},{99,0} }) << 5;
+    QTest::newRow("test2.1100-98.0") << PolishItemSpans({ {2,1100},{98,0} }) << 5+5;
+}
+
+void tst_qquickitem::polishLoopDetection()
+{
+    QFETCH(PolishItemSpans, listOfItemsToPolish);
+    QFETCH(int, expectedNumberOfWarnings);
+
+    QQuickWindow window;
+    window.resize(200, 200);
+    window.show();
+
+    TestPolishItem *item = nullptr;
+    int count = 0;
+    for (PolishItemSpan s : listOfItemsToPolish) {
+        for (int i = 0; i < s.itemCount; ++i) {
+            item = new TestPolishItem(window.contentItem());
+            item->setSize(QSizeF(200, 100));
+            item->repolishLoopCount = s.repolishCount;
+            item->setObjectName(QString::fromLatin1("obj%1").arg(count++));
+        }
+    }
+
+    for (int i = 0; i < expectedNumberOfWarnings; ++i) {
+        QTest::ignoreMessage(QtWarningMsg, QRegularExpression(".*possible QQuickItem..polish.. loop.*"));
+        QTest::ignoreMessage(QtWarningMsg, QRegularExpression(".*TestPolishItem.* called polish.. inside updatePolish.. of TestPolishItem.*"));
+    }
+
+    QList<QQuickItem*> items = window.contentItem()->childItems();
+    for (int i = 0; i < items.count(); ++i) {
+        static_cast<TestPolishItem*>(items.at(i))->doPolish();
+    }
+    item = static_cast<TestPolishItem*>(items.first());
+    // item is the last item, so we wait until the last item reached 0
+    QVERIFY(QTest::qWaitFor([=](){return item->repolishLoopCount == 0 && item->wasPolished;}));
+}
+
 void tst_qquickitem::wheelEvent_data()
 {
     QTest::addColumn<bool>("visible");