Avoid needless notifications when destroying layouts

When deleting a layout with children, it ends up in ~QQuickItem(), which in
turn will call setParentItem(0).  setParentItem(0) will in turn call
setEffectiveVisibleRecur(), which will recurse down all its descendants.
Therefore, deleting a top level layout might trigger item change listeners
for *all* its descendants, not only its direct children.

This behavior might even cause crashes: The visibility changes will then
trigger an invalidation of the layout, which will propagate up the parent
hierarchy, and potentially call invalidate() on a partially-destroyed
layout, which then might crash.

Change-Id: I48e11d57f69e9011ced6c3a0b51e3d89b24ad5c1
Task-number: QTBUG-55103
Reviewed-by: Shawn Rutledge <shawn.rutledge@qt.io>

3 files changed, 29 insertions, 5 deletions

diff --git a/src/imports/layouts/qquicklayout.cpp b/src/imports/layouts/qquicklayout.cpp
index 3786d21727..fd2ff4a73e 100644
--- a/src/imports/layouts/qquicklayout.cpp
+++ b/src/imports/layouts/qquicklayout.cpp
@@ -762,9 +762,11 @@ bool QQuickLayout::shouldIgnoreItem(QQuickItem *child, QQuickLayoutAttached *&in
 void QQuickLayout::itemChange(ItemChange change, const ItemChangeData &value)
 {
     if (change == ItemChildAddedChange) {
+        Q_D(QQuickLayout);
         QQuickItem *item = value.item;
         qmlobject_connect(item, QQuickItem, SIGNAL(baselineOffsetChanged(qreal)), this, QQuickLayout, SLOT(invalidateSenderItem()));
         QQuickItemPrivate::get(item)->addItemChangeListener(this, QQuickItemPrivate::SiblingOrder | QQuickItemPrivate::ImplicitWidth | QQuickItemPrivate::ImplicitHeight | QQuickItemPrivate::Destroyed | QQuickItemPrivate::Visibility);
+        d->m_hasItemChangeListeners = true;
         if (isReady())
             updateLayoutItems();
     } else if (change == ItemChildRemovedChange) {
@@ -802,6 +804,30 @@ bool QQuickLayout::isReady() const
     return d_func()->m_isReady;
 }
 
+/*!
+ * \brief QQuickLayout::deactivateRecur
+ * \internal
+ *
+ * Call this from the dtor of the top-level layout.
+ * Otherwise, it will trigger lots of unneeded item change listeners (itemVisibleChanged()) for all its descendants
+ * that will have its impact thrown away.
+ */
+void QQuickLayout::deactivateRecur()
+{
+    if (d_func()->m_hasItemChangeListeners) {
+        for (int i = 0; i < itemCount(); ++i) {
+            QQuickItem *item = itemAt(i);
+            // When deleting a layout with children, there is no reason for the children to inform the layout that their
+            // e.g. visibility got changed. The layout already knows that all its children will eventually become invisible, so
+            // we therefore remove its change listener.
+            QQuickItemPrivate::get(item)->removeItemChangeListener(this, QQuickItemPrivate::SiblingOrder | QQuickItemPrivate::ImplicitWidth | QQuickItemPrivate::ImplicitHeight | QQuickItemPrivate::Destroyed | QQuickItemPrivate::Visibility);
+            if (QQuickLayout *layout = qobject_cast<QQuickLayout*>(item))
+                layout->deactivateRecur();
+        }
+        d_func()->m_hasItemChangeListeners = false;
+    }
+}
+
 void QQuickLayout::itemSiblingOrderChanged(QQuickItem *item)
 {
     Q_UNUSED(item);
diff --git a/src/imports/layouts/qquicklayout_p.h b/src/imports/layouts/qquicklayout_p.h
index eece6f8658..113498eb2b 100644
--- a/src/imports/layouts/qquicklayout_p.h
+++ b/src/imports/layouts/qquicklayout_p.h
@@ -95,6 +95,7 @@ public:
     void itemChange(ItemChange change, const ItemChangeData &value) Q_DECL_OVERRIDE;
     void geometryChanged(const QRectF &newGeometry, const QRectF &oldGeometry)  Q_DECL_OVERRIDE;
     bool isReady() const;
+    void deactivateRecur();
 
 
     /* QQuickItemChangeListener */
@@ -134,6 +135,7 @@ public:
 protected:
     unsigned m_isReady : 1;
     unsigned m_disableRearrange : 1;
+    unsigned m_hasItemChangeListeners : 1;      // if false, we don't need to remove its item change listeners...
     mutable QSet<QQuickItem *> m_ignoredItems;
 };
 
diff --git a/src/imports/layouts/qquicklinearlayout.cpp b/src/imports/layouts/qquicklinearlayout.cpp
index 13fdd496c2..50b3eed87e 100644
--- a/src/imports/layouts/qquicklinearlayout.cpp
+++ b/src/imports/layouts/qquicklinearlayout.cpp
@@ -305,11 +305,7 @@ QQuickGridLayoutBase::~QQuickGridLayoutBase()
 
     // Remove item listeners so we do not act on signalling unnecessarily
     // (there is no point, as the layout will be torn down anyway).
-    for (int i = 0; i < itemCount(); ++i) {
-        QQuickItem *item = itemAt(i);
-        QQuickItemPrivate::get(item)->removeItemChangeListener(this, QQuickItemPrivate::SiblingOrder | QQuickItemPrivate::ImplicitWidth | QQuickItemPrivate::ImplicitHeight | QQuickItemPrivate::Destroyed | QQuickItemPrivate::Visibility);
-    }
-
+    deactivateRecur();
     delete d->styleInfo;
 }