From 5f4af1b5b5a96875b42750a778dcf6695a844623 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20Erik=20Nilsen?= Date: Fri, 22 Jan 2010 17:25:38 +0100 Subject: 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 --- src/gui/graphicsview/qgraphicsscene.cpp | 42 ++++++++++++++--------- src/gui/graphicsview/qgraphicsscene_p.h | 1 - tests/auto/qgraphicsscene/tst_qgraphicsscene.cpp | 43 ++++++++++++++++++++++-- 3 files changed, 68 insertions(+), 18 deletions(-) diff --git a/src/gui/graphicsview/qgraphicsscene.cpp b/src/gui/graphicsview/qgraphicsscene.cpp index 7e182d0..ae48bee 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::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 a3e36d0..54d8130 100644 --- a/src/gui/graphicsview/qgraphicsscene_p.h +++ b/src/gui/graphicsview/qgraphicsscene_p.h @@ -111,7 +111,6 @@ public: QVector unpolishedItems; QList 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 c08a628e..547e7f5 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 children = item->childItems(); + QCOMPARE(children.count(), 10); + foreach (QGraphicsItem *child, children) + QVERIFY(!static_cast(child)->polished); + + QApplication::processEvents(); + foreach (QGraphicsItem *child, children) + QVERIFY(static_cast(child)->polished); +} + void tst_QGraphicsScene::isActive() { QGraphicsScene scene1; -- cgit v0.12