Fix Map destructor incorrectly deleting MapItemViews

Currently the views are explicitly destroyed, leading to
issues, in particular for views declared inside the Map.
Change it to simply remove these views from the map.
Child views will be destroyed in ~QObject.

[ChangeLog] Fixed crash when destroying Maps containing
MapItemViews.

Change-Id: Iff9b1afd6b17b55671b1f999b1bf69f172a05483
Fixes: QTBUG-69195
Fixes: QTBUG-74337
Reviewed-by: Eskil Abrahamsen Blomfeldt <eskil.abrahamsen-blomfeldt@qt.io>

3 files changed, 75 insertions, 40 deletions

diff --git a/src/location/declarativemaps/qdeclarativegeomap.cpp b/src/location/declarativemaps/qdeclarativegeomap.cpp
index 63587efe..106c4dae 100644
--- a/src/location/declarativemaps/qdeclarativegeomap.cpp
+++ b/src/location/declarativemaps/qdeclarativegeomap.cpp
@@ -234,33 +234,45 @@ QDeclarativeGeoMap::~QDeclarativeGeoMap()
         m_map->clearMapItems();
     }
 
-    // This forces the destruction of the associated items now, not when QObject destructor is called, at which point
-    // QDeclarativeGeoMap is long gone
+    // Remove the items from the map, making them deletable.
+    // Go in the same order as in removeMapChild: views, groups, then items
     if (!m_mapViews.isEmpty()) {
-        for (QDeclarativeGeoMapItemView *v : qAsConst(m_mapViews)) {
+        const auto mapViews = m_mapViews;
+        for (QDeclarativeGeoMapItemView *v : mapViews) { // so that removeMapItemView_real can safely modify m_mapViews;
             if (!v)
                 continue;
-            if (v->parent() == this) {
-                delete v;
-            } else {
-                // FIXME: removeInstantiatedItems should abort, as well as exit transitions terminated
-                v->removeInstantiatedItems(false);
-                v->m_map = nullptr;
-            }
+
+            QQuickItem *parent = v->parentItem();
+            QDeclarativeGeoMapItemGroup *group = qobject_cast<QDeclarativeGeoMapItemGroup *>(parent);
+            if (group)
+                continue; // Ignore non-top-level MIVs. They will be recursively processed.
+                          // Identify them as being parented by a MapItemGroup.
+
+            removeMapItemView_real(v);
         }
     }
-    // remove any map items associations
-    for (int i = 0; i < m_mapItems.count(); ++i) {
-        if (m_mapItems.at(i))
-            m_mapItems.at(i).data()->setMap(0,0);
-    }
 
-    // remove any map item groups associations
-    for (int i = 0; i < m_mapItemGroups.count(); ++i) {
-        if (m_mapItemGroups.at(i))
-            m_mapItemGroups.at(i).data()->setQuickMap(nullptr);
+    if (!m_mapItemGroups.isEmpty()) {
+        const auto mapGroups = m_mapItemGroups;
+        for (QDeclarativeGeoMapItemGroup *g : mapGroups) {
+            if (!g)
+                continue;
+
+            QQuickItem *parent =g->parentItem();
+            QDeclarativeGeoMapItemGroup *group = qobject_cast<QDeclarativeGeoMapItemGroup *>(parent);
+            if (group)
+                continue; // Ignore non-top-level Groups. They will be recursively processed.
+                          // Identify them as being parented by a MapItemGroup.
+
+            removeMapItemGroup_real(g);
+        }
     }
 
+    // remove any remaining map items associations
+    const auto mapItems = m_mapItems;
+    for (auto mi: mapItems)
+        removeMapItem_real(mi.data());
+
     if (m_copyrights.data())
         delete m_copyrights.data();
     m_copyrights.clear();
@@ -2094,14 +2106,10 @@ bool QDeclarativeGeoMap::removeMapItemView_real(QDeclarativeGeoMapItemView *item
     if (!itemView || itemView->m_map != this) // can't remove a view that is already added to another map
         return false;
 
-    // Leaving this as void since the removal is async (potentially transitioned)
-    // && the delegates *could* be empty mapItemGroups.
-    itemView->removeInstantiatedItems();
+    itemView->removeInstantiatedItems(false); // remove the items without using transitions AND abort ongoing ones
     itemView->m_map = 0;
-    // it can be removed from the list at this point, since no operations that require a Map have to be done
-    // anymore on destruction.
     m_mapViews.removeOne(itemView);
-    return removeMapItemGroup_real(itemView); // at this point, delegate instances have been removed.
+    return removeMapItemGroup_real(itemView); // at this point, all delegate instances have been removed.
 }
 
 /*!
diff --git a/src/location/declarativemaps/qdeclarativegeomapitemview.cpp b/src/location/declarativemaps/qdeclarativegeomapitemview.cpp
index d404fd47..90736d23 100644
--- a/src/location/declarativemaps/qdeclarativegeomapitemview.cpp
+++ b/src/location/declarativemaps/qdeclarativegeomapitemview.cpp
@@ -107,7 +107,8 @@ QDeclarativeGeoMapItemView::QDeclarativeGeoMapItemView(QQuickItem *parent)
 
 QDeclarativeGeoMapItemView::~QDeclarativeGeoMapItemView()
 {
-    removeInstantiatedItems();
+    // No need to remove instantiated items: if the MIV has instantiated items because it has been added
+    // to a Map (or is child of a Map), the Map destructor takes care of removing it and the instantiated items.
 }
 
 /*!
@@ -309,7 +310,7 @@ void QDeclarativeGeoMapItemView::removeInstantiatedItems(bool transition)
     if (!m_map)
         return;
 
-    // FIXME: removeInstantiatedItems should abort, as well as exit transitions terminated QTBUG-69195
+    // with transition = false removeInstantiatedItems aborts ongoing exit transitions //QTBUG-69195
     // Backward as removeItemFromMap modifies m_instantiatedItems
     for (int i = m_instantiatedItems.size() -1; i >= 0 ; i--)
         removeDelegateFromMap(i, transition);
@@ -356,24 +357,38 @@ QList<QQuickItem *> QDeclarativeGeoMapItemView::mapItems()
     return m_instantiatedItems;
 }
 
+QQmlInstanceModel::ReleaseFlags QDeclarativeGeoMapItemView::disposeDelegate(QQuickItem *item)
+{
+    disconnect(item, 0, this, 0);
+    removeDelegateFromMap(item);
+    item->setParentItem(nullptr);   // Needed because
+    item->setParent(nullptr);       // m_delegateModel->release(item) does not destroy the item most of the times!!
+    QQmlInstanceModel::ReleaseFlags releaseStatus = m_delegateModel->release(item);
+    return releaseStatus;
+}
+
 void QDeclarativeGeoMapItemView::removeDelegateFromMap(int index, bool transition)
 {
     if (index >= 0 && index < m_instantiatedItems.size()) {
         QQuickItem *item = m_instantiatedItems.takeAt(index);
         if (!item) { // not yet incubated
-            // Don't cancel incubation explicitly, as DelegateModel apparently takes care of incubating elements when the model
-            // remove those indices.
+            // Don't cancel incubation explicitly when model rows are removed, as DelegateModel
+            // apparently takes care of incubating elements when the model remove those indices.
+            // Cancel them explicitly only when a MIV is removed from a map.
+            if (!transition)
+                m_delegateModel->cancel(index);
             return;
         }
         // item can be either a QDeclarativeGeoMapItemBase or a QDeclarativeGeoMapItemGroup (subclass)
-        if (m_exit && m_map &&  transition) {
+        if (m_exit && m_map && transition) {
             transitionItemOut(item);
         } else {
-            disconnect(item, 0, this, 0);
-            removeDelegateFromMap(item);
-            item->setParentItem(nullptr);   // Needed because
-            item->setParent(nullptr);       // m_delegateModel->release(item) does not destroy the item most of the times!!
-            QQmlInstanceModel::ReleaseFlags releaseStatus = m_delegateModel->release(item);
+            if (m_exit && m_map && !transition) {
+                // check if the exit transition is still running, if so stop it.
+                // This can happen when explicitly calling Map.removeMapItemView, soon after adding it.
+                terminateExitTransition(item);
+            }
+            QQmlInstanceModel::ReleaseFlags releaseStatus = disposeDelegate(item);
 #ifdef QT_DEBUG
             if (releaseStatus == QQmlInstanceModel::Referenced)
                 qWarning() << "item "<< index << "(" << item << ") still referenced";
@@ -436,16 +451,26 @@ void QDeclarativeGeoMapItemView::transitionItemOut(QQuickItem *o)
     }
 }
 
+void QDeclarativeGeoMapItemView::terminateExitTransition(QQuickItem *o)
+{
+    QDeclarativeGeoMapItemGroup *group = qobject_cast<QDeclarativeGeoMapItemGroup *>(o);
+    if (group && group->m_transitionManager) {
+        group->m_transitionManager->cancel();
+        return;
+    }
+    QDeclarativeGeoMapItemBase *item = qobject_cast<QDeclarativeGeoMapItemBase *>(o);
+    if (item && item->m_transitionManager) {
+        item->m_transitionManager->cancel();
+        return;
+    }
+}
+
 void QDeclarativeGeoMapItemView::exitTransitionFinished()
 {
     QQuickItem *item = qobject_cast<QQuickItem *>(sender());
     if (!item)
         return;
-    disconnect(item, 0, this, 0);
-    removeDelegateFromMap(item);
-    item->setParentItem(nullptr);
-    item->setParent(nullptr);
-    QQmlInstanceModel::ReleaseFlags releaseStatus = m_delegateModel->release(item);
+    QQmlInstanceModel::ReleaseFlags releaseStatus = disposeDelegate(item);
 #ifdef QT_DEBUG
     if (releaseStatus == QQmlInstanceModel::Referenced)
         qWarning() << "item "<<item<<" still referenced";
diff --git a/src/location/declarativemaps/qdeclarativegeomapitemview_p.h b/src/location/declarativemaps/qdeclarativegeomapitemview_p.h
index 43ca685a..abac5bd5 100644
--- a/src/location/declarativemaps/qdeclarativegeomapitemview_p.h
+++ b/src/location/declarativemaps/qdeclarativegeomapitemview_p.h
@@ -130,6 +130,8 @@ private:
     void removeDelegateFromMap(int index, bool transition = true);
     void removeDelegateFromMap(QQuickItem *o);
     void transitionItemOut(QQuickItem *o);
+    void terminateExitTransition(QQuickItem *o);
+    QQmlInstanceModel::ReleaseFlags disposeDelegate(QQuickItem *item);
 
     void insertInstantiatedItem(int index, QQuickItem *o, bool createdItem);
     void addItemToMap(QDeclarativeGeoMapItemBase *item, int index, bool createdItem);