Fix focus proxy deletion bugs/crashes in QGraphicsItem.

This change would have been much simpler if either QGraphicsItem
inherited QObject, or if we had some similar QPointer-like class that
supported QGraphicsItem. The issue is this: Each item can delegate
another item to be its focus proxy. That item can be a parent or child,
or something completely unrelated. Either of the two items can be
deleted independently.

The former solution was to store backpointers in a map in the scene.
Problem is, the items may not be in a scene when this happens, they
may be removed from the scene, and the items may be moved between
two scenes.

The bad part about this fix is that it adds another pointer to
QGraphicsItemPrivate.

Reviewed-by: Shane Kearns <shane.kearns@sosco.com>

5 files changed, 66 insertions, 14 deletions

diff --git a/src/gui/graphicsview/qgraphicsitem.cpp b/src/gui/graphicsview/qgraphicsitem.cpp
index c3e5501..537dab7 100644
--- a/src/gui/graphicsview/qgraphicsitem.cpp
+++ b/src/gui/graphicsview/qgraphicsitem.cpp
@@ -1196,10 +1196,12 @@ QGraphicsItem::~QGraphicsItem()
         Q_ASSERT(d_ptr->children.isEmpty());
     }
 
-    if (d_ptr->scene)
+    if (d_ptr->scene) {
         d_ptr->scene->d_func()->removeItemHelper(this);
-    else
+    } else {
+        d_ptr->resetFocusProxy();
         d_ptr->setParentItemHelper(0);
+    }
 
     if (d_ptr->transformData) {
         for(int i = 0; i < d_ptr->transformData->graphicsTransforms.size(); ++i) {
@@ -2613,13 +2615,11 @@ void QGraphicsItem::setFocusProxy(QGraphicsItem *item)
     }
 
     QGraphicsItem *lastFocusProxy = d_ptr->focusProxy;
+    if (lastFocusProxy)
+        lastFocusProxy->d_ptr->focusProxyRefs.removeOne(&d_ptr->focusProxy);
     d_ptr->focusProxy = item;
-    if (d_ptr->scene) {
-        if (lastFocusProxy)
-            d_ptr->scene->d_func()->focusProxyReverseMap.remove(lastFocusProxy, this);
-        if (item)
-            d_ptr->scene->d_func()->focusProxyReverseMap.insert(item, this);
-    }
+    if (item)
+        item->d_ptr->focusProxyRefs << &d_ptr->focusProxy;
 }
 
 /*!
@@ -4626,6 +4626,19 @@ void QGraphicsItemPrivate::clearSubFocus()
 /*!
     \internal
 
+    Sets the focusProxy pointer to 0 for all items that have this item as their
+    focusProxy. ### Qt 5: Use QPointer instead.
+*/
+void QGraphicsItemPrivate::resetFocusProxy()
+{
+    for (int i = 0; i < focusProxyRefs.size(); ++i)
+        *focusProxyRefs.at(i) = 0;
+    focusProxyRefs.clear();
+}
+
+/*!
+    \internal
+
     Tells us if it is a proxy widget
 */
 bool QGraphicsItemPrivate::isProxyWidget() const
diff --git a/src/gui/graphicsview/qgraphicsitem_p.h b/src/gui/graphicsview/qgraphicsitem_p.h
index 982cdfc..c654d4f 100644
--- a/src/gui/graphicsview/qgraphicsitem_p.h
+++ b/src/gui/graphicsview/qgraphicsitem_p.h
@@ -398,6 +398,7 @@ public:
 
     void setSubFocus();
     void clearSubFocus();
+    void resetFocusProxy();
 
     inline QTransform transformToParent() const;
     inline void ensureSortedChildren();
@@ -419,6 +420,7 @@ public:
     int siblingIndex;
     int depth;
     QGraphicsItem *focusProxy;
+    QList<QGraphicsItem **> focusProxyRefs;
     QGraphicsItem *subFocusItem;
     Qt::InputMethodHints imHints;
 
diff --git a/src/gui/graphicsview/qgraphicsscene.cpp b/src/gui/graphicsview/qgraphicsscene.cpp
index bdd1ac6..2178850 100644
--- a/src/gui/graphicsview/qgraphicsscene.cpp
+++ b/src/gui/graphicsview/qgraphicsscene.cpp
@@ -494,11 +494,7 @@ void QGraphicsScenePrivate::removeItemHelper(QGraphicsItem *item)
     item->d_func()->scene = 0;
 
     // Unregister focus proxy.
-    QMultiHash<QGraphicsItem *, QGraphicsItem *>::iterator it = focusProxyReverseMap.find(item);
-    while (it != focusProxyReverseMap.end() && it.key() == item) {
-        it.value()->d_ptr->focusProxy = 0;
-        it = focusProxyReverseMap.erase(it);
-    }
+    item->d_ptr->resetFocusProxy();
 
     // Remove from parent, or unregister from toplevels.
     if (QGraphicsItem *parentItem = item->parentItem()) {
diff --git a/src/gui/graphicsview/qgraphicsscene_p.h b/src/gui/graphicsview/qgraphicsscene_p.h
index 8b53306..836522d 100644
--- a/src/gui/graphicsview/qgraphicsscene_p.h
+++ b/src/gui/graphicsview/qgraphicsscene_p.h
@@ -131,7 +131,6 @@ public:
     QGraphicsWidget *activeWindow;
     int activationRefCount;
     void setFocusItemHelper(QGraphicsItem *item, Qt::FocusReason focusReason);
-    QMultiHash<QGraphicsItem *, QGraphicsItem *> focusProxyReverseMap;
 
     QList<QGraphicsWidget *> popupWidgets;
     void addPopup(QGraphicsWidget *widget);
diff --git a/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp b/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp
index eadf8b7..a623b50 100644
--- a/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp
+++ b/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp
@@ -280,6 +280,7 @@ private slots:
     void autoDetectFocusProxy();
     void subFocus();
     void reverseCreateAutoFocusProxy();
+    void focusProxyDeletion();
 
     // task specific tests below me
     void task141694_textItemEnsureVisible();
@@ -7516,5 +7517,46 @@ void tst_QGraphicsItem::reverseCreateAutoFocusProxy()
     QVERIFY(text2->hasFocus());
 }
 
+void tst_QGraphicsItem::focusProxyDeletion()
+{
+    QGraphicsRectItem *rect = new QGraphicsRectItem;
+    QGraphicsRectItem *rect2 = new QGraphicsRectItem;
+    rect->setFocusProxy(rect2);
+    QCOMPARE(rect->focusProxy(), (QGraphicsItem *)rect2);
+
+    delete rect2;
+    QCOMPARE(rect->focusProxy(), (QGraphicsItem *)0);
+
+    rect2 = new QGraphicsRectItem;
+    rect->setFocusProxy(rect2);
+    delete rect; // don't crash
+
+    rect = new QGraphicsRectItem;
+    rect->setFocusProxy(rect2);
+    QGraphicsScene *scene = new QGraphicsScene;
+    scene->addItem(rect);
+    scene->addItem(rect2);
+    delete rect2;
+    QCOMPARE(rect->focusProxy(), (QGraphicsItem *)0);
+
+    rect2 = new QGraphicsRectItem;
+    QTest::ignoreMessage(QtWarningMsg, "QGraphicsItem::setFocusProxy: focus proxy must be in same scene");
+    rect->setFocusProxy(rect2);
+    QCOMPARE(rect->focusProxy(), (QGraphicsItem *)0);
+    scene->addItem(rect2);
+    rect->setFocusProxy(rect2);
+    QCOMPARE(rect->focusProxy(), (QGraphicsItem *)rect2);
+    delete rect; // don't crash
+
+    rect = new QGraphicsRectItem;
+    rect2 = new QGraphicsRectItem;
+    rect->setFocusProxy(rect2);
+    QCOMPARE(rect->focusProxy(), (QGraphicsItem *)rect2);
+    scene->addItem(rect);
+    scene->addItem(rect2);
+    rect->setFocusProxy(rect2);
+    delete scene; // don't crash
+}
+
 QTEST_MAIN(tst_QGraphicsItem)
 #include "tst_qgraphicsitem.moc"