wxAuiManager preview issue under wxSmith cause C::B can't exit correctly

[ollydbg]

I found a very annoying issue. Here are steps to reproduce:
1, open C::B
2, create a simple wxWidgets app with the wizard(choose wxFrame based, wxSmith as the GUI designer)
3, add a wxAuiManager to the main Frame
4, save all
5, close C::B

The result is: I see the codeblocks.exe process is still in the Windows task manager(its memory size is about 35M to 40M), I have to kill this process.

Note, if you don't have wxAuiManager object in the wxFrame, you don't have such issue.

I have tested this steps under:
1, 32bit official C::B 17.12 under Win7(64bit), which is wx2.8.12 based.
2, 32bit C::B trunk build myself with wx3.12, gcc 7.2.
Both have such issue.

I try to debug this issue, but it looks like the process hang issue after the function call: void MainFrame::OnApplicationClose(wxCloseEvent& event), so it is hard to debug. Also, since C::B is halt (it stalls inside the Win32 wait object functions), I don't have a call stack here.

This issue firstly happens in a complex wxsmith project, but I just reduce the wxsmith and finally see that a single wxAuiManager object in wxsmith will cause this issue.

This issue is not related to code generation, because I event don't need to build the cbp project, just open the wxsmith gui designer(the preview window) will cause this issue.

Any ideas?
 

[oBFusCATed]

1. Does reverting rev11530 changes anything?
2. Have you tried to reproduce the problem in some wx sample?
3. Have you tried with a debug wx build?

[BlueHazzard]

Could it be this:
https://sourceforge.net/p/codeblocks/tickets/789/

2) AuiManager::UnInit() has to be added to the destructor of the window code

in the destructor of the preview?

[ollydbg]

Could it be this:
https://sourceforge.net/p/codeblocks/tickets/789/

2) AuiManager::UnInit() has to be added to the destructor of the window code

in the destructor of the preview?

There is a UnInit() function call in the preview wxAuiManager derived object: in file: src\plugins\contrib\wxSmithAui\wxAuiManager\wxSmithAuiManager.h

Code

#ifndef WXSMITHAUIMANAGER_H
#define WXSMITHAUIMANAGER_H

#include <wx/aui/aui.h>
#include <wx/wx.h>
#include <wx/event.h>

#include <prep.h>

class wxSmithAuiManager : public wxAuiManager
{
    public:
        wxSmithAuiManager(wxWindow* managed_wnd = NULL, unsigned int flags = wxAUI_MGR_DEFAULT)
            : wxAuiManager(managed_wnd, flags)
        {
            Connect(wxEVT_DESTROY,(wxObjectEventFunction)&wxSmithAuiManager::OnDestroy);
        }

        virtual ~wxSmithAuiManager() {}

        void OnDestroy(cb_unused wxWindowDestroyEvent& event)
        {
            UnInit();
        }
    protected:
    private:
};

#endif // WXSMITHAUIMANAGER_H

So, it will be called when the preview window closed?

[ollydbg]

1. Does reverting rev11530 changes anything?

I'm not sure, but mostly I think it is not related to this commit.

SHA-1: 28734b8204bff66d8a9d6b8e7bb2f4eb8ad83561

* * batch build: Fix segmentation fault after batch build (ticket #738)

> The problem happens because there is a Manager::Yield() in
  MainFrame::OnApplicationClose. This call to yield makes wxWidgets execute
  all events in its event queue and unfortunately there are events related
  to delayed destruction of the current window which get executed at a bad
  time.
> If I remove the Yield call the bug goes away. Not sure what would break
  by this change, but we'll probably see.
> I've tested if the APP_SHUTDOWN event is still sent and received. I found
  no problems with this.

git-svn-id: https://svn.code.sf.net/p/codeblocks/code/trunk@11530 2a5c6006-c6dd-42ca-98ab-0921f2732cef

But the issue happens in official C::B 17.12 version(r11256), which is long before commit r11530.

2. Have you tried to reproduce the problem in some wx sample?

The issue happens in preview of the wxSmith, the generated executable by C::B doesn't have such issue.

3. Have you tried with a debug wx build?

Note tried yet, building wx3.12 under gcc 7.2 is quite slow, will take some time to try it.

[ollydbg]

...
There is a UnInit() function call in the preview wxAuiManager derived object: in file: src\plugins\contrib\wxSmithAui\wxAuiManager\wxSmithAuiManager.h

Code

#ifndef WXSMITHAUIMANAGER_H
#define WXSMITHAUIMANAGER_H

#include <wx/aui/aui.h>
#include <wx/wx.h>
#include <wx/event.h>

#include <prep.h>

class wxSmithAuiManager : public wxAuiManager
{
    public:
        wxSmithAuiManager(wxWindow* managed_wnd = NULL, unsigned int flags = wxAUI_MGR_DEFAULT)
            : wxAuiManager(managed_wnd, flags)
        {
            Connect(wxEVT_DESTROY,(wxObjectEventFunction)&wxSmithAuiManager::OnDestroy);
        }

        virtual ~wxSmithAuiManager() {}

        void OnDestroy(cb_unused wxWindowDestroyEvent& event)
        {
            UnInit();
        }
    protected:
    private:
};

#endif // WXSMITHAUIMANAGER_H

So, it will be called when the preview window closed?

Oh, bad/good news, I just set a breakpoint in the  UnInit(); line, and When I close debugee C::B, this line does not reached. and debugee C::B now go to endless wait status. So my guess is that why this function call is missing here?

EDIT:
The UnInit() is called after I add one page in the wxAuiManager. When there is no items controlled by wxAuiManager, it is not called.(in the wxsmith preview, wxAuiManager object only constructed when there is at least one item inside it)

[ollydbg]

Good news, I think I have find the reason of this issue.

With this patch, I can avoid the hang process when C::B exits. (especially follow my 5 steps in my first post in this thread)

under function: bool wxsAuiManager::OnCanAddToParent(wxsParent* Parent,bool ShowMessage), it need to test whether a wxAuiManager can be child of a Parent window.

Code

 src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp b/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp
index bbd3d967..60735822 100644
--- a/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp
+++ b/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp
@@ -640,14 +640,14 @@ bool wxsAuiManager::OnCanAddToParent(wxsParent* Parent,bool ShowMessage)
             wxMessageBox(_("wxAuiManager can't be added to a sizer. Add panels first."));
         return false;
     }
-
+#if 0
     if ( !wxDynamicCast(Parent->BuildPreview(new wxFrame(0,-1,wxEmptyString),0),wxWindow) )
     {
         if ( ShowMessage )
             wxMessageBox(_("wxAuiManager can only be added to a wxWindow descendant."));
         return false;
     }
-
+#endif // 0
     return wxsParent::OnCanAddToParent(Parent,ShowMessage);
 }
 

I'm really don't know what does this code means?

Code

new wxFrame(0,-1,wxEmptyString)

This means we allocate a wxFrame under the desktop? The first argument is 0 means the desktop?

I see there are three places which use "new wxFrame(0,...)" in our C::B's source code, and they all exists under the folder:
two places in src\plugins\contrib\wxSmithAui\wxAuiManager\wxsAuiManager.cpp
one place in src\plugins\contrib\wxSmithAui\wxAuiToolBar\wxsAuiToolBar.cpp

I guess this usage of create a wxFrame is wrong, do you have any ideas how to fix this issue? Thanks.

[sodev]

The first argument is actually a pointer and passing a nullptr means to create frame without parent which is fully independend.

I think the reason for CodeBlocks to not close is that no one ever destroys these temporary frames (unless that BuildPreview method does it internally) and the "dangling" frames keep wxWidgets alive.

[ollydbg]

The first argument is actually a pointer and passing a nullptr means to create frame without parent which is fully independend.

I think the reason for CodeBlocks to not close is that no one ever destroys these temporary frames (unless that BuildPreview method does it internally) and the "dangling" frames keep wxWidgets alive.

Thanks for the explanation. Now, I will try to call the Destroy() function of the frame objects when this function exits. (Documents are here: https://docs.wxwidgets.org/3.1/overview_windowdeletion.html)

[ollydbg]

I change the code like below:

Code

    wxFrame * tempFrame = new wxFrame(0,-1,wxEmptyString);
    if ( !wxDynamicCast(Parent->BuildPreview(tempFrame,0),wxWindow) )
    {
        if ( ShowMessage )
            wxMessageBox(_("wxAuiManager can only be added to a wxWindow descendant."));
        tempFrame->Destroy();
        return false;
    }
    tempFrame->Destroy();

It looks like this code does not make C::B endless wait on exit.

I will change the other "dangling" frame places in wxAui related code in wxsmith.

[ollydbg]

Code

 
 src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp | 11 +++++++----
 src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp |  4 +++-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp b/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp
index bbd3d967..3b9bfcbb 100644
--- a/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp
+++ b/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp
@@ -640,14 +640,15 @@ bool wxsAuiManager::OnCanAddToParent(wxsParent* Parent,bool ShowMessage)
             wxMessageBox(_("wxAuiManager can't be added to a sizer. Add panels first."));
         return false;
     }
-
-    if ( !wxDynamicCast(Parent->BuildPreview(new wxFrame(0,-1,wxEmptyString),0),wxWindow) )
+    wxFrame * tempFrame = new wxFrame(0,-1,wxEmptyString);
+    if ( !wxDynamicCast(Parent->BuildPreview(tempFrame,0),wxWindow) )
     {
         if ( ShowMessage )
             wxMessageBox(_("wxAuiManager can only be added to a wxWindow descendant."));
+        tempFrame->Destroy();
         return false;
     }
-
+    tempFrame->Destroy();
     return wxsParent::OnCanAddToParent(Parent,ShowMessage);
 }
 
@@ -665,7 +666,8 @@ void wxsAuiManager::OnAddChildQPP(wxsItem* Child,wxsAdvQPP* QPP)
     if ( ChildExtra->m_FirstAdd )
     {
         ChildExtra->m_FirstAdd = false;
-        if ( wxDynamicCast(Child->BuildPreview(new wxFrame(0,-1,wxEmptyString),0),wxAuiToolBar) )
+        wxFrame * tempFrame = new wxFrame(0,-1,wxEmptyString);
+        if ( wxDynamicCast(Child->BuildPreview(tempFrame,0),wxAuiToolBar) )
         {
             ChildExtra->m_StandardPane = wxsAuiPaneInfoExtra::ToolbarPane;
             ChildExtra->m_DockableFlags  = wxsAuiDockableProperty::Dockable;
@@ -680,6 +682,7 @@ void wxsAuiManager::OnAddChildQPP(wxsItem* Child,wxsAdvQPP* QPP)
             if ( ChildExtra->m_Layer == 0 ) ChildExtra->m_Layer = 10;
             NotifyPropertyChange();
         }
+        tempFrame->Destroy();
     }
 }
 
diff --git a/src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp b/src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp
index 66b67edc..7f0f8eff 100644
--- a/src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp
+++ b/src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp
@@ -126,7 +126,9 @@ wxsAuiToolBar::~wxsAuiToolBar()
 
 bool wxsAuiToolBar::OnCanAddChild(wxsItem* Item,bool ShowMessage)
 {
-    bool IsControl = wxDynamicCast(Item->BuildPreview(new wxFrame(0,-1,wxEmptyString),0),wxControl);
+    wxFrame * tempFrame = new wxFrame(0,-1,wxEmptyString);
+    bool IsControl = wxDynamicCast(Item->BuildPreview(tempFrame,0),wxControl);
+    tempFrame->Destroy();
     bool IsAuiToolBarItem = Item->GetClassName().Contains(_T("wxAuiToolBar"));
     if ( !IsControl && !IsAuiToolBarItem )
     {

This is the full patch. OK to commit to trunk?

[sodev]

Dont use 0 but nullptr, dont use -1 but wxID_ANY, dont use raw pointer but something managed, something like:

Code

auto tempFrame = std::shared_ptr<wxFrame>(new wxFrame(nullptr, wxID_ANY, wxEmptyString), [](wxFrame* frame) { frame->Destroy(); });

This way you dont need explicit Destroy() calls. Would be better to use std::unique_ptr, but there the deleter is part of the signature and i currently dont know how to use that with a nice lambda expression and dont have a compiler on my cell phone to play around with .

But beside that, why does the code even create these temporary frames and is it really safe to destroy them right after the method call? Why to create them at all then?

[ollydbg]

Dont use 0 but nullptr, dont use -1 but wxID_ANY, dont use raw pointer but something managed, something like:

Code

auto tempFrame = std::shared_ptr<wxFrame>(new wxFrame(nullptr, wxID_ANY, wxEmptyString), [](wxFrame* frame) { frame->Destroy(); });

This way you dont need explicit Destroy() calls. Would be better to use std::unique_ptr, but there the deleter is part of the signature and i currently dont know how to use that with a nice lambda expression and dont have a compiler on my cell phone to play around with .

Thanks for the suggestion, and in-fact I'm not good at writing lambda expressions  , here is improved patch as your suggested.

Code

 src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp | 9 ++++++---
 src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp | 4 +++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp b/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp
index bbd3d967..3cf86b3e 100644
--- a/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp
+++ b/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp
@@ -640,8 +640,9 @@ bool wxsAuiManager::OnCanAddToParent(wxsParent* Parent,bool ShowMessage)
             wxMessageBox(_("wxAuiManager can't be added to a sizer. Add panels first."));
         return false;
     }
-
-    if ( !wxDynamicCast(Parent->BuildPreview(new wxFrame(0,-1,wxEmptyString),0),wxWindow) )
+    auto tempFrame = std::shared_ptr<wxFrame>(new wxFrame(nullptr, wxID_ANY, wxEmptyString),
+                                              [](wxFrame* frame) {frame->Destroy();}); //deleter
+    if ( !wxDynamicCast(Parent->BuildPreview(tempFrame.get(),0),wxWindow) )
     {
         if ( ShowMessage )
             wxMessageBox(_("wxAuiManager can only be added to a wxWindow descendant."));
@@ -665,7 +666,9 @@ void wxsAuiManager::OnAddChildQPP(wxsItem* Child,wxsAdvQPP* QPP)
     if ( ChildExtra->m_FirstAdd )
     {
         ChildExtra->m_FirstAdd = false;
-        if ( wxDynamicCast(Child->BuildPreview(new wxFrame(0,-1,wxEmptyString),0),wxAuiToolBar) )
+        auto tempFrame = std::shared_ptr<wxFrame>(new wxFrame(nullptr, wxID_ANY, wxEmptyString),
+                                                  [](wxFrame* frame) {frame->Destroy();}); //deleter
+        if ( wxDynamicCast(Child->BuildPreview(tempFrame.get(),0),wxAuiToolBar) )
         {
             ChildExtra->m_StandardPane = wxsAuiPaneInfoExtra::ToolbarPane;
             ChildExtra->m_DockableFlags  = wxsAuiDockableProperty::Dockable;
diff --git a/src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp b/src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp
index 66b67edc..af2adb70 100644
--- a/src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp
+++ b/src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp
@@ -126,7 +126,9 @@ wxsAuiToolBar::~wxsAuiToolBar()
 
 bool wxsAuiToolBar::OnCanAddChild(wxsItem* Item,bool ShowMessage)
 {
-    bool IsControl = wxDynamicCast(Item->BuildPreview(new wxFrame(0,-1,wxEmptyString),0),wxControl);
+    auto tempFrame = std::shared_ptr<wxFrame>(new wxFrame(nullptr, wxID_ANY, wxEmptyString),
+                                              [](wxFrame* frame) {frame->Destroy();}); //deleter
+    bool IsControl = wxDynamicCast(Item->BuildPreview(tempFrame.get(),0),wxControl);
     bool IsAuiToolBarItem = Item->GetClassName().Contains(_T("wxAuiToolBar"));
     if ( !IsControl && !IsAuiToolBarItem )
     {

But beside that, why does the code even create these temporary frames and is it really safe to destroy them right after the method call? Why to create them at all then?

I don't know such logic here either.

[oBFusCATed]

Can you add comments what is the purpose of the shared pointers? Why aren't you using a unique_ptr? I don't see any sharing happening here.
Can you please not use auto? In this case it doesn't make sense to use it.

[ollydbg]

Can you add comments what is the purpose of the shared pointers? Why aren't you using a unique_ptr? I don't see any sharing happening here.

Oh, thanks for the review, it is a copy-paste error, it definitely should be std::unique_ptr.

Can you please not use auto? In this case it doesn't make sense to use it.

Here is the updated patch, any comments?

Code

 .../contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp | 19 ++++++++++++++++---
 .../contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp | 13 ++++++++++++-
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp b/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp
index bbd3d967..5a66636c 100644
--- a/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp
+++ b/src/plugins/contrib/wxSmithAui/wxAuiManager/wxsAuiManager.cpp
@@ -27,6 +27,16 @@
 
 using namespace wxsFlags;
 
+struct TempFrameDeleter
+{
+    void operator()(wxFrame* frame)
+    {
+        frame->Destroy();
+    }
+};
+
+using TempFrameUniquePtr = std::unique_ptr<wxFrame, TempFrameDeleter>;
+
 namespace
 {
     #include "../images/wxsAuiManager16.xpm"
@@ -640,8 +650,8 @@ bool wxsAuiManager::OnCanAddToParent(wxsParent* Parent,bool ShowMessage)
             wxMessageBox(_("wxAuiManager can't be added to a sizer. Add panels first."));
         return false;
     }
-
-    if ( !wxDynamicCast(Parent->BuildPreview(new wxFrame(0,-1,wxEmptyString),0),wxWindow) )
+    TempFrameUniquePtr tempFrame(new wxFrame(nullptr, wxID_ANY, wxEmptyString));
+    if ( !wxDynamicCast(Parent->BuildPreview(tempFrame.get(),0),wxWindow) )
     {
         if ( ShowMessage )
             wxMessageBox(_("wxAuiManager can only be added to a wxWindow descendant."));
@@ -651,6 +661,8 @@ bool wxsAuiManager::OnCanAddToParent(wxsParent* Parent,bool ShowMessage)
     return wxsParent::OnCanAddToParent(Parent,ShowMessage);
 }
 
+
+
 void wxsAuiManager::OnAddChildQPP(wxsItem* Child,wxsAdvQPP* QPP)
 {
     wxsParent::OnAddChildQPP(Child,QPP);
@@ -665,7 +677,8 @@ void wxsAuiManager::OnAddChildQPP(wxsItem* Child,wxsAdvQPP* QPP)
     if ( ChildExtra->m_FirstAdd )
     {
         ChildExtra->m_FirstAdd = false;
-        if ( wxDynamicCast(Child->BuildPreview(new wxFrame(0,-1,wxEmptyString),0),wxAuiToolBar) )
+        TempFrameUniquePtr tempFrame(new wxFrame(nullptr, wxID_ANY, wxEmptyString));
+        if ( wxDynamicCast(Child->BuildPreview(tempFrame.get(),0),wxAuiToolBar) )
         {
             ChildExtra->m_StandardPane = wxsAuiPaneInfoExtra::ToolbarPane;
             ChildExtra->m_DockableFlags  = wxsAuiDockableProperty::Dockable;
diff --git a/src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp b/src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp
index 66b67edc..3870d1cb 100644
--- a/src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp
+++ b/src/plugins/contrib/wxSmithAui/wxAuiToolBar/wxsAuiToolBar.cpp
@@ -32,6 +32,16 @@
 
 using namespace wxsFlags;
 
+struct TempFrameDeleter
+{
+    void operator()(wxFrame* frame)
+    {
+        frame->Destroy();
+    }
+};
+
+using TempFrameUniquePtr = std::unique_ptr<wxFrame, TempFrameDeleter>;
+
 //(*Headers(wxsAuiToolBarParentQP)
 #include <wx/checkbox.h>
 #include <wx/sizer.h>
@@ -126,7 +136,8 @@ wxsAuiToolBar::~wxsAuiToolBar()
 
 bool wxsAuiToolBar::OnCanAddChild(wxsItem* Item,bool ShowMessage)
 {
-    bool IsControl = wxDynamicCast(Item->BuildPreview(new wxFrame(0,-1,wxEmptyString),0),wxControl);
+    TempFrameUniquePtr tempFrame(new wxFrame(nullptr, wxID_ANY, wxEmptyString));
+    bool IsControl = wxDynamicCast(Item->BuildPreview(tempFrame.get(),0),wxControl);
     bool IsAuiToolBarItem = Item->GetClassName().Contains(_T("wxAuiToolBar"));
     if ( !IsControl && !IsAuiToolBarItem )
     {

EDIT:
I don't think add the same part in two cpp file is a good idea.

Code

+struct TempFrameDeleter
+{
+    void operator()(wxFrame* frame)
+    {
+        frame->Destroy();
+    }
+};
+
+using TempFrameUniquePtr = std::unique_ptr<wxFrame, TempFrameDeleter>;

EDIT2:
Follow this page: c++ - Simple custom deleter lambda supplied to std::unique_ptr: why use by-reference capture default ([&]) over no-capture ([])? - Stack Overflow
This is more compact way of define the unique_ptr, OK with below code?

Code

        std::unique_ptr<wxFrame, std::function<void(wxFrame*)>> tempFrame(new wxFrame(nullptr, wxID_ANY, wxEmptyString),
                                                                          [&](wxFrame* frame){ frame->Destroy(); });  // deleter