Skip to content

Commit

Permalink
Fix crashes related to TabWidget::removeTab().
Browse files Browse the repository at this point in the history
- 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.
  • Loading branch information
acolwell authored and rodlie committed Jan 24, 2025
1 parent 7eea73e commit aefaa1c
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,8 @@ This is used internally by :func:`moveTab(tab,pane)<NatronGui.GuiApp.moveTab>`.

:param index: :class:`int`

Same as :func:`removeTab(tab)<NatronGui.PyTabWidget.removeTab>` but the *index* of a tab
is given instead.
Similar to :func:`removeTab(tab)<NatronGui.PyTabWidget.removeTab>` 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)
Expand Down
1 change: 1 addition & 0 deletions Gui/PythonPanels.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,7 @@ PyPanel::PyPanel(const QString& scriptName,

PyPanel::~PyPanel()
{
getGui()->unregisterTab(this);
getGui()->unregisterPyPanel(this);
}

Expand Down
24 changes: 15 additions & 9 deletions Gui/TabWidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1061,7 +1061,7 @@ TabWidget::insertTab(int index,
insertTab(index, QIcon(), widget, object);
}

PanelWidget*
void
TabWidget::removeTab(int index,
bool userAction)
{
Expand All @@ -1070,12 +1070,13 @@ TabWidget::removeTab(int index,

tabAt(index, &tab, &obj);
if (!tab || !obj) {
return 0;
return;
}

ViewerTab* isViewer = dynamic_cast<ViewerTab*>(tab);
Histogram* isHisto = dynamic_cast<Histogram*>(tab);
NodeGraph* isGraph = dynamic_cast<NodeGraph*>(tab);
Natron::Python::PyPanel* isPyPanel = dynamic_cast<Natron::Python::PyPanel*>(tab);
int newIndex = -1;
if (isGraph) {
/*
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand All @@ -1167,8 +1177,6 @@ TabWidget::removeTab(int index,
}
w->setParent(_imp->gui);


return tab;
} // TabWidget::removeTab

void
Expand All @@ -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);
}
}

Expand Down
11 changes: 7 additions & 4 deletions Gui/TabWidget.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down

0 comments on commit aefaa1c

Please sign in to comment.