DEV-43463: Keep LLEventPump's LLStandardSignal alive during post()

Replace LLEventPump's boost::scoped_ptr<LLStandardSignal> with
boost::shared_ptr. Take a local stack copy of that shared_ptr in post()
methods, and invoke the signal through that copy. This guards against scenario
in which LLEventPump gets destroyed during signal invocation. (See Jira for
details.) Re-enable Mani's test case that used to crash.
Introduce ll_template_cast<> to allow a template function to recognize a
parameter of a particular type.
Introduce LLListenerWrapper mechanism to support wrapper objects for
LLEventPump listeners. You instantiate an LLListenerWrapper subclass object
inline in the listen() call (typically with llwrap<>), passing it the real
listener, trusting it to forward the eventual call.
Introduce prototypical LLCoutListener and LLLogListener subclasses for
illustrative and diagnostic purposes. Test that LLLogListener doesn't block
recognizing LLEventTrackable base class bound into wrapped listener.

7 files changed, 477 insertions, 16 deletions

diff --git a/indra/llcommon/CMakeLists.txt b/indra/llcommon/CMakeLists.txt
index e41c75846b..a08f9b9ab4 100644
--- a/indra/llcommon/CMakeLists.txt
+++ b/indra/llcommon/CMakeLists.txt
@@ -163,6 +163,7 @@ set(llcommon_HEADER_FILES
     llinstancetracker.h
     llkeythrottle.h
     lllazy.h
+    lllistenerwrapper.h
     lllinkedqueue.h
     llliveappconfig.h
     lllivefile.h
@@ -220,6 +221,7 @@ set(llcommon_HEADER_FILES
     llversionserver.h
     llversionviewer.h
     llworkerthread.h
+    ll_template_cast.h
     metaclass.h
     metaclasst.h
     metaproperty.h
diff --git a/indra/llcommon/ll_template_cast.h b/indra/llcommon/ll_template_cast.h
new file mode 100644
index 0000000000..cff58ce00d
--- /dev/null
+++ b/indra/llcommon/ll_template_cast.h
@@ -0,0 +1,160 @@
+/**
+ * @file   ll_template_cast.h
+ * @author Nat Goodspeed
+ * @date   2009-11-21
+ * @brief  Define ll_template_cast function
+ * 
+ * $LicenseInfo:firstyear=2009&license=viewergpl$
+ * Copyright (c) 2009, Linden Research, Inc.
+ * $/LicenseInfo$
+ */
+
+#if ! defined(LL_LL_TEMPLATE_CAST_H)
+#define LL_LL_TEMPLATE_CAST_H
+
+/**
+ * Implementation for ll_template_cast() (q.v.).
+ *
+ * Default implementation: trying to cast two completely unrelated types
+ * returns 0. Typically you'd specify T and U as pointer types, but in fact T
+ * can be any type that can be initialized with 0.
+ */
+template <typename T, typename U>
+struct ll_template_cast_impl
+{
+    T operator()(U)
+    {
+        return 0;
+    }
+};
+
+/**
+ * ll_template_cast<T>(some_value) is for use in a template function when
+ * some_value might be of arbitrary type, but you want to recognize type T
+ * specially.
+ *
+ * It's designed for use with pointer types. Example:
+ * @code
+ * struct SpecialClass
+ * {
+ *     void someMethod(const std::string&) const;
+ * };
+ *
+ * template <class REALCLASS>
+ * void somefunc(const REALCLASS& instance)
+ * {
+ *     const SpecialClass* ptr = ll_template_cast<const SpecialClass*>(&instance);
+ *     if (ptr)
+ *     {
+ *         ptr->someMethod("Call method only available on SpecialClass");
+ *     }
+ * }
+ * @endcode
+ *
+ * Why is this better than dynamic_cast<>? Because unless OtherClass is
+ * polymorphic, the following won't even compile (gcc 4.0.1):
+ * @code
+ * OtherClass other;
+ * SpecialClass* ptr = dynamic_cast<SpecialClass*>(&other);
+ * @endcode
+ * to say nothing of this:
+ * @code
+ * void function(int);
+ * SpecialClass* ptr = dynamic_cast<SpecialClass*>(&function);
+ * @endcode
+ * ll_template_cast handles these kinds of cases by returning 0.
+ */
+template <typename T, typename U>
+T ll_template_cast(U value)
+{
+    return ll_template_cast_impl<T, U>()(value);
+}
+
+/**
+ * Implementation for ll_template_cast() (q.v.).
+ *
+ * Implementation for identical types: return same value.
+ */
+template <typename T>
+struct ll_template_cast_impl<T, T>
+{
+    T operator()(T value)
+    {
+        return value;
+    }
+};
+
+/**
+ * LL_TEMPLATE_CONVERTIBLE(dest, source) asserts that, for a value @c s of
+ * type @c source, <tt>ll_template_cast<dest>(s)</tt> will return @c s --
+ * presuming that @c source can be converted to @c dest by the normal rules of
+ * C++.
+ *
+ * By default, <tt>ll_template_cast<dest>(s)</tt> will return 0 unless @c s's
+ * type is literally identical to @c dest. (This is because of the
+ * straightforward application of template specialization rules.) That can
+ * lead to surprising results, e.g.:
+ *
+ * @code
+ * Foo myFoo;
+ * const Foo* fooptr = ll_template_cast<const Foo*>(&myFoo);
+ * @endcode
+ *
+ * Here @c fooptr will be 0 because <tt>&myFoo</tt> is of type <tt>Foo*</tt>
+ * -- @em not <tt>const Foo*</tt>. (Declaring <tt>const Foo myFoo;</tt> would
+ * force the compiler to do the right thing.)
+ *
+ * More disappointingly:
+ * @code
+ * struct Base {};
+ * struct Subclass: public Base {};
+ * Subclass object;
+ * Base* ptr = ll_template_cast<Base*>(&object);
+ * @endcode
+ *
+ * Here @c ptr will be 0 because <tt>&object</tt> is of type
+ * <tt>Subclass*</tt> rather than <tt>Base*</tt>. We @em want this cast to
+ * succeed, but without our help ll_template_cast can't recognize it.
+ *
+ * The following would suffice:
+ * @code
+ * LL_TEMPLATE_CONVERTIBLE(Base*, Subclass*);
+ * ...
+ * Base* ptr = ll_template_cast<Base*>(&object);
+ * @endcode
+ *
+ * However, as noted earlier, this is easily fooled:
+ * @code
+ * const Base* ptr = ll_template_cast<const Base*>(&object);
+ * @endcode
+ * would still produce 0 because we haven't yet seen:
+ * @code
+ * LL_TEMPLATE_CONVERTIBLE(const Base*, Subclass*);
+ * @endcode
+ *
+ * @TODO
+ * This macro should use Boost type_traits facilities for stripping and
+ * re-adding @c const and @c volatile qualifiers so that invoking
+ * LL_TEMPLATE_CONVERTIBLE(dest, source) will automatically generate all
+ * permitted permutations. It's really not fair to the coder to require
+ * separate:
+ * @code
+ * LL_TEMPLATE_CONVERTIBLE(Base*, Subclass*);
+ * LL_TEMPLATE_CONVERTIBLE(const Base*, Subclass*);
+ * LL_TEMPLATE_CONVERTIBLE(const Base*, const Subclass*);
+ * @endcode
+ *
+ * (Naturally we omit <tt>LL_TEMPLATE_CONVERTIBLE(Base*, const Subclass*)</tt>
+ * because that's not permitted by normal C++ assignment anyway.)
+ */
+#define LL_TEMPLATE_CONVERTIBLE(DEST, SOURCE)   \
+template <>                                     \
+struct ll_template_cast_impl<DEST, SOURCE>      \
+{                                               \
+    DEST operator()(SOURCE wrapper)             \
+    {                                           \
+        return wrapper;                         \
+    }                                           \
+}
+
+#endif /* ! defined(LL_LL_TEMPLATE_CAST_H) */
diff --git a/indra/llcommon/llevents.cpp b/indra/llcommon/llevents.cpp
index 4bdfe5a867..31fdd9e60a 100644
--- a/indra/llcommon/llevents.cpp
+++ b/indra/llcommon/llevents.cpp
@@ -459,11 +459,25 @@ void LLEventPump::stopListening(const std::string& name)
 bool LLEventStream::post(const LLSD& event)
 {
     if (! mEnabled)
+    {
         return false;
+    }
+    // NOTE NOTE NOTE: Any new access to member data beyond this point should
+    // cause us to move our LLStandardSignal object to a pimpl class along
+    // with said member data. Then the local shared_ptr will preserve both.
+
+    // DEV-43463: capture a local copy of mSignal. We've turned up a
+    // cross-coroutine scenario (described in the Jira) in which this post()
+    // call could end up destroying 'this', the LLEventPump subclass instance
+    // containing mSignal, during the call through *mSignal. So -- capture a
+    // *stack* instance of the shared_ptr, ensuring that our heap
+    // LLStandardSignal object will live at least until post() returns, even
+    // if 'this' gets destroyed during the call.
+    boost::shared_ptr<LLStandardSignal> signal(mSignal);
     // Let caller know if any one listener handled the event. This is mostly
     // useful when using LLEventStream as a listener for an upstream
     // LLEventPump.
-    return (*mSignal)(event);
+    return (*signal)(event);
 }
 
 /*****************************************************************************
@@ -492,9 +506,16 @@ void LLEventQueue::flush()
     // be processed in the *next* flush() call.
     EventQueue queue(mEventQueue);
     mEventQueue.clear();
+    // NOTE NOTE NOTE: Any new access to member data beyond this point should
+    // cause us to move our LLStandardSignal object to a pimpl class along
+    // with said member data. Then the local shared_ptr will preserve both.
+
+    // DEV-43463: capture a local copy of mSignal. See LLEventStream::post()
+    // for detailed comments.
+    boost::shared_ptr<LLStandardSignal> signal(mSignal);
     for ( ; ! queue.empty(); queue.pop_front())
     {
-        (*mSignal)(queue.front());
+        (*signal)(queue.front());
     }
 }
 
diff --git a/indra/llcommon/llevents.h b/indra/llcommon/llevents.h
index f52cf33fd8..5646407f6a 100644
--- a/indra/llcommon/llevents.h
+++ b/indra/llcommon/llevents.h
@@ -44,6 +44,7 @@
 #include "llsd.h"
 #include "llsingleton.h"
 #include "lldependencies.h"
+#include "ll_template_cast.h"
 
 /*==========================================================================*|
 // override this to allow binding free functions with more parameters
@@ -266,6 +267,14 @@ namespace LLEventDetail
      */
     template <typename LISTENER>
     LLBoundListener visit_and_connect(const LISTENER& listener,
+                                      const ConnectFunc& connect_func)
+    {
+        return visit_and_connect("", listener, connect_func);
+    }
+    /// overload of visit_and_connect() when we have a string identifier available
+    template <typename LISTENER>
+    LLBoundListener visit_and_connect(const std::string& name,
+                                      const LISTENER& listener,
                                       const ConnectFunc& connect_func);
 } // namespace LLEventDetail
 
@@ -468,7 +477,8 @@ public:
         // This is why listen() is a template. Conversion from boost::bind()
         // to LLEventListener performs type erasure, so it's important to look
         // at the boost::bind object itself before that happens.
-        return LLEventDetail::visit_and_connect(listener,
+        return LLEventDetail::visit_and_connect(name,
+                                                listener,
                                                 boost::bind(&LLEventPump::listen_impl,
                                                             this,
                                                             name,
@@ -522,7 +532,7 @@ private:
 
 protected:
     /// implement the dispatching
-    boost::scoped_ptr<LLStandardSignal> mSignal;
+    boost::shared_ptr<LLStandardSignal> mSignal;
 
     /// valve open?
     bool mEnabled;
@@ -664,6 +674,60 @@ private:
     LLSD mReqid;
 };
 
+/**
+ * Base class for LLListenerWrapper. See visit_and_connect() and llwrap(). We
+ * provide virtual @c accept_xxx() methods, customization points allowing a
+ * subclass access to certain data visible at LLEventPump::listen() time.
+ * Example subclass usage:
+ *
+ * @code
+ * myEventPump.listen("somename",
+ *                    llwrap<MyListenerWrapper>(boost::bind(&MyClass::method, instance, _1)));
+ * @endcode
+ *
+ * Because of the anticipated usage (note the anonymous temporary
+ * MyListenerWrapper instance in the example above), the @c accept_xxx()
+ * methods must be @c const.
+ */
+class LL_COMMON_API LLListenerWrapperBase
+{
+public:
+    /// New instance. The accept_xxx() machinery makes it important to use
+    /// shared_ptrs for our data. Many copies of this object are made before
+    /// the instance that actually ends up in the signal, yet accept_xxx()
+    /// will later be called on the @em original instance. All copies of the
+    /// same original instance must share the same data.
+    LLListenerWrapperBase():
+        mName(new std::string),
+        mConnection(new LLBoundListener)
+    {
+    }
+    /// Copy constructor. Copy shared_ptrs to original instance data.
+    LLListenerWrapperBase(const LLListenerWrapperBase& that):
+        mName(that.mName),
+        mConnection(that.mConnection)
+    {
+    }
+
+    /// Ask LLEventPump::listen() for the listener name
+    virtual void accept_name(const std::string& name) const
+    {
+        *mName = name;
+    }
+
+    /// Ask LLEventPump::listen() for the new connection
+    virtual void accept_connection(const LLBoundListener& connection) const
+    {
+        *mConnection = connection;
+    }
+
+protected:
+    /// Listener name.
+    boost::shared_ptr<std::string> mName;
+    /// Connection.
+    boost::shared_ptr<LLBoundListener> mConnection;
+};
+
 /*****************************************************************************
 *   Underpinnings
 *****************************************************************************/
@@ -898,7 +962,8 @@ namespace LLEventDetail
      * LLStandardSignal, returning LLBoundListener.
      */
     template <typename LISTENER>
-    LLBoundListener visit_and_connect(const LISTENER& raw_listener,
+    LLBoundListener visit_and_connect(const std::string& name,
+                                      const LISTENER& raw_listener,
                                       const ConnectFunc& connect_func)
     {
         // Capture the listener
@@ -913,14 +978,20 @@ namespace LLEventDetail
         // which type details have been erased. unwrap() comes from
         // Boost.Signals, in case we were passed a boost::ref().
         visit_each(visitor, LLEventDetail::unwrap(raw_listener));
-        // Make the connection using passed function. At present, wrapping
-        // this functionality into this function is a bit silly: we don't
-        // really need a visit_and_connect() function any more, just a visit()
-        // function. The definition of this function dates from when, after
-        // visit_each(), after establishing the connection, we had to
-        // postprocess the new connection with the visitor object. That's no
-        // longer necessary.
-        return connect_func(listener);
+        // Make the connection using passed function.
+        LLBoundListener connection(connect_func(listener));
+        // If the LISTENER is an LLListenerWrapperBase subclass, pass it the
+        // desired information. It's important that we pass the raw_listener
+        // so the compiler can make decisions based on its original type.
+        const LLListenerWrapperBase* lwb =
+            ll_template_cast<const LLListenerWrapperBase*>(&raw_listener);
+        if (lwb)
+        {
+            lwb->accept_name(name);
+            lwb->accept_connection(connection);
+        }
+        // In any case, show new connection to caller.
+        return connection;
     }
 } // namespace LLEventDetail
 
diff --git a/indra/llcommon/lllistenerwrapper.h b/indra/llcommon/lllistenerwrapper.h
new file mode 100644
index 0000000000..e7bad1423a
--- /dev/null
+++ b/indra/llcommon/lllistenerwrapper.h
@@ -0,0 +1,181 @@
+/**
+ * @file   lllistenerwrapper.h
+ * @author Nat Goodspeed
+ * @date   2009-11-30
+ * @brief  Introduce LLListenerWrapper template
+ * 
+ * $LicenseInfo:firstyear=2009&license=viewergpl$
+ * Copyright (c) 2009, Linden Research, Inc.
+ * $/LicenseInfo$
+ */
+
+#if ! defined(LL_LLLISTENERWRAPPER_H)
+#define LL_LLLISTENERWRAPPER_H
+
+#include "llevents.h"               // LLListenerWrapperBase
+#include <boost/visit_each.hpp>
+
+/**
+ * Template base class for coding wrappers for LLEventPump listeners.
+ *
+ * Derive your listener wrapper from LLListenerWrapper. You must use
+ * LLLISTENER_WRAPPER_SUBCLASS() so your subclass will play nicely with
+ * boost::visit_each (q.v.). That way boost::signals2 can still detect
+ * derivation from LLEventTrackable, and so forth.
+ */
+template <typename LISTENER>
+class LL_COMMON_API LLListenerWrapper: public LLListenerWrapperBase
+{
+public:
+    /// Wrap an arbitrary listener object
+    LLListenerWrapper(const LISTENER& listener):
+        mListener(listener)
+    {}
+
+    /// call
+    virtual bool operator()(const LLSD& event)
+    {
+        return mListener(event);
+    }
+
+    /// Allow boost::visit_each() to peek at our mListener.
+    template <class V>
+    void accept_visitor(V& visitor) const
+    {
+        using boost::visit_each;
+        visit_each(visitor, mListener, 0);
+    }
+
+private:
+    LISTENER mListener;
+};
+
+/**
+ * Specialize boost::visit_each() (leveraging ADL) to peek inside an
+ * LLListenerWrapper<T> to traverse its LISTENER. We borrow the
+ * accept_visitor() pattern from boost::bind(), avoiding the need to make
+ * mListener public.
+ */
+template <class V, typename T>
+void visit_each(V& visitor, const LLListenerWrapper<T>& wrapper, int)
+{
+    wrapper.accept_visitor(visitor);
+}
+
+/// use this (sigh!) for each subclass of LLListenerWrapper<T> you write
+#define LLLISTENER_WRAPPER_SUBCLASS(CLASS)                              \
+template <class V, typename T>                                          \
+void visit_each(V& visitor, const CLASS<T>& wrapper, int)               \
+{                                                                       \
+    visit_each(visitor, static_cast<const LLListenerWrapper<T>&>(wrapper), 0); \
+}                                                                       \
+                                                                        \
+/* Have to state this explicitly, rather than using LL_TEMPLATE_CONVERTIBLE, */ \
+/* because the source type is itself a template. */                     \
+template <typename T>                                                   \
+struct ll_template_cast_impl<const LLListenerWrapperBase*, const CLASS<T>*> \
+{                                                                       \
+    const LLListenerWrapperBase* operator()(const CLASS<T>* wrapper)    \
+    {                                                                   \
+        return wrapper;                                                 \
+    }                                                                   \
+}
+
+/**
+ * Make an instance of a listener wrapper. Every wrapper class must be a
+ * template accepting a listener object of arbitrary type. In particular, the
+ * type of a boost::bind() expression is deliberately undocumented. So we
+ * can't just write Wrapper<CorrectType>(boost::bind(...)). Instead we must
+ * write llwrap<Wrapper>(boost::bind(...)).
+ */
+template <template<typename> class WRAPPER, typename T>
+WRAPPER<T> LL_COMMON_API llwrap(const T& listener)
+{
+    return WRAPPER<T>(listener);
+}
+
+/**
+ * This LLListenerWrapper template subclass is used to report entry/exit to an
+ * event listener, by changing this:
+ * @code
+ * someEventPump.listen("MyClass",
+ *                      boost::bind(&MyClass::method, ptr, _1));
+ * @endcode
+ * to this:
+ * @code
+ * someEventPump.listen("MyClass",
+ *                      llwrap<LLCoutListener>(
+ *                      boost::bind(&MyClass::method, ptr, _1)));
+ * @endcode
+ */
+template <class LISTENER>
+class LL_COMMON_API LLCoutListener: public LLListenerWrapper<LISTENER>
+{
+    typedef LLListenerWrapper<LISTENER> super;
+
+public:
+    /// Wrap an arbitrary listener object
+    LLCoutListener(const LISTENER& listener):
+        super(listener)
+    {}
+
+    /// call
+    virtual bool operator()(const LLSD& event)
+    {
+        std::cout << "Entering listener " << *super::mName << " with " << event << std::endl;
+        bool handled = super::operator()(event);
+        std::cout << "Leaving  listener " << *super::mName;
+        if (handled)
+        {
+            std::cout << " (handled)";
+        }
+        std::cout << std::endl;
+        return handled;
+    }
+};
+
+LLLISTENER_WRAPPER_SUBCLASS(LLCoutListener);
+
+/**
+ * This LLListenerWrapper template subclass is used to log entry/exit to an
+ * event listener, by changing this:
+ * @code
+ * someEventPump.listen("MyClass",
+ *                      boost::bind(&MyClass::method, ptr, _1));
+ * @endcode
+ * to this:
+ * @code
+ * someEventPump.listen("MyClass",
+ *                      llwrap<LLLogListener>(
+ *                      boost::bind(&MyClass::method, ptr, _1)));
+ * @endcode
+ */
+template <class LISTENER>
+class LL_COMMON_API LLLogListener: public LLListenerWrapper<LISTENER>
+{
+    typedef LLListenerWrapper<LISTENER> super;
+
+public:
+    /// Wrap an arbitrary listener object
+    LLLogListener(const LISTENER& listener):
+        super(listener)
+    {}
+
+    /// call
+    virtual bool operator()(const LLSD& event)
+    {
+        LL_DEBUGS("LLLogListener") << "Entering listener " << *super::mName << " with " << event << LL_ENDL;
+        bool handled = super::operator()(event);
+        LL_DEBUGS("LLLogListener") << "Leaving  listener " << *super::mName;
+        if (handled)
+        {
+            LL_CONT << " (handled)";
+        }
+        LL_CONT << LL_ENDL;
+        return handled;
+    }
+};
+
+LLLISTENER_WRAPPER_SUBCLASS(LLLogListener);
+
+#endif /* ! defined(LL_LLLISTENERWRAPPER_H) */
diff --git a/indra/test/llevents_tut.cpp b/indra/test/llevents_tut.cpp
index 31130c3c79..e58b10ce07 100644
--- a/indra/test/llevents_tut.cpp
+++ b/indra/test/llevents_tut.cpp
@@ -21,6 +21,7 @@
 #define testable public
 #include "llevents.h"
 #undef testable
+#include "lllistenerwrapper.h"
 // STL headers
 // std headers
 #include <iostream>
@@ -639,6 +640,33 @@ namespace tut
         heaptest.post(2);
     }
 
+    template<> template<>
+    void events_object::test<15>()
+    {
+        // This test ensures that using an LLListenerWrapper subclass doesn't
+        // block Boost.Signals2 from recognizing a bound LLEventTrackable
+        // subclass.
+        set_test_name("listen(llwrap<LLLogListener>(boost::bind(...TempTrackableListener ref...)))");
+        bool live = false;
+        LLEventPump& heaptest(pumps.obtain("heaptest"));
+        LLBoundListener connection;
+        {
+            TempTrackableListener tempListener("temp", live);
+            ensure("TempTrackableListener constructed", live);
+            connection = heaptest.listen(tempListener.getName(),
+                                         llwrap<LLLogListener>(
+                                         boost::bind(&TempTrackableListener::call,
+                                                     boost::ref(tempListener), _1)));
+            heaptest.post(1);
+            check_listener("received", tempListener, 1);
+        } // presumably this will make tempListener go away?
+        // verify that
+        ensure("TempTrackableListener destroyed", ! live);
+        ensure("implicit disconnect", ! connection.connected());
+        // now just make sure we don't blow up trying to access a freed object!
+        heaptest.post(2);
+    }
+
     class TempSharedListener: public TempListener,
                               public boost::enable_shared_from_this<TempSharedListener>
     {
@@ -649,7 +677,7 @@ namespace tut
     };
 
     template<> template<>
-    void events_object::test<15>()
+    void events_object::test<16>()
     {
         set_test_name("listen(boost::bind(...TempSharedListener ref...))");
 #if 0
diff --git a/indra/viewer_components/login/tests/lllogin_test.cpp b/indra/viewer_components/login/tests/lllogin_test.cpp
index 99ea796ad0..56c21016bd 100644
--- a/indra/viewer_components/login/tests/lllogin_test.cpp
+++ b/indra/viewer_components/login/tests/lllogin_test.cpp
@@ -416,7 +416,6 @@ namespace tut
 		ensure_equals("Failed to offline", listener.lastEvent()["state"].asString(), "offline");
 	}
 
-/*
     template<> template<>
     void llviewerlogin_object::test<5>()
     {
@@ -452,5 +451,4 @@ namespace tut
 
 		ensure_equals("SRV Failure", listener.lastEvent()["change"].asString(), "fail.login"); 
 	}
-*/
 }