From f2f0fa7fd0efc221f1358dd4e440b5d51a5fb8b4 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Thu, 18 Jul 2024 13:29:34 -0400
Subject: Ditch `LLEventTrackable` aka `boost::signals2::trackable`.
Remove documented `LLEventPump` support for `LLEventTrackable`. That claimed
support was always a little bit magical/fragile. IF:
* a class included `LLEventTrackable` as a base class AND
* an instance of that class was managed by `boost::shared_ptr` AND
* you passed one of that class's methods and the `boost::shared_ptr`
specifically to `boost::bind()` AND
* the resulting `boost::bind()` object was passed into `LLEventPump::listen()`
THEN the promise was that on destruction of that object, that listener would
automatically be disconnected -- instead of leaving a dangling pointer bound
into the `LLEventPump`, causing a crash on the next `LLEventPump::post()` call.
The only existing code in the viewer code base that exercised `LLEventTrackable`
functionality was in test programs. When the viewer calls `LLEventPump::listen()`,
it typically stores the resulting connection object in an `LLTempBoundListener`
variable, which guarantees disconnection on destruction of that variable.
The fact that `LLEventTrackable` support is specific to `boost::bind()`, that it
silently fails to keep its promise with `std::bind()` or a lambda or any other
form of C++ callable, makes it untrustworthy for new code.
Note that the code base still uses `boost::signals2::trackable` for other
`boost::signals2::signal` instances not associated with `LLEventPump`. We are
not changing those at this time.
---
indra/llcommon/llevents.h | 45 +----------------
indra/llui/llnotifications.h | 2 +-
indra/llui/llnotificationslistener.cpp | 16 +++---
indra/newview/llspeakers.h | 2 +-
indra/test/llevents_tut.cpp | 58 +---------------------
.../viewer_components/login/tests/lllogin_test.cpp | 4 +-
6 files changed, 15 insertions(+), 112 deletions(-)
diff --git a/indra/llcommon/llevents.h b/indra/llcommon/llevents.h
index 8ef3a5af95..abc25ba400 100644
--- a/indra/llcommon/llevents.h
+++ b/indra/llcommon/llevents.h
@@ -363,57 +363,14 @@ testable:
InstanceTypes mTypes;
};
-/*****************************************************************************
-* LLEventTrackable
-*****************************************************************************/
-/**
- * LLEventTrackable wraps boost::signals2::trackable, which resembles
- * boost::trackable. Derive your listener class from LLEventTrackable instead,
- * and use something like
- * <tt>LLEventPump::listen(boost::bind(&YourTrackableSubclass::method,
- * instance, _1))</tt>. This will implicitly disconnect when the object
- * referenced by @c instance is destroyed.
- *
- * @note
- * LLEventTrackable doesn't address a couple of cases:
- * * Object destroyed during call
- * - You enter a slot call in thread A.
- * - Thread B destroys the object, which of course disconnects it from any
- * future slot calls.
- * - Thread A's call uses 'this', which now refers to a defunct object.
- * Undefined behavior results.
- * * Call during destruction
- * - @c MySubclass is derived from LLEventTrackable.
- * - @c MySubclass registers one of its own methods using
- * <tt>LLEventPump::listen()</tt>.
- * - The @c MySubclass object begins destruction. <tt>~MySubclass()</tt>
- * runs, destroying state specific to the subclass. (For instance, a
- * <tt>Foo*</tt> data member is <tt>delete</tt>d but not zeroed.)
- * - The listening method will not be disconnected until
- * <tt>~LLEventTrackable()</tt> runs.
- * - Before we get there, another thread posts data to the @c LLEventPump
- * instance, calling the @c MySubclass method.
- * - The method in question relies on valid @c MySubclass state. (For
- * instance, it attempts to dereference the <tt>Foo*</tt> pointer that was
- * <tt>delete</tt>d but not zeroed.)
- * - Undefined behavior results.
- */
-typedef boost::signals2::trackable LLEventTrackable;
-
/*****************************************************************************
* LLEventPump
*****************************************************************************/
/**
* LLEventPump is the base class interface through which we access the
* concrete subclasses such as LLEventStream.
- *
- * @NOTE
- * LLEventPump derives from LLEventTrackable so that when you "chain"
- * LLEventPump instances together, they will automatically disconnect on
- * destruction. Please see LLEventTrackable documentation for situations in
- * which this may be perilous across threads.
*/
-class LL_COMMON_API LLEventPump: public LLEventTrackable
+class LL_COMMON_API LLEventPump
{
public:
static const std::string ANONYMOUS; // constant for anonymous listeners.
diff --git a/indra/llui/llnotifications.h b/indra/llui/llnotifications.h
index ab4f009a80..206c521592 100644
--- a/indra/llui/llnotifications.h
+++ b/indra/llui/llnotifications.h
@@ -734,7 +734,7 @@ typedef std::multimap<std::string, LLNotificationPtr> LLNotificationMap;
// all of the built-in tests should attach to the "Visible" channel
//
class LLNotificationChannelBase :
- public LLEventTrackable,
+ public boost::signals2::trackable,
public LLRefCount
{
LOG_CLASS(LLNotificationChannelBase);
diff --git a/indra/llui/llnotificationslistener.cpp b/indra/llui/llnotificationslistener.cpp
index ace9e37e25..2ad1689a45 100644
--- a/indra/llui/llnotificationslistener.cpp
+++ b/indra/llui/llnotificationslistener.cpp
@@ -204,7 +204,7 @@ void LLNotificationsListener::ignore(const LLSD& params) const
}
}
-class LLNotificationsListener::Forwarder: public LLEventTrackable
+class LLNotificationsListener::Forwarder: public boost::signals2::trackable
{
LOG_CLASS(LLNotificationsListener::Forwarder);
public:
@@ -213,8 +213,10 @@ public:
mRespond(false)
{
// Connect to the specified channel on construction. Because
- // LLEventTrackable is a base, we should automatically disconnect when
- // destroyed.
+ // boost::signals2::trackable is a base, because we use boost::bind()
+ // below, and because connectPassedFilter() directly calls
+ // boost::signals2::signal::connect(), we should automatically
+ // disconnect when destroyed.
LLNotificationChannelPtr channelptr(llnotifications.getChannel(channel));
if (channelptr)
{
@@ -252,10 +254,10 @@ void LLNotificationsListener::forward(const LLSD& params)
if (! forward)
{
// This is a request to stop forwarding notifications on the specified
- // channel. The rest of the params don't matter.
- // Because mForwarders contains scoped_ptrs, erasing the map entry
- // DOES delete the heap Forwarder object. Because Forwarder derives
- // from LLEventTrackable, destroying it disconnects it from the
+ // channel. The rest of the params don't matter. Because mForwarders
+ // contains scoped_ptrs, erasing the map entry DOES delete the heap
+ // Forwarder object. Because Forwarder derives from
+ // boost::signals2::trackable, destroying it disconnects it from the
// channel.
mForwarders.erase(channel);
return;
diff --git a/indra/newview/llspeakers.h b/indra/newview/llspeakers.h
index 234de42953..d3304dba59 100644
--- a/indra/newview/llspeakers.h
+++ b/indra/newview/llspeakers.h
@@ -37,7 +37,7 @@ class LLSpeakerMgr;
class LLAvatarName;
// data for a given participant in a voice channel
-class LLSpeaker : public LLRefCount, public LLOldEvents::LLObservable, public LLHandleProvider<LLSpeaker>, public boost::signals2::trackable
+class LLSpeaker : public LLRefCount, public LLOldEvents::LLObservable, public LLHandleProvider<LLSpeaker>
{
public:
typedef enum e_speaker_type
diff --git a/indra/test/llevents_tut.cpp b/indra/test/llevents_tut.cpp
index 875ca9ad89..f9cc99203b 100644
--- a/indra/test/llevents_tut.cpp
+++ b/indra/test/llevents_tut.cpp
@@ -429,7 +429,7 @@ void events_object::test<9>()
{
set_test_name("listen(boost::bind(...TempListener...))");
// listen() can't do anything about a plain TempListener instance:
- // it's not managed with shared_ptr, nor is it an LLEventTrackable subclass
+ // it's not managed with shared_ptr
bool live = false;
LLEventPump& heaptest(pumps.obtain("heaptest"));
LLBoundListener connection;
@@ -453,60 +453,4 @@ void events_object::test<9>()
heaptest.stopListening("temp");
}
-class TempTrackableListener: public TempListener, public LLEventTrackable
-{
-public:
- TempTrackableListener(const std::string& name, bool& liveFlag):
- TempListener(name, liveFlag)
- {}
-};
-
-template<> template<>
-void events_object::test<10>()
-{
- set_test_name("listen(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(),
- 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);
-}
-
-template<> template<>
-void events_object::test<11>()
-{
- set_test_name("listen(boost::bind(...TempTrackableListener pointer...))");
- bool live = false;
- LLEventPump& heaptest(pumps.obtain("heaptest"));
- LLBoundListener connection;
- {
- TempTrackableListener* newListener(new TempTrackableListener("temp", live));
- ensure("TempTrackableListener constructed", live);
- connection = heaptest.listen(newListener->getName(),
- boost::bind(&TempTrackableListener::call,
- newListener, _1));
- heaptest.post(1);
- check_listener("received", *newListener, 1);
- // explicitly destroy newListener
- delete newListener;
- }
- // 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);
-}
-
} // namespace tut
diff --git a/indra/viewer_components/login/tests/lllogin_test.cpp b/indra/viewer_components/login/tests/lllogin_test.cpp
index 8aea3b37aa..f051f8c67f 100644
--- a/indra/viewer_components/login/tests/lllogin_test.cpp
+++ b/indra/viewer_components/login/tests/lllogin_test.cpp
@@ -66,7 +66,7 @@
* Helper classes
*****************************************************************************/
// This is a listener to receive results from lllogin.
-class LoginListener: public LLEventTrackable
+class LoginListener
{
std::string mName;
LLSD mLastEvent;
@@ -137,7 +137,7 @@ public:
}
};
-class LLXMLRPCListener: public LLEventTrackable
+class LLXMLRPCListener
{
std::string mName;
LLSD mEvent;
--
cgit v1.2.3