DRTVWR-494: Move most LLSingleton cleanup back to destructor

instead of deleteSingleton().

Specifically, clear static SingletonData and remove the instance from the
MasterList in the destructor.

Empirically, some consumers are manually deleting LLSingleton instances,
instead of calling deleteSingleton(). If deleteSingleton() handles cleanup
rather than the destructor, we're left with dangling pointers in the Master
List.

We don't also call cleanupSingleton() from the destructor because only
deleteSingleton() promises to call cleanupSingleton(). Hopefully whoever is
directly deleting an LLSingleton subclass instance isn't relying on
cleanupSingleton().

2 files changed, 26 insertions, 24 deletions

diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp
index bf594f122c..4c76206d8d 100644
--- a/indra/llcommon/llsingleton.cpp
+++ b/indra/llcommon/llsingleton.cpp
@@ -384,7 +384,7 @@ LLSingletonBase::vec_t LLSingletonBase::dep_sort()
     SingletonDeps sdeps;
     // Lock while traversing the master list 
     MasterList::LockedMaster master;
-    BOOST_FOREACH(LLSingletonBase* sp, master.get())
+    for (LLSingletonBase* sp : master.get())
     {
         // Build the SingletonDeps structure by adding, for each
         // LLSingletonBase* sp in the master list, sp itself. It has no
@@ -401,14 +401,14 @@ LLSingletonBase::vec_t LLSingletonBase::dep_sort()
     // extracts just the first (key) element from each sorted_iterator, then
     // uses vec_t's range constructor... but frankly this is more
     // straightforward, as long as we remember the above reserve() call!
-    BOOST_FOREACH(SingletonDeps::sorted_iterator::value_type pair, sdeps.sort())
+    for (const SingletonDeps::sorted_iterator::value_type& pair : sdeps.sort())
     {
         ret.push_back(pair.first);
     }
     // The master list is not itself pushed onto the master list. Add it as
     // the very last entry -- it is the LLSingleton on which ALL others
     // depend! -- so our caller will process it.
-    ret.push_back(MasterList::getInstance());
+    ret.push_back(&master.Lock::get());
     return ret;
 }
 
diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h
index 0c11e54910..5ee40a658a 100644
--- a/indra/llcommon/llsingleton.h
+++ b/indra/llcommon/llsingleton.h
@@ -428,6 +428,21 @@ protected:
         LLSingleton_manage_master<DERIVED_TYPE>().add(this);
     }
 
+protected:
+    virtual ~LLSingleton()
+    {
+        // This phase of cleanup is performed in the destructor rather than in
+        // deleteSingleton() to defend against manual deletion. When we moved
+        // cleanup to deleteSingleton(), we hit crashes due to dangling
+        // pointers in the MasterList.
+        LockStatic lk;
+        lk->mInstance  = nullptr;
+        lk->mInitState = DELETED;
+
+        // Remove this instance from the master list.
+        LLSingleton_manage_master<DERIVED_TYPE>().remove(this);
+    }
+
 public:
     /**
      * @brief Immediately delete the singleton.
@@ -452,29 +467,16 @@ public:
      */
     static void deleteSingleton()
     {
-        DERIVED_TYPE* lameduck;
-        {
-            LockStatic lk;
-            // Capture the instance and clear SingletonData. This sequence
-            // guards against the chance that the destructor throws, somebody
-            // catches it and there's a subsequent call to getInstance().
-            lameduck = lk->mInstance;
-            lk->mInstance = nullptr;
-            lk->mInitState = DELETED;
-            // At this point we can safely unlock SingletonData during the
-            // remaining cleanup. If another thread calls deleteSingleton() (or
-            // getInstance(), or whatever) it won't find our instance, now
-            // referenced only as 'lameduck'.
-        }
+        // Hold the lock while we call cleanupSingleton() and the destructor.
+        // Our destructor also instantiates LockStatic, requiring a recursive
+        // mutex.
+        LockStatic lk;
         // of course, only cleanup and delete if there's something there
-        if (lameduck)
+        if (lk->mInstance)
         {
-            // remove this instance from the master list BEFORE attempting
-            // cleanup so possible destructor exception won't leave the master
-            // list confused
-            LLSingleton_manage_master<DERIVED_TYPE>().remove(lameduck);
-            lameduck->cleanup_();
-            delete lameduck;
+            lk->mInstance->cleanup_();
+            delete lk->mInstance;
+            // destructor clears mInstance (and mInitState)
         }
     }