From 122092707c608e930fbade660be2cb3f7a9c1946 Mon Sep 17 00:00:00 2001 From: Berthold Stoeger Date: Thu, 21 Jan 2021 13:51:03 +0100 Subject: [PATCH] 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 --- stats/statsview.cpp | 58 +++++++++++++++++++++++++++++++++++++-------- stats/statsview.h | 8 ++++++- 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; class RootNode : public QSGNode { public: - RootNode(QQuickWindow *w); + RootNode(StatsView &view); + ~RootNode(); + StatsView &view; std::unique_ptr backgroundNode; // solid background // We entertain one node per Z-level. std::array, (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(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(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.