Lua: Improve memory management

Previously the PluginSpec would create a keep around a single lua vm
that was then shared with the actual plugin instance. This made it
unclear when the VM was freed.

Now we instead cleanup the lua vm used to fetch the plugin infos
immediately and instead create a fresh vm for the actual plugin
instance that can then follow the normal Plugin lifecylce.

Change-Id: I81bb9ecf57706c2ba1b0d8db83ab26b3b8e944f2
Reviewed-by: David Schulz <david.schulz@qt.io>

src/plugins/lua/luaengine.cpp

@@ -4,6 +4,7 @@
 #include "luaengine.h"
 
 #include "luapluginspec.h"
+#include "luatr.h"
 
 #include <coreplugin/icore.h>
 #include <coreplugin/messagemanager.h>
@@ -172,14 +173,6 @@ expected_str<void> LuaEngine::connectHooks(
     return {};
 }
 
-expected_str<void> LuaEngine::connectHooks(sol::state_view lua, const sol::table &hookTable)
-{
-    if (!hookTable)
-        return {};
-
-    return instance().connectHooks(lua, hookTable, "");
-}
-
 expected_str<LuaPluginSpec *> LuaEngine::loadPlugin(const Utils::FilePath &path)
 {
     auto contents = path.fileContents();
@@ -204,12 +197,16 @@ expected_str<LuaPluginSpec *> LuaEngine::loadPlugin(const Utils::FilePath &path)
     sol::table pluginInfo = result.get<sol::table>();
     if (!pluginInfo.valid())
         return make_unexpected(QString("Script did not return a table with plugin info"));
-    return LuaPluginSpec::create(path, std::move(lua), pluginInfo);
+    return LuaPluginSpec::create(path, pluginInfo);
 }
 
-expected_str<void> LuaEngine::prepareSetup(
-    sol::state_view &lua, const LuaPluginSpec &pluginSpec, sol::optional<sol::table> hookTable)
+expected_str<sol::protected_function> LuaEngine::prepareSetup(
+    sol::state_view lua, const LuaPluginSpec &pluginSpec)
 {
+    auto contents = pluginSpec.filePath().fileContents();
+    if (!contents)
+        return make_unexpected(contents.error());
+
     // TODO: Only open libraries requested by the plugin
     lua.open_libraries(
         sol::lib::base,
@@ -261,10 +258,29 @@ expected_str<void> LuaEngine::prepareSetup(
     for (const auto &func : d->m_autoProviders)
         func(lua);
 
-    if (hookTable)
-        return LuaEngine::connectHooks(lua, *hookTable);
+    sol::protected_function_result result = lua.safe_script(
+        std::string_view(contents->data(), contents->size()),
+        sol::script_pass_on_error,
+        pluginSpec.filePath().fileName().toUtf8().constData());
 
-    return {};
+    auto pluginTable = result.get<sol::optional<sol::table>>();
+    if (!pluginTable)
+        return make_unexpected(Tr::tr("Script did not return a table"));
+
+    auto hookTable = pluginTable->get<sol::optional<sol::table>>("hooks");
+
+    if (hookTable) {
+        auto connectResult = connectHooks(lua, *hookTable, {});
+        if (!connectResult)
+            return make_unexpected(connectResult.error());
+    }
+
+    auto setupFunction = pluginTable->get_or<sol::function>("setup", {});
+
+    if (!setupFunction)
+        return make_unexpected(Tr::tr("Plugin info table did not contain a setup function"));
+
+    return setupFunction;
 }
 
 bool LuaEngine::isCoroutine(lua_State *state)

src/plugins/lua/luaengine.h

@@ -55,15 +55,13 @@ public:
     static LuaEngine &instance();
 
     Utils::expected_str<LuaPluginSpec *> loadPlugin(const Utils::FilePath &path);
-    Utils::expected_str<void> prepareSetup(
-        sol::state_view &lua, const LuaPluginSpec &pluginSpec, sol::optional<sol::table> hookTable);
+    Utils::expected_str<sol::protected_function> prepareSetup(
+        sol::state_view lua, const LuaPluginSpec &pluginSpec);
 
     static void registerProvider(const QString &packageName, const PackageProvider &provider);
     static void autoRegister(const std::function<void(sol::state_view)> &registerFunction);
     static void registerHook(QString name, const std::function<void(sol::function)> &hookProvider);
 
-    static Utils::expected_str<void> connectHooks(sol::state_view lua, const sol::table &hookTable);
-
     static bool isCoroutine(lua_State *state);
 
     static sol::table toTable(const sol::state_view &lua, const QJsonValue &v);

src/plugins/lua/luapluginspec.cpp

@@ -34,28 +34,19 @@ class LuaPluginSpecPrivate
 {
 public:
     FilePath pluginScriptPath;
-
-    sol::state lua;
-    sol::table pluginTable;
-
-    sol::function setupFunction;
+    bool printToOutputPane = false;
+    std::unique_ptr<sol::state> activeLuaState;
 };
 
 LuaPluginSpec::LuaPluginSpec()
     : d(new LuaPluginSpecPrivate())
 {}
 
-expected_str<LuaPluginSpec *> LuaPluginSpec::create(const FilePath &filePath,
-                                                    sol::state lua,
-                                                    sol::table pluginTable)
+expected_str<LuaPluginSpec *> LuaPluginSpec::create(const FilePath &filePath, sol::table pluginTable)
 {
     std::unique_ptr<LuaPluginSpec> pluginSpec(new LuaPluginSpec());
 
-    pluginSpec->d->lua = std::move(lua);
-    pluginSpec->d->pluginTable = pluginTable;
-
-    pluginSpec->d->setupFunction = pluginTable.get_or<sol::function>("setup", {});
-    if (!pluginSpec->d->setupFunction)
+    if (!pluginTable.get_or<sol::function>("setup", {}))
         return make_unexpected(QString("Plugin info table did not contain a setup function"));
 
     QJsonValue v = LuaEngine::toJson(pluginTable);
@@ -75,6 +66,7 @@ expected_str<LuaPluginSpec *> LuaPluginSpec::create(const FilePath &filePath,
     pluginSpec->setLocation(filePath.parentDir());
 
     pluginSpec->d->pluginScriptPath = filePath;
+    pluginSpec->d->printToOutputPane = pluginTable.get_or("printToOutputPane", false);
 
     return pluginSpec.release();
 }
@@ -94,16 +86,19 @@ bool LuaPluginSpec::loadLibrary()
 }
 bool LuaPluginSpec::initializePlugin()
 {
-    expected_str<void> setupResult
-        = LuaEngine::instance()
-              .prepareSetup(d->lua, *this, d->pluginTable.get<sol::optional<sol::table>>("hooks"));
+    QTC_ASSERT(!d->activeLuaState, return false);
+
+    std::unique_ptr<sol::state> activeLuaState = std::make_unique<sol::state>();
+
+    expected_str<sol::protected_function> setupResult
+        = LuaEngine::instance().prepareSetup(*activeLuaState, *this);
 
     if (!setupResult) {
         setError(Lua::Tr::tr("Failed to prepare plugin setup: %1").arg(setupResult.error()));
         return false;
     }
 
-    auto result = d->setupFunction.call();
+    auto result = setupResult->call();
 
     if (result.get_type() == sol::type::boolean && result.get<bool>() == false) {
         setError(Lua::Tr::tr("Plugin setup function returned false"));
@@ -117,6 +112,8 @@ bool LuaPluginSpec::initializePlugin()
         }
     }
 
+    d->activeLuaState = std::move(activeLuaState);
+
     setState(PluginSpec::State::Initialized);
     return true;
 }
@@ -133,14 +130,18 @@ bool LuaPluginSpec::delayedInitialize()
 }
 ExtensionSystem::IPlugin::ShutdownFlag LuaPluginSpec::stop()
 {
+    d->activeLuaState.reset();
     return ExtensionSystem::IPlugin::ShutdownFlag::SynchronousShutdown;
 }
 
-void LuaPluginSpec::kill() {}
+void LuaPluginSpec::kill()
+{
+    d->activeLuaState.reset();
+}
 
 bool LuaPluginSpec::printToOutputPane() const
 {
-    return d->pluginTable.get_or("printToOutputPane", false);
+    return d->printToOutputPane;
 }
 
 } // namespace Lua

src/plugins/lua/luapluginspec.h

@@ -39,9 +39,8 @@ class LuaPluginSpec : public ExtensionSystem::PluginSpec
     LuaPluginSpec();
 
 public:
-    static Utils::expected_str<LuaPluginSpec *> create(const Utils::FilePath &filePath,
-                                                       sol::state lua,
-                                                       sol::table pluginTable);
+    static Utils::expected_str<LuaPluginSpec *> create(
+        const Utils::FilePath &filePath, sol::table pluginTable);
 
     ExtensionSystem::IPlugin *plugin() const override;