Merge branch 'lua_callback' into 'master'

Fix potential memory leak in LuaUtil::Callback

See merge request OpenMW/openmw!2733

apps/openmw/CMakeLists.txt

@@ -60,7 +60,7 @@ add_openmw_dir (mwscript
 
 add_openmw_dir (mwlua
     luamanagerimp object worldview userdataserializer eventqueue
-    luabindings localscripts playerscripts objectbindings cellbindings asyncbindings
+    luabindings localscripts playerscripts objectbindings cellbindings
     camerabindings uibindings inputbindings nearbybindings postprocessingbindings stats debugbindings
     types/types types/door types/actor types/container types/weapon types/npc types/creature types/activator types/book types/lockpick types/probe types/apparatus types/potion types/ingredient types/misc types/repair types/armor types/light types/static types/clothing
     worker

apps/openmw/mwlua/asyncbindings.cpp

@@ -1,72 +0,0 @@
-#include "luabindings.hpp"
-
-#include "worldview.hpp"
-
-namespace sol
-{
-    template <>
-    struct is_automagical<MWLua::AsyncPackageId> : std::false_type
-    {
-    };
-
-    template <>
-    struct is_automagical<LuaUtil::Callback> : std::false_type
-    {
-    };
-}
-
-namespace MWLua
-{
-
-    struct TimerCallback
-    {
-        AsyncPackageId mAsyncId;
-        std::string mName;
-    };
-
-    sol::function getAsyncPackageInitializer(const Context& context)
-    {
-        using TimerType = LuaUtil::ScriptsContainer::TimerType;
-        sol::usertype<AsyncPackageId> api = context.mLua->sol().new_usertype<AsyncPackageId>("AsyncPackage");
-        api["registerTimerCallback"]
-            = [](const AsyncPackageId& asyncId, std::string_view name, sol::main_protected_function callback) {
-                  asyncId.mContainer->registerTimerCallback(asyncId.mScriptId, name, std::move(callback));
-                  return TimerCallback{ asyncId, std::string(name) };
-              };
-        api["newSimulationTimer"] = [world = context.mWorldView](const AsyncPackageId&, double delay,
-                                        const TimerCallback& callback, sol::main_object callbackArg) {
-            callback.mAsyncId.mContainer->setupSerializableTimer(TimerType::SIMULATION_TIME,
-                world->getSimulationTime() + delay, callback.mAsyncId.mScriptId, callback.mName,
-                std::move(callbackArg));
-        };
-        api["newGameTimer"] = [world = context.mWorldView](const AsyncPackageId&, double delay,
-                                  const TimerCallback& callback, sol::main_object callbackArg) {
-            callback.mAsyncId.mContainer->setupSerializableTimer(TimerType::GAME_TIME, world->getGameTime() + delay,
-                callback.mAsyncId.mScriptId, callback.mName, std::move(callbackArg));
-        };
-        api["newUnsavableSimulationTimer"] = [world = context.mWorldView](const AsyncPackageId& asyncId, double delay,
-                                                 sol::main_protected_function callback) {
-            asyncId.mContainer->setupUnsavableTimer(
-                TimerType::SIMULATION_TIME, world->getSimulationTime() + delay, asyncId.mScriptId, std::move(callback));
-        };
-        api["newUnsavableGameTimer"] = [world = context.mWorldView](const AsyncPackageId& asyncId, double delay,
-                                           sol::main_protected_function callback) {
-            asyncId.mContainer->setupUnsavableTimer(
-                TimerType::GAME_TIME, world->getGameTime() + delay, asyncId.mScriptId, std::move(callback));
-        };
-        api["callback"] = [](const AsyncPackageId& asyncId, sol::main_protected_function fn) -> LuaUtil::Callback {
-            return LuaUtil::Callback{ std::move(fn), asyncId.mHiddenData };
-        };
-
-        sol::usertype<LuaUtil::Callback> callbackType = context.mLua->sol().new_usertype<LuaUtil::Callback>("Callback");
-        callbackType[sol::meta_function::call]
-            = [](const LuaUtil::Callback& callback, sol::variadic_args va) { return callback.call(sol::as_args(va)); };
-
-        auto initializer = [](sol::table hiddenData) {
-            LuaUtil::ScriptId id = hiddenData[LuaUtil::ScriptsContainer::sScriptIdKey];
-            return AsyncPackageId{ id.mContainer, id.mIndex, hiddenData };
-        };
-        return sol::make_object(context.mLua->sol(), initializer);
-    }
-
-}

apps/openmw/mwlua/luabindings.hpp

@@ -34,15 +34,6 @@ namespace MWLua
     void initCellBindingsForLocalScripts(const Context&);
     void initCellBindingsForGlobalScripts(const Context&);
 
-    // Implemented in asyncbindings.cpp
-    struct AsyncPackageId
-    {
-        LuaUtil::ScriptsContainer* mContainer;
-        int mScriptId;
-        sol::table mHiddenData;
-    };
-    sol::function getAsyncPackageInitializer(const Context&);
-
     // Implemented in camerabindings.cpp
     sol::table initCameraPackage(const Context&);
 

apps/openmw/mwlua/luamanagerimp.cpp

@@ -16,6 +16,7 @@
 
 #include <components/l10n/manager.hpp>
 
+#include <components/lua/asyncpackage.hpp>
 #include <components/lua/utilpackage.hpp>
 
 #include <components/lua_ui/content.hpp>
@@ -95,7 +96,10 @@ namespace MWLua
         LocalScripts::initializeSelfPackage(localContext);
         LuaUtil::LuaStorage::initLuaBindings(mLua.sol());
 
-        mLua.addCommonPackage("openmw.async", getAsyncPackageInitializer(context));
+        mLua.addCommonPackage("openmw.async",
+            LuaUtil::getAsyncPackageInitializer(
+                mLua.sol(), [this] { return mWorldView.getSimulationTime(); },
+                [this] { return mWorldView.getGameTime(); }));
         mLua.addCommonPackage("openmw.util", LuaUtil::initUtilPackage(mLua.sol()));
         mLua.addCommonPackage("openmw.core", initCorePackage(context));
         mLua.addCommonPackage("openmw.types", initTypesPackage(context));

apps/openmw/mwlua/nearbybindings.cpp

@@ -115,9 +115,9 @@ namespace MWLua
             MWBase::Environment::get().getWorld()->castRenderingRay(res, from, to, false, false);
             return res;
         };
-        api["asyncCastRenderingRay"] = [context](const LuaUtil::Callback& callback, const osg::Vec3f& from,
+        api["asyncCastRenderingRay"] = [context](
-                                           const osg::Vec3f& to) {
+                                           const sol::table& callback, const osg::Vec3f& from, const osg::Vec3f& to) {
-            context.mLuaManager->addAction([context, callback, from, to] {
+            context.mLuaManager->addAction([context, callback = LuaUtil::Callback::fromLua(callback), from, to] {
                 MWPhysics::RayCastingResult res;
                 MWBase::Environment::get().getWorld()->castRenderingRay(res, from, to, false, false);
                 context.mLuaManager->queueCallback(callback, sol::main_object(context.mLua->sol(), sol::in_place, res));

apps/openmw_test_suite/lua/test_async.cpp

@@ -1,8 +1,8 @@
 #include "gmock/gmock.h"
 #include <gtest/gtest.h>
 
+#include <components/lua/asyncpackage.hpp>
 #include <components/lua/luastate.hpp>
-#include <components/lua/scriptscontainer.hpp>
 
 #include "../testing_util.hpp"
 

apps/openmw_test_suite/lua/test_scriptscontainer.cpp

@@ -3,6 +3,7 @@
 
 #include <components/esm/luascripts.hpp>
 
+#include <components/lua/asyncpackage.hpp>
 #include <components/lua/luastate.hpp>
 #include <components/lua/scriptscontainer.hpp>
 

apps/openmw_test_suite/lua/test_storage.cpp

@@ -2,7 +2,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
-#include <components/lua/scriptscontainer.hpp>
+#include <components/lua/asyncpackage.hpp>
 #include <components/lua/storage.hpp>
 
 namespace
@@ -24,15 +24,16 @@ namespace
         LuaUtil::LuaStorage storage(mLua);
 
         std::vector<std::string> callbackCalls;
-        LuaUtil::Callback callback{ sol::make_object(mLua,
+        sol::table callbackHiddenData(mLua, sol::create);
-                                        [&](const std::string& section, const sol::optional<std::string>& key) {
+        callbackHiddenData[LuaUtil::ScriptsContainer::sScriptIdKey] = LuaUtil::ScriptId{};
-                                            if (key)
+        sol::table callback(mLua, sol::create);
-                                                callbackCalls.push_back(section + "_" + *key);
+        callback[1] = [&](const std::string& section, const sol::optional<std::string>& key) {
-                                            else
+            if (key)
-                                                callbackCalls.push_back(section + "_*");
+                callbackCalls.push_back(section + "_" + *key);
-                                        }),
+            else
-            sol::table(mLua, sol::create) };
+                callbackCalls.push_back(section + "_*");
-        callback.mHiddenData[LuaUtil::ScriptsContainer::sScriptIdKey] = LuaUtil::ScriptId{};
+        };
+        callback[2] = LuaUtil::AsyncPackageId{ nullptr, 0, callbackHiddenData };
 
         mLua["mutable"] = storage.getMutableSection("test");
         mLua["ro"] = storage.getReadOnlySection("test");

apps/openmw_test_suite/lua/test_ui_content.cpp

@@ -42,7 +42,7 @@ namespace
         mLuaState.sol()["M"] = makeContent(makeTable()).getMetatable();
         std::string testScript = R"(
             assert(not pcall(function() setmetatable(makeContent{}, {}) end), 'Metatable is not protected')
-            assert(getmetatable(makeContent{}) ~= M, 'Metatable is not protected')
+            assert(getmetatable(makeContent{}) == false, 'Metatable is not protected')
         )";
         EXPECT_NO_THROW(mLuaState.sol().safe_script(testScript));
     }

components/CMakeLists.txt

@@ -34,7 +34,7 @@ endif (GIT_CHECKOUT)
 # source files
 
 add_component_dir (lua
-    luastate scriptscontainer utilpackage serialization configuration l10n storage
+    luastate scriptscontainer asyncpackage utilpackage serialization configuration l10n storage
     )
 
 add_component_dir (l10n

components/lua/asyncpackage.cpp

@@ -0,0 +1,93 @@
+#include "asyncpackage.hpp"
+
+namespace sol
+{
+    template <>
+    struct is_automagical<LuaUtil::AsyncPackageId> : std::false_type
+    {
+    };
+
+    template <>
+    struct is_automagical<LuaUtil::Callback> : std::false_type
+    {
+    };
+}
+
+namespace LuaUtil
+{
+
+    struct TimerCallback
+    {
+        AsyncPackageId mAsyncId;
+        std::string mName;
+    };
+
+    Callback Callback::fromLua(const sol::table& t)
+    {
+        return Callback{ t.raw_get<sol::main_protected_function>(1), t.raw_get<AsyncPackageId>(2).mHiddenData };
+    }
+
+    bool Callback::isLuaCallback(const sol::object& t)
+    {
+        if (!t.is<sol::table>())
+            return false;
+        sol::object meta = sol::table(t)[sol::metatable_key];
+        if (!meta.is<sol::table>())
+            return false;
+        return sol::table(meta).raw_get_or<bool, std::string_view, bool>("isCallback", false);
+    }
+
+    sol::function getAsyncPackageInitializer(
+        lua_State* L, std::function<double()> simulationTimeFn, std::function<double()> gameTimeFn)
+    {
+        sol::state_view lua(L);
+        using TimerType = ScriptsContainer::TimerType;
+        sol::usertype<AsyncPackageId> api = lua.new_usertype<AsyncPackageId>("AsyncPackage");
+        api["registerTimerCallback"]
+            = [](const AsyncPackageId& asyncId, std::string_view name, sol::main_protected_function callback) {
+                  asyncId.mContainer->registerTimerCallback(asyncId.mScriptId, name, std::move(callback));
+                  return TimerCallback{ asyncId, std::string(name) };
+              };
+        api["newSimulationTimer"] = [simulationTimeFn](const AsyncPackageId&, double delay,
+                                        const TimerCallback& callback, sol::main_object callbackArg) {
+            callback.mAsyncId.mContainer->setupSerializableTimer(TimerType::SIMULATION_TIME, simulationTimeFn() + delay,
+                callback.mAsyncId.mScriptId, callback.mName, std::move(callbackArg));
+        };
+        api["newGameTimer"] = [gameTimeFn](const AsyncPackageId&, double delay, const TimerCallback& callback,
+                                  sol::main_object callbackArg) {
+            callback.mAsyncId.mContainer->setupSerializableTimer(TimerType::GAME_TIME, gameTimeFn() + delay,
+                callback.mAsyncId.mScriptId, callback.mName, std::move(callbackArg));
+        };
+        api["newUnsavableSimulationTimer"]
+            = [simulationTimeFn](const AsyncPackageId& asyncId, double delay, sol::main_protected_function callback) {
+                  asyncId.mContainer->setupUnsavableTimer(
+                      TimerType::SIMULATION_TIME, simulationTimeFn() + delay, asyncId.mScriptId, std::move(callback));
+              };
+        api["newUnsavableGameTimer"]
+            = [gameTimeFn](const AsyncPackageId& asyncId, double delay, sol::main_protected_function callback) {
+                  asyncId.mContainer->setupUnsavableTimer(
+                      TimerType::GAME_TIME, gameTimeFn() + delay, asyncId.mScriptId, std::move(callback));
+              };
+
+        sol::table callbackMeta = sol::table::create(L);
+        callbackMeta[sol::meta_function::call] = [](const sol::table& callback, sol::variadic_args va) {
+            return Callback::fromLua(callback).call(sol::as_args(va));
+        };
+        callbackMeta[sol::meta_function::to_string] = [] { return "Callback"; };
+        callbackMeta[sol::meta_function::metatable] = false;
+        callbackMeta["isCallback"] = true;
+        api["callback"] = [callbackMeta](const AsyncPackageId& asyncId, sol::main_protected_function fn) -> sol::table {
+            sol::table c = sol::table::create(fn.lua_state(), 2);
+            c.raw_set(1, std::move(fn), 2, asyncId);
+            c[sol::metatable_key] = callbackMeta;
+            return c;
+        };
+
+        auto initializer = [](sol::table hiddenData) {
+            ScriptId id = hiddenData[ScriptsContainer::sScriptIdKey];
+            return AsyncPackageId{ id.mContainer, id.mIndex, hiddenData };
+        };
+        return sol::make_object(lua, initializer);
+    }
+
+}

components/lua/asyncpackage.hpp

@@ -0,0 +1,57 @@
+#ifndef COMPONENTS_LUA_ASYNCPACKAGE_H
+#define COMPONENTS_LUA_ASYNCPACKAGE_H
+
+#include "scriptscontainer.hpp"
+
+namespace LuaUtil
+{
+    struct AsyncPackageId
+    {
+        ScriptsContainer* mContainer;
+        int mScriptId;
+        sol::table mHiddenData;
+    };
+    sol::function getAsyncPackageInitializer(
+        lua_State* L, std::function<double()> simulationTimeFn, std::function<double()> gameTimeFn);
+
+    // Wrapper for a Lua function.
+    // Holds information about the script the function belongs to.
+    // Needed to prevent callback calls if the script was removed.
+    struct Callback
+    {
+        sol::main_protected_function mFunc;
+        sol::table mHiddenData; // same object as Script::mHiddenData in ScriptsContainer
+
+        static bool isLuaCallback(const sol::object&);
+        static Callback fromLua(const sol::table&);
+
+        bool isValid() const { return mHiddenData[ScriptsContainer::sScriptIdKey] != sol::nil; }
+
+        template <typename... Args>
+        sol::object call(Args&&... args) const
+        {
+            sol::optional<ScriptId> scriptId = mHiddenData[ScriptsContainer::sScriptIdKey];
+            if (scriptId.has_value())
+                return LuaUtil::call(scriptId.value(), mFunc, std::forward<Args>(args)...);
+            else
+                Log(Debug::Debug) << "Ignored callback to the removed script "
+                                  << mHiddenData.get<std::string>(ScriptsContainer::sScriptDebugNameKey);
+            return sol::nil;
+        }
+
+        template <typename... Args>
+        void tryCall(Args&&... args) const
+        {
+            try
+            {
+                this->call(std::forward<Args>(args)...);
+            }
+            catch (std::exception& e)
+            {
+                Log(Debug::Error) << "Error in callback: " << e.what();
+            }
+        }
+    };
+}
+
+#endif // COMPONENTS_LUA_ASYNCPACKAGE_H

components/lua/scriptscontainer.hpp

@@ -269,43 +269,6 @@ namespace LuaUtil
 
         static int64_t sInstanceCount; // debug information, shown in Lua profiler
     };
-
-    // Wrapper for a Lua function.
-    // Holds information about the script the function belongs to.
-    // Needed to prevent callback calls if the script was removed.
-    struct Callback
-    {
-        sol::main_protected_function mFunc;
-        sol::table mHiddenData; // same object as Script::mHiddenData in ScriptsContainer
-
-        bool isValid() const { return mHiddenData[ScriptsContainer::sScriptIdKey] != sol::nil; }
-
-        template <typename... Args>
-        sol::object call(Args&&... args) const
-        {
-            sol::optional<ScriptId> scriptId = mHiddenData[ScriptsContainer::sScriptIdKey];
-            if (scriptId.has_value())
-                return LuaUtil::call(scriptId.value(), mFunc, std::forward<Args>(args)...);
-            else
-                Log(Debug::Debug) << "Ignored callback to the removed script "
-                                  << mHiddenData.get<std::string>(ScriptsContainer::sScriptDebugNameKey);
-            return sol::nil;
-        }
-
-        template <typename... Args>
-        void tryCall(Args&&... args) const
-        {
-            try
-            {
-                this->call(std::forward<Args>(args)...);
-            }
-            catch (std::exception& e)
-            {
-                Log(Debug::Error) << "Error in callback: " << e.what();
-            }
-        }
-    };
-
 }
 
 #endif // COMPONENTS_LUA_SCRIPTSCONTAINER_H

components/lua/storage.cpp

@@ -111,7 +111,7 @@ namespace LuaUtil
             return section.mSection->get(key).getCopy(s);
         };
         sview["asTable"] = [](const SectionView& section) { return section.mSection->asTable(); };
-        sview["subscribe"] = [](const SectionView& section, const Callback& callback) {
+        sview["subscribe"] = [](const SectionView& section, const sol::table& callback) {
             std::vector<Callback>& callbacks = section.mSection->mCallbacks;
             if (!callbacks.empty() && callbacks.size() == callbacks.capacity())
             {
@@ -119,7 +119,7 @@ namespace LuaUtil
                     std::remove_if(callbacks.begin(), callbacks.end(), [&](const Callback& c) { return !c.isValid(); }),
                     callbacks.end());
             }
-            callbacks.push_back(callback);
+            callbacks.push_back(Callback::fromLua(callback));
         };
         sview["reset"] = [](const SectionView& section, const sol::optional<sol::table>& newValues) {
             if (section.mReadOnly)

components/lua/storage.hpp

@@ -4,7 +4,7 @@
 #include <map>
 #include <sol/sol.hpp>
 
-#include "scriptscontainer.hpp"
+#include "asyncpackage.hpp"
 #include "serialization.hpp"
 
 namespace LuaUtil

components/lua_ui/content.lua

@@ -135,6 +135,6 @@ M.__pairs = function(self)
     return next, self, 1
 end
 M.__ipairs = M.__pairs
-M.__metatable = {}
+M.__metatable = false
 
 return M

components/lua_ui/element.cpp

@@ -106,11 +106,11 @@ namespace LuaUi
                 throw std::logic_error("The \"events\" layout field must be a table of callbacks");
             auto events = eventsObj.as<sol::table>();
             events.for_each([ext](const sol::object& name, const sol::object& callback) {
-                if (name.is<std::string>() && callback.is<LuaUtil::Callback>())
+                if (name.is<std::string>() && LuaUtil::Callback::isLuaCallback(callback))
-                    ext->setCallback(name.as<std::string>(), callback.as<LuaUtil::Callback>());
+                    ext->setCallback(name.as<std::string>(), LuaUtil::Callback::fromLua(callback));
                 else if (!name.is<std::string>())
                     Log(Debug::Warning) << "UI event key must be a string";
-                else if (!callback.is<LuaUtil::Callback>())
+                else
                     Log(Debug::Warning) << "UI event handler for key \"" << name.as<std::string>()
                                         << "\" must be an openmw.async.callback";
             });

components/lua_ui/widget.hpp

@@ -7,7 +7,7 @@
 #include <MyGUI_Widget.h>
 #include <sol/sol.hpp>
 
-#include <components/lua/scriptscontainer.hpp>
+#include <components/lua/asyncpackage.hpp>
 
 #include "properties.hpp"