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.

2 files changed, 65 insertions, 70 deletions

diff --git a/indra/llcommon/tests/llinstancetracker_test.cpp b/indra/llcommon/tests/llinstancetracker_test.cpp
index d94fc0c56d..9b89159625 100644
--- a/indra/llcommon/tests/llinstancetracker_test.cpp
+++ b/indra/llcommon/tests/llinstancetracker_test.cpp
@@ -41,7 +41,6 @@
 #include <boost/scoped_ptr.hpp>
 // other Linden headers
 #include "../test/lltut.h"
-#include "wrapllerrs.h"
 
 struct Badness: public std::runtime_error
 {
@@ -112,24 +111,22 @@ namespace tut
     void object::test<2>()
     {
         ensure_equals(Unkeyed::instanceCount(), 0);
-        Unkeyed* dangling = NULL;
+        std::weak_ptr<Unkeyed> dangling;
         {
             Unkeyed one;
             ensure_equals(Unkeyed::instanceCount(), 1);
-            Unkeyed* found = Unkeyed::getInstance(&one);
-            ensure_equals(found, &one);
+            std::weak_ptr<Unkeyed> found = one.getWeak();
+            ensure(! found.expired());
             {
                 boost::scoped_ptr<Unkeyed> two(new Unkeyed);
                 ensure_equals(Unkeyed::instanceCount(), 2);
-                Unkeyed* found = Unkeyed::getInstance(two.get());
-                ensure_equals(found, two.get());
             }
             ensure_equals(Unkeyed::instanceCount(), 1);
-            // store an unwise pointer to a temp Unkeyed instance
-            dangling = &one;
+            // store a weak pointer to a temp Unkeyed instance
+            dangling = found;
         } // make that instance vanish
         // check the now-invalid pointer to the destroyed instance
-        ensure("getInstance(T*) failed to track destruction", ! Unkeyed::getInstance(dangling));
+        ensure("weak_ptr<Unkeyed> failed to track destruction", dangling.expired());
         ensure_equals(Unkeyed::instanceCount(), 0);
     }
 
@@ -142,7 +139,8 @@ namespace tut
         // reimplement LLInstanceTracker using, say, a hash map instead of a
         // std::map. We DO insist that every key appear exactly once.
         typedef std::vector<std::string> StringVector;
-        StringVector keys(Keyed::beginKeys(), Keyed::endKeys());
+        auto snap = Keyed::key_snapshot();
+        StringVector keys(snap.begin(), snap.end());
         std::sort(keys.begin(), keys.end());
         StringVector::const_iterator ki(keys.begin());
         ensure_equals(*ki++, "one");
@@ -153,17 +151,15 @@ namespace tut
         ensure("didn't reach end", ki == keys.end());
 
         // Use a somewhat different approach to order independence with
-        // beginInstances(): explicitly capture the instances we know in a
+        // instance_snapshot(): explicitly capture the instances we know in a
         // set, and delete them as we iterate through.
         typedef std::set<Keyed*> InstanceSet;
         InstanceSet instances;
         instances.insert(&one);
         instances.insert(&two);
         instances.insert(&three);
-        for (Keyed::instance_iter ii(Keyed::beginInstances()), iend(Keyed::endInstances());
-             ii != iend; ++ii)
+        for (auto& ref : Keyed::instance_snapshot())
         {
-            Keyed& ref = *ii;
             ensure_equals("spurious instance", instances.erase(&ref), 1);
         }
         ensure_equals("unreported instance", instances.size(), 0);
@@ -180,11 +176,10 @@ namespace tut
         instances.insert(&two);
         instances.insert(&three);
 
-		for (Unkeyed::instance_iter ii(Unkeyed::beginInstances()), iend(Unkeyed::endInstances()); ii != iend; ++ii)
-		{
-			Unkeyed& ref = *ii;
-			ensure_equals("spurious instance", instances.erase(&ref), 1);
-		}
+        for (auto& ref : Unkeyed::instance_snapshot())
+        {
+            ensure_equals("spurious instance", instances.erase(&ref), 1);
+        }
 
         ensure_equals("unreported instance", instances.size(), 0);
     }
@@ -192,49 +187,49 @@ namespace tut
     template<> template<>
     void object::test<5>()
     {
-        set_test_name("delete Keyed with outstanding instance_iter");
-        std::string what;
-        Keyed* keyed = new Keyed("delete Keyed with outstanding instance_iter");
-        {
-            WrapLLErrs wrapper;
-            Keyed::instance_iter i(Keyed::beginInstances());
-            what = wrapper.catch_llerrs([&keyed](){
-                    delete keyed;
-                });
-        }
-        ensure(! what.empty());
+        std::string desc("delete Keyed with outstanding instance_snapshot");
+        set_test_name(desc);
+        Keyed* keyed = new Keyed(desc);
+        // capture a snapshot but do not yet traverse it
+        auto snapshot = Keyed::instance_snapshot();
+        // delete the one instance
+        delete keyed;
+        // traversing the snapshot should reflect the deletion
+        // avoid ensure_equals() because it requires the ability to stream the
+        // two values to std::ostream
+        ensure(snapshot.begin() == snapshot.end());
     }
 
     template<> template<>
     void object::test<6>()
     {
-        set_test_name("delete Keyed with outstanding key_iter");
-        std::string what;
-        Keyed* keyed = new Keyed("delete Keyed with outstanding key_it");
-        {
-            WrapLLErrs wrapper;
-            Keyed::key_iter i(Keyed::beginKeys());
-            what = wrapper.catch_llerrs([&keyed](){
-                    delete keyed;
-                });
-        }
-        ensure(! what.empty());
+        std::string desc("delete Keyed with outstanding key_snapshot");
+        set_test_name(desc);
+        Keyed* keyed = new Keyed(desc);
+        // capture a snapshot but do not yet traverse it
+        auto snapshot = Keyed::key_snapshot();
+        // delete the one instance
+        delete keyed;
+        // traversing the snapshot should reflect the deletion
+        // avoid ensure_equals() because it requires the ability to stream the
+        // two values to std::ostream
+        ensure(snapshot.begin() == snapshot.end());
     }
 
     template<> template<>
     void object::test<7>()
     {
-        set_test_name("delete Unkeyed with outstanding instance_iter");
+        set_test_name("delete Unkeyed with outstanding instance_snapshot");
         std::string what;
         Unkeyed* unkeyed = new Unkeyed;
-        {
-            WrapLLErrs wrapper;
-            Unkeyed::instance_iter i(Unkeyed::beginInstances());
-            what = wrapper.catch_llerrs([&unkeyed](){
-                    delete unkeyed;
-                });
-        }
-        ensure(! what.empty());
+        // capture a snapshot but do not yet traverse it
+        auto snapshot = Unkeyed::instance_snapshot();
+        // delete the one instance
+        delete unkeyed;
+        // traversing the snapshot should reflect the deletion
+        // avoid ensure_equals() because it requires the ability to stream the
+        // two values to std::ostream
+        ensure(snapshot.begin() == snapshot.end());
     }
 
     template<> template<>
@@ -246,11 +241,9 @@ namespace tut
         // We can't use the iterator-range InstanceSet constructor because
         // beginInstances() returns an iterator that dereferences to an
         // Unkeyed&, not an Unkeyed*.
-        for (Unkeyed::instance_iter uki(Unkeyed::beginInstances()),
-                                    ukend(Unkeyed::endInstances());
-             uki != ukend; ++uki)
+        for (auto& ref : Unkeyed::instance_snapshot())
         {
-            existing.insert(&*uki);
+            existing.insert(&ref);
         }
         try
         {
@@ -273,11 +266,9 @@ namespace tut
         // instances was also present in the original set. If that's not true,
         // it's because our new Unkeyed ended up in the updated set despite
         // its constructor exception.
-        for (Unkeyed::instance_iter uki(Unkeyed::beginInstances()),
-                                    ukend(Unkeyed::endInstances());
-             uki != ukend; ++uki)
+        for (auto& ref : Unkeyed::instance_snapshot())
         {
-            ensure("failed to remove instance", existing.find(&*uki) != existing.end());
+            ensure("failed to remove instance", existing.find(&ref) != existing.end());
         }
     }
 } // namespace tut
diff --git a/indra/llcommon/tests/llleap_test.cpp b/indra/llcommon/tests/llleap_test.cpp
index bf0a74d10d..9d71e327d8 100644
--- a/indra/llcommon/tests/llleap_test.cpp
+++ b/indra/llcommon/tests/llleap_test.cpp
@@ -49,24 +49,28 @@ const size_t BUFFERED_LENGTH = 1023*1024; // try wrangling just under a megabyte
 
 #endif
 
-void waitfor(const std::vector<LLLeap*>& instances, int timeout=60)
+// capture std::weak_ptrs to LLLeap instances so we can tell when they expire
+typedef std::vector<std::weak_ptr<LLLeap>> LLLeapVector;
+
+void waitfor(const LLLeapVector& instances, int timeout=60)
 {
     int i;
     for (i = 0; i < timeout; ++i)
     {
         // Every iteration, test whether any of the passed LLLeap instances
         // still exist (are still running).
-        std::vector<LLLeap*>::const_iterator vli(instances.begin()), vlend(instances.end());
-        for ( ; vli != vlend; ++vli)
+        bool found = false;
+        for (auto& ptr : instances)
         {
-            // getInstance() returns NULL if it's terminated/gone, non-NULL if
-            // it's still running
-            if (LLLeap::getInstance(*vli))
+            if (! ptr.expired())
+            {
+                found = true;
                 break;
+            }
         }
         // If we made it through all of 'instances' without finding one that's
         // still running, we're done.
-        if (vli == vlend)
+        if (! found)
         {
 /*==========================================================================*|
             std::cout << instances.size() << " LLLeap instances terminated in "
@@ -86,8 +90,8 @@ void waitfor(const std::vector<LLLeap*>& instances, int timeout=60)
 
 void waitfor(LLLeap* instance, int timeout=60)
 {
-    std::vector<LLLeap*> instances;
-    instances.push_back(instance);
+    LLLeapVector instances;
+    instances.push_back(instance->getWeak());
     waitfor(instances, timeout);
 }
 
@@ -218,11 +222,11 @@ namespace tut
         NamedTempFile script("py",
                              "import time\n"
                              "time.sleep(1)\n");
-        std::vector<LLLeap*> instances;
+        LLLeapVector instances;
         instances.push_back(LLLeap::create(get_test_name(),
-                                           sv(list_of(PYTHON)(script.getName()))));
+                                           sv(list_of(PYTHON)(script.getName())))->getWeak());
         instances.push_back(LLLeap::create(get_test_name(),
-                                           sv(list_of(PYTHON)(script.getName()))));
+                                           sv(list_of(PYTHON)(script.getName())))->getWeak());
         // In this case we're simply establishing that two LLLeap instances
         // can coexist without throwing exceptions or bombing in any other
         // way. Wait for them to terminate.