lua_emplace<T>() should permit GC despite LL.atexit() safety net.

lua_emplace<T>() was passing LL.atexit() a closure binding the new userdata
with a cleanup function. The trouble with that was that a strong reference to
the new userdata would prevent it ever being garbage collected, even if that
was the only remaining reference.

Instead, create a new weak table referencing the userdata, and bind that into
the cleanup function's closure. Then if the only remaining reference to the
userdata is from the weak table, the userdata can be collected.

Make lua_emplace_call_gc<T>() check the bound weak table in case the userdata
has in fact been collected.

Also, in lua_toclass<T>(), use luaL_checkudata() to synopsize comparing the
putative userdata's metatable against the one synthesized by lua_emplace<T>().
This saves several explicit steps.

1 files changed, 74 insertions, 27 deletions

diff --git a/indra/llcommon/lua_function.h b/indra/llcommon/lua_function.h
index 8b93053a46..7a3d9e7dd7 100644
--- a/indra/llcommon/lua_function.h
+++ b/indra/llcommon/lua_function.h
@@ -213,19 +213,21 @@ std::string lua_emplace_metaname(const std::string& Tname = LLError::Log::classn
  * On the stack belonging to the passed lua_State, push a Lua userdata object
  * with a newly-constructed C++ object std::optional<T>(args...). The new
  * userdata has a metadata table with a __gc() function to ensure that when
- * the userdata instance is garbage-collected, ~T() is called.
+ * the userdata instance is garbage-collected, ~T() is called. Also call
+ * LL.atexit(lua_emplace_call_gc<T>(object)) to make ~LuaState() call ~T().
  *
  * We wrap the userdata object as std::optional<T> so we can explicitly
  * destroy the contained T, and detect that we've done so.
  *
  * Usage:
  * lua_emplace<T>(L, T constructor args...);
+ * // L's Lua stack top is now a userdata containing T
  */
 template <class T, typename... ARGS>
 void lua_emplace(lua_State* L, ARGS&&... args)
 {
     using optT = std::optional<T>;
-    luaL_checkstack(L, 3, nullptr);
+    luaL_checkstack(L, 5, nullptr);
     auto ptr = lua_newuserdata(L, sizeof(optT));
     // stack is uninitialized userdata
     // For now, assume (but verify) that lua_newuserdata() returns a
@@ -254,38 +256,94 @@ void lua_emplace(lua_State* L, ARGS&&... args)
     lua_setmetatable(L, -2);
     // Stack is now userdata, initialized with T(args),
     // with metatable.__gc pointing to lua_emplace_gc<T>.
+
     // But wait, there's more! Use our atexit() function to ensure that this
-    // C++ object is eventually cleaned up even if the garbage collector never
+    // C++ object is eventually destroyed even if the garbage collector never
     // gets around to it.
     lua_getglobal(L, "LL");
     // stack contains userdata, LL
     lua_getfield(L, -1, "atexit");
     // stack contains userdata, LL, LL.atexit
+    // ditch LL
+    lua_replace(L, -2);
+    // stack contains userdata, LL.atexit
+
+    // We have a bit of a problem here. We want to allow the garbage collector
+    // to collect the userdata if it must; but we also want to register a
+    // cleanup function to destroy the value if (usual case) it has NOT been
+    // garbage-collected. The problem is that if we bind into atexit()'s queue
+    // a strong reference to the userdata, we ensure that the garbage
+    // collector cannot collect it, making our metatable with __gc function
+    // completely moot. And we must assume that lua_pushcclosure() binds a
+    // strong reference to each value passed as a closure.
+
+    // The solution is to use one more indirection: create a weak table whose
+    // sole entry is the userdata. If all other references to the new userdata
+    // are forgotten, so the only remaining reference is the weak table, the
+    // userdata can be collected. Then we can bind that weak table as the
+    // closure value for our cleanup function.
+    // The new weak table will have at most 1 array value, 0 other keys.
+    lua_createtable(L, 1, 0);
+    // stack contains userdata, LL.atexit, weak_table
+    if (luaL_newmetatable(L, "weak_values"))
+    {
+        // stack contains userdata, LL.atexit, weak_table, weak_values
+        // just created "weak_values" metatable: populate it
+        // Registry.weak_values = {__mode="v"}
+        lua_pushliteral(L, "v");
+        // stack contains userdata, LL.atexit, weak_table, weak_values, "v"
+        lua_setfield(L, -2, "__mode");
+    }
+    // stack contains userdata, LL.atexit, weak_table, weak_values
+    // setmetatable(weak_table, weak_values)
+    lua_setmetatable(L, -2);
+    // stack contains userdata, LL.atexit, weak_table
+    lua_pushinteger(L, 1);
+    // stack contains userdata, LL.atexit, weak_table, 1
     // duplicate userdata
-    lua_pushvalue(L, -3);
-    // stack contains userdata, LL, LL.atexit, userdata
-    // push a closure binding (lua_emplace_call_gc<T>, userdata)
+    lua_pushvalue(L, -4);
+    // stack contains userdata, LL.atexit, weak_table, 1, userdata
+    // weak_table[1] = userdata
+    lua_settable(L, -3);
+    // stack contains userdata, LL.atexit, weak_table
+
+    // push a closure binding (lua_emplace_call_gc<T>, weak_table)
     auto callgcname{ stringize("lua_emplace_call_gc<", Tname, ">") };
     lua_pushcclosure(L, lua_emplace_call_gc<T>, callgcname.c_str(), 1);
-    // stack contains userdata, LL, LL.atexit, closure
+    // stack contains userdata, LL.atexit, closure
     // Call LL.atexit(closure)
     lua_call(L, 1, 0);
-    // stack contains userdata, LL
-    lua_pop(L, 1);
     // stack contains userdata -- return that
 }
 
 namespace {
 
-// passed to LL.atexit(closure(lua_emplace_call_gc<T>, userdata));
+// passed to LL.atexit(closure(lua_emplace_call_gc<T>, weak_table{userdata}));
 // retrieves bound userdata to pass to lua_emplace_gc<T>()
 template <class T>
 int lua_emplace_call_gc(lua_State* L)
 {
-    luaL_checkstack(L, 1, nullptr);
-    // retrieve the first (only) bound upvalue and push to stack top as the
-    // argument for lua_emplace_gc<T>()
+    luaL_checkstack(L, 2, nullptr);
+    // retrieve the first (only) bound upvalue and push to stack top
     lua_pushvalue(L, lua_upvalueindex(1));
+    // This is the weak_table bound by lua_emplace<T>(). Its one and only
+    // entry should be the lua_emplace<T>() userdata -- unless userdata has
+    // been garbage collected. Retrieve weak_table[1].
+    lua_pushinteger(L, 1);
+    // stack contains weak_table, 1
+    lua_gettable(L, -2);
+    // stack contains weak_table, weak_table[1]
+    // If our userdata was garbage-collected, there is no weak_table[1],
+    // and we just retrieved nil.
+    if (lua_isnil(L, -1))
+    {
+        lua_pop(L, 2);
+        return 0;
+    }
+    // stack contains weak_table, userdata
+    // ditch weak_table
+    lua_replace(L, -2);
+    // pass userdata to lua_emplace_gc<T>()
     return lua_emplace_gc<T>(L);
 }
 
@@ -325,23 +383,12 @@ template <class T>
 T* lua_toclass(lua_State* L, int index)
 {
     using optT = std::optional<T>;
-    luaL_checkstack(L, 2, nullptr);
+    // recreate the name lua_emplace<T>() uses for its metatable
+    auto metaname{ lua_emplace_metaname<T>() };
     // get void* pointer to userdata (if that's what it is)
-    auto ptr{ lua_touserdata(L, index) };
+    void* ptr{ luaL_checkudata(L, index, metaname.c_str()) };
     if (! ptr)
         return nullptr;
-    // push the metatable for this userdata, if any
-    if (! lua_getmetatable(L, index))
-        return nullptr;
-    // now push the metatable created by lua_emplace<T>()
-    auto metaname{ lua_emplace_metaname<T>() };
-    luaL_getmetatable(L, metaname.c_str());
-    auto equal{ lua_equal(L, -1, -2) };
-    // Having compared the userdata's metatable with the one set by
-    // lua_emplace<T>(), we no longer need either metatable on the stack.
-    lua_pop(L, 2);
-    if (! equal)
-        return nullptr;
     // Derive the optT* from ptr. If in future lua_emplace() must manually
     // align our optT* within the Lua-provided void*, adjust accordingly.
     optT* tptr(static_cast<optT*>(ptr));