From f848df74c3a0624e0dbce4ad968926d6fe5e4499 Mon Sep 17 00:00:00 2001 From: tmartsum Date: Fri, 8 Dec 2023 06:47:02 +0100 Subject: [PATCH] Improve safety in code (#588) Delete areas later so that that they can be accessed by (inherited) dock widgets in dtor. Add some QPointer to prevent crashes. Hence allow users to do more while dock widgets etc are being destroyed. --- src/DockAreaWidget.cpp | 7 ++--- src/DockContainerWidget.cpp | 33 +++++++++++++------ src/DockFocusController.cpp | 1 + src/DockManager.cpp | 63 +++++++++++++++++++++++-------------- src/DockWidget.cpp | 2 +- 5 files changed, 69 insertions(+), 37 deletions(-) diff --git a/src/DockAreaWidget.cpp b/src/DockAreaWidget.cpp index bcc05aa..b289415 100644 --- a/src/DockAreaWidget.cpp +++ b/src/DockAreaWidget.cpp @@ -756,7 +756,7 @@ int CDockAreaWidget::openDockWidgetsCount() const int Count = 0; for (int i = 0; i < d->ContentsLayout->count(); ++i) { - if (!dockWidget(i)->isClosed()) + if (dockWidget(i) && !dockWidget(i)->isClosed()) { ++Count; } @@ -772,7 +772,7 @@ QList CDockAreaWidget::openedDockWidgets() const for (int i = 0; i < d->ContentsLayout->count(); ++i) { CDockWidget* DockWidget = dockWidget(i); - if (!DockWidget->isClosed()) + if (DockWidget && !DockWidget->isClosed()) { DockWidgetList.append(dockWidget(i)); } @@ -786,7 +786,7 @@ int CDockAreaWidget::indexOfFirstOpenDockWidget() const { for (int i = 0; i < d->ContentsLayout->count(); ++i) { - if (!dockWidget(i)->isClosed()) + if (dockWidget(i) && !dockWidget(i)->isClosed()) { return i; } @@ -809,7 +809,6 @@ CDockWidget* CDockAreaWidget::dockWidget(int Index) const return qobject_cast(d->ContentsLayout->widget(Index)); } - //============================================================================ void CDockAreaWidget::reorderDockWidget(int fromIndex, int toIndex) { diff --git a/src/DockContainerWidget.cpp b/src/DockContainerWidget.cpp index 7abf7ae..b9610ef 100644 --- a/src/DockContainerWidget.cpp +++ b/src/DockContainerWidget.cpp @@ -141,7 +141,7 @@ public: CDockContainerWidget* _this; QPointer DockManager; unsigned int zOrderIndex = 0; - QList DockAreas; + QList> DockAreas; QList AutoHideWidgets; QMap SideTabBarWidgets; QGridLayout* Layout = nullptr; @@ -299,7 +299,11 @@ public: VisibleDockAreaCount = 0; for (auto DockArea : DockAreas) { - VisibleDockAreaCount += DockArea->isHidden() ? 0 : 1; + if (!DockArea) + { + continue; + } + VisibleDockAreaCount += (DockArea->isHidden() ? 0 : 1); } } @@ -924,7 +928,10 @@ void DockContainerWidgetPrivate::addDockAreasToList(const QList NewDockAreas) { - DockAreas.append(NewDockAreas); + for (auto *newDockArea : NewDockAreas) + { + DockAreas.append(newDockArea); + } for (auto DockArea : NewDockAreas) { QObject::connect(DockArea, @@ -1641,7 +1648,7 @@ CDockAreaWidget* CDockContainerWidget::dockAreaAt(const QPoint& GlobalPos) const { for (const auto& DockArea : d->DockAreas) { - if (DockArea->isVisible() && DockArea->rect().contains(DockArea->mapFromGlobal(GlobalPos))) + if (DockArea && DockArea->isVisible() && DockArea->rect().contains(DockArea->mapFromGlobal(GlobalPos))) { return DockArea; } @@ -1678,7 +1685,7 @@ int CDockContainerWidget::visibleDockAreaCount() const int Result = 0; for (auto DockArea : d->DockAreas) { - Result += DockArea->isHidden() ? 0 : 1; + Result += (!DockArea || DockArea->isHidden()) ? 0 : 1; } return Result; @@ -1802,7 +1809,7 @@ QList CDockContainerWidget::openedDockAreas() const QList Result; for (auto DockArea : d->DockAreas) { - if (!DockArea->isHidden()) + if (DockArea && !DockArea->isHidden()) { Result.append(DockArea); } @@ -1818,7 +1825,7 @@ QList CDockContainerWidget::openedDockWidgets() const QList DockWidgetList; for (auto DockArea : d->DockAreas) { - if (!DockArea->isHidden()) + if (DockArea && !DockArea->isHidden()) { DockWidgetList.append(DockArea->openedDockWidgets()); } @@ -1833,7 +1840,7 @@ bool CDockContainerWidget::hasOpenDockAreas() const { for (auto DockArea : d->DockAreas) { - if (!DockArea->isHidden()) + if (DockArea && !DockArea->isHidden()) { return true; } @@ -2058,6 +2065,10 @@ QList CDockContainerWidget::dockWidgets() const QList Result; for (const auto DockArea : d->DockAreas) { + if (!DockArea) + { + continue; + } Result.append(DockArea->dockWidgets()); } @@ -2090,6 +2101,10 @@ CDockWidget::DockWidgetFeatures CDockContainerWidget::features() const CDockWidget::DockWidgetFeatures Features(CDockWidget::AllDockWidgetFeatures); for (const auto DockArea : d->DockAreas) { + if (!DockArea) + { + continue; + } Features &= DockArea->features(); } @@ -2109,7 +2124,7 @@ void CDockContainerWidget::closeOtherAreas(CDockAreaWidget* KeepOpenArea) { for (const auto DockArea : d->DockAreas) { - if (DockArea == KeepOpenArea) + if (!DockArea || DockArea == KeepOpenArea) { continue; } diff --git a/src/DockFocusController.cpp b/src/DockFocusController.cpp index a6e05cb..27acc4a 100644 --- a/src/DockFocusController.cpp +++ b/src/DockFocusController.cpp @@ -115,6 +115,7 @@ DockFocusControllerPrivate::DockFocusControllerPrivate( //============================================================================ void DockFocusControllerPrivate::updateDockWidgetFocus(CDockWidget* DockWidget) { + if (!DockWidget) return; if (!DockWidget->features().testFlag(CDockWidget::DockWidgetFocusable)) { return; diff --git a/src/DockManager.cpp b/src/DockManager.cpp index 0f87c58..0415bbe 100644 --- a/src/DockManager.cpp +++ b/src/DockManager.cpp @@ -103,8 +103,8 @@ static QString FloatingContainersTitle; struct DockManagerPrivate { CDockManager* _this; - QList FloatingWidgets; - QList HiddenFloatingWidgets; + QList> FloatingWidgets; + QList> HiddenFloatingWidgets; QList Containers; CDockOverlay* ContainerOverlay; CDockOverlay* DockAreaOverlay; @@ -153,7 +153,10 @@ struct DockManagerPrivate // Hide updates of floating widgets from user for (auto FloatingWidget : FloatingWidgets) { - FloatingWidget->hide(); + if (FloatingWidget) + { + FloatingWidget->hide(); + } } } @@ -333,7 +336,8 @@ bool DockManagerPrivate::restoreStateFromXml(const QByteArray &state, int versi int FloatingWidgetIndex = DockContainerCount - 1; for (int i = FloatingWidgetIndex; i < FloatingWidgets.count(); ++i) { - auto* floatingWidget = FloatingWidgets[i]; + CFloatingDockContainer* floatingWidget = FloatingWidgets[i]; + if (!floatingWidget) continue; _this->removeDockContainer(floatingWidget->dockContainer()); floatingWidget->deleteLater(); } @@ -536,25 +540,27 @@ CDockManager::CDockManager(QWidget *parent) : CDockManager::~CDockManager() { // fix memory leaks, see https://github.com/githubuser0xFFFF/Qt-Advanced-Docking-System/issues/307 - std::vector areas; - for ( int i = 0; i != dockAreaCount(); ++i ) - { - auto area = dockArea(i); - if ( area->dockManager() == this ) - areas.push_back( area ); - // else, this is surprising, looks like this CDockAreaWidget is child of two different CDockManager - // this is reproductible by https://github.com/githubuser0xFFFF/Qt-Advanced-Docking-System/issues/585 testcase - // then, when a CDockManager deletes itself, it deletes the CDockAreaWidget, then, the other - // CDockManager will try to delete the CDockAreaWidget again and this will crash - // So let's just delete CDockAreaWidget we are the parent of! - } - for ( auto area : areas ) - { - for ( auto widget : area->dockWidgets() ) - delete widget; + std::vector> areas; + for (int i = 0; i != dockAreaCount(); ++i) + { + areas.push_back( dockArea(i) ); + } + for ( auto area : areas ) + { + if (!area || area->dockManager() != this) continue; - delete area; - } + // QPointer delete safety - just in case some dock wigdet in destruction + // deletes another related/twin or child dock widget. + std::vector> deleteWidgets; + for ( auto widget : area->dockWidgets() ) + { + deleteWidgets.push_back(widget); + } + for ( auto ptrWdg : deleteWidgets) + { + delete ptrWdg; + } + } auto FloatingWidgets = d->FloatingWidgets; for (auto FloatingWidget : FloatingWidgets) @@ -562,6 +568,12 @@ CDockManager::~CDockManager() delete FloatingWidget; } + // Delete Dock Widgets before Areas so widgets can access them late (like dtor) + for ( auto area : areas ) + { + delete area; + } + delete d; } @@ -732,7 +744,12 @@ const QList CDockManager::dockContainers() const //============================================================================ const QList CDockManager::floatingWidgets() const { - return d->FloatingWidgets; + QList res; + for (auto &fl : d->FloatingWidgets) + { + if (fl) res.append(fl); + } + return res; } diff --git a/src/DockWidget.cpp b/src/DockWidget.cpp index 2d4cfe2..d89ef00 100644 --- a/src/DockWidget.cpp +++ b/src/DockWidget.cpp @@ -81,7 +81,7 @@ struct DockWidgetPrivate CDockWidgetTab* TabWidget = nullptr; CDockWidget::DockWidgetFeatures Features = CDockWidget::DefaultDockWidgetFeatures; CDockManager* DockManager = nullptr; - CDockAreaWidget* DockArea = nullptr; + QPointer DockArea; QAction* ToggleViewAction = nullptr; bool Closed = false; QScrollArea* ScrollArea = nullptr;