wxWidgets / Phoenix

wxPython's Project Phoenix. A new implementation of wxPython, better, stronger, faster than he was before.
http://wxpython.org/
2.22k stars 509 forks source link

Crash when creating submenus #1648

Closed dhyams closed 3 years ago

dhyams commented 4 years ago

Operating system: Windows 10 or Mac OSX wxPython version & source: 4.1.0 binary from PyPI Python version & source: 2.7.12

Description of the problem:

If one tries to create a submenu with a wx.MenuItem, it crashes the application with a segmentation fault. Happens if you just right click to pop the menu, and then click off to dismiss the menu. To demonstrate, change the following line in the PopupMenu.py wxPython demo. This is right around line 96. PopupMenu_bug.zip

        #menu.Append(self.popupID7, "Test Submenu", sm) # original line of code
        sm = wx.MenuItem(menu, self.popupID7, "Test Submenu", subMenu=sm) 
        menu.Append(sm)        

Doing it that way had always worked in wxPython 3. Attached is the full demo code that crashes.

dhyams commented 4 years ago

Update: I found that if I instead replaced with

   #menu.Append(self.popupID7, "Test Submenu", sm) # original line of code
   item = wx.MenuItem(menu, self.popupID7, "Test Submenu", subMenu=sm) 
   menu.Append(item)        

it does not crash and works just fine. However, the change of variable name should not matter; it seems like the garbage collection is jumping in too early to destroy the submenu referred to by sm, perhaps??

I'm not sure what is happening here.

Metallicow commented 4 years ago

https://wxpython.org/Phoenix/docs/html/wx.Menu.html#wx.Menu.Append The documentation says it is deprecated. ... so yea, you can expect it to just disappear at any time... apparently this was that time.

dhyams commented 4 years ago

But; I'm not using the depreciated overload of wx.Menu.Append; I'm using the very last one, which is not. (In fact the deprecated one is the one currently used in the demo, but as you can see in the comments above, I'm commenting that one out, in favor of the two lines below).

dhyams commented 4 years ago

Trying to illustrate better what is happening; here is another sample. Here is a close surrogate to what my original application was doing. Creating the submenu in its own function and passing it back without saving it in a variable first (thereby not increasing its reference count). This code is a slightly modified form of the PopupMenu.py demo. I've marked what I modified with hashes:

        menu.Append(self.popupID6, "Six")

        ################
        # This crashes, because no one has held a reference to the submenu on the Python side.
        item = wx.MenuItem(menu, self.popupID7, "Test Submenu", subMenu=self.CreateSubMenu()) 
        menu.Append(item)
        #################

        # Popup the menu.  If an item is selected then its handler
        # will be called before PopupMenu returns.
        self.PopupMenu(menu)
        menu.Destroy()

    ####################
    def CreateSubMenu(self):
        sm = wx.Menu()
        sm.Append(self.popupID8, "sub item 1")
        sm.Append(self.popupID9, "sub item 2")
        return sm
    ####################

This second one doesn't crash, because I save a reference to the submenu on the Python side. But I shouldn't have to do that.

        menu.Append(self.popupID6, "Six")

        ################
        # This works without crashing, because we hold a reference on the Python side
        sm = self.CreateSubMenu()
        item = wx.MenuItem(menu, self.popupID7, "Test Submenu", subMenu=sm) 
        menu.Append(item)
        #################

        # Popup the menu.  If an item is selected then its handler
        # will be called before PopupMenu returns.
        self.PopupMenu(menu)
        menu.Destroy()

    ####################
    def CreateSubMenu(self):
        sm = wx.Menu()
        sm.Append(self.popupID8, "sub item 1")
        sm.Append(self.popupID9, "sub item 2")
        return sm
    ####################
swt2c commented 4 years ago

Your reproducer code isn't complete unfortunately:

$ python3 PopupMenu_bug.py 
Traceback (most recent call last):
  File "PopupMenu_bug.py", line 5, in <module>
    import images
ModuleNotFoundError: No module named 'images'
dhyams commented 4 years ago

The code that I submitted was meant to be run in the demo directory of the wxPython demos, because I was trying to change as little as possible to reproduce the bug. I'll resubmit a code shortly that is standalone.

dhyams commented 4 years ago

Attached is a standalone code that demonstrates.

PopupMenu_bug_standalone.zip

swt2c commented 4 years ago

Sorry about that. I missed the part of it being part of the demo. I see the problem now.

dhyams commented 4 years ago

No problem! Glad to provide a standalone to make things easier.

RobinD42 commented 3 years ago

This issue has been mentioned on Discuss wxPython. There might be relevant details there:

https://discuss.wxpython.org/t/seg-fault-with-menus/34699/5

RobinD42 commented 3 years ago

BTW,

However, the change of variable name should not matter; it seems like the garbage collection is jumping in too early to destroy the submenu referred to by sm, perhaps??

It's because you were assigning the wx.MenuItem to sm, thereby disconnecting it from the previous object sm was referring to (the wx.Menu). So yes, because of the ownership bug the menu was being collected early after that line of code.

Metallicow commented 3 years ago

@dhyams I apologize if I was slightly off in recognizing the issue. Your example explanation, and @swt2c solution shows that indeed you was correct on the bug issue. I just never noticed, I guess.... I'm happy that another bug amongs't many has gotten squashed. Had not you made this issue, I think others might have either missed it or it would have got lost. Thank you, for your efforts!

Metallicow commented 3 years ago

This is one of those examples where 49 of the 50 students learned to do it 1 way and the other learned both ways. theoretically, both answers are right depending on how you look at the problem, but the 49 others either will say you are wrong or igrore you for a corner case.

Good catch. This is what I would call a pass thru issue.

sveinbr commented 3 years ago

I think this error is still present in 4.1.1 - it can be reproduced with this code:

def do_crash():
    import wx
    ans = []
    app = wx.App(False)
    frame = wx.Frame(None)
    m = wx.Menu()
    m.Append(wx.Window.NewControlId(),'OK')
    m.Bind(wx.EVT_MENU, lambda x: ans.append('OK'))
    frame.PopupMenu(m, pos = wx.GetMousePosition())
    return ans[0]
print(do_crash())

which shows a popup menu with one choice 'OK', and produces the following output when you click on it:

OK Segmentation fault

swt2c commented 3 years ago

@sveinbr that may be a separate bug. This bug was about submenus. I would open a new issue.