Opened 11 months ago

Closed 9 months ago

#15743 closed defect (fixed)

Crash when a TLW Destroy()-ed when its parent is already in process of destruction

Reported by: catalin Owned by: vadz
Priority: low Milestone: 3.0.1
Component: GUI-all Version: dev-latest
Keywords: Cc:
Blocked By: Blocking:
Patch: yes

Description

A wxColourDialog must be explicitly deleted, otherwise it leaks memory.

If its parent is a non-TLW, when Destroy()-ed from the TLW's destructor it crashes the app.

A patch against minimal sample that reproduces the problem is attached.

Attachments (4)

test15743.patch download (1.6 KB) - added by catalin 11 months ago.
test15743_2.patch download (1.3 KB) - added by catalin 11 months ago.
test15743_3.patch download (1.3 KB) - added by catalin 11 months ago.
fix15743.patch download (1.1 KB) - added by catalin 11 months ago.

Download all attachments as: .zip

Change History (18)

Changed 11 months ago by catalin

comment:1 Changed 11 months ago by vadz

  • Component changed from wxMSW to GUI-all
  • Owner set to vadz
  • Status changed from new to accepted
  • Summary changed from Crash when a wxColourDialog with a non-TLW parent is Destroy()-ed to Crash when a TLW with a non-TLW parent is Destroy()-ed

wxColourDialog is supposed to be used as a modal dialog, i.e. created on the stack and destroyed after it's used, so you don't usually need to Destroy() it explicitly.

As for the crash, this is indeed a problem, but it's not really related to wxColourDialog at all, it's just due to the fact that the main frame is destroyed first. I think I have a simple fix for this though...

comment:2 Changed 11 months ago by ericj

Isn't it just plain wrong to use a non-TLW as parent? (Even if the api allows it)

comment:3 Changed 11 months ago by VZ

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [75341]) Fix crash when Destroy()-ing a TLW with a non-TLW parent.

Generalize the code in wxTopLevelWindowBase dtor checking for the children of
the TLW being destroyed pending for deletion themselves to work when the child
TLW is an indirect child, i.e. was created with a child window of this TLW as
parent and not this TLW itself.

Closes #15743.

comment:4 Changed 11 months ago by VZ

(In [75342]) Fix crash when Destroy()-ing a TLW with a non-TLW parent.

Generalize the code in wxTopLevelWindowBase dtor checking for the children of
the TLW being destroyed pending for deletion themselves to work when the child
TLW is an indirect child, i.e. was created with a child window of this TLW as
parent and not this TLW itself.

Closes #15743.

Changed 11 months ago by catalin

comment:5 Changed 11 months ago by catalin

  • Resolution fixed deleted
  • Status changed from closed to reopened

r75342 indeed fixes the initial crash. But I can still get something very similar, see the second patch to minimal sample.
This time it looks more wxColourDialog specific. I cannot reproduce it with wxDialog or wxFrame.

comment:6 follow-up: Changed 11 months ago by vadz

  • Priority changed from normal to low
  • Status changed from reopened to confirmed

This should crash with any wxDialog, if it doesn't, it's just by sheer luck. Destroy() will delete the dialog later, but by this time its parent pointer is no longer valid because it had been already destroyed.

Unfortunately I don't really know what to do about this. The only advice I can give is to avoid creating the dialogs with non-TLWs as parents...

Changed 11 months ago by catalin

comment:7 in reply to: ↑ 6 Changed 11 months ago by catalin

Replying to vadz:

The only advice I can give is to avoid creating the dialogs with non-TLWs as parents...

I tried that but it doesn't change things. See the 3rd patch to minimal sample.

Again, even with a TLW as parent, if the wxColourDialog is created and left to be deleted by its parent it will leak memory; if Destroy() is called for it the app crashes..

comment:8 Changed 11 months ago by vadz

  • Summary changed from Crash when a TLW with a non-TLW parent is Destroy()-ed to Crash when a TLW Destroy()-ed when its parent is already in process of destruction

Yes, sorry, the problem is not (only) the parent but the fact that it's destroyed too late: it must be destroyed before its (TLW) parent, not after. So the way to do it correctly is actually

  • samples/minimal/minimal.cpp

    diff --git a/samples/minimal/minimal.cpp b/samples/minimal/minimal.cpp
    index a78e462..3677d01 100644
    a b  
    4040    #include "../sample.xpm" 
    4141#endif 
    4242 
     43#include <wx/colordlg.h> 
     44 
    4345// ---------------------------------------------------------------------------- 
    4446// private classes 
    4547// ---------------------------------------------------------------------------- 
    class MyFrame : public wxFrame 
    6365public: 
    6466    // ctor(s) 
    6567    MyFrame(const wxString& title); 
     68    ~MyFrame() { delete m_clrDlg; } 
    6669 
    6770    // event handlers (these functions should _not_ be virtual) 
    6871    void OnQuit(wxCommandEvent& event); 
    6972    void OnAbout(wxCommandEvent& event); 
    7073 
    7174private: 
     75    wxColourDialog* m_clrDlg; 
     76 
    7277    // any class wishing to process wxWidgets events must use this macro 
    7378    DECLARE_EVENT_TABLE() 
    7479}; 
    bool MyApp::OnInit() 
    172177    CreateStatusBar(2); 
    173178    SetStatusText("Welcome to wxWidgets!"); 
    174179#endif // wxUSE_STATUSBAR 
     180 
     181    m_clrDlg = new wxColourDialog(this); 
    175182} 
    176183 
    177184 

I'm not sure what can we do to avoid crashes with your patches. Perhaps we could check if the parent of a TLW that we Destroy() is already being deleted and delete it immediately in this case instead of appending it to the pending deletion list. This might result in other problems but at least it won't result in an immediate crash...

Changed 11 months ago by catalin

comment:9 Changed 11 months ago by catalin

  • Patch set

Replying to vadz:

Perhaps we could check if the parent of a TLW that we Destroy() is already being deleted and delete it immediately in this case instead of appending it to the pending deletion list. This might result in other problems but at least it won't result in an immediate crash...

Right, this sounds ok. Or maybe if the parent is already being deleted just don't add the TLW to the pending deletion list and leave it to be deleted by its parent?
The attached patch fixes the crash and looks like everything is ok.

The mem leak remains when a wxColourDialog is used...

comment:10 Changed 10 months ago by vadz

I'll apply (a slightly modified version of) the patch, but I don't see any memory leaks, once the crash is fixed. If you still do, could you please try to find out what is leaking?

comment:11 Changed 10 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [75447]) Avoid crashes when deleting owned top level windows.

Don't delay the TLW destruction if it has a parent and its parent is already
being deleted: we can't delay the inevitable in this case and only succeed in
making the program crash if we try.

Closes #15743.

comment:12 Changed 10 months ago by catalin

No more memory leaks after the crash was fixed. Everything looks fine, thanks.

comment:13 Changed 9 months ago by pcor

  • Milestone set to 3.0.1
  • Resolution changed from fixed to port to stable
  • Status changed from closed to portneeded

Seems like r75447 should be applied to 3.0 branch.

comment:14 Changed 9 months ago by VZ

  • Resolution changed from port to stable to fixed
  • Status changed from portneeded to closed

(In [75833]) Avoid crashes when deleting owned top level windows.

Don't delay the TLW destruction if it has a parent and its parent is already
being deleted: we can't delay the inevitable in this case and only succeed in
making the program crash if we try.

Closes #15743.

Note: See TracTickets for help on using tickets.