From 481e63ffa7d8932cf22fb7252ffc1d670f7509eb Mon Sep 17 00:00:00 2001
From: Evil Eye <malusluminis@hotmail.com>
Date: Sun, 29 Sep 2024 22:36:31 +0200
Subject: [PATCH] Prevent stale pointers in UI widgets

---
 apps/openmw/mwlua/luamanagerimp.cpp |  5 +++++
 apps/openmw/mwlua/luamanagerimp.hpp |  1 +
 components/lua_ui/element.cpp       | 35 +++++++++++++++++++++++------
 components/lua_ui/widget.cpp        |  1 +
 components/lua_ui/widget.hpp        |  2 +-
 5 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/apps/openmw/mwlua/luamanagerimp.cpp b/apps/openmw/mwlua/luamanagerimp.cpp
index cf55ac63e6..9f45fdb744 100644
--- a/apps/openmw/mwlua/luamanagerimp.cpp
+++ b/apps/openmw/mwlua/luamanagerimp.cpp
@@ -238,6 +238,11 @@ namespace MWLua
     }
 
     void LuaManager::synchronizedUpdate()
+    {
+        mLua.protectedCall([&](LuaUtil::LuaView&) { synchronizedUpdateUnsafe(); });
+    }
+
+    void LuaManager::synchronizedUpdateUnsafe()
     {
         if (mNewGameStarted)
         {
diff --git a/apps/openmw/mwlua/luamanagerimp.hpp b/apps/openmw/mwlua/luamanagerimp.hpp
index 5fa20d377f..d75b033a43 100644
--- a/apps/openmw/mwlua/luamanagerimp.hpp
+++ b/apps/openmw/mwlua/luamanagerimp.hpp
@@ -170,6 +170,7 @@ namespace MWLua
         LocalScripts* createLocalScripts(const MWWorld::Ptr& ptr,
             std::optional<LuaUtil::ScriptIdsWithInitializationData> autoStartConf = std::nullopt);
         void reloadAllScriptsImpl();
+        void synchronizedUpdateUnsafe();
 
         bool mInitialized = false;
         bool mGlobalScriptsStarted = false;
diff --git a/components/lua_ui/element.cpp b/components/lua_ui/element.cpp
index f3f873a583..2d8462e273 100644
--- a/components/lua_ui/element.cpp
+++ b/components/lua_ui/element.cpp
@@ -113,6 +113,7 @@ namespace LuaUi
             ContentView content(LuaUtil::cast<sol::table>(contentObj));
             result.resize(content.size());
             size_t minSize = std::min(children.size(), content.size());
+            std::vector<WidgetExtension*> toDestroy;
             for (size_t i = 0; i < minSize; i++)
             {
                 WidgetExtension* ext = children[i];
@@ -121,7 +122,7 @@ namespace LuaUi
                 {
                     WidgetExtension* root = pluckElementRoot(child, depth);
                     if (ext != root)
-                        destroyChild(ext);
+                        toDestroy.emplace_back(ext);
                     result[i] = root;
                 }
                 else
@@ -133,14 +134,12 @@ namespace LuaUi
                     }
                     else
                     {
-                        destroyChild(ext);
+                        toDestroy.emplace_back(ext);
                         ext = createWidget(newLayout, false, depth);
                     }
                     result[i] = ext;
                 }
             }
-            for (size_t i = minSize; i < children.size(); i++)
-                destroyChild(children[i]);
             for (size_t i = minSize; i < content.size(); i++)
             {
                 sol::object child = content.at(i);
@@ -149,6 +148,11 @@ namespace LuaUi
                 else
                     result[i] = createWidget(child.as<sol::table>(), false, depth);
             }
+            // Don't destroy anything until element creation has had a chance to throw
+            for (size_t i = minSize; i < children.size(); i++)
+                destroyChild(children[i]);
+            for (WidgetExtension* ext : toDestroy)
+                destroyChild(ext);
             return result;
         }
 
@@ -217,7 +221,9 @@ namespace LuaUi
         std::string setLayer(WidgetExtension* ext, const sol::table& layout)
         {
             MyGUI::ILayer* layerNode = ext->widget()->getLayer();
-            std::string currentLayer = layerNode ? layerNode->getName() : std::string();
+            std::string_view currentLayer;
+            if (layerNode)
+                currentLayer = layerNode->getName();
             std::string newLayer = layout.get_or(LayoutKeys::layer, std::string());
             if (!newLayer.empty() && !MyGUI::LayerManager::getInstance().isExist(newLayer))
                 throw std::logic_error(std::string("Layer ") + newLayer + " doesn't exist");
@@ -278,9 +284,20 @@ namespace LuaUi
                 WidgetExtension* parent = mRoot->getParent();
                 auto children = parent->children();
                 auto it = std::find(children.begin(), children.end(), mRoot);
-                mRoot = createWidget(layout(), true, 0);
                 assert(it != children.end());
-                *it = mRoot;
+                try
+                {
+                    mRoot = createWidget(layout(), true, 0);
+                    *it = mRoot;
+                }
+                catch (...)
+                {
+                    // Remove mRoot from its parent's children even if we couldn't replace it
+                    children.erase(it);
+                    parent->setChildren(children);
+                    mRoot = nullptr;
+                    throw;
+                }
                 parent->setChildren(children);
                 mRoot->updateCoord();
             }
@@ -300,6 +317,10 @@ namespace LuaUi
         {
             if (mRoot != nullptr)
             {
+                // If someone decided to destroy an element used as another element's content, we need to detach it
+                // first so the parent doesn't end up holding a stale pointer
+                if (WidgetExtension* parent = mRoot->getParent())
+                    parent->detachChildrenIf([&](WidgetExtension* child) { return child == mRoot; });
                 destroyRoot(mRoot);
                 mRoot = nullptr;
             }
diff --git a/components/lua_ui/widget.cpp b/components/lua_ui/widget.cpp
index e7e1053ab7..71416be8c8 100644
--- a/components/lua_ui/widget.cpp
+++ b/components/lua_ui/widget.cpp
@@ -126,6 +126,7 @@ namespace LuaUi
     {
         mParent = nullptr;
         widget()->detachFromWidget();
+        widget()->detachFromLayer();
     }
 
     WidgetExtension* WidgetExtension::findDeep(std::string_view flagName)
diff --git a/components/lua_ui/widget.hpp b/components/lua_ui/widget.hpp
index 24962f6820..0ec688d3bb 100644
--- a/components/lua_ui/widget.hpp
+++ b/components/lua_ui/widget.hpp
@@ -179,7 +179,7 @@ namespace LuaUi
 
         void updateVisible();
 
-        void detachChildrenIf(auto&& predicate, std::vector<WidgetExtension*> children)
+        void detachChildrenIf(auto&& predicate, std::vector<WidgetExtension*>& children)
         {
             for (auto it = children.begin(); it != children.end();)
             {