Fix crash when adding/removing points during animation

Adding/removing points during the animation of the previous
add/remove operation resulted in a mismatch between visible
point count and the actual point count, leading to crashes
in code that assumed both to be the same. Added necessary
safeguards and improved autotests to detect these cases.

Task-number: QTRD-1984
Change-Id: I544d10a69e760a40d4c90a4f02de61d4d1bb974f
Reviewed-by: Tomi Korpipää <tomi.korpipaa@digia.com>

6 files changed, 96 insertions, 9 deletions

diff --git a/src/animations/xyanimation.cpp b/src/animations/xyanimation.cpp
index 9f66b4ab..e8ad2802 100644
--- a/src/animations/xyanimation.cpp
+++ b/src/animations/xyanimation.cpp
@@ -59,15 +59,21 @@ void XYAnimation::setup(const QVector<QPointF> &oldPoints, const QVector<QPointF
 
     int x = m_oldPoints.count();
     int y = m_newPoints.count();
-
-    if (x - y == 1  && index >= 0 && y > 0) {
+    int diff = x - y;
+    int requestedDiff = oldPoints.count() - y;
+
+    // m_oldPoints can be whatever between 0 and actual points count if new animation setup
+    // interrupts a previous animation, so only do remove and add animations if both
+    // stored diff and requested diff indicate add or remove. Also ensure that index is not
+    // invalid.
+    if (diff == 1 && requestedDiff == 1 && index >= 0 && y > 0 && index <= y) {
         //remove point
         m_newPoints.insert(index, index > 0 ? newPoints[index - 1] : newPoints[index]);
         m_index = index;
         m_type = RemovePointAnimation;
     }
 
-    if (x - y == -1  && index >= 0) {
+    if (diff == -1 && requestedDiff == -1 && index >= 0 && index <= x) {
         //add point
         m_oldPoints.insert(index, index > 0 ? newPoints[index - 1] : newPoints[index]);
         m_index = index;
diff --git a/src/linechart/linechartitem.cpp b/src/linechart/linechartitem.cpp
index dd5f540d..ef2a3502 100644
--- a/src/linechart/linechartitem.cpp
+++ b/src/linechart/linechartitem.cpp
@@ -113,6 +113,9 @@ void LineChartItem::updateGeometry()
         qreal rightMarginLine = centerPoint.x() + margin;
         qreal horizontal = centerPoint.y();
 
+        // See ScatterChartItem::updateGeometry() for explanation why seriesLastIndex is needed
+        const int seriesLastIndex = m_series->count() - 1;
+
         for (int i = 1; i < points.size(); i++) {
             // Interpolating line fragments would be ugly when thick pen is used,
             // so we work around it by utilizing three separate
@@ -126,7 +129,7 @@ void LineChartItem::updateGeometry()
             // degrees and both of the points are within the margin, one in the top half and one in the
             // bottom half of the chart, the bottom one gets clipped incorrectly.
             // However, this should be rare occurrence in any sensible chart.
-            currentSeriesPoint = m_series->pointAt(i);
+            currentSeriesPoint = m_series->pointAt(qMin(seriesLastIndex, i));
             currentGeometryPoint = points.at(i);
             pointOffGrid = (currentSeriesPoint.x() < minX || currentSeriesPoint.x() > maxX);
 
diff --git a/src/scatterchart/scatterchartitem.cpp b/src/scatterchart/scatterchartitem.cpp
index fecbeb1b..6889fdc7 100644
--- a/src/scatterchart/scatterchartitem.cpp
+++ b/src/scatterchart/scatterchartitem.cpp
@@ -127,12 +127,20 @@ void ScatterChartItem::updateGeometry()
     QRectF clipRect(QPointF(0,0),domain()->size());
 
     QVector<bool> offGridStatus = offGridStatusVector();
+    const int seriesLastIndex = m_series->count() - 1;
 
     for (int i = 0; i < points.size(); i++) {
         QGraphicsItem *item = items.at(i);
         const QPointF &point = points.at(i);
         const QRectF &rect = item->boundingRect();
-        m_markerMap[item] = m_series->pointAt(i);
+        // During remove/append animation series may have different number of points,
+        // so ensure we don't go over the index. Animation handling itself ensures that
+        // if there is actually no points in the series, then it won't generate a fake point,
+        // so we can be assured there is always at least one point in m_series here.
+        // Note that marker map values can be technically incorrect during the animation,
+        // if it was caused by an insert, but this shouldn't be a problem as the points are
+        // fake anyway.
+        m_markerMap[item] = m_series->pointAt(qMin(seriesLastIndex, i));
         item->setPos(point.x() - rect.width() / 2, point.y() - rect.height() / 2);
 
         if (!m_visible || offGridStatus.at(i))
diff --git a/src/splinechart/splinechartitem.cpp b/src/splinechart/splinechartitem.cpp
index 4a46a724..4f1162c1 100644
--- a/src/splinechart/splinechartitem.cpp
+++ b/src/splinechart/splinechartitem.cpp
@@ -141,6 +141,9 @@ void SplineChartItem::updateGeometry()
         qreal rightMarginLine = centerPoint.x() + margin;
         qreal horizontal = centerPoint.y();
 
+        // See ScatterChartItem::updateGeometry() for explanation why seriesLastIndex is needed
+        const int seriesLastIndex = m_series->count() - 1;
+
         for (int i = 1; i < points.size(); i++) {
             // Interpolating spline fragments accurately is not trivial, and would anyway be ugly
             // when thick pen is used, so we work around it by utilizing three separate
@@ -154,7 +157,7 @@ void SplineChartItem::updateGeometry()
             // degrees and both of the points are within the margin, one in the top half and one in the
             // bottom half of the chart, the bottom one gets clipped incorrectly.
             // However, this should be rare occurrence in any sensible chart.
-            currentSeriesPoint = m_series->pointAt(i);
+            currentSeriesPoint = m_series->pointAt(qMin(seriesLastIndex, i));
             currentGeometryPoint = points.at(i);
             pointOffGrid = (currentSeriesPoint.x() < minX || currentSeriesPoint.x() > maxX);
 
diff --git a/src/xychart/xychart.cpp b/src/xychart/xychart.cpp
index 3d442c5a..a018119a 100644
--- a/src/xychart/xychart.cpp
+++ b/src/xychart/xychart.cpp
@@ -73,9 +73,13 @@ QVector<bool> XYChart::offGridStatusVector()
 
     QVector<bool> returnVector;
     returnVector.resize(m_points.size());
+    // During remove/append animation series may have different number of points,
+    // so ensure we don't go over the index. No need to check for zero points, this
+    // will not be called in such a situation.
+    const int seriesLastIndex = m_series->count() - 1;
 
     for (int i = 0; i < m_points.size(); i++) {
-        const QPointF &seriesPoint = m_series->pointAt(i);
+        const QPointF &seriesPoint = m_series->pointAt(qMin(seriesLastIndex, i));
         if (seriesPoint.x() < minX
             || seriesPoint.x() > maxX
             || seriesPoint.y() < minY
diff --git a/tests/auto/qxyseries/tst_qxyseries.cpp b/tests/auto/qxyseries/tst_qxyseries.cpp
index 7988e9b7..d3c15a1b 100644
--- a/tests/auto/qxyseries/tst_qxyseries.cpp
+++ b/tests/auto/qxyseries/tst_qxyseries.cpp
@@ -84,8 +84,13 @@ void tst_QXYSeries::seriesOpacity()
 void tst_QXYSeries::append_data()
 {
     QTest::addColumn< QList<QPointF> >("points");
-    QTest::newRow("0,0 1,1 2,2 3,3") << (QList<QPointF>() << QPointF(0,0) << QPointF(1,1) << QPointF(2,2) << QPointF(3,3));
-    QTest::newRow("0,0 -1,-1 -2,-2 -3,-3") << (QList<QPointF>() << QPointF(0,0) << QPointF(-1,-1) << QPointF(-2,-2) << QPointF(-3,-3));
+    QTest::addColumn< QList<QPointF> >("otherPoints");
+    QTest::newRow("0,0 1,1 2,2 3,3")
+            << (QList<QPointF>() << QPointF(0,0) << QPointF(1,1) << QPointF(2,2) << QPointF(3,3))
+            << (QList<QPointF>() << QPointF(4,4) << QPointF(5,5) << QPointF(6,6) << QPointF(7,7));
+    QTest::newRow("0,0 -1,-1 -2,-2 -3,-3")
+            << (QList<QPointF>() << QPointF(0,0) << QPointF(-1,-1) << QPointF(-2,-2) << QPointF(-3,-3))
+            << (QList<QPointF>() << QPointF(-4,-4) << QPointF(-5,-5) << QPointF(-6,-6) << QPointF(-7,-7));
 }
 
 
@@ -97,12 +102,19 @@ void tst_QXYSeries::append_raw_data()
 void tst_QXYSeries::append_raw()
 {
     QFETCH(QList<QPointF>, points);
+    QFETCH(QList<QPointF>, otherPoints);
     QSignalSpy spy0(m_series, SIGNAL(clicked(QPointF)));
     QSignalSpy addedSpy(m_series, SIGNAL(pointAdded(int)));
     m_series->append(points);
     TRY_COMPARE(spy0.count(), 0);
     TRY_COMPARE(addedSpy.count(), points.count());
     QCOMPARE(m_series->points(), points);
+
+    // Process events between appends
+    foreach (const QPointF &point, otherPoints) {
+        m_series->append(point);
+        QApplication::processEvents();
+    }
 }
 
 void tst_QXYSeries::chart_append_data()
@@ -196,6 +208,29 @@ void tst_QXYSeries::remove_raw()
         m_series->remove(points[i]);
     }
     QCOMPARE(m_series->points().count(), 0);
+
+    QApplication::processEvents();
+
+    // Process events between removes
+    m_series->append(points);
+    QCOMPARE(m_series->points(), points);
+    foreach (const QPointF &point, points) {
+        m_series->remove(point);
+        QApplication::processEvents();
+    }
+
+    // Actual meaningful delay between removes, but still shorter than animation duration
+    // (simulate e.g. spamming a hypothetical "remove last point"-button)
+    QList<QPointF> bunchOfPoints;
+    for (int i = 0; i < 10; i++)
+        bunchOfPoints.append(QPointF(i, (qreal) rand() / (qreal) RAND_MAX));
+    m_series->replace(bunchOfPoints);
+    QCOMPARE(m_series->points(), bunchOfPoints);
+    QTest::qWait(1500); // Wait for append animations to be over
+    for (int i = bunchOfPoints.count() - 1; i >= 0; i--) {
+        m_series->remove(bunchOfPoints.at(i));
+        QTest::qWait(50);
+    }
 }
 
 void tst_QXYSeries::remove_chart_data()
@@ -238,6 +273,8 @@ void tst_QXYSeries::clear_raw()
     m_series->clear();
     TRY_COMPARE(spy0.count(), 0);
     QCOMPARE(m_series->points().count(), 0);
+
+    QApplication::processEvents();
 }
 
 void tst_QXYSeries::clear_chart_data()
@@ -272,6 +309,7 @@ void tst_QXYSeries::replace_raw_data()
 void tst_QXYSeries::replace_raw()
 {
     QFETCH(QList<QPointF>, points);
+    QFETCH(QList<QPointF>, otherPoints);
     QSignalSpy pointReplacedSpy(m_series, SIGNAL(pointReplaced(int)));
     QSignalSpy pointsReplacedSpy(m_series, SIGNAL(pointsReplaced()));
     m_series->append(points);
@@ -303,6 +341,31 @@ void tst_QXYSeries::replace_raw()
     m_series->replace(allPoints);
     TRY_COMPARE(pointReplacedSpy.count(), points.count());
     TRY_COMPARE(pointsReplacedSpy.count(), 1);
+
+    m_series->replace(points);
+    QApplication::processEvents();
+
+    // Process events between replaces
+    for (int i = 0; i < points.count(); ++i) {
+        m_series->replace(points.at(i), otherPoints.at(i));
+        QApplication::processEvents();
+    }
+
+    newPoints = m_series->points();
+    QCOMPARE(newPoints.count(), points.count());
+    for (int i = 0; i < points.count(); ++i) {
+        QCOMPARE(otherPoints.at(i).x(), newPoints.at(i).x());
+        QCOMPARE(otherPoints.at(i).y(), newPoints.at(i).y());
+    }
+
+    // Append followed by a replace shouldn't crash
+    m_series->clear();
+    m_series->append(QPointF(22,22));
+    m_series->append(QPointF(23,23));
+    QApplication::processEvents();
+    m_series->replace(QPointF(23,23), otherPoints.at(1));
+    QCOMPARE(m_series->points().at(1).x(), otherPoints.at(1).x());
+    QCOMPARE(m_series->points().at(1).y(), otherPoints.at(1).y());
 }