DRTVWR-494: Defend LLInstanceTracker against multi-thread usage.

The previous implementation went to some effort to crash if anyone attempted
to create or destroy an LLInstanceTracker subclass instance during traversal.
That restriction is manageable within a single thread, but becomes unworkable
if it's possible that a given subclass might be used on more than one thread.

Remove LLInstanceTracker::instance_iter, beginInstances(), endInstances(),
also key_iter, beginKeys() and endKeys(). Instead, introduce key_snapshot()
and instance_snapshot(), the only means of iterating over LLInstanceTracker
instances. (These are intended to resemble functions, but in fact the current
implementation simply presents the classes.) Iterating over a captured
snapshot defends against container modifications during traversal. The term
'snapshot' reminds the coder that a new instance created during traversal will
not be considered. To defend against instance deletion during traversal, a
snapshot stores std::weak_ptrs which it lazily dereferences, skipping on the
fly any that have expired.

Dereferencing instance_snapshot::iterator gets you a reference rather than a
pointer. Because some use cases want to delete all existing instances, add an
instance_snapshot::deleteAll() method that extracts the pointer. Those cases
used to require explicitly copying instance pointers into a separate
container; instance_snapshot() now takes care of that. It remains the caller's
responsibility to ensure that all instances of that LLInstanceTracker subclass
were allocated on the heap.

Replace unkeyed static LLInstanceTracker::getInstance(T*) -- which returned
nullptr if that instance had been destroyed -- with new getWeak() method
returning std::weak_ptr<T>. Caller must detect expiration of that weak_ptr.

Adjust tests accordingly.

Use of std::weak_ptr to detect expired instances requires engaging
std::shared_ptr in the constructor. We now store shared_ptrs in the static
containers (std::map for keyed, std::set for unkeyed).

Make LLInstanceTrackerBase a template parameterized on the type of the static
data it manages. For that reason, hoist static data class declarations out of
the class definitions to an LLInstanceTrackerStuff namespace.

Remove the static atomic sIterationNestDepth and its methods incrementDepth(),
decrementDepth() and getDepth(), since they were used only to forbid creation
and destruction during traversal.

Add a std::mutex to static data. Introduce an internal LockStatic class that
locks the mutex while providing a pointer to static data, making that the only
way to access the static data.

The LLINSTANCETRACKER_DTOR_NOEXCEPT macro goes away because we no longer
expect ~LLInstanceTracker() to throw an exception in test programs.
That affects LLTrace::StatBase as well as LLInstanceTracker itself.

Adapt consumers to the new LLInstanceTracker API.

1 files changed, 404 insertions, 302 deletions

diff --git a/indra/llcommon/llinstancetracker.h b/indra/llcommon/llinstancetracker.h
index 363d0bcbd5..76b201ad8c 100644
--- a/indra/llcommon/llinstancetracker.h
+++ b/indra/llcommon/llinstancetracker.h
@@ -28,354 +28,456 @@
 #ifndef LL_LLINSTANCETRACKER_H
 #define LL_LLINSTANCETRACKER_H
 
-#include <atomic>
 #include <map>
+#include <set>
+#include <vector>
 #include <typeinfo>
+#include <mutex>
+#include <memory>
+#include <type_traits>
 
-#include "llstringtable.h"
 #include <boost/iterator/transform_iterator.hpp>
 #include <boost/iterator/indirect_iterator.hpp>
+#include <boost/iterator/filter_iterator.hpp>
 
-// As of 2017-05-06, as far as nat knows, only clang supports __has_feature().
-// Unfortunately VS2013's preprocessor shortcut logic doesn't prevent it from
-// producing (fatal) warnings for defined(__clang__) && __has_feature(...).
-// Have to work around that.
-#if ! defined(__clang__)
-#define __has_feature(x) 0
-#endif // __clang__
-
-#if defined(LL_TEST_llinstancetracker) && __has_feature(cxx_noexcept)
-// ~LLInstanceTracker() performs llassert_always() validation. That's fine in
-// production code, since the llassert_always() is implemented as an LL_ERRS
-// message, which will crash-with-message. In our integration test executable,
-// though, this llassert_always() throws an exception instead so we can test
-// error conditions and continue running the test. However -- as of C++11,
-// destructors are implicitly noexcept(true). Unless we mark
-// ~LLInstanceTracker() noexcept(false), the test executable crashes even on
-// the ATTEMPT to throw.
-#define LLINSTANCETRACKER_DTOR_NOEXCEPT noexcept(false)
-#else
-// If we're building for production, or in fact building *any other* test, or
-// we're using a compiler that doesn't support __has_feature(), or we're not
-// compiling with a C++ version that supports noexcept -- don't specify it.
-#define LLINSTANCETRACKER_DTOR_NOEXCEPT
-#endif
-
+/*****************************************************************************
+*   LLInstanceTrackerBase
+*****************************************************************************/
 /**
  * Base class manages "class-static" data that must actually have singleton
  * semantics: one instance per process, rather than one instance per module as
  * sometimes happens with data simply declared static.
  */
+namespace LLInstanceTrackerStuff
+{
+    struct StaticBase
+    {
+        // We need to be able to lock static data while manipulating it.
+        typedef std::mutex mutex_t;
+        mutex_t mMutex;
+    };
+} // namespace LLInstanceTrackerStuff
+
+template <class Static>
 class LL_COMMON_API LLInstanceTrackerBase
 {
 protected:
-    /// It's not essential to derive your STATICDATA (for use with
-    /// getStatic()) from StaticBase; it's just that both known
-    /// implementations do.
-    struct StaticBase
+    typedef Static StaticData;
+
+    // Instantiate this class to obtain a pointer to the canonical static
+    // instance of class Static while holding a lock on that instance. Use of
+    // Static::mMutex presumes either that Static is derived from StaticBase,
+    // or that Static declares some other suitable mMutex.
+    class LockStatic
     {
-        StaticBase():
-            sIterationNestDepth(0)
+        typedef std::unique_lock<decltype(Static::mMutex)> lock_t;
+    public:
+        LockStatic():
+            mData(getStatic()),
+            mLock(mData->mMutex)
         {}
-
-		void incrementDepth();
-		void decrementDepth();
-		U32 getDepth();
-	private:
-#ifdef LL_WINDOWS
-		std::atomic_uint32_t sIterationNestDepth;
-#else
-		std::atomic_uint sIterationNestDepth;
-#endif
-	};
+        Static* get() const { return mData; }
+        operator Static*() const { return get(); }
+        Static* operator->() const { return get(); }
+        // sometimes we must explicitly unlock...
+        void unlock()
+        {
+            // but once we do, access is no longer permitted
+            mData = nullptr;
+            mLock.unlock();
+        }
+    protected:
+        Static* mData;
+        lock_t mLock;
+    private:
+        Static* getStatic()
+        {
+            static Static sData;
+            return &sData;
+        }
+    };
 };
 
-LL_COMMON_API void assert_main_thread();
-
+/*****************************************************************************
+*   LLInstanceTracker with key
+*****************************************************************************/
 enum EInstanceTrackerAllowKeyCollisions
 {
-	LLInstanceTrackerErrorOnCollision,
-	LLInstanceTrackerReplaceOnCollision
+    LLInstanceTrackerErrorOnCollision,
+    LLInstanceTrackerReplaceOnCollision
 };
 
+namespace LLInstanceTrackerStuff
+{
+    template <typename KEY, typename VALUE>
+    struct StaticMap: public StaticBase
+    {
+        typedef std::map<KEY, VALUE> InstanceMap;
+        InstanceMap mMap;
+    };
+} // LLInstanceTrackerStuff
+
 /// This mix-in class adds support for tracking all instances of the specified class parameter T
 /// The (optional) key associates a value of type KEY with a given instance of T, for quick lookup
 /// If KEY is not provided, then instances are stored in a simple set
 /// @NOTE: see explicit specialization below for default KEY==void case
-/// @NOTE: this class is not thread-safe unless used as read-only
-template<typename T, typename KEY = void, EInstanceTrackerAllowKeyCollisions KEY_COLLISION_BEHAVIOR = LLInstanceTrackerErrorOnCollision>
-class LLInstanceTracker : public LLInstanceTrackerBase
+template<typename T, typename KEY = void,
+         EInstanceTrackerAllowKeyCollisions KEY_COLLISION_BEHAVIOR = LLInstanceTrackerErrorOnCollision>
+class LLInstanceTracker :
+    public LLInstanceTrackerBase<LLInstanceTrackerStuff::StaticMap<KEY, std::shared_ptr<T>>>
 {
-	typedef LLInstanceTracker<T, KEY> self_t;
-	typedef typename std::multimap<KEY, T*> InstanceMap;
-	struct StaticData: public StaticBase
-	{
-		InstanceMap sMap;
-	};
-	static StaticData& getStatic() { static StaticData sData; return sData;}
-	static InstanceMap& getMap_() { return getStatic().sMap; }
+    typedef LLInstanceTrackerBase<LLInstanceTrackerStuff::StaticMap<KEY, std::shared_ptr<T>>> super;
+    using typename super::StaticData;
+    using typename super::LockStatic;
+    typedef typename StaticData::InstanceMap InstanceMap;
 
 public:
-	class instance_iter : public boost::iterator_facade<instance_iter, T, boost::forward_traversal_tag>
-	{
-	public:
-		typedef boost::iterator_facade<instance_iter, T, boost::forward_traversal_tag> super_t;
-		
-		instance_iter(const typename InstanceMap::iterator& it)
-		:	mIterator(it)
-		{
-			getStatic().incrementDepth();
-		}
-
-		~instance_iter()
-		{
-			getStatic().decrementDepth();
-		}
-
-
-	private:
-		friend class boost::iterator_core_access;
-
-		void increment() { mIterator++; }
-		bool equal(instance_iter const& other) const
-		{
-			return mIterator == other.mIterator;
-		}
-
-		T& dereference() const
-		{
-			return *(mIterator->second);
-		}
-
-		typename InstanceMap::iterator mIterator;
-	};
-
-	class key_iter : public boost::iterator_facade<key_iter, KEY, boost::forward_traversal_tag>
-	{
-	public:
-		typedef boost::iterator_facade<key_iter, KEY, boost::forward_traversal_tag> super_t;
-
-		key_iter(typename InstanceMap::iterator it)
-		:	mIterator(it)
-		{
-			getStatic().incrementDepth();
-		}
-
-		key_iter(const key_iter& other)
-		:	mIterator(other.mIterator)
-		{
-			getStatic().incrementDepth();
-		}
-
-		~key_iter()
-		{
-			getStatic().decrementDepth();
-		}
-
-
-	private:
-		friend class boost::iterator_core_access;
-
-		void increment() { mIterator++; }
-		bool equal(key_iter const& other) const
-		{
-			return mIterator == other.mIterator;
-		}
-
-		KEY& dereference() const
-		{
-			return const_cast<KEY&>(mIterator->first);
-		}
-
-		typename InstanceMap::iterator mIterator;
-	};
-
-	static T* getInstance(const KEY& k)
-	{
-		const InstanceMap& map(getMap_());
-		typename InstanceMap::const_iterator found = map.find(k);
-		return (found == map.end()) ? NULL : found->second;
-	}
-
-	static instance_iter beginInstances() 
-	{	
-		return instance_iter(getMap_().begin()); 
-	}
-
-	static instance_iter endInstances() 
-	{
-		return instance_iter(getMap_().end());
-	}
-
-	static S32 instanceCount() 
-	{ 
-		return getMap_().size(); 
-	}
-
-	static key_iter beginKeys()
-	{
-		return key_iter(getMap_().begin());
-	}
-	static key_iter endKeys()
-	{
-		return key_iter(getMap_().end());
-	}
+    // snapshot of std::pair<const KEY, std::shared_ptr<T>> pairs
+    class snapshot
+    {
+        // It's very important that what we store in this snapshot are
+        // weak_ptrs, NOT shared_ptrs. That's how we discover whether any
+        // instance has been deleted during the lifespan of a snapshot.
+        typedef std::vector<std::pair<const KEY, std::weak_ptr<T>>> VectorType;
+        // Dereferencing our iterator produces a std::shared_ptr for each
+        // instance that still exists. Since we store weak_ptrs, that involves
+        // two chained transformations:
+        // - a transform_iterator to lock the weak_ptr and return a shared_ptr
+        // - a filter_iterator to skip any shared_ptr that has become invalid.
+        // It is very important that we filter lazily, that is, during
+        // traversal. Any one of our stored weak_ptrs might expire during
+        // traversal.
+        typedef std::pair<const KEY, std::shared_ptr<T>> strong_pair;
+        // Note for future reference: nat has not yet had any luck (up to
+        // Boost 1.67) trying to use boost::transform_iterator with a hand-
+        // coded functor, only with actual functions. In my experience, an
+        // internal boost::result_of() operation fails, even with an explicit
+        // result_type typedef. But this works.
+        static strong_pair strengthen(typename VectorType::value_type& pair)
+        {
+            return { pair.first, pair.second.lock() };
+        }
+        static bool dead_skipper(const strong_pair& pair)
+        {
+            return bool(pair.second);
+        }
+
+    public:
+        snapshot():
+            // populate our vector with a snapshot of (locked!) InstanceMap
+            // note, this assigns pair<KEY, shared_ptr> to pair<KEY, weak_ptr>
+            mData(mLock->mMap.begin(), mLock->mMap.end())
+        {
+            // release the lock once we've populated mData
+            mLock.unlock();
+        }
+
+        // You can't make a transform_iterator (or anything else) that
+        // literally stores a C++ function (decltype(strengthen)) -- but you
+        // can make a transform_iterator based on a _function pointer._
+        typedef boost::transform_iterator<decltype(strengthen)*,
+                                          typename VectorType::iterator> strong_iterator;
+        typedef boost::filter_iterator<decltype(dead_skipper)*, strong_iterator> iterator;
+
+        iterator begin() { return make_iterator(mData.begin()); }
+        iterator end()   { return make_iterator(mData.end()); }
+
+    private:
+        iterator make_iterator(typename VectorType::iterator iter)
+        {
+            // transform_iterator only needs the base iterator and the transform.
+            // filter_iterator wants the predicate and both ends of the range.
+            return iterator(dead_skipper,
+                            strong_iterator(iter, strengthen),
+                            strong_iterator(mData.end(), strengthen));
+        }
+
+        LockStatic mLock;           // lock static data during construction
+        VectorType mData;
+    };
+
+    // iterate over this for references to each instance
+    class instance_snapshot: public snapshot
+    {
+    private:
+        static T& instance_getter(typename snapshot::iterator::reference pair)
+        {
+            return *pair.second;
+        }
+    public:
+        typedef boost::transform_iterator<decltype(instance_getter)*,
+                                          typename snapshot::iterator> iterator;
+        iterator begin() { return iterator(snapshot::begin(), instance_getter); }
+        iterator end()   { return iterator(snapshot::end(),   instance_getter); }
+
+        void deleteAll()
+        {
+            for (auto it(snapshot::begin()), end(snapshot::end()); it != end; ++it)
+            {
+                delete it->second.get();
+            }
+        }
+    };                   
+
+    // iterate over this for each key
+    class key_snapshot: public snapshot
+    {
+    private:
+        static KEY key_getter(typename snapshot::iterator::reference pair)
+        {
+            return pair.first;
+        }
+    public:
+        typedef boost::transform_iterator<decltype(key_getter)*,
+                                          typename snapshot::iterator> iterator;
+        iterator begin() { return iterator(snapshot::begin(), key_getter); }
+        iterator end()   { return iterator(snapshot::end(),   key_getter); }
+    };
+
+    static T* getInstance(const KEY& k)
+    {
+        LockStatic lock;
+        const InstanceMap& map(lock->mMap);
+        typename InstanceMap::const_iterator found = map.find(k);
+        return (found == map.end()) ? NULL : found->second.get();
+    }
+
+    static S32 instanceCount() 
+    { 
+        return LockStatic()->mMap.size(); 
+    }
 
 protected:
-	LLInstanceTracker(const KEY& key) 
-	{ 
-		// make sure static data outlives all instances
-		getStatic();
-		add_(key); 
-	}
-	virtual ~LLInstanceTracker() LLINSTANCETRACKER_DTOR_NOEXCEPT
-	{ 
-		// it's unsafe to delete instances of this type while all instances are being iterated over.
-		llassert_always(getStatic().getDepth() == 0);
-		remove_();
-	}
-	virtual void setKey(KEY key) { remove_(); add_(key); }
-	virtual const KEY& getKey() const { return mInstanceKey; }
+    LLInstanceTracker(const KEY& key) 
+    {
+        // We do not intend to manage the lifespan of this object with
+        // shared_ptr, so give it a no-op deleter. We store shared_ptrs in our
+        // InstanceMap specifically so snapshot can store weak_ptrs so we can
+        // detect deletions during traversals.
+        std::shared_ptr<T> ptr(static_cast<T*>(this), [](T*){});
+        LockStatic lock;
+        add_(lock, key, ptr);
+    }
+public:
+    virtual ~LLInstanceTracker()
+    {
+        LockStatic lock;
+        remove_(lock);
+    }
+protected:
+    virtual void setKey(KEY key)
+    {
+        LockStatic lock;
+        // Even though the shared_ptr we store in our map has a no-op deleter
+        // for T itself, letting the use count decrement to 0 will still
+        // delete the use-count object. Capture the shared_ptr we just removed
+        // and re-add it to the map with the new key.
+        auto ptr = remove_(lock);
+        add_(lock, key, ptr);
+    }
+public:
+    virtual const KEY& getKey() const { return mInstanceKey; }
 
 private:
-	LLInstanceTracker( const LLInstanceTracker& );
-	const LLInstanceTracker& operator=( const LLInstanceTracker& );
-
-	void add_(const KEY& key) 
-	{ 
-		mInstanceKey = key; 
-		InstanceMap& map = getMap_();
-		typename InstanceMap::iterator insertion_point_it = map.lower_bound(key);
-		if (insertion_point_it != map.end() 
-			&& insertion_point_it->first == key)
-		{ // found existing entry with that key
-			switch(KEY_COLLISION_BEHAVIOR)
-			{
-				case LLInstanceTrackerErrorOnCollision:
-				{
-					// use assert here instead of LL_ERRS(), otherwise the error will be ignored since this call is made during global object initialization
-					llassert_always_msg(false, "Instance with this same key already exists!");
-					break;
-				}
-				case LLInstanceTrackerReplaceOnCollision:
-				{
-					// replace pointer, but leave key (should have compared equal anyway)
-					insertion_point_it->second = static_cast<T*>(this);
-					break;
-				}
-				default:
-					break;
-			}
-		}
-		else
-		{ // new key
-			map.insert(insertion_point_it, std::make_pair(key, static_cast<T*>(this)));
-		}
-	}
-	void remove_()
-	{
-		InstanceMap& map = getMap_();
-		typename InstanceMap::iterator iter = map.find(mInstanceKey);
-		if (iter != map.end())
-		{
-			map.erase(iter);
-		}
-	}
+    LLInstanceTracker( const LLInstanceTracker& ) = delete;
+    LLInstanceTracker& operator=( const LLInstanceTracker& ) = delete;
+
+    // for logging
+    template <typename K>
+    static K report(K key) { return key; }
+    static std::string report(const std::string& key) { return "'" + key + "'"; }
+    static std::string report(const char* key) { return report(std::string(key)); }
+
+    // caller must instantiate LockStatic
+    void add_(LockStatic& lock, const KEY& key, const std::shared_ptr<T>& ptr) 
+    { 
+        mInstanceKey = key; 
+        InstanceMap& map = lock->mMap;
+        switch(KEY_COLLISION_BEHAVIOR)
+        {
+        case LLInstanceTrackerErrorOnCollision:
+        {
+            // map stores shared_ptr to self
+            auto pair = map.emplace(key, ptr);
+            if (! pair.second)
+            {
+                LL_ERRS("LLInstanceTracker") << "Instance with key " << report(key)
+                                             << " already exists!" << LL_ENDL;
+            }
+            break;
+        }
+        case LLInstanceTrackerReplaceOnCollision:
+            map[key] = ptr;
+            break;
+        default:
+            break;
+        }
+    }
+    std::shared_ptr<T> remove_(LockStatic& lock)
+    {
+        InstanceMap& map = lock->mMap;
+        typename InstanceMap::iterator iter = map.find(mInstanceKey);
+        if (iter != map.end())
+        {
+            auto ret = iter->second;
+            map.erase(iter);
+            return ret;
+        }
+        return {};
+    }
 
 private:
-	KEY mInstanceKey;
+    KEY mInstanceKey;
 };
 
+/*****************************************************************************
+*   LLInstanceTracker without key
+*****************************************************************************/
+namespace LLInstanceTrackerStuff
+{
+    template <typename VALUE>
+    struct StaticSet: public StaticBase
+    {
+        typedef std::set<VALUE> InstanceSet;
+        InstanceSet mSet;
+    };
+} // LLInstanceTrackerStuff
+
+// TODO:
+// - For the case of omitted KEY template parameter, consider storing
+//   std::map<T*, std::shared_ptr<T>> instead of std::set<std::shared_ptr<T>>.
+//   That might let us share more of the implementation between KEY and
+//   non-KEY LLInstanceTracker subclasses.
+// - Even if not that, consider trying to unify the snapshot implementations.
+//   The trouble is that the 'iterator' published by each (and by their
+//   subclasses) must reflect the specific type of the callables that
+//   distinguish them. (Maybe make instance_snapshot() and key_snapshot()
+//   factory functions that pass lambdas to a factory function for the generic
+//   template class?)
+
 /// explicit specialization for default case where KEY is void
 /// use a simple std::set<T*>
 template<typename T, EInstanceTrackerAllowKeyCollisions KEY_COLLISION_BEHAVIOR>
-class LLInstanceTracker<T, void, KEY_COLLISION_BEHAVIOR> : public LLInstanceTrackerBase
+class LLInstanceTracker<T, void, KEY_COLLISION_BEHAVIOR> :
+    public LLInstanceTrackerBase<LLInstanceTrackerStuff::StaticSet<std::shared_ptr<T>>>
 {
-	typedef LLInstanceTracker<T, void> self_t;
-	typedef typename std::set<T*> InstanceSet;
-	struct StaticData: public StaticBase
-	{
-		InstanceSet sSet;
-	};
-	static StaticData& getStatic() { static StaticData sData; return sData; }
-	static InstanceSet& getSet_() { return getStatic().sSet; }
+    typedef LLInstanceTrackerBase<LLInstanceTrackerStuff::StaticSet<std::shared_ptr<T>>> super;
+    using typename super::StaticData;
+    using typename super::LockStatic;
+    typedef typename StaticData::InstanceSet InstanceSet;
 
 public:
+    /**
+     * Storing a dumb T* somewhere external is a bad idea, since
+     * LLInstanceTracker subclasses are explicitly destroyed rather than
+     * managed by smart pointers. It's legal to declare stack instances of an
+     * LLInstanceTracker subclass. But it's reasonable to store a
+     * std::weak_ptr<T>, which will become invalid when the T instance is
+     * destroyed.
+     */
+    std::weak_ptr<T> getWeak()
+    {
+        return mSelf;
+    }
+    
+    static S32 instanceCount() { return LockStatic()->mSet.size(); }
 
-	/**
-	 * Does a particular instance still exist? Of course, if you already have
-	 * a T* in hand, you need not call getInstance() to @em locate the
-	 * instance -- unlike the case where getInstance() accepts some kind of
-	 * key. Nonetheless this method is still useful to @em validate a
-	 * particular T*, since each instance's destructor removes itself from the
-	 * underlying set.
-	 */
-	static T* getInstance(T* k)
-	{
-		const InstanceSet& set(getSet_());
-		typename InstanceSet::const_iterator found = set.find(k);
-		return (found == set.end())? NULL : *found;
-	}
-	static S32 instanceCount() { return getSet_().size(); }
-
-	class instance_iter : public boost::iterator_facade<instance_iter, T, boost::forward_traversal_tag>
-	{
-	public:
-		instance_iter(const typename InstanceSet::iterator& it)
-		:	mIterator(it)
-		{
-			getStatic().incrementDepth();
-		}
-
-		instance_iter(const instance_iter& other)
-		:	mIterator(other.mIterator)
-		{
-			getStatic().incrementDepth();
-		}
-
-		~instance_iter()
-		{
-			getStatic().decrementDepth();
-		}
-
-	private:
-		friend class boost::iterator_core_access;
-
-		void increment() { mIterator++; }
-		bool equal(instance_iter const& other) const
-		{
-			return mIterator == other.mIterator;
-		}
-
-		T& dereference() const
-		{
-			return **mIterator;
-		}
-
-		typename InstanceSet::iterator mIterator;
-	};
-
-	static instance_iter beginInstances() {	return instance_iter(getSet_().begin()); }
-	static instance_iter endInstances() { return instance_iter(getSet_().end()); }
+    // snapshot of std::shared_ptr<T> pointers
+    class snapshot
+    {
+        // It's very important that what we store in this snapshot are
+        // weak_ptrs, NOT shared_ptrs. That's how we discover whether any
+        // instance has been deleted during the lifespan of a snapshot.
+        typedef std::vector<std::weak_ptr<T>> VectorType;
+        // Dereferencing our iterator produces a std::shared_ptr for each
+        // instance that still exists. Since we store weak_ptrs, that involves
+        // two chained transformations:
+        // - a transform_iterator to lock the weak_ptr and return a shared_ptr
+        // - a filter_iterator to skip any shared_ptr that has become invalid.
+        typedef std::shared_ptr<T> strong_ptr;
+        static strong_ptr strengthen(typename VectorType::value_type& ptr)
+        {
+            return ptr.lock();
+        }
+        static bool dead_skipper(const strong_ptr& ptr)
+        {
+            return bool(ptr);
+        }
+
+    public:
+        snapshot():
+            // populate our vector with a snapshot of (locked!) InstanceSet
+            // note, this assigns stored shared_ptrs to weak_ptrs for snapshot
+            mData(mLock->mSet.begin(), mLock->mSet.end())
+        {
+            // release the lock once we've populated mData
+            mLock.unlock();
+        }
+
+        typedef boost::transform_iterator<decltype(strengthen)*,
+                                          typename VectorType::iterator> strong_iterator;
+        typedef boost::filter_iterator<decltype(dead_skipper)*, strong_iterator> iterator;
+
+        iterator begin() { return make_iterator(mData.begin()); }
+        iterator end()   { return make_iterator(mData.end()); }
+
+    private:
+        iterator make_iterator(typename VectorType::iterator iter)
+        {
+            // transform_iterator only needs the base iterator and the transform.
+            // filter_iterator wants the predicate and both ends of the range.
+            return iterator(dead_skipper,
+                            strong_iterator(iter, strengthen),
+                            strong_iterator(mData.end(), strengthen));
+        }
+
+        LockStatic mLock;           // lock static data during construction
+        VectorType mData;
+    };
+
+    // iterate over this for references to each instance
+    struct instance_snapshot: public snapshot
+    {
+        typedef boost::indirect_iterator<typename snapshot::iterator> iterator;
+        iterator begin() { return iterator(snapshot::begin()); }
+        iterator end()   { return iterator(snapshot::end()); }
+
+        void deleteAll()
+        {
+            for (auto it(snapshot::begin()), end(snapshot::end()); it != end; ++it)
+            {
+                delete it->get();
+            }
+        }
+    };
 
 protected:
-	LLInstanceTracker()
-	{
-		// make sure static data outlives all instances
-		getStatic();
-		getSet_().insert(static_cast<T*>(this));
-	}
-	virtual ~LLInstanceTracker() LLINSTANCETRACKER_DTOR_NOEXCEPT
-	{
-		// it's unsafe to delete instances of this type while all instances are being iterated over.
-		llassert_always(getStatic().getDepth() == 0);
-		getSet_().erase(static_cast<T*>(this));
-	}
-
-	LLInstanceTracker(const LLInstanceTracker& other)
-	{
-		getSet_().insert(static_cast<T*>(this));
-	}
+    LLInstanceTracker()
+    {
+        // Since we do not intend for this shared_ptr to manage lifespan, give
+        // it a no-op deleter.
+        std::shared_ptr<T> ptr(static_cast<T*>(this), [](T*){});
+        // save corresponding weak_ptr for future reference
+        mSelf = ptr;
+        // Also store it in our class-static set to track this instance.
+        LockStatic()->mSet.emplace(ptr);
+    }
+public:
+    virtual ~LLInstanceTracker()
+    {
+        // convert weak_ptr to shared_ptr because that's what we store in our
+        // InstanceSet
+        LockStatic()->mSet.erase(mSelf.lock());
+    }
+protected:
+    LLInstanceTracker(const LLInstanceTracker& other):
+        LLInstanceTracker()
+    {}
+
+private:
+    // Storing a weak_ptr to self is a bit like deriving from
+    // std::enable_shared_from_this(), except more explicit.
+    std::weak_ptr<T> mSelf;
 };
 
 #endif