From aefaa1c2a9663da81ec4b6253cc06900d836bca1 Mon Sep 17 00:00:00 2001 From: Aaron Colwell <300262+acolwell@users.noreply.github.com> Date: Tue, 14 Jan 2025 17:36:23 -0800 Subject: [PATCH] Fix crashes related to TabWidget::removeTab(). - Add code to ~PyPanel() to unregister itself. - Added code in TabWidget::removeTab() to specifically handle PyPanel objects. The code was crashing in cases where the removeTab() call was deleting the last reference to the PyPanel object and Python was destroying the object. The new code avoids use-after-free crashes in this situation. - Updated documentation to properly indicate that PyPanel objects may be destroyed if removeTab() deletes the last reference. - Changed TabWidget::removeTab() return value since the function can't guarantee a valid object for the type it was returning. Only one call site needed to be updated and it was effectively ignoring the value anyways. --- .../PythonReference/NatronGui/PyTabWidget.rst | 4 ++-- Gui/PythonPanels.cpp | 1 + Gui/TabWidget.cpp | 24 ++++++++++++------- Gui/TabWidget.h | 11 +++++---- 4 files changed, 25 insertions(+), 15 deletions(-) diff --git a/Documentation/source/devel/PythonReference/NatronGui/PyTabWidget.rst b/Documentation/source/devel/PythonReference/NatronGui/PyTabWidget.rst index 3f36c83704..bcc6f65c76 100644 --- a/Documentation/source/devel/PythonReference/NatronGui/PyTabWidget.rst +++ b/Documentation/source/devel/PythonReference/NatronGui/PyTabWidget.rst @@ -201,8 +201,8 @@ This is used internally by :func:`moveTab(tab,pane)`. :param index: :class:`int` -Same as :func:`removeTab(tab)` but the *index* of a tab -is given instead. +Similar to :func:`removeTab(tab)` but the *index* of a tab +is given instead. The tab may be destroyed if this object has the only reference to the tab. .. method:: NatronGui.PyTabWidget.setCurrentIndex(index) diff --git a/Gui/PythonPanels.cpp b/Gui/PythonPanels.cpp index ea0e5b29fe..82d44a9390 100644 --- a/Gui/PythonPanels.cpp +++ b/Gui/PythonPanels.cpp @@ -430,6 +430,7 @@ PyPanel::PyPanel(const QString& scriptName, PyPanel::~PyPanel() { + getGui()->unregisterTab(this); getGui()->unregisterPyPanel(this); } diff --git a/Gui/TabWidget.cpp b/Gui/TabWidget.cpp index 22dcc77856..312e409362 100644 --- a/Gui/TabWidget.cpp +++ b/Gui/TabWidget.cpp @@ -1061,7 +1061,7 @@ TabWidget::insertTab(int index, insertTab(index, QIcon(), widget, object); } -PanelWidget* +void TabWidget::removeTab(int index, bool userAction) { @@ -1070,12 +1070,13 @@ TabWidget::removeTab(int index, tabAt(index, &tab, &obj); if (!tab || !obj) { - return 0; + return; } ViewerTab* isViewer = dynamic_cast(tab); Histogram* isHisto = dynamic_cast(tab); NodeGraph* isGraph = dynamic_cast(tab); + Natron::Python::PyPanel* isPyPanel = dynamic_cast(tab); int newIndex = -1; if (isGraph) { /* @@ -1142,9 +1143,18 @@ TabWidget::removeTab(int index, //tab->setParent(_imp->gui); } - + // Note: The tab may get destroyed inside this call if the python variable on the app + // object that removeTabToPython() deletes is the only reference to the tab and the + // tab is a PyPanel created by Python code. _imp->removeTabToPython( tab, obj->getScriptName() ); + if (isPyPanel) { + // At this point tab might have been destroyed if it was a PyPanel + // and no other python code has a reference to it. + // tab, w, and obj are all likely invalid now so just return. + return; + } + if (userAction) { if (isViewer && _imp->gui) { /*special care is taken if this is a viewer: we also @@ -1154,7 +1164,7 @@ TabWidget::removeTab(int index, _imp->gui->removeHistogram(isHisto); //Return because at this point isHisto is invalid - return tab; + return; } else { ///Do not delete unique widgets such as the properties bin, node graph or curve editor w->setVisible(false); @@ -1167,8 +1177,6 @@ TabWidget::removeTab(int index, } w->setParent(_imp->gui); - - return tab; } // TabWidget::removeTab void @@ -1188,9 +1196,7 @@ TabWidget::removeTab(PanelWidget* widget, } if (index != -1) { - PanelWidget* tab = removeTab(index, userAction); - assert(tab == widget); - Q_UNUSED(tab); + removeTab(index, userAction); } } diff --git a/Gui/TabWidget.h b/Gui/TabWidget.h index b285c39b7f..6706372400 100644 --- a/Gui/TabWidget.h +++ b/Gui/TabWidget.h @@ -189,9 +189,10 @@ GCC_DIAG_SUGGEST_OVERRIDE_ON void insertTab(int index, PanelWidget* widget, ScriptObject* object); - /*Removes from the TabWidget, but does not delete the widget. - Returns NULL if the index is not in a good range.*/ - PanelWidget* removeTab(int index, bool userAction); + /*Removes from the TabWidget, but does not delete the widget if it is owned by C++ code. If + the widget was owned by Python code (e.g. a PyPanel), then the widget may get destroyed + if no other references exist in Python.*/ + void removeTab(int index, bool userAction); /*Get the header name of the tab at index "index".*/ QString getTabLabel(int index) const; @@ -201,7 +202,9 @@ GCC_DIAG_SUGGEST_OVERRIDE_ON void setTabLabel(PanelWidget* tab, const QString & name); - /*Removes from the TabWidget, but does not delete the widget.*/ + /*Removes from the TabWidget, but does not delete the widget if it is owned by C++ code. If + the widget was owned by Python code (e.g. a PyPanel), then the widget may get destroyed + if no other references exist in Python.*/ void removeTab(PanelWidget* widget, bool userAction); int count() const;