statistics: delete chart items when root node is deleted

When reparenting the statistics widget, QtQuick deletes
the rootNode and all the child nodes. It is unclear whether
this is a bug or intended behavior. In any case, it means
that the pointers to QSG nodes in the chart items become
stale.

To avoid this, delete all chart items in the root node's
destructor, before QtQuick can do anything. It is unclear
from which context this is called (render or UI) and whether
this is even valid. In some tests, it seemed to work.

The difficulty is that all the stale pointers to chart items
have to be deleted as well. All in all, the QSG memory
management is a big nuisance and very brittle.

Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

2 files changed, 55 insertions, 11 deletions

diff --git a/stats/statsview.cpp b/stats/statsview.cpp
index eb51810f8..5514aa35d 100644
--- a/stats/statsview.cpp
+++ b/stats/statsview.cpp
@@ -89,18 +89,20 @@ using ZNode = HideableQSGNode<QSGNode>;
 class RootNode : public QSGNode
 {
 public:
-	RootNode(QQuickWindow *w);
+	RootNode(StatsView &view);
+	~RootNode();
+	StatsView &view;
 	std::unique_ptr<QSGRectangleNode> backgroundNode; // solid background
 	// We entertain one node per Z-level.
 	std::array<std::unique_ptr<ZNode>, (size_t)ChartZValue::Count> zNodes;
 };
 
-RootNode::RootNode(QQuickWindow *w)
+RootNode::RootNode(StatsView &view) : view(view)
 {
 	// Add a background rectangle with a solid color. This could
 	// also be done on the widget level, but would have to be done
 	// separately for desktop and mobile, so do it here.
-	backgroundNode.reset(w->createRectangleNode());
+	backgroundNode.reset(view.w()->createRectangleNode());
 	backgroundNode->setColor(backgroundColor);
 	appendChildNode(backgroundNode.get());
 
@@ -110,21 +112,31 @@ RootNode::RootNode(QQuickWindow *w)
 	}
 }
 
-QSGNode *StatsView::updatePaintNode(QSGNode *oldNode, QQuickItem::UpdatePaintNodeData *)
+RootNode::~RootNode()
 {
-	// The QtQuick drawing interface is utterly bizzare with a distinct 1980ies-style memory management.
-	// This is just a copy of what is found in Qt's documentation.
-	RootNode *n = static_cast<RootNode *>(oldNode);
-	if (!n)
-		n = rootNode = new RootNode(window());
+	view.emergencyShutdown();
+}
 
-	// Delete all chart items that are marked for deletion.
+void StatsView::freeDeletedChartItems()
+{
 	ChartItem *nextitem;
 	for (ChartItem *item = deletedItems.first; item; item = nextitem) {
 		nextitem = item->next;
 		delete item;
 	}
 	deletedItems.clear();
+}
+
+QSGNode *StatsView::updatePaintNode(QSGNode *oldNode, QQuickItem::UpdatePaintNodeData *)
+{
+	// The QtQuick drawing interface is utterly bizzare with a distinct 1980ies-style memory management.
+	// This is just a copy of what is found in Qt's documentation.
+	RootNode *n = static_cast<RootNode *>(oldNode);
+	if (!n)
+		n = rootNode = new RootNode(*this);
+
+	// Delete all chart items that are marked for deletion.
+	freeDeletedChartItems();
 
 	if (backgroundDirty) {
 		rootNode->backgroundNode->setRect(plotRect);
@@ -140,6 +152,32 @@ QSGNode *StatsView::updatePaintNode(QSGNode *oldNode, QQuickItem::UpdatePaintNod
 	return n;
 }
 
+// When reparenting the QQuickWidget, QtQuick decides to delete our rootNode
+// and with it all the QSG nodes, even though we have *not* given the
+// permission to do so! If the widget is reused, we try to delete the
+// stale items, whose nodes have already been deleted by QtQuick, leading
+// to a double-free(). Instead of searching for the cause of this behavior,
+// let's just hook into the rootNodes destructor and delete the objects
+// in a controlled manner, so that QtQuick has no more access to them.
+void StatsView::emergencyShutdown()
+{
+	// Mark clean and dirty chart items for deletion...
+	cleanItems.splice(deletedItems);
+	dirtyItems.splice(deletedItems);
+
+	// ...and delete them.
+	freeDeletedChartItems();
+
+	// Now delete all the pointers we might have to chart features,
+	// axes, etc. Note that all pointers to chart items are non
+	// owning, so this only resets stale references, but does not
+	// lead to any additional deletion of chart items.
+	reset();
+
+	// The rootNode is being deleted -> remove the reference to that
+	rootNode = nullptr;
+}
+
 void StatsView::addQSGNode(QSGNode *node, ChartZValue z)
 {
 	int idx = std::clamp((int)z, 0, (int)ChartZValue::Count - 1);
diff --git a/stats/statsview.h b/stats/statsview.h
index 3623c01eb..cc497792b 100644
--- a/stats/statsview.h
+++ b/stats/statsview.h
@@ -43,13 +43,14 @@ public:
 	~StatsView();
 
 	void plot(const StatsState &state);
-	void updateFeatures(const StatsState &state); // Updates the visibility of chart features, such as legend, regression, etc.
+	void updateFeatures(const StatsState &state);	// Updates the visibility of chart features, such as legend, regression, etc.
 	QQuickWindow *w() const;			// Make window available to items
 	QSizeF size() const;
 	QRectF plotArea() const;
 	void addQSGNode(QSGNode *node, ChartZValue z);	// Must only be called in render thread!
 	void registerChartItem(ChartItem &item);
 	void registerDirtyChartItem(ChartItem &item);
+	void emergencyShutdown();			// Called when QQuick decides to delete out root node.
 
 	// Create a chart item and add it to the scene.
 	// The item must not be deleted by the caller, but can be
@@ -151,6 +152,10 @@ private:
 
 	// There are three double linked lists of chart items:
 	// clean items, dirty items and items to be deleted.
+	// Note that only the render thread must delete chart items,
+	// and therefore these lists are the only owning pointers
+	// to chart items. All other pointers are non-owning and
+	// can therefore become stale.
 	struct ChartItemList {
 		ChartItemList();
 		ChartItem *first, *last;
@@ -161,6 +166,7 @@ private:
 	};
 	ChartItemList cleanItems, dirtyItems, deletedItems;
 	void deleteChartItemInternal(ChartItem &item);
+	void freeDeletedChartItems();
 };
 
 // This implementation detail must be known to users of the class.