(Bug): Natron crashes after repeatedly opening and closing a custom pypanel with a simple QPushButton

[Emmett-Brown82]

Make sure to follow our issue report guidelines

• I'm using the latest version of Natron (not required but recommended)
• I've restarted Natron and the issue persists
• I've run Natron via the command line and the issue persists
• I've followed the contributing guidelines to the best of my understanding
• My issue is not on the issue tracker or in a pull request already (go search for it and dig around a little bit!)
• This bug is reproducible

Natron version

Natron 2.5.0

Operating system

Rocky Linux 8 (as a virtual machine with VMWare Workstation 17.5.2)

System specs

RAM: 32GB
Processor: AMD Ryzen
Graphics Card: Nvidia RTX 2050

Did you install Natron using the official installer?

• Yes, I used the official installer
• No, I installed from a binary archive
• No, I compiled Natron from sources
• No, I installed Natron via another method

Custom installation path

/opt/Natron-2.5.0

What were you trying to do?

Hi, i am trying to use the init.py and initGui.py scripts to register a custom pypanel. This custom pypanel only has a QPushButton and i add it through the self.addWidget(self.pushButton) method.

I do a stress test: opening and closing repeatedly the same tab with the button makes Natron crash at the 4th time. This is reproducible in windows and in Rocky linux 8 as well.

There seems to be a problem with the memory management between PySide 1.2.4 (Qt 4.8.7) and Python 3.10.5 (the one that ships with Natron 2.5.0)

The backtrace calls at some point the focusChanged signal of the QApplication. Seems the problem is there.

What did you expect to happen? What happened instead?

(C) 2018-2022 The Natron developers
(C) 2013-2018 INRIA and Alexandre Gauthier-Foichat

Use the --help or -h option to print usage.<<<
Info: init.py script found and loaded at /home/mmcfly/mcfly/mcfly/engine_provider/engine_plugins/natronlib/init.py
Info: initGui.py script found and loaded at /home/mmcfly/mcfly/mcfly/engine_provider/engine_plugins/natronlib/initGui.py
Caught segmentation fault (SIGSEGV) from thread Main(0x252eb50), faulty address is 0x7fd21ba5be07 from 0xf4240
Backtrace:
[Frame 1]: /lib64/libstdc++.so.6(__dynamic_cast+0x27) [0x7fd21ba5be07]
[Frame 2]: /lib64/libstdc++.so.6(__dynamic_cast+0x27) [0x7fd21ba5be07]
[Frame 3]: /opt/Natron-2.50/bin/Natron() [0x85adfd]
[Frame 4]: /opt/Natron-2.50/bin/Natron() [0x85aeb9]
[Frame 5]: /opt/Natron-2.50/bin/Natron() [0x85af3e]
[Frame 6]: /opt/Natron-2.50/bin/../lib/libQtCore.so.4(_ZN11QMetaObject8activateEP7QObjectPKS_iPPv+0x700) [0x7fd21c391530]
[Frame 7]: /opt/Natron-2.50/bin/../lib/libQtGui.so.4(ZN12QApplication12focusChangedEP7QWidgetS1+0x3e) [0x7fd21c856a1e]
[Frame 8]: /opt/Natron-2.50/bin/../lib/libQtGui.so.4(_ZN7QWidget8setFocusEN2Qt11FocusReasonE+0x1a2) [0x7fd21c8a48f2]
[Frame 9]: /opt/Natron-2.50/bin/Natron() [0x621c0d]
[Frame 10]: /opt/Natron-2.50/bin/../lib/libQtGui.so.4(_ZN7QWidget5eventEP6QEvent+0x19e) [0x7fd21c8a8bce]
[Frame 11]: /opt/Natron-2.50/bin/../lib/libQtGui.so.4(_ZN19QApplicationPrivate13notify_helperEP7QObjectP6QEvent+0x7d) [0x7fd21c85769d]
[Frame 12]: /opt/Natron-2.50/bin/../lib/libQtCore.so.4(_ZN16QCoreApplication14notifyInternalEP7QObjectP6QEvent+0xc3) [0x7fd21c380403]
[Frame 13]: /opt/Natron-2.50/bin/../lib/libQtGui.so.4(ZN19QApplicationPrivate18dispatchEnterLeaveEP7QWidgetS1+0x4a4) [0x7fd21c85c9a4]
[Frame 14]: /opt/Natron-2.50/bin/../lib/libQtGui.so.4(_ZN12QApplication15x11ProcessEventEP7_XEvent+0x121b) [0x7fd21c8cc56b]
[Frame 15]: /opt/Natron-2.50/bin/../lib/libQtGui.so.4(+0x2d7226) [0x7fd21c8f1226]
Natron Version 2.5

Step-by-step reproduction instructions

1. Start Natron
2. register a pypanel as specified in the docs in the pane2 (next to properties tab)
3. close the tab
4. open it again
5. close the tab
6. open it again.
7. close the tab --> the tab closes and so does Natron, with the provided backtrace dumped to a file or console.

Additional details

No response

[Emmett-Brown82]

Hi @acolwell !

here i provide an initGui.py script. Just pop it off somewhere and set the NATRON_PLUGIN_PATH to the containing folder of the script.

Find here attached an example of a the crash.

natron_test.mp4

import NatronGui

PANELNAME = "MYPYPANEL"

class NatronPluginPyPanel(NatronGui.PyPanel):

    def __init__(self, scriptName, label, useUserParameters, app):
        NatronGui.PyPanel.__init__(self,scriptName, label, useUserParameters, app)
        self.setObjectName(PANELNAME)
        self.show()


def appendPyPanel(*args, **kwargs):
    """
        app1 is the name of the current PyGui application instance.
        Looks weird. But works like this.
    :return:
    """
    close_if_exists()
    
    append_if_doesnt_exist()


def append_if_doesnt_exist():
    """
        Dont use update_gui -> processEvents()
    :return:
    """
    qapp = NatronGui.natron.getGuiInstance(0)
    if getattr(qapp, "panel", None) is None or qapp.getUserPanel(PANELNAME) is None:
        # since the script is not doing any use of the ParamHolder parameters we set it to False
        useUserParametersGUI = False
        panel = NatronPluginPyPanel("my_plugin", "MyPlugin", useUserParametersGUI, qapp)
        pane = qapp.getTabWidget("pane2")
        pane = qapp.getTabWidget("pane1") if pane is None else pane
        pane.appendTab(panel)
        
        # do this to revive the C++ ref object's ref count
        panel = __get_mcfly_panel_widget_v2()
        qapp.registerPythonPanel(panel, "MyPlugin")
        setattr(qapp, "panel", panel)
    
        
def close_if_exists():
    qapp = NatronGui.natron.getGuiInstance(0)
    panel = None
    if hasattr(qapp, "panel"):
        panel = getattr(qapp, "panel", None)
        if panel is not None:
            qapp.unregisterPythonPanel(panel)
        
    isclosed = __close_mcfly_panel_tab_v4()
     
    if isclosed:
       setattr(qapp, "panel", None)
        
def __close_mcfly_panel_tab_v4():
    qapp = NatronGui.natron.getGuiInstance(0)
    pane = qapp.getTabWidget("pane2")
    widget = __get_mcfly_panel_widget_v2()
    if widget is not None:
        for idx in range(pane.count()):
            tablbl = pane.getTabLabel(idx)
            if "myplugin" in tablbl.lower():
                pane.closeTab(idx)
                #print("closed!")
                return True
    return False

def __get_mcfly_panel_widget_v2():
    """
        Use this to find the McflyPyPanel
    :return:
    """
    try:
        import PySide2.QtWidgets as QtWidgets
    except ImportError as ie:
        import PySide.QtGui as QtWidgets
        
    for w in QtWidgets.QApplication.instance().topLevelWidgets():
        # this  line is Critical to revive the C++ Qt pointer
        pypanel = w.findChild(QtWidgets.QWidget, PANELNAME)
        if pypanel is not None: return pypanel
    return None

import NatronGui

menu_toolbox_path = str("JC/JC Create PyPanel")
menu_toolbox_funcname = str("appendPyPanel")


NatronGui.natron.addMenuCommand(menu_toolbox_path, menu_toolbox_funcname)

Thanks.

[acolwell]

@Emmett-Brown82 thanks for the plugin and repro video. I am able to reproduce this crash locally on Windows. I'm a little busy today and tomorrow, but I'll try to figure out what is causing this crash sometime this week.

[Emmett-Brown82]

@acolwell you are welcome!

If it helps, i think the problem could be related maybe to the way we retrieve the panel. In PySide1/2 it is well known that you need to retrieve the widget, not by a python variable, but doing the: self.findChild(QtWidgets.QWidget, "[widgetObjectName]") because retrieving the widgets this way makes sure the reference counter doesnt go to 0 and hence the C++ widget pointer is not accidentally deleted.

My bet, the problem is related to something like that.

Plus, if you have a look at the backtrace: it is the QApplication.instance().focusChanged signal emitted what is causing the memory access violation. Probably because the newly created widget doesnt exist anymore and the python variable exists, but the C++ object it is referencing, doesnt anymore.

Hope it helps.

By the way, it has the same behaviour with Natron 2.5.0 Official Release (PySide/Qt4).

Side note: i have experience integrating different plugins in different DCCs and one thing i dont understand is that, the UI is based on Qt right? then how come i am able to create a QApplication([]) inside Natron?? Shouldnt that be created by default?

Thanks for the effort @acolwell . Let me know if i can be of any help in the meantime.

Best,
JC

[acolwell]

So I'm going to preface this with a few things. I am a relatively new developer working on Natron. I've only been working on the project for 2 years and I have not done much with python plugins or the python bindings. The vast majority of the code was written before I got involved so I don't have any historical context as to why things are the way they are. I can only tell you what I observe the code doing and what the documentation states.

Here is what I've found so far:

1. The code is indeed crashing because it looks like Natron tries to set the visibility of a widget that has already been destroyed. I'm still trying to understand how the offending code is intended to work and the various object lifetimes. Things are not being destroyed in the order I'm expecting and I'm trying to track down why.
2. The plugin code you provided does not appear to be following the guidance in the documentation https://natron.readthedocs.io/en/rb-2.5/devel/pysidePanelExample.html#sourcecodeex . Specifically your code is using NatronGui.natron.getGuiInstance(0) instead of the app variable to access Natron's API. Because of this, your code is not working the way you might expect. AFIACT NatronGui.natron.getGuiInstance(0) will not return you the same object on successive calls. I suspect this might be surprising to you since it was a little surprising to me as well. I have no idea why the code is this way and have no idea whether other code historically depends on this behavior. Because different objects are returned, your code that adds an attribute to the app to store the panel doesn't likely do what you want it to (i.e. it is setting/getting on different objects). I believe you should change your code to use the app variable to access Natron like the documentation example does. I believe this should get you closer to your intended behavior.
3. The code in __get_mcfly_panel_widget_v2() seems unnecessary to me. You claim that it is "well known" that this is needed. Could you please point me to some references that back up this claim so I can understand if it truly applies to Natron or not. Based on what I see, this code is causing you to replace a panel you just created with python with the old widget that you just closed. This seems rather odd and likely part of the problem. I can't know for sure until I see some documentation about why such code might be necessary. Nothing like this appears in the example code in the Natron documentation so it leads me to wonder if it is really needed. Any references, with further context, to help me understand would be greatly appreciated.

In general, I agree that Natron should not crash even if the python code is not quite right. I'm still trying to figure out what exactly the intended behavior/control flow/lifetimes are supposed to be so please bear with me. In the meantime, I hope these few findings might help you modify your code so you an avoid the crashing behavior for now. I'll keep digging to see if I can make Natron more robust in this area.

[Emmett-Brown82]

Hi @acolwell thanks for the effort and your quick reply.
I'll try to stick to the three points to mentioned earlier after performing some tests and accomodating the code to the one the official documentation provides.

1. Yes, the biggest problem lies IMHO in the fact that the Qt/PySide widgets are not correctly managed in a Qt "main event management" application logic. Thus, destroy and recreating a widget becomes problematic. I ll get back at this later on.
2. AFAIK i havent had any different behaviour if using the self declared variable "app1" besides the method "getGuiInstance(0)". But, just in case, i did as u suggested and tested this following new code "ala" Official Natron Documentation:

from qtpy.QtGui import *
from qtpy.QtCore import *
from qtpy.QtWidgets import *
#To import the variable "natron"
from NatronGui import *

#Callback called when a parameter of the player changes
#The variable paramName is declared by Natron; indicating the name of the parameter which just had its value changed
def myPlayerParamChangedCallback(paramName, app, userEdited):

    viewer = app.getViewer("Viewer1")
    if viewer == None:
        return
    if paramName == "previous":
        viewer.seek(viewer.getCurrentFrame() - 1)
    elif paramName == "backward":
        viewer.startBackward()
    elif paramName == "forward":
        viewer.startForward()
    elif paramName == "next":
        viewer.seek(viewer.getCurrentFrame() + 1)
    elif paramName == "stop":
        viewer.pause()

def createMyPlayer():

    app.player = NatronGui.PyPanel("fr.inria.myplayer","My Player",True,app)
    app.player.previousFrameButton = app.player.createButtonParam("previous","Previous")
    app.player.previousFrameButton.setAddNewLine(False)

    app.player.playBackwardButton = app.player.createButtonParam("backward","Rewind")
    app.player.playBackwardButton.setAddNewLine(False)
    
    app.player.stopButton = app.player.createButtonParam("stop","Pause")
    app.player.stopButton.setAddNewLine(False)

    app.player.playForwardButton = app.player.createButtonParam("forward","Play")
    app.player.playForwardButton.setAddNewLine(False)

    app.player.nextFrameButton = app.player.createButtonParam("next","Next")

    app.player.helpLabel = app.player.createStringParam("help","Help")
    app.player.helpLabel.setType(NatronEngine.StringParam.TypeEnum.eStringTypeLabel)
    app.player.helpLabel.set("<br><b>Previous:</b> Seek the previous frame on the timeline</br>"
                        "<br><b>Rewind:</b> Play backward</br>"
                        "<br><b>Pause:</b> Pauses the playback</br>"
                        "<br><b>Play:</b> Play forward</br>"
                        "<br><b>Next:</b> Seek the next frame on the timeline</br>")
                        
    app.player.refreshUserParamsGUI()
    app.player.setParamChangedCallback("myPlayerParamChangedCallback")

    #Add it to the "pane2" tab widget
    app.pane2.appendTab(app.player);
    
    #Register the tab to the application, so it is saved into the layout of the project
    #and can appear in the Panes sub-menu of the "Manage layout" button (in top left-hand corner of each tab widget)
    app.registerPythonPanel(app.player,"createMyPlayer")


#A small panel to load and visualize icons/images
class IconViewer(NatronGui.PyPanel):

    #Register a custom signal
    userFileChanged = QtCore.Signal()

    #Slots should be decorated:
    #http://qt-project.org/wiki/Signals_and_Slots_in_PySide
    
    #This is called upon a user click on the button
    @QtCore.Slot()
    def onButtonClicked(self):
        location = self.currentApp.getFilenameDialog(("jpg","png","bmp","tif"))
        if location:
            self.locationEdit.setText(location)
            
            #Save the file
            self.onUserDataChanged()
        
        self.userFileChanged.emit()
    
    #This is called when the user finish editing of the line edit (when return is pressed or focus out)
    @QtCore.Slot()
    def onLocationEditEditingFinished(self):
        #Save the file
        self.onUserDataChanged()
        self.userFileChanged.emit()
    
    #This is called when our custom userFileChanged signal is emitted
    @QtCore.Slot()
    def onFileChanged(self):
        self.label.setPixmap(QPixmap(self.locationEdit.text()))
    
    
    def __init__(self,scriptName,label,app):
        
        #Init base class, important! otherwise signals/slots won't work.
        NatronGui.PyPanel.__init__(self,scriptName, label, False, app)
        
        #Store the current app as it might no longer be pointing to the app at the time being called
        #when a slot will be invoked later on
        self.currentApp = app
        
        #Set the layout
        self.setLayout( QVBoxLayout())
        
        #Create a widget container for the line edit + button
        fileContainer = QWidget(self)
        fileLayout = QHBoxLayout()
        fileContainer.setLayout(fileLayout)
        
        #Create the line edit, make it expand horizontally
        self.locationEdit = QLineEdit(fileContainer)
        self.locationEdit.setSizePolicy(QSizePolicy.Expanding, QSizePolicy.Preferred)
        
        #Create a pushbutton
        self.button = QPushButton(fileContainer)
        #Decorate it with the open-file pixmap built-in into Natron
        buttonPixmap = natron.getIcon(NatronEngine.Natron.PixmapEnum.NATRON_PIXMAP_OPEN_FILE)
        self.button.setIcon(QIcon(buttonPixmap))
        
        #Add widgets to the layout
        fileLayout.addWidget(self.locationEdit)
        fileLayout.addWidget(self.button)
        
        #Use a QLabel to display the images
        self.label = QLabel(self)
        
        #Init the label with the icon of Natron
        natronPixmap = natron.getIcon(NatronEngine.Natron.PixmapEnum.NATRON_PIXMAP_APP_ICON)
        self.label.setPixmap(natronPixmap)
        #Built-in icons of Natron are in the resources
        self.locationEdit.setText(":/Resources/Images/natronIcon256_linux.png")
        
        #Make it expand in both directions so it takes all space
        self.label.setSizePolicy(QSizePolicy.Expanding, QSizePolicy.Expanding)

        #Add widgets to the layout
        self.layout().addWidget(fileContainer)
        self.layout().addWidget(self.label)
        
        #Make signal/slot connections
        self.button.clicked.connect(self.onButtonClicked)
        self.locationEdit.editingFinished.connect(self.onLocationEditEditingFinished)
        self.userFileChanged.connect(self.onFileChanged)

    # We override the save() function and save the filename
    def save(self):
        return self.locationEdit.text()

    # We override the restore(data) function and restore the current image
    def restore(self,data):

        self.locationEdit.setText(data)
        self.label.setPixmap(QPixmap(data))

#To be called to create a new icon viewer panel:
#Note that *app* should be defined. Generally when called from onProjectCreatedCallback
#this is set, but when called from the Script Editor you should set it yourself beforehand:
#app = app1
#See http://natron.readthedocs.org/en/python/natronobjects.html for more info
def createIconViewer():
    
    if hasattr(app,"p"):
        #The icon viewer already exists, it we override the app.p variable, then it will destroy the previous widget
        #and create a new one but we don't really need it
        
        #The warning will be displayed in the Script Editor
        print("Note for us developers: this widget already exists!")
        
        ## JC Test 1: Dont destroy the widget, just get it back from living QObject
        #app.pane2.appendTab(app.p)
        #return 
        ## End Test 1
        
        ## JC Test 2: Do destroy the widget, create it back again
        app.pane2.removeTab(app.p)
        app.p.setParent(None)
        app.p.deleteLater()
        app.p = None
        # End Test 2
    
    #Create our icon viewer
    app.p = IconViewer("fr.inria.iconViewer","Icon viewer",app)
    
    #Add it to the "pane2" tab widget
    app.pane2.appendTab(app.p);
    
    #Register the tab to the application, so it is saved into the layout of the project
    #and can appear in the Panes sub-menu of the "Manage layout" button (in top left-hand corner of each tab widget)
    app.registerPythonPanel(app.p,"createIconViewer")


#Callback set in the "After project created" parameter in the Preferences-->Python tab of Natron
#This will automatically create our panels when a new project is created
def onProjectCreatedCallback(app):
    #Always create our icon viewer on project creation, you must register this call-back in the
    #"After project created callback" parameter of the Preferences-->Python tab.
    createIconViewer()

    createMyPlayer()

#Add a custom menu entry with a shortcut to create our icon viewer
natron.addMenuCommand("Inria/Scripts/IconViewer","createIconViewer",QtCore.Qt.Key.Key_L,QtCore.Qt.KeyboardModifier.ShiftModifier)

You will notice the exact same code as u suggested but with three "JC Tweak Test 1, 2,3" in lines ranging from 177-190. Those three tests are independent to one another. I made this to prove my point:

def createIconViewer():
    
    if hasattr(app,"p"):
        #The icon viewer already exists, it we override the app.p variable, then it will destroy the previous widget
        #and create a new one but we don't really need it
        
        #The warning will be displayed in the Script Editor
        print("Note for us developers: this widget already exists!")
        
        ## JC Test 1: Dont destroy the widget, just get it back from living QObject
        #app.pane2.appendTab(app.p)
        #return 
        ## End Test 1
        
        ## JC Test 2: Do destroy the widget, create it back again
        #app.pane2.removeTab(app.p)
        #app.p.setParent(None)
        #app.p.deleteLater()
        #app.p = None
        # End Test 2
        
        ## JC Test 3: do destroy the widget with closeTab instead
        app.pane2.closeTab(1)
        app.p = None
        # End of Test 3

Test 1: No QWidget / Panel destroyed! ==> when closing the tabWidget apparently the Qt object is still alive, so appending it again and removing, appending without destroying it, works Ok. Unfortunately, for the purposes of integrating Natron into a set of tools, we should be able to recreate the widget: destroying it and then creating it again by allocating a new pointer.

Test 2: Do destroy the widget "ala" Qt mode: call from which doesnt destroy the widget in Natron, and the set the parent's widget to None and call deleteLater() Qt destructor in the next Qt event loop. Result: this causes a crash after repeatedly reopening the panel. (see attached .mp4)

natron-test-2.mp4

Test 3: Do destroy the panel but this time by calling which does destroy the widget. Result: still same crash.

3. The "well known" thing is just that this is how Qt works and more specifically how Qt bindings like PyQtX or PySideX work. I will try to provide some documentation reference.

I will try to download the source code and have a look as well, although it is been ages since i havent coded in C++, so, in that sense, bear with me as well a little bit ;). As soon as i have any piece of news i ll let u know.

UPDATE: if you test exclusively the code provided by the official docs you got after several clicks when opening the IconViewer a crash. Try it not editing the code with the <JC Tweak Test 1,2,3>, you will notice it crashes. (just leave the print inside the <if hasattr(...)>

UPDATE 2: the Qt event loop is working fine. The widgets are capturing the events correctly, so basically, the QApplication class instance and the lack of it in Natron, may not be exactly the problem. The focus IMHO should be put in the lifetime cycle of QWidgets. The problem is not how PySide1/2 manages QWidgets' lifetime, but rather, how Natron handles Qt objects when destroying them, as you stated in your last post. I totally agree with that. Following this, i will try to provide documentation, but QWidgets pointers are "reference-counted" in Qt. My hint would be to try to track down when this count is zero and see if it makes sense that the object being destroyed. I wouldnt mind to help by videoconferencing at some point if needed.

Best,
JC

[acolwell]

@Emmett-Brown82 thanks for your responses and patience. I believe I found the source of the crashes and I created pull request #1030 that I believe should fix them. Basically pointers to deleted widgets were getting used and triggering crashes.

You should be able to try out your code in this Windows installer build that I did before creating the pull request. This is not an official build obviously, but it does contain the fix in the pull request and it would give you an opportunity to test the fix ASAP.

[Emmett-Brown82]

Hi @acolwell !

Thanks for the effort.

Everything is working fine now. I am able to close (destroy the widget) and create it again. I also noticed you already modified the docs as claimed in your #1030 . I believe the two existing methods: removeTab() and closeTab() are now merged into only removeTab() which takes the role of the latter in not only removing the widget but also destroying it.

Can i ask when are we expecting the 2.6.0 release? or, if it is really taking more time, is there a 2.5.1 Official release planned for the next days? it would be awesome to have this fix delivered as soon as possible, as i believe it is affecting Natron since 2.5.0 Official (Windows and Linux, and PySide1/PySide2), and probably way before this release. Furthermore, i am working in Linux, so i would have to get the Linux Release of Natron 2.X.Y (whichever version should be).

Thanks for your work and the quick reply. This was a major fix that affects independent platform releases.

If i can do anything else to help, just name it.

Best,
JC