#14906: caf: Make PdmUiEditorHandle::updateUi robust against re-entrancy and exceptions

Crash signature 0f6ddddd87ad.

On exit, showProperties( nullptr ) reaches PdmUiEditorHandle::updateUi() while m_isConfiguringUi is still true, and CAF_ASSERT( !m_isConfiguringUi ) aborts. An exception in configureAndUpdateUi() or a re-entrant update leaves the editor locked.

Skip a re-entrant update instead of aborting, and use QScopedValueRollback so the flag is reset even if configureAndUpdateUi() throws. New tests ReentrantUpdateIsSkipped and UpdateAfterExceptionInConfigure.

Call stack:
[5] caf::PdmUiEditorHandle::updateUi(QString const&) at ResInsight/Fwk/AppFwk/cafProjectDataModel/cafPdmUiCore/cafPdmUiEditorHandle.cpp:65
[6] caf::PdmUiPropertyView::showProperties(caf::PdmObjectHandle*) at ResInsight/Fwk/AppFwk/cafUserInterface/cafPdmUiPropertyView.cpp:213
[7] RiuMainWindow::cleanupGuiCaseClose() at ResInsight/ApplicationLibCode/UserInterface/RiuMainWindow.cpp:312
[8] RiaGuiApplication::onProjectBeingClosed() at ResInsight/ApplicationLibCode/Application/RiaGuiApplication.cpp:1409
[9] RiaApplication::closeProject() at ResInsight/ApplicationLibCode/Application/RiaApplication.cpp:1040
[10] RicExitApplicationFeature::onActionTriggered(bool) at ResInsight/ApplicationLibCode/Commands/ApplicationCommands/RicExitApplicationFeature.cpp:70
[11] caf::CmdFeature::actionTriggered(bool) at ResInsight/Fwk/AppFwk/cafCommand/cafCmdFeature.cpp:176
[20] RiaGuiApplication::notify(QObject*, QEvent*) at ResInsight/ApplicationLibCode/Application/RiaGuiApplication.cpp:1828
[26] RiaGuiApplication::notify(QObject*, QEvent*) at ResInsight/ApplicationLibCode/Application/RiaGuiApplication.cpp:1828
[37] main at ResInsight/ApplicationExeCode/RiaMain.cpp:267
[38] __libc_start_main at :0

Fwk/AppFwk/cafProjectDataModel/cafPdmUiCore/cafPdmUiEditorHandle.cpp

@@ -36,7 +36,7 @@
 
 #include "cafPdmUiEditorHandle.h"
 
-#include "cafAssert.h"
+#include <QScopedValueRollback>
 
 namespace caf
 {
@@ -62,11 +62,15 @@ PdmUiEditorHandle::~PdmUiEditorHandle()
 //--------------------------------------------------------------------------------------------------
 void PdmUiEditorHandle::updateUi( const QString& uiConfigName )
 {
-    CAF_ASSERT( !m_isConfiguringUi );
-    m_isConfiguringUi   = true;
+    // Skip a re-entrant update, the ongoing update will configure the editor
+    if ( m_isConfiguringUi ) return;
+
     m_currentConfigName = uiConfigName;
-    this->configureAndUpdateUi( uiConfigName );
-    m_isConfiguringUi = false;
+    {
+        // Reset the flag also if configureAndUpdateUi() throws, otherwise all later updates of this editor are blocked
+        QScopedValueRollback<bool> configuringUiGuard( m_isConfiguringUi, true );
+        this->configureAndUpdateUi( uiConfigName );
+    }
 
     // Allow derived classes to update additional state (e.g., validation)
     this->updateEditorState();
@@ -79,10 +83,14 @@ void PdmUiEditorHandle::updateUi( const QString& uiConfigName )
 //--------------------------------------------------------------------------------------------------
 void PdmUiEditorHandle::updateUi()
 {
-    CAF_ASSERT( !m_isConfiguringUi );
-    m_isConfiguringUi = true;
-    this->configureAndUpdateUi( m_currentConfigName );
-    m_isConfiguringUi = false;
+    // Skip a re-entrant update, the ongoing update will configure the editor
+    if ( m_isConfiguringUi ) return;
+
+    {
+        // Reset the flag also if configureAndUpdateUi() throws, otherwise all later updates of this editor are blocked
+        QScopedValueRollback<bool> configuringUiGuard( m_isConfiguringUi, true );
+        this->configureAndUpdateUi( m_currentConfigName );
+    }
 
     // Allow derived classes to update additional state (e.g., validation)
     this->updateEditorState();

Fwk/AppFwk/cafUserInterface/cafUserInterface_UnitTests/CMakeLists.txt

@@ -22,6 +22,7 @@ set(PROJECT_FILES
     cafPdmUiNumberFormatTest.cpp
     cafPdmUiCheckBoxAndTextEditorTest.cpp
     cafPdmUiPropertyViewDialogTest.cpp
+    cafPdmUiEditorHandleTest.cpp
     cafIconProviderTest.cpp
     cafProgressInfoTest.cpp
     gtest/gtest-all.cpp

Fwk/AppFwk/cafUserInterface/cafUserInterface_UnitTests/cafPdmUiEditorHandleTest.cpp

@@ -0,0 +1,64 @@
+
+#include "gtest/gtest.h"
+
+#include "cafPdmUiEditorHandle.h"
+
+#include <stdexcept>
+
+using namespace caf;
+
+namespace
+{
+class TestEditor : public caf::PdmUiEditorHandle
+{
+public:
+    int  configureCount        = 0;
+    bool throwOnNextConfigure  = false;
+    bool updateFromConfigureUi = false;
+
+protected:
+    void configureAndUpdateUi( const QString& uiConfigName ) override
+    {
+        configureCount++;
+
+        if ( throwOnNextConfigure )
+        {
+            throwOnNextConfigure = false;
+            throw std::runtime_error( "configureAndUpdateUi failed" );
+        }
+
+        if ( updateFromConfigureUi ) updateUi( uiConfigName );
+    }
+};
+} // namespace
+
+//--------------------------------------------------------------------------------------------------
+/// A re-entrant updateUi() from configureAndUpdateUi() used to abort on CAF_ASSERT( !m_isConfiguringUi )
+//--------------------------------------------------------------------------------------------------
+TEST( PdmUiEditorHandleTest, ReentrantUpdateIsSkipped )
+{
+    TestEditor editor;
+    editor.updateFromConfigureUi = true;
+
+    editor.updateUi( "" );
+    EXPECT_EQ( 1, editor.configureCount );
+
+    editor.updateUi();
+    EXPECT_EQ( 2, editor.configureCount );
+}
+
+//--------------------------------------------------------------------------------------------------
+/// An exception from configureAndUpdateUi() used to leave the editor in configuring state, and the next updateUi()
+/// aborted on CAF_ASSERT( !m_isConfiguringUi )
+//--------------------------------------------------------------------------------------------------
+TEST( PdmUiEditorHandleTest, UpdateAfterExceptionInConfigure )
+{
+    TestEditor editor;
+    editor.throwOnNextConfigure = true;
+
+    EXPECT_THROW( editor.updateUi( "" ), std::runtime_error );
+    EXPECT_EQ( 1, editor.configureCount );
+
+    editor.updateUi( "" );
+    EXPECT_EQ( 2, editor.configureCount );
+}