Store script's LuaListener in userdata in lua_State's Registry.

Instead of deriving LuaListener from LLInstanceTracker with an int key,
generating a unique int key and storing that key in the Registry, use new
lua_emplace<LuaState>() to store the LuaListener directly in a Lua userdata
object in the Lua Registry.

Because lua_emplace<T>() uses LL.atexit() to guarantee that ~LuaState will
destroy the T object, we no longer need ~LuaState() to make a special call
specifically to destroy the LuaListener, if any. So we no longer need
LuaState::getListener() separate from obtainListener().

Since LuaListener is no longer an LLInstanceTracker subclass, make
LuaState::obtainListener() return LuaListener& rather than LuaListener::ptr_t.

4 files changed, 38 insertions, 89 deletions

diff --git a/indra/llcommon/lua_function.cpp b/indra/llcommon/lua_function.cpp
index edd49feed9..cd1a0cd562 100644
--- a/indra/llcommon/lua_function.cpp
+++ b/indra/llcommon/lua_function.cpp
@@ -520,10 +520,6 @@ LuaState::~LuaState()
     // pop Registry["atexit"] (either table or nil)
     lua_pop(mState, 1);
 
-    // Did somebody call obtainListener() on this LuaState?
-    // That is, is there a LuaListener key in its registry?
-    LuaListener::destruct(getListener());
-
     lua_close(mState);
 
     if (mCallback)
@@ -677,43 +673,30 @@ std::pair<int, LLSD> LuaState::expr(const std::string& desc, const std::string&
     return result;
 }
 
-LuaListener::ptr_t LuaState::getListener(lua_State* L)
+LuaListener& LuaState::obtainListener(lua_State* L)
 {
-    // have to use one more stack slot
-    luaL_checkstack(L, 1, nullptr);
-    LuaListener::ptr_t listener;
-    // Does this lua_State already have a LuaListener stored in the registry?
-    auto keytype{ lua_getfield(L, LUA_REGISTRYINDEX, "event.listener") };
-    llassert(keytype == LUA_TNIL || keytype == LUA_TNUMBER);
-    if (keytype == LUA_TNUMBER)
+    luaL_checkstack(L, 2, nullptr);
+    lua_getfield(L, LUA_REGISTRYINDEX, "LuaListener");
+    // compare lua_type() because lua_isuserdata() also accepts light userdata
+    if (lua_type(L, -1) != LUA_TUSERDATA)
     {
-        // We do already have a LuaListener. Retrieve it.
-        int isint;
-        listener = LuaListener::getInstance(lua_tointegerx(L, -1, &isint));
-        // Nobody should have destroyed this LuaListener instance!
-        llassert(isint && listener);
+        llassert(lua_type(L, -1) == LUA_TNIL);
+        lua_pop(L, 1);
+        // push a userdata containing new LuaListener, binding L
+        lua_emplace<LuaListener>(L, L);
+        // duplicate the top stack entry so we can store one copy
+        lua_pushvalue(L, -1);
+        lua_setfield(L, LUA_REGISTRYINDEX, "LuaListener");
     }
-    // pop the int "event.listener" key
+    // At this point, one way or the other, the stack top should be (a Lua
+    // userdata containing) our LuaListener.
+    LuaListener* listener{ lua_toclass<LuaListener>(L, -1) };
+    // userdata objects created by lua_emplace<T>() are bound on the atexit()
+    // queue, and are thus never garbage collected: they're destroyed only
+    // when ~LuaState() walks that queue. That's why we dare pop the userdata
+    // value off the stack while still depending on a pointer into its data.
     lua_pop(L, 1);
-    return listener;
-}
-
-LuaListener::ptr_t LuaState::obtainListener(lua_State* L)
-{
-    auto listener{ getListener(L) };
-    if (! listener)
-    {
-        // have to use one more stack slot
-        luaL_checkstack(L, 1, nullptr);
-        // instantiate a new LuaListener, binding the L state -- but use a
-        // no-op deleter: we do NOT want this ptr_t to manage the lifespan of
-        // this new LuaListener!
-        listener.reset(new LuaListener(L), [](LuaListener*){});
-        // set its key in the field where we'll look for it later
-        lua_pushinteger(L, listener->getKey());
-        lua_setfield(L, LUA_REGISTRYINDEX, "event.listener");
-    }
-    return listener;
+    return *listener;
 }
 
 /*****************************************************************************
@@ -938,10 +921,10 @@ lua_function(
     lua_settop(L, 0);
 
     auto& outpump{ LLEventPumps::instance().obtain("lua output") };
-    auto listener{ LuaState::obtainListener(L) };
+    auto& listener{ LuaState::obtainListener(L) };
     LLEventStream replyPump("leaphelp", true);
     // ask the LuaListener's LeapListener and suspend calling coroutine until reply
-    auto reply{ llcoro::postAndSuspend(request, listener->getCommandName(), replyPump, "reply") };
+    auto reply{ llcoro::postAndSuspend(request, listener.getCommandName(), replyPump, "reply") };
     reply.erase("reqid");
 
     if (auto error = reply["error"]; error.isString())
diff --git a/indra/llcommon/lua_function.h b/indra/llcommon/lua_function.h
index 5bbcbc441f..8b93053a46 100644
--- a/indra/llcommon/lua_function.h
+++ b/indra/llcommon/lua_function.h
@@ -102,16 +102,10 @@ public:
 
     operator lua_State*() const { return mState; }
 
-    // Return LuaListener for this LuaState if we already have one, else empty
-    // shared_ptr.
-    std::shared_ptr<LuaListener> getListener() { return getListener(mState); }
-    // Find or create LuaListener for this LuaState, returning its ptr_t.
-    std::shared_ptr<LuaListener> obtainListener() { return obtainListener(mState); }
-    // Return LuaListener for passed lua_State if we already have one, else
-    // empty shared_ptr.
-    static std::shared_ptr<LuaListener> getListener(lua_State* L);
-    // Find or create LuaListener for passed lua_State, returning its ptr_t.
-    static std::shared_ptr<LuaListener> obtainListener(lua_State* L);
+    // Find or create LuaListener for this LuaState.
+    LuaListener& obtainListener() { return obtainListener(mState); }
+    // Find or create LuaListener for passed lua_State.
+    static LuaListener& obtainListener(lua_State* L);
 
 private:
     script_finished_fn mCallback;
@@ -350,7 +344,7 @@ T* lua_toclass(lua_State* L, int index)
         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(ptr);
+    optT* tptr(static_cast<optT*>(ptr));
     // make sure our optT isn't empty
     if (! *tptr)
         return nullptr;
diff --git a/indra/llcommon/lualistener.cpp b/indra/llcommon/lualistener.cpp
index 5c4989e891..6cb87e8af2 100644
--- a/indra/llcommon/lualistener.cpp
+++ b/indra/llcommon/lualistener.cpp
@@ -15,8 +15,7 @@
 #include "lualistener.h"
 // STL headers
 // std headers
-#include <cstdlib>                  // std::rand()
-#include <cstring>                  // std::memcpy()
+#include <iomanip>                  // std::quoted()
 // external library headers
 #include "luau/lua.h"
 // other Linden headers
@@ -28,11 +27,11 @@ const int MAX_QSIZE = 1000;
 
 std::ostream& operator<<(std::ostream& out, const LuaListener& self)
 {
-    return out << "LuaListener(" << self.getReplyName() << ", " << self.getCommandName() << ")";
+    return out << "LuaListener(" << std::quoted(self.mCoroName) << ", "
+               << self.getReplyName() << ", " << self.getCommandName() << ")";
 }
 
 LuaListener::LuaListener(lua_State* L):
-    super(getUniqueKey()),
     mCoroName(LLCoros::getName()),
     mListener(new LLLeapListener(
         "LuaListener",
@@ -49,24 +48,13 @@ LuaListener::LuaListener(lua_State* L):
                 // viewer shutdown, close the queue to wake up getNext().
                 mQueue.close();
             }))
-{}
+{
+    LL_DEBUGS("Lua") << "LuaListener(" << std::quoted(mCoroName) << ")" << LL_ENDL;
+}
 
 LuaListener::~LuaListener()
-{}
-
-int LuaListener::getUniqueKey()
 {
-    // Find a random key that does NOT already correspond to a LuaListener
-    // instance. Passing a duplicate key to LLInstanceTracker would do Bad
-    // Things.
-    int key;
-    do
-    {
-        key = std::rand();
-    } while (LuaListener::getInstance(key));
-    // This is theoretically racy, if we were instantiating new
-    // LuaListeners on multiple threads. Don't.
-    return key;
+    LL_DEBUGS("Lua") << "~LuaListener(" << std::quoted(mCoroName) << ")" << LL_ENDL;
 }
 
 std::string LuaListener::getReplyName() const
@@ -86,7 +74,7 @@ bool LuaListener::queueEvent(const std::string& pump, const LLSD& data)
     // capacity or we'd block the post() call trying to propagate this event!
     if (auto size = mQueue.size(); size > MAX_QSIZE)
     {
-        LL_WARNS("Lua") << "LuaListener queue for " << getReplyName()
+        LL_WARNS("Lua") << "LuaListener queue for " << mCoroName
                         << " exceeds " << MAX_QSIZE << ": " << size
                         << " -- discarding event" << LL_ENDL;
     }
@@ -107,7 +95,7 @@ LuaListener::PumpData LuaListener::getNext()
     catch (const LLThreadSafeQueueInterrupt&)
     {
         // mQueue has been closed. The only way that happens is when we detect
-        // viewer shutdown. Terminate the calling coroutine.
+        // viewer shutdown. Terminate the calling Lua coroutine.
         LLCoros::checkStop();
         return {};
     }
diff --git a/indra/llcommon/lualistener.h b/indra/llcommon/lualistener.h
index 85fb093cd6..68131dfa27 100644
--- a/indra/llcommon/lualistener.h
+++ b/indra/llcommon/lualistener.h
@@ -12,8 +12,7 @@
 #if ! defined(LL_LUALISTENER_H)
 #define LL_LUALISTENER_H
 
-#include "llevents.h"
-#include "llinstancetracker.h"
+#include "llevents.h"               // LLTempBoundListener
 #include "llsd.h"
 #include "llthreadsafequeue.h"
 #include <iosfwd>                   // std::ostream
@@ -27,25 +26,11 @@ class LLLeapListener;
 /**
  * LuaListener is based on LLLeap. It serves an analogous function.
  *
- * Each LuaListener instance has an int key, generated randomly to
- * inconvenience malicious Lua scripts wanting to mess with others. The idea
- * is that a given lua_State stores in its Registry:
- * - "event.listener": the int key of the corresponding LuaListener, if any
- * The original thought was that LuaListener would itself store the Lua
- * function -- but surprisingly, there is no C/C++ type in the API that stores
- * a Lua function.
- *
- * (We considered storing in "event.listener" the LuaListener pointer itself
- * as a light userdata, but the problem would be if Lua code overwrote that.
- * We want to prevent any Lua script from crashing the viewer, intentionally
- * or otherwise. Safer to use a key lookup.)
- *
  * Like LLLeap, each LuaListener instance also has an associated
  * LLLeapListener to respond to LLEventPump management commands.
  */
-class LuaListener: public LLInstanceTracker<LuaListener, int>
+class LuaListener
 {
-    using super = LLInstanceTracker<LuaListener, int>;
 public:
     LuaListener(lua_State* L);
 
@@ -68,7 +53,6 @@ public:
     friend std::ostream& operator<<(std::ostream& out, const LuaListener& self);
 
 private:
-    static int getUniqueKey();
     bool queueEvent(const std::string& pump, const LLSD& data);
 
     LLThreadSafeQueue<PumpData> mQueue;