From 6636a2ac9ecdc6b1a6dda0556319399648f01d2f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 30 Jan 2024 20:15:02 +0100 Subject: [PATCH 1/4] Fix recent regression with destroying wxDataViewCtrl on app exit As mentioned in the commit message of b628237245 (Fix crash due to using wxLog during shutdown in a better way, 2024-01-26), this change could affect existing code using wxTheApp and it did affect wxDataViewCtrl and not in a good way: destroying it during application shutdown started to crash now that wxTheApp is null when it happens. Fix this by skipping the idle time notification when shutting down, it's not useful anyhow by then. --- src/generic/datavgen.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index f5e111e519..d03300d7f0 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -5605,7 +5605,10 @@ wxDataViewCtrl::~wxDataViewCtrl() #if wxUSE_ACCESSIBILITY SetAccessible(nullptr); - wxAccessible::NotifyEvent(wxACC_EVENT_OBJECT_DESTROY, this, wxOBJID_CLIENT, wxACC_SELF); + // There is no need to notify anybody if we're destroyed as part of + // application shutdown and doing it would just crash in this case. + if ( wxTheApp ) + wxAccessible::NotifyEvent(wxACC_EVENT_OBJECT_DESTROY, this, wxOBJID_CLIENT, wxACC_SELF); #endif // wxUSE_ACCESSIBILITY } From c965c872903b3ca7768a09a409c9322ef852cfde Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 30 Jan 2024 20:36:41 +0100 Subject: [PATCH 2/4] wxX11: don't call CleanUp() from wxApp::Exit() This is not done in any other ports and shouldn't be necessary in this one neither, CleanUp() will be called from wxEntryCleanup() later anyhow. --- include/wx/x11/app.h | 2 -- src/x11/app.cpp | 8 -------- 2 files changed, 10 deletions(-) diff --git a/include/wx/x11/app.h b/include/wx/x11/app.h index 1ae32973b4..c0adc4bfb5 100644 --- a/include/wx/x11/app.h +++ b/include/wx/x11/app.h @@ -41,8 +41,6 @@ public: // override base class (pure) virtuals // ----------------------------------- - virtual void Exit(); - virtual void WakeUpIdle(); virtual bool OnInitGui(); diff --git a/src/x11/app.cpp b/src/x11/app.cpp index ae631b283d..4558c98207 100644 --- a/src/x11/app.cpp +++ b/src/x11/app.cpp @@ -789,11 +789,3 @@ Window wxGetWindowParent(Window window) return (Window) 0; #endif } - -void wxApp::Exit() -{ - wxApp::CleanUp(); - - wxAppConsole::Exit(); -} - From a7aea0febfeb6f842fbca7186bfd96b80c91bd11 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 30 Jan 2024 21:07:29 +0100 Subject: [PATCH 3/4] Do common wxApp cleanup before port-specific part in all ports Some ports called wxAppBase::CleanUp() before performing port-specific cleanup, while some did it afterwards. The former seems preferable, as some actions performed in wxAppBase (such as showing log dialogs) may not work any more after the port-specific cleanup is done, while there should be no dependencies in the other direction. So standardize on calling wxAppBase version first in all ports, which also makes them more consistent with each other. --- src/gtk/app.cpp | 4 ++-- src/osx/carbon/app.cpp | 5 +++-- src/x11/app.cpp | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/gtk/app.cpp b/src/gtk/app.cpp index 8537b4f8cf..b8604051f5 100644 --- a/src/gtk/app.cpp +++ b/src/gtk/app.cpp @@ -568,6 +568,8 @@ bool wxApp::Initialize(int& argc_, wxChar **argv_) void wxApp::CleanUp() { + wxAppBase::CleanUp(); + if (m_idleSourceId != 0) g_source_remove(m_idleSourceId); @@ -577,8 +579,6 @@ void wxApp::CleanUp() g_type_class_unref(gt); gdk_threads_leave(); - - wxAppBase::CleanUp(); } void wxApp::WakeUpIdle() diff --git a/src/osx/carbon/app.cpp b/src/osx/carbon/app.cpp index eba9652e47..0683e8dc40 100644 --- a/src/osx/carbon/app.cpp +++ b/src/osx/carbon/app.cpp @@ -358,13 +358,14 @@ int wxApp::OnRun() void wxApp::CleanUp() { wxMacAutoreleasePool autoreleasepool; + + wxAppBase::CleanUp(); + #if wxUSE_TOOLTIPS wxToolTip::RemoveToolTips() ; #endif DoCleanUp(); - - wxAppBase::CleanUp(); } //---------------------------------------------------------------------- diff --git a/src/x11/app.cpp b/src/x11/app.cpp index 4558c98207..9da804569e 100644 --- a/src/x11/app.cpp +++ b/src/x11/app.cpp @@ -198,10 +198,10 @@ bool wxApp::Initialize(int& argC, wxChar **argV) void wxApp::CleanUp() { + wxAppBase::CleanUp(); + wxDELETE(wxWidgetHashTable); wxDELETE(wxClientWidgetHashTable); - - wxAppBase::CleanUp(); } wxApp::wxApp() From 509fcb02146d47921d04f7936491a93a6aaf9dd9 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 30 Jan 2024 21:10:41 +0100 Subject: [PATCH 4/4] Avoid resetting wxTheApp too early while still avoiding crashes Both recent changes trying to order resetting wxTheApp and flushing the logs failed: 055c4cbed5 (Fix crash on shutdown if wxConfig couldn't be saved, 2024-01-04) did because it reset wxTheApp too late and wxLogGui was created when it was already unsafe to use, while b628237245 (Fix crash due to using wxLog during shutdown in a better way, 2024-01-26) failed because it reset wxTheApp too early, resulting in it being null during execution of window destructors which could expect wxTheApp to still be available. And while 6636a2ac9e (Fix recent regression with destroying wxDataViewCtrl on app exit, 2024-01-30) fixed one of the problems due to the latter change, there are no guarantees that there are no more such problems, especially in user-defined destructors in the application code. So instead of resetting wxTheApp before or after calling CleanUp(), now do it in the middle of wxAppBase::CleanUp(): just after destroying all the windows (and so executing any user-defined destructors) but before the application object becomes truly unusable. Note that this relies on the previous commit, as before it the application became unusable even before wxAppBase::CleanUp() execution started in some ports (notably wxGTK). See #24252. --- src/common/appcmn.cpp | 16 ++++++++++++++++ src/common/init.cpp | 30 ++++++------------------------ 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/src/common/appcmn.cpp b/src/common/appcmn.cpp index 8574d4fe8e..875e412e56 100644 --- a/src/common/appcmn.cpp +++ b/src/common/appcmn.cpp @@ -149,6 +149,22 @@ void wxAppBase::CleanUp() // and any remaining TLWs DeleteAllTLWs(); +#if wxUSE_LOG + // flush the logged messages if any and don't use the current probably + // unsafe log target any more: the default one (wxLogGui) can't be used + // after the resources are freed below and the user supplied one might be + // even worse (using any wxWidgets GUI function is unsafe starting from + // now) + // + // notice that wxLog will still recreate a default log target if any + // messages are logged but that one will be safe to use until the very end + delete wxLog::SetActiveTarget(nullptr); +#endif // wxUSE_LOG + + // Starting from now, the application object is no longer valid and + // shouldn't be used any longer, so reset the global pointer to it. + wxApp::SetInstance(nullptr); + // undo everything we did in Initialize() above wxBitmap::CleanUpHandlers(); diff --git a/src/common/init.cpp b/src/common/init.cpp index d186cfb913..39cb90b41b 100644 --- a/src/common/init.cpp +++ b/src/common/init.cpp @@ -440,22 +440,6 @@ bool wxEntryStart(int& argc, char **argv) // clean up // ---------------------------------------------------------------------------- -// cleanup done before destroying wxTheApp -static void DoCommonPreCleanup() -{ -#if wxUSE_LOG - // flush the logged messages if any and don't use the current probably - // unsafe log target any more: the default one (wxLogGui) can't be used - // after the resources are freed which happens when we return and the user - // supplied one might be even more unsafe (using any wxWidgets GUI function - // is unsafe starting from now) - // - // notice that wxLog will still recreate a default log target if any - // messages are logged but that one will be safe to use until the very end - delete wxLog::SetActiveTarget(nullptr); -#endif // wxUSE_LOG -} - // cleanup done after destroying wxTheApp static void DoCommonPostCleanup() { @@ -488,20 +472,18 @@ static void DoCommonPostCleanup() void wxEntryCleanup() { - DoCommonPreCleanup(); - - // delete the application object - if ( wxTheApp ) + if ( wxAppConsole * const app = wxApp::GetInstance() ) { + app->CleanUp(); + // reset the global pointer to it to nullptr before destroying it as in // some circumstances this can result in executing the code using - // wxTheApp and using half-destroyed object is no good - wxAppConsole * const app = wxApp::GetInstance(); + // wxTheApp and using half-destroyed object is no good (note that it + // usually would be already reset by wxAppBase::CleanUp(), but ensure + // that it is definitely done by doing it here too) wxApp::SetInstance(nullptr); - app->CleanUp(); - delete app; }