core.git: vcl/source

[Noel Grandin (via logerrit)]

 vcl/source/window/dialog.cxx |   45 +++++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

New commits:
commit d2280c07bfc0887d8784aff04ac5ea20981b17b8
Author:     Noel Grandin <noel.gran...@collabora.co.uk>
AuthorDate: Fri Jan 31 10:45:31 2025 +0200
Commit:     Noel Grandin <noel.gran...@collabora.co.uk>
CommitDate: Fri Jan 31 12:07:35 2025 +0100

    make Dialog destruction a little safer
    
    rather keep the Impl object around for the lifetime of the Dialog
    object.
    
    where I saw a backtrace that looked like
    
    vcl/source/window/dialog.cxx:1098: short Dialog::Execute(): Assertion
    `mpDialogImpl' failed.
     Dialog::Execute()
     SalInstanceDialog::run()
     ScSpellingEngine::ShowFinishDialog()
     ScConversionEngineBase::FindNextConversionCell()
     ScSpellingEngine::SpellNextDocument()
     ScSpellDialogChildWindow::GetNextWrongSentence(bool)
    
    Change-Id: I5858f3cd126185bc4f59932dfa53f6a6a3a66aa6
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/180980
    Reviewed-by: Noel Grandin <noel.gran...@collabora.co.uk>
    Tested-by: Jenkins

diff --git a/vcl/source/window/dialog.cxx b/vcl/source/window/dialog.cxx
index ecf2302bfe4d..d407626388d6 100644
--- a/vcl/source/window/dialog.cxx
+++ b/vcl/source/window/dialog.cxx
@@ -373,6 +373,11 @@ struct DialogImpl
 #endif
 
     ~DialogImpl()
+    {
+        disposeAndClear();
+    }
+
+    void disposeAndClear()
     {
         for (VclPtr<PushButton> & pOwnedButton : maOwnedButtons)
             pOwnedButton.disposeAndClear();
@@ -619,7 +624,7 @@ void Dialog::dispose()
 {
     bool bTunnelingEnabled = mpDialogImpl->m_bLOKTunneling;
 
-    mpDialogImpl.reset();
+    mpDialogImpl->disposeAndClear();
     RemoveFromDlgList();
     mpActionArea.clear();
     mpContentArea.clear();
@@ -1095,20 +1100,11 @@ short Dialog::Execute()
         OSL_FAIL( "Dialog::Execute() - Dialog destroyed in Execute()" );
     }
 
-    assert(mpDialogImpl);
 
-    if (mpDialogImpl)
-    {
-        tools::Long nRet = mpDialogImpl->mnResult;
-        mpDialogImpl->mnResult = -1;
+    tools::Long nRet = mpDialogImpl->mnResult;
+    mpDialogImpl->mnResult = -1;
 
-        return static_cast<short>(nRet);
-    }
-    else
-    {
-        SAL_WARN( "vcl", "Dialog::Execute() : missing mpDialogImpl " );
-        return 0;
-    }
+    return static_cast<short>(nRet);
 }
 
 // virtual
@@ -1143,6 +1139,9 @@ void Dialog::EndDialog( tools::Long nResult )
     if (!mbInExecute || isDisposed())
         return;
 
+    // do not let this object be destroyed from underneath us
+    VclPtr<vcl::Window> xWindow = this;
+
     const bool bModal = GetType() != WindowType::MODELESSDIALOG;
 
     Hide();
@@ -1186,8 +1185,7 @@ void Dialog::EndDialog( tools::Long nResult )
     if ( mpDialogImpl->mbStartedModal )
         ImplEndExecuteModal();
 
-    // coverity[check_after_deref] - ImplEndExecuteModal might trigger 
destruction of mpDialogImpl
-    if ( mpDialogImpl && mpDialogImpl->maEndCtx.isSet() )
+    if ( mpDialogImpl->maEndCtx.isSet() )
     {
         // We have a special case with async-dialogs that re-execute 
themselves.
         // In order to prevent overwriting state we need here, we need to 
extract
@@ -1214,26 +1212,23 @@ void Dialog::EndDialog( tools::Long nResult )
 
         // Destroy ourselves, if we have a context with VclPtr owner, and
         // we have not been re-executed.
-        if (!mpDialogImpl || !mpDialogImpl->maEndCtx.isSet())
+        if (!mpDialogImpl->maEndCtx.isSet())
             xOwner.disposeAndClear();
         xOwnerDialogController.reset();
         xOwnerSelf.reset();
     }
     else
     {
-        if ( mpDialogImpl && mpDialogImpl->mbStartedModal )
+        if ( mpDialogImpl->mbStartedModal )
         {
             mpDialogImpl->mbStartedModal = false;
             mpDialogImpl->mnResult = -1;
         }
         mbInExecute = false;
-        if ( mpDialogImpl)
-        {
-            // Destroy ourselves (if we have a context with VclPtr owner)
-            std::shared_ptr<weld::DialogController> xOwnerDialogController = 
std::move(mpDialogImpl->maEndCtx.mxOwnerDialogController);
-            std::shared_ptr<weld::Dialog> xOwnerSelf = 
std::move(mpDialogImpl->maEndCtx.mxOwnerSelf);
-            mpDialogImpl->maEndCtx.mxOwner.disposeAndClear();
-        }
+        // Destroy ourselves (if we have a context with VclPtr owner)
+        std::shared_ptr<weld::DialogController> xOwnerDialogController = 
std::move(mpDialogImpl->maEndCtx.mxOwnerDialogController);
+        std::shared_ptr<weld::Dialog> xOwnerSelf = 
std::move(mpDialogImpl->maEndCtx.mxOwnerSelf);
+        mpDialogImpl->maEndCtx.mxOwner.disposeAndClear();
     }
 }
 
@@ -1666,7 +1661,7 @@ void Dialog::Activate()
 
 void Dialog::Command(const CommandEvent& rCEvt)
 {
-    if (mpDialogImpl && mpDialogImpl->m_aPopupMenuHdl.Call(rCEvt))
+    if (mpDialogImpl->m_aPopupMenuHdl.Call(rCEvt))
         return;
     SystemWindow::Command(rCEvt);
 }