From a5f7f88932c6911fb65552d65d62efdcf496beec Mon Sep 17 00:00:00 2001
From: Alexis Menard <alexis.menard@nokia.com>
Date: Thu, 22 Oct 2009 18:11:34 +0200
Subject: Fix crash in QGraphicsView BSP discovered in Amarok.

Basically some items were not properly remove in the BSP which means
that if you delete one of items, the BSP tree may contain dangling pointers.
The problem was in removeItemHelper in QGraphicsScene were the child
were removed after reparenting to 0 the topmost parent. The
sceneBoundingRect for children was invalid which means that we were removing
them in the wrong position inside the BSP. Reparenting to 0 means that the
sceneBoundingRect will be the boundingRect but wasn't the case before
(for the topmost parent).

Reviewed-by:bnilsen
---
 src/gui/graphicsview/qgraphicsscene.cpp            | 14 +++---
 .../tst_qgraphicssceneindex.cpp                    | 58 ++++++++++++++++++++++
 2 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/src/gui/graphicsview/qgraphicsscene.cpp b/src/gui/graphicsview/qgraphicsscene.cpp
index a624b10..03c8a97 100644
--- a/src/gui/graphicsview/qgraphicsscene.cpp
+++ b/src/gui/graphicsview/qgraphicsscene.cpp
@@ -518,6 +518,14 @@ void QGraphicsScenePrivate::removeItemHelper(QGraphicsItem *item)
 
     item->d_func()->scene = 0;
 
+    //We need to remove all children first because they might use their parent
+    //attributes (e.g. sceneTransform).
+    if (!item->d_ptr->inDestructor) {
+        // Remove all children recursively
+        for (int i = 0; i < item->d_ptr->children.size(); ++i)
+            q->removeItem(item->d_ptr->children.at(i));
+    }
+
     // Unregister focus proxy.
     item->d_ptr->resetFocusProxy();
 
@@ -564,12 +572,6 @@ void QGraphicsScenePrivate::removeItemHelper(QGraphicsItem *item)
             ++iterator;
     }
 
-    if (!item->d_ptr->inDestructor) {
-        // Remove all children recursively
-        for (int i = 0; i < item->d_ptr->children.size(); ++i)
-            q->removeItem(item->d_ptr->children.at(i));
-    }
-
     if (item->isPanel() && item->isVisible() && item->panelModality() != QGraphicsItem::NonModal)
         leaveModal(item);
 
diff --git a/tests/auto/qgraphicssceneindex/tst_qgraphicssceneindex.cpp b/tests/auto/qgraphicssceneindex/tst_qgraphicssceneindex.cpp
index 1109e5e..9dfd486 100644
--- a/tests/auto/qgraphicssceneindex/tst_qgraphicssceneindex.cpp
+++ b/tests/auto/qgraphicssceneindex/tst_qgraphicssceneindex.cpp
@@ -66,6 +66,7 @@ private slots:
     void movingItems();
     void connectedToSceneRectChanged();
     void items();
+    void removeItems();
     void clear();
 
 private:
@@ -268,6 +269,63 @@ void tst_QGraphicsSceneIndex::items()
     QCOMPARE(scene.items().size(), 3);
 }
 
+class RectWidget : public QGraphicsWidget
+{
+    Q_OBJECT
+public:
+    RectWidget(QGraphicsItem *parent = 0) : QGraphicsWidget(parent)
+    {
+    }
+
+    void paint(QPainter *painter, const QStyleOptionGraphicsItem *option, QWidget *widget)
+    {
+        painter->setBrush(brush);
+        painter->drawRect(boundingRect());
+    }
+public:
+    QBrush brush;
+};
+
+void tst_QGraphicsSceneIndex::removeItems()
+{
+     QGraphicsScene scene;
+
+    RectWidget *parent = new RectWidget;
+    parent->brush = QBrush(QColor(Qt::magenta));
+    parent->setGeometry(250, 250, 400, 400);
+
+    RectWidget *widget = new RectWidget(parent);
+    widget->brush = QBrush(QColor(Qt::blue));
+    widget->setGeometry(10, 10, 200, 200);
+
+    RectWidget *widgetChild1 = new RectWidget(widget);
+    widgetChild1->brush = QBrush(QColor(Qt::green));
+    widgetChild1->setGeometry(20, 20, 100, 100);
+
+    RectWidget *widgetChild2 = new RectWidget(widgetChild1);
+    widgetChild2->brush = QBrush(QColor(Qt::yellow));
+    widgetChild2->setGeometry(25, 25, 50, 50);
+
+    scene.addItem(parent);
+
+    QGraphicsView view(&scene);
+    view.resize(600, 600);
+    view.show();
+    QApplication::setActiveWindow(&view);
+    QTest::qWaitForWindowShown(&view);
+
+    QApplication::processEvents();
+
+    scene.removeItem(widgetChild1);
+
+    delete widgetChild1;
+
+    //We move the parent
+    scene.items(295, 295, 50, 50);
+
+    //This should not crash
+}
+
 void tst_QGraphicsSceneIndex::clear()
 {
     class MyItem : public QGraphicsItem
-- 
cgit v0.12