Merge branch 'fix_lua_memoryleak' into 'master'

Fix Lua memory leak (#7128)

Closes #7128

See merge request OpenMW/openmw!2774

apps/openmw_test_suite/lua/test_lua.cpp

@@ -51,10 +51,26 @@ return {
 }
 )X");
 
+    std::string genBigScript()
+    {
+        std::stringstream buf;
+        buf << "return function()\n";
+        buf << "  x = {}\n";
+        for (int i = 0; i < 1000; ++i)
+            buf << "  x[" << i * 2 << "] = " << i << "\n";
+        buf << "  return x\n";
+        buf << "end\n";
+        return buf.str();
+    }
+
+    TestingOpenMW::VFSTestFile bigScriptFile(genBigScript());
+    TestingOpenMW::VFSTestFile requireBigScriptFile("local x = require('big') ; return {x}");
+
     struct LuaStateTest : Test
     {
         std::unique_ptr<VFS::Manager> mVFS = TestingOpenMW::createTestVFS({ { "aaa/counter.lua", &counterFile },
-            { "bbb/tests.lua", &testsFile }, { "invalid.lua", &invalidScriptFile } });
+            { "bbb/tests.lua", &testsFile }, { "invalid.lua", &invalidScriptFile }, { "big.lua", &bigScriptFile },
+            { "requireBig.lua", &requireBigScriptFile } });
 
         LuaUtil::ScriptsConfiguration mCfg;
         LuaUtil::LuaState mLua{ mVFS.get(), &mCfg };
@@ -172,4 +188,22 @@ return {
         EXPECT_THAT(LuaUtil::getLuaVersion(), HasSubstr("Lua"));
     }
 
+    TEST_F(LuaStateTest, RemovedScriptsGarbageCollecting)
+    {
+        auto getMem = [&] {
+            for (int i = 0; i < 5; ++i)
+                lua_gc(mLua.sol(), LUA_GCCOLLECT, 0);
+            return mLua.getTotalMemoryUsage();
+        };
+        int64_t memWithScript;
+        {
+            sol::object s = mLua.runInNewSandbox("requireBig.lua");
+            memWithScript = getMem();
+        }
+        for (int i = 0; i < 100; ++i) // run many times to make small memory leaks visible
+            mLua.runInNewSandbox("requireBig.lua");
+        int64_t memWithoutScript = getMem();
+        // At this moment all instances of the script should be garbage-collected.
+        EXPECT_LT(memWithoutScript, memWithScript);
+    }
 }

components/lua/luastate.cpp

@@ -126,11 +126,14 @@ namespace LuaUtil
                     id = self->mActiveScriptIdStack.back();
                 bigAllocDelta = nsize;
             }
-            if (id.mContainer)
+            if (id.mIndex >= 0)
             {
                 if (static_cast<size_t>(id.mIndex) >= self->mMemoryUsage.size())
                     self->mMemoryUsage.resize(id.mIndex + 1);
                 self->mMemoryUsage[id.mIndex] += bigAllocDelta;
+            }
+            if (id.mContainer)
+            {
                 id.mContainer->addMemoryUsage(id.mIndex, bigAllocDelta);
                 if (newPtr && nsize > smallAllocSize)
                     self->mBigAllocOwners.emplace(newPtr, AllocOwner{ id.mContainer->mThis, id.mIndex });
@@ -180,6 +183,13 @@ namespace LuaUtil
 
         mSol["writeToLog"] = [](std::string_view s) { Log(Debug::Level::Info) << s; };
 
+        mSol["setEnvironment"]
+            = [](const sol::environment& env, const sol::function& fn) { sol::set_environment(env, fn); };
+        mSol["loadFromVFS"] = [this](std::string_view packageName) {
+            return loadScriptAndCache(packageNameToVfsPath(packageName, mVFS));
+        };
+        mSol["loadInternalLib"] = [this](std::string_view packageName) { return loadInternalLib(packageName); };
+
         // Some fixes for compatibility between different Lua versions
         if (mSol["unpack"] == sol::nil)
             mSol["unpack"] = mSol["table"]["unpack"];
@@ -205,6 +215,19 @@ namespace LuaUtil
             end
             printGen = function(name) return function(...) return printToLog(name, ...) end end
 
+            function requireGen(env, loaded, loadFn)
+                return function(packageName)
+                    local p = loaded[packageName]
+                    if p == nil then
+                        local loader = loadFn(packageName)
+                        setEnvironment(env, loader)
+                        p = loader(packageName)
+                        loaded[packageName] = p
+                    end
+                    return p
+                end
+            end
+
             function createStrictIndexFn(tbl)
                 return function(_, key)
                     local res = tbl[key]
@@ -324,16 +347,7 @@ namespace LuaUtil
             loaded[key] = maybeRunLoader(value);
         for (const auto& [key, value] : packages)
             loaded[key] = maybeRunLoader(value);
-        env["require"] = [this, env, loaded, hiddenData](std::string_view packageName) mutable {
-            sol::object package = loaded[packageName];
-            if (package != sol::nil)
-                return package;
-            sol::protected_function packageLoader = loadScriptAndCache(packageNameToVfsPath(packageName, mVFS));
-            sol::set_environment(env, packageLoader);
-            package = call(packageLoader, packageName);
-            loaded[packageName] = package;
-            return package;
-        };
+        env["require"] = mSol["requireGen"](env, loaded, mSol["loadFromVFS"]);
 
         sol::set_environment(env, script);
         return call(scriptId, script);
@@ -345,14 +359,7 @@ namespace LuaUtil
         sol::table loaded(mSol, sol::create);
         for (const std::string& s : safePackages)
             loaded[s] = static_cast<sol::object>(mSandboxEnv[s]);
-        env["require"] = [this, loaded, env](const std::string& module) mutable {
-            if (loaded[module] != sol::nil)
-                return loaded[module];
-            sol::protected_function initializer = loadInternalLib(module);
-            sol::set_environment(env, initializer);
-            loaded[module] = call({}, initializer, module);
-            return loaded[module];
-        };
+        env["require"] = mSol["requireGen"](env, loaded, mSol["loadInternalLib"]);
         return env;
     }
 

components/lua/luastate.hpp

@@ -23,7 +23,7 @@ namespace LuaUtil
     struct ScriptId
     {
         ScriptsContainer* mContainer = nullptr;
-        int mIndex; // index in LuaUtil::ScriptsConfiguration
+        int mIndex = -1; // index in LuaUtil::ScriptsConfiguration
     };
 
     struct LuaStateSettings