WIP: Trying to diagnose and fix test program shutdown crash

5 files changed, 107 insertions, 55 deletions

diff --git a/indra/llcommon/lazyeventapi.cpp b/indra/llcommon/lazyeventapi.cpp
index 91db0ee4a6..eebed374c3 100644
--- a/indra/llcommon/lazyeventapi.cpp
+++ b/indra/llcommon/lazyeventapi.cpp
@@ -47,7 +47,9 @@ LL::LazyEventAPIBase::~LazyEventAPIBase()
     // case, do NOT unregister their name out from under them!
     // If this is a static instance being destroyed at process shutdown,
     // LLEventPumps will probably have been cleaned up already.
-    if (mRegistered && ! LLEventPumps::wasDeleted())
+    // That said, in a test program, LLEventPumps might never have been
+    // constructed to start with.
+    if (mRegistered && LLEventPumps::instanceExists())
     {
         // unregister the callback to this doomed instance
         LLEventPumps::instance().unregisterPumpFactory(mParams.name);
diff --git a/indra/llcommon/llinstancetracker.h b/indra/llcommon/llinstancetracker.h
index 921f743ada..aba9f1187b 100644
--- a/indra/llcommon/llinstancetracker.h
+++ b/indra/llcommon/llinstancetracker.h
@@ -80,6 +80,8 @@ class LLInstanceTracker
     {
         InstanceMap mMap;
     };
+    // Unfortunately there's no umbrella class that owns all LLInstanceTracker
+    // instances, so there's no good place to call LockStatic::cleanup().
     typedef llthread::LockStatic<StaticData> LockStatic;
 
 public:
@@ -169,23 +171,7 @@ public:
         }
 
         // lock static data during construction
-#if ! LL_WINDOWS
         LockStatic mLock;
-#else  // LL_WINDOWS
-        // We want to be able to use (e.g.) our instance_snapshot subclass as:
-        // for (auto& inst : T::instance_snapshot()) ...
-        // But when this snapshot base class directly contains LockStatic, as
-        // above, Visual Studio 2017 requires us to code instead:
-        // for (auto& inst : std::move(T::instance_snapshot())) ...
-        // nat thinks this should be unnecessary, as an anonymous class
-        // instance is already a temporary. It shouldn't need to be cast to
-        // rvalue reference (the role of std::move()). clang evidently agrees,
-        // as the short form works fine with Xcode on Mac.
-        // To support the succinct usage, instead of directly storing
-        // LockStatic, store std::shared_ptr<LockStatic>, which is copyable.
-        std::shared_ptr<LockStatic> mLockp{std::make_shared<LockStatic>()};
-        LockStatic& mLock{*mLockp};
-#endif // LL_WINDOWS
         VectorType mData;
     };
     using snapshot = snapshot_of<T>;
@@ -384,6 +370,7 @@ class LLInstanceTracker<T, void, KEY_COLLISION_BEHAVIOR>
     {
         InstanceSet mSet;
     };
+    // see LockStatic comment in the above specialization for non-void KEY
     typedef llthread::LockStatic<StaticData> LockStatic;
 
 public:
@@ -461,23 +448,7 @@ public:
         }
 
         // lock static data during construction
-#if ! LL_WINDOWS
         LockStatic mLock;
-#else  // LL_WINDOWS
-        // We want to be able to use our instance_snapshot subclass as:
-        // for (auto& inst : T::instance_snapshot()) ...
-        // But when this snapshot base class directly contains LockStatic, as
-        // above, Visual Studio 2017 requires us to code instead:
-        // for (auto& inst : std::move(T::instance_snapshot())) ...
-        // nat thinks this should be unnecessary, as an anonymous class
-        // instance is already a temporary. It shouldn't need to be cast to
-        // rvalue reference (the role of std::move()). clang evidently agrees,
-        // as the short form works fine with Xcode on Mac.
-        // To support the succinct usage, instead of directly storing
-        // LockStatic, store std::shared_ptr<LockStatic>, which is copyable.
-        std::shared_ptr<LockStatic> mLockp{std::make_shared<LockStatic>()};
-        LockStatic& mLock{*mLockp};
-#endif // LL_WINDOWS
         VectorType mData;
     };
     using snapshot = snapshot_of<T>;
diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp
index 0acf9513d8..3049e81139 100644
--- a/indra/llcommon/llsingleton.cpp
+++ b/indra/llcommon/llsingleton.cpp
@@ -33,6 +33,7 @@
 #include "llerrorcontrol.h"
 #include "llexception.h"
 #include "llmainthreadtask.h"
+#include "../test/writestr.h"
 #include <algorithm>
 #include <iostream>                 // std::cerr in dire emergency
 #include <sstream>
@@ -65,6 +66,12 @@ private:
     mutex_t mMutex;
 
 public:
+    ~MasterList()
+    {
+        writestr(2, "~MasterList()");
+    }
+
+public:
     // Instantiate this to both obtain a reference to MasterList::instance()
     // and lock its mutex for the lifespan of this Lock instance.
     class Lock
@@ -147,6 +154,7 @@ public:
 private:
     list_t& get_initializing_()
     {
+        writestr(2, "MasterList::get_initializing_()");
         LLSingletonBase::list_t* current = mInitializing.get();
         if (! current)
         {
@@ -163,6 +171,7 @@ private:
     // we pop the list to empty, reset() the running coroutine's local_ptr.
     void cleanup_initializing_()
     {
+        writestr(2, "MasterList::cleanup_initializing_()");
         mInitializing.reset(nullptr);
     }
 };
@@ -272,17 +281,33 @@ void LLSingletonBase::reset_initializing(list_t::size_type size)
 
 void LLSingletonBase::MasterList::LockedInitializing::log(const char* verb, const char* name)
 {
-        LL_DEBUGS("LLSingleton") << verb << ' ' << demangle(name) << ';';
-        if (mList)
+    LL_DEBUGS("LLSingleton") << verb << ' ' << demangle(name) << ';';
+    if (mList)
+    {
+        for (list_t::const_reverse_iterator ri(mList->rbegin()), rend(mList->rend());
+             ri != rend; ++ri)
         {
-            for (list_t::const_reverse_iterator ri(mList->rbegin()), rend(mList->rend());
-                 ri != rend; ++ri)
-            {
-                LLSingletonBase* sb(*ri);
-                LL_CONT << ' ' << classname(sb);
-            }
+            LLSingletonBase* sb(*ri);
+            LL_CONT << ' ' << classname(sb);
         }
-        LL_ENDL;
+    }
+    LL_ENDL;
+}
+
+void LLSingletonBase::capture_dependency(LLSingletonBase* sb)
+{
+    // If we're called very late during application shutdown, the Boost.Fibers
+    // library may have shut down, and MasterList::mInitializing.get() might
+    // blow up. But if we're called that late, there's really no point in
+    // trying to capture this dependency. 
+    writestr(2, "LLSingletonBase::capture_dependency() trampoline");
+    if (boost::fibers::context::active())
+    {
+        writestr(2, "still active");
+        sb->capture_dependency();
+    }
+    else
+        writestr(2, "no longer active");
 }
 
 void LLSingletonBase::capture_dependency()
diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h
index 25afccccc0..f44dcd77a8 100644
--- a/indra/llcommon/llsingleton.h
+++ b/indra/llcommon/llsingleton.h
@@ -112,6 +112,8 @@ protected:
     // If a given call to B::getInstance() happens during either A::A() or
     // A::initSingleton(), record that A directly depends on B.
     void capture_dependency();
+    // trampoline to non-static member function
+    static void capture_dependency(LLSingletonBase*);
 
     // delegate logging calls to llsingleton.cpp
 public:
@@ -202,7 +204,7 @@ struct LLSingleton_manage_master
     }
     void capture_dependency(LLSingletonBase* sb)
     {
-        sb->capture_dependency();
+        LLSingletonBase::capture_dependency(sb);
     }
 };
 
@@ -434,6 +436,11 @@ protected:
 
         // Remove this instance from the master list.
         LLSingleton_manage_master<DERIVED_TYPE>().remove(this);
+        // We should no longer need our LockStatic -- but the fact that we can
+        // query or even resurrect a deleted LLSingleton means we basically
+        // have to shrug and leak our SingletonData. It's not large, and this
+        // only happens at shutdown anyway.
+//      lk.cleanup();
     }
 
 public:
diff --git a/indra/llcommon/lockstatic.h b/indra/llcommon/lockstatic.h
index 7cc9b7eec0..35160521a7 100644
--- a/indra/llcommon/lockstatic.h
+++ b/indra/llcommon/lockstatic.h
@@ -14,6 +14,8 @@
 #define LL_LOCKSTATIC_H
 
 #include "mutex.h"                  // std::unique_lock
+#include "llerror.h"
+#include "llexception.h"
 
 namespace llthread
 {
@@ -26,9 +28,15 @@ class LockStatic
 {
     typedef std::unique_lock<decltype(Static::mMutex)> lock_t;
 public:
+    // trying to lock Static after cleanup() has been called
+    struct Dead: public LLException
+    {
+        Dead(const std::string& what): LLException(what) {}
+    };
+
     LockStatic():
         mData(getStatic()),
-        mLock(mData->mMutex)
+        mLock(getLock(mData))
     {}
     Static* get() const { return mData; }
     operator Static*() const { return get(); }
@@ -40,31 +48,70 @@ public:
         mData = nullptr;
         mLock.unlock();
     }
+    // explicit destruction
+    // We used to store a static instance of Static in getStatic(). The
+    // trouble with that is that at some point during final termination
+    // cleanup, the compiler calls ~Static(), destroying the mutex. If some
+    // later static object's destructor tries to lock our Static, we blow up
+    // trying to lock a destroyed mutex object. This can happen, for instance,
+    // if some class's destructor tries to reference an LLSingleton.
+    // Since a plain dumb pointer has no destructor, the compiler leaves it
+    // alone, so the referenced heap Static instance can survive until we
+    // explicitly call this method.
+    void cleanup()
+    {
+        // certainly don't claim to lock after this point!
+        mData = nullptr;
+        Static*& ptrref{ getStatic() };
+        Static* ptr{ ptrref };
+        ptrref = nullptr;
+        delete ptr;
+    }
 protected:
     Static* mData;
     lock_t mLock;
 private:
-    Static* getStatic()
+    static lock_t getLock(Static* data)
+    {
+        // data can be false if cleanup() has already been called. If so, no
+        // code in the caller is valid that depends on this instance. We dare
+        // to throw an exception because trying to lock Static after it's been
+        // deleted is not part of normal processing. There are callers who
+        // want to handle this exception, but it should indeed be treated as
+        // exceptional.
+        if (! data)
+        {
+            LLTHROW(Dead("LockStatic<" + LLError::Log::classname<LockStatic<Static>>() +
+                         "> called after cleanup()"));
+        }
+        // Usual case: data isn't nullptr, carry on.
+        return lock_t(data->mMutex);
+    }
+
+    Static*& getStatic()
     {
-        // Static::mMutex must be function-local static rather than class-
-        // static. Some of our consumers must function properly (therefore
-        // lock properly) even when the containing module's static variables
-        // have not yet been runtime-initialized. A mutex requires
+        // Our Static instance must be function-local static rather than
+        // class-static. Some of our consumers must function properly
+        // (therefore lock properly) even when the containing module's static
+        // variables have not yet been runtime-initialized. A mutex requires
         // construction. A static class member might not yet have been
         // constructed.
         //
-        // We could store a dumb mutex_t*, notice when it's NULL and allocate a
-        // heap mutex -- but that's vulnerable to race conditions. And we can't
-        // defend the dumb pointer with another mutex.
+        // We could store a dumb mutex_t* class member, notice when it's NULL
+        // and allocate a heap mutex -- but that's vulnerable to race
+        // conditions. And we can't defend the dumb pointer with another
+        // mutex.
         //
         // We could store a std::atomic<mutex_t*> -- but a default-constructed
         // std::atomic<T> does not contain a valid T, even a default-constructed
         // T! Which means std::atomic, too, requires runtime initialization.
         //
         // But a function-local static is guaranteed to be initialized exactly
-        // once: the first time control reaches that declaration.
-        static Static sData;
-        return &sData;
+        // once: the first time control reaches that declaration. Importantly,
+        // since a plain dumb pointer has no destructor, the compiler lets our
+        // heap Static instance survive until someone calls cleanup() (above).
+        static Static* sData{ new Static };
+        return sData;
     }
 };