Potential crash when adding items from QGraphicsWidget::polishEvent().

These were processed immediately, so there was a fair chance that we
could end up doing a virtual function call on items that were not fully
constructed. This patch is also an optimization, since we never remove
anything from the vector.

Auto-test included.

Reviewed-by: Jan-Arve

3 files changed, 68 insertions, 18 deletions

diff --git a/src/gui/graphicsview/qgraphicsscene.cpp b/src/gui/graphicsview/qgraphicsscene.cpp
index 7e182d0af5..ae48bee86f 100644
--- a/src/gui/graphicsview/qgraphicsscene.cpp
+++ b/src/gui/graphicsview/qgraphicsscene.cpp
@@ -292,7 +292,6 @@ QGraphicsScenePrivate::QGraphicsScenePrivate()
       processDirtyItemsEmitted(false),
       selectionChanging(0),
       needSortTopLevelItems(true),
-      unpolishedItemsModified(true),
       holesInTopLevelSiblingIndex(false),
       topLevelSequentialOrdering(true),
       scenePosDescendantsUpdatePending(false),
@@ -429,23 +428,38 @@ void QGraphicsScenePrivate::unregisterTopLevelItem(QGraphicsItem *item)
 */
 void QGraphicsScenePrivate::_q_polishItems()
 {
-    QVector<QGraphicsItem *>::Iterator it = unpolishedItems.begin();
+    if (unpolishedItems.isEmpty())
+        return;
+
     const QVariant booleanTrueVariant(true);
-    while (!unpolishedItems.isEmpty()) {
-        QGraphicsItem *item = *it;
-        it = unpolishedItems.erase(it);
-        unpolishedItemsModified = false;
-        item->d_ptr->pendingPolish = false;
-        if (!item->d_ptr->explicitlyHidden) {
+    QGraphicsItem *item = 0;
+    QGraphicsItemPrivate *itemd = 0;
+    const int oldUnpolishedCount = unpolishedItems.count();
+
+    for (int i = 0; i < oldUnpolishedCount; ++i) {
+        item = unpolishedItems.at(i);
+        if (!item)
+            continue;
+        itemd = item->d_ptr.data();
+        itemd->pendingPolish = false;
+        if (!itemd->explicitlyHidden) {
             item->itemChange(QGraphicsItem::ItemVisibleChange, booleanTrueVariant);
             item->itemChange(QGraphicsItem::ItemVisibleHasChanged, booleanTrueVariant);
         }
-        if (item->isWidget()) {
+        if (itemd->isWidget) {
             QEvent event(QEvent::Polish);
             QApplication::sendEvent((QGraphicsWidget *)item, &event);
         }
-        if (unpolishedItemsModified)
-            it = unpolishedItems.begin();
+    }
+
+    if (unpolishedItems.count() == oldUnpolishedCount) {
+        // No new items were added to the vector.
+        unpolishedItems.clear();
+    } else {
+        // New items were appended; keep them and remove the old ones.
+        unpolishedItems.remove(0, oldUnpolishedCount);
+        unpolishedItems.squeeze();
+        QMetaObject::invokeMethod(q_ptr, "_q_polishItems", Qt::QueuedConnection);
     }
 }
 
@@ -641,10 +655,8 @@ void QGraphicsScenePrivate::removeItemHelper(QGraphicsItem *item)
     cachedItemsUnderMouse.removeAll(item);
     if (item->d_ptr->pendingPolish) {
         const int unpolishedIndex = unpolishedItems.indexOf(item);
-        if (unpolishedIndex != -1) {
-            unpolishedItems.remove(unpolishedIndex);
-            unpolishedItemsModified = true;
-        }
+        if (unpolishedIndex != -1)
+            unpolishedItems[unpolishedIndex] = 0;
         item->d_ptr->pendingPolish = false;
     }
     resetDirtyItem(item);
diff --git a/src/gui/graphicsview/qgraphicsscene_p.h b/src/gui/graphicsview/qgraphicsscene_p.h
index a3e36d0c97..54d813092f 100644
--- a/src/gui/graphicsview/qgraphicsscene_p.h
+++ b/src/gui/graphicsview/qgraphicsscene_p.h
@@ -111,7 +111,6 @@ public:
     QVector<QGraphicsItem *> unpolishedItems;
     QList<QGraphicsItem *> topLevelItems;
     bool needSortTopLevelItems;
-    bool unpolishedItemsModified;
     bool holesInTopLevelSiblingIndex;
     bool topLevelSequentialOrdering;
 
diff --git a/tests/auto/qgraphicsscene/tst_qgraphicsscene.cpp b/tests/auto/qgraphicsscene/tst_qgraphicsscene.cpp
index c08a628eed..547e7f539e 100644
--- a/tests/auto/qgraphicsscene/tst_qgraphicsscene.cpp
+++ b/tests/auto/qgraphicsscene/tst_qgraphicsscene.cpp
@@ -270,6 +270,7 @@ private slots:
     void initialFocus_data();
     void initialFocus();
     void polishItems();
+    void polishItems2();
     void isActive();
     void siblingIndexAlwaysValid();
 
@@ -3942,14 +3943,23 @@ void tst_QGraphicsScene::initialFocus()
 class PolishItem : public QGraphicsTextItem
 {
 public:
-    PolishItem(QGraphicsItem *parent = 0) : QGraphicsTextItem(parent) { }
+    PolishItem(QGraphicsItem *parent = 0)
+        : QGraphicsTextItem(parent), polished(false), deleteChildrenInPolish(true), addChildrenInPolish(false) { }
 
+    bool polished;
+    bool deleteChildrenInPolish;
+    bool addChildrenInPolish;
 protected:
     QVariant itemChange(GraphicsItemChange change, const QVariant& value)
     {
         if (change == ItemVisibleChange) {
-            if (value.toBool())
+            polished = true;
+            if (deleteChildrenInPolish)
                 qDeleteAll(childItems());
+            if (addChildrenInPolish) {
+                for (int i = 0; i < 10; ++i)
+                    new PolishItem(this);
+            }
         }
         return QGraphicsItem::itemChange(change, value);
     }
@@ -3966,6 +3976,35 @@ void tst_QGraphicsScene::polishItems()
     QMetaObject::invokeMethod(&scene,"_q_polishItems");
 }
 
+void tst_QGraphicsScene::polishItems2()
+{
+    QGraphicsScene scene;
+    PolishItem *item = new PolishItem;
+    item->addChildrenInPolish = true;
+    item->deleteChildrenInPolish = true;
+    // These children should be deleted in the polish.
+    for (int i = 0; i < 20; ++i)
+        new PolishItem(item);
+    scene.addItem(item);
+
+    // Wait for the polish event to be delivered.
+    QVERIFY(!item->polished);
+    QApplication::processEvents();
+    QVERIFY(item->polished);
+
+    // We deleted the children we added above, but we also
+    // added 10 new children. These should be polished in the next
+    // event loop iteration.
+    QList<QGraphicsItem *> children = item->childItems();
+    QCOMPARE(children.count(), 10);
+    foreach (QGraphicsItem *child, children)
+        QVERIFY(!static_cast<PolishItem *>(child)->polished);
+
+    QApplication::processEvents();
+    foreach (QGraphicsItem *child, children)
+        QVERIFY(static_cast<PolishItem *>(child)->polished);
+}
+
 void tst_QGraphicsScene::isActive()
 {
     QGraphicsScene scene1;