SL-13979 Crash of logging system at LLError::Settings::getInstance()

LLSingleton depends onto logging system, having logging system be based on LLSingleton causes crashes and deadlocks

4 files changed, 42 insertions, 84 deletions

diff --git a/indra/llcommon/llerror.cpp b/indra/llcommon/llerror.cpp
index 411412c883..f876b8ee4a 100644
--- a/indra/llcommon/llerror.cpp
+++ b/indra/llcommon/llerror.cpp
@@ -332,12 +332,9 @@ namespace LLError
 		}
 		// huh, that's odd, we should see one or the other prefix -- but don't
 		// try to log unless logging is already initialized
-		if (is_available())
-		{
-			// in Python, " or ".join(vector) -- but in C++, a PITB
-			LL_DEBUGS() << "Did not see 'class' or 'struct' prefix on '"
-				<< name << "'" << LL_ENDL;
-		}
+		// in Python, " or ".join(vector) -- but in C++, a PITB
+		LL_DEBUGS() << "Did not see 'class' or 'struct' prefix on '"
+			<< name << "'" << LL_ENDL;
 		return name;
 
 #else  // neither GCC nor Visual Studio
@@ -438,9 +435,12 @@ namespace
 	typedef std::vector<LLError::RecorderPtr> Recorders;
 	typedef std::vector<LLError::CallSite*> CallSiteVector;
 
-	class Globals : public LLSingleton<Globals>
+	class Globals
 	{
-		LLSINGLETON(Globals);
+    public:
+        static Globals* getInstance();
+    protected:
+		Globals();
 	public:
 		std::ostringstream messageStream;
 		bool messageStreamInUse;
@@ -460,6 +460,16 @@ namespace
 	{
 	}
 
+    Globals* Globals::getInstance()
+    {
+        // According to C++11 Function-Local Initialization
+        // of static variables is supposed to be thread safe
+        // without risk of deadlocks.
+        static Globals inst;
+
+        return &inst;
+    }
+
 	void Globals::addCallSite(LLError::CallSite& site)
 	{
 		callSites.push_back(&site);
@@ -512,14 +522,17 @@ namespace LLError
 
 	typedef LLPointer<SettingsConfig> SettingsConfigPtr;
 
-	class Settings : public LLSingleton<Settings>
+	class Settings
 	{
-		LLSINGLETON(Settings);
+    public:
+        static Settings* getInstance();
+    protected:
+		Settings();
 	public:
 		SettingsConfigPtr getSettingsConfig();
 
 		void reset();
-		SettingsStoragePtr saveAndReset();
+		SettingsStoragePtr saveAndReset(); 
 		void restore(SettingsStoragePtr pSettingsStorage);
 		
 	private:
@@ -553,6 +566,16 @@ namespace LLError
 	{
 	}
 
+    Settings* Settings::getInstance()
+    {
+        // According to C++11 Function-Local Initialization
+        // of static variables is supposed to be thread safe
+        // without risk of deadlocks.
+        static Settings inst;
+
+        return &inst;
+    }
+
 	SettingsConfigPtr Settings::getSettingsConfig()
 	{
 		return mSettingsConfig;
@@ -577,11 +600,6 @@ namespace LLError
 		SettingsConfigPtr newSettingsConfig(dynamic_cast<SettingsConfig *>(pSettingsStorage.get()));
 		mSettingsConfig = newSettingsConfig;
 	}
-
-	bool is_available()
-	{
-		return Settings::instanceExists() && Globals::instanceExists();
-	}
 }
 
 namespace LLError
@@ -1028,7 +1046,7 @@ namespace LLError
     std::pair<boost::shared_ptr<RECORDER>, Recorders::iterator>
     findRecorderPos()
     {
-        SettingsConfigPtr s = Settings::instance().getSettingsConfig();
+        SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig();
         // Since we promise to return an iterator, use a classic iterator
         // loop.
         auto end{s->mRecorders.end()};
@@ -1071,7 +1089,7 @@ namespace LLError
         auto found = findRecorderPos<RECORDER>();
         if (found.first)
         {
-            SettingsConfigPtr s = Settings::instance().getSettingsConfig();
+            SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig();
             s->mRecorders.erase(found.second);
         }
         return bool(found.first);
@@ -1307,14 +1325,6 @@ namespace LLError
 			return false;
 		}
 
-		// If we hit a logging request very late during shutdown processing,
-		// when either of the relevant LLSingletons has already been deleted,
-		// DO NOT resurrect them.
-		if (Settings::wasDeleted() || Globals::wasDeleted())
-		{
-			return false;
-		}
-
 		SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig();
 		
 		s->mShouldLogCallCounter++;
@@ -1353,10 +1363,8 @@ namespace LLError
 	std::ostringstream* Log::out()
 	{
 		LLMutexTrylock lock(getMutex<LOG_MUTEX>(),5);
-		// If we hit a logging request very late during shutdown processing,
-		// when either of the relevant LLSingletons has already been deleted,
-		// DO NOT resurrect them.
-		if (lock.isLocked() && ! (Settings::wasDeleted() || Globals::wasDeleted()))
+
+		if (lock.isLocked())
 		{
 			Globals* g = Globals::getInstance();
 
@@ -1378,14 +1386,6 @@ namespace LLError
 			return;
 		}
 
-		// If we hit a logging request very late during shutdown processing,
-		// when either of the relevant LLSingletons has already been deleted,
-		// DO NOT resurrect them.
-		if (Settings::wasDeleted() || Globals::wasDeleted())
-		{
-			return;
-		}
-
 		if(strlen(out->str().c_str()) < 128)
 		{
 			strcpy(message, out->str().c_str());
@@ -1418,14 +1418,6 @@ namespace LLError
 			return;
 		}
 
-		// If we hit a logging request very late during shutdown processing,
-		// when either of the relevant LLSingletons has already been deleted,
-		// DO NOT resurrect them.
-		if (Settings::wasDeleted() || Globals::wasDeleted())
-		{
-			return;
-		}
-
 		Globals* g = Globals::getInstance();
 		SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig();
 
diff --git a/indra/llcommon/llerrorcontrol.h b/indra/llcommon/llerrorcontrol.h
index bfa2269025..25786d5457 100644
--- a/indra/llcommon/llerrorcontrol.h
+++ b/indra/llcommon/llerrorcontrol.h
@@ -203,11 +203,6 @@ namespace LLError
 
 	LL_COMMON_API std::string abbreviateFile(const std::string& filePath);
 	LL_COMMON_API int shouldLogCallCount();
-
-	// Check whether Globals exists. This should only be used by LLSingleton
-	// infrastructure to avoid trying to log when our internal LLSingleton is
-	// unavailable -- circularity ensues.
-	LL_COMMON_API bool is_available();
 };
 
 #endif // LL_LLERRORCONTROL_H
diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp
index d3d25201b2..83a4b64e8f 100644
--- a/indra/llcommon/llsingleton.cpp
+++ b/indra/llcommon/llsingleton.cpp
@@ -28,7 +28,7 @@
 #include "llsingleton.h"
 
 #include "llerror.h"
-#include "llerrorcontrol.h"         // LLError::is_available()
+#include "llerrorcontrol.h"
 #include "lldependencies.h"
 #include "llexception.h"
 #include "llcoros.h"
@@ -41,8 +41,6 @@
 namespace {
 void log(LLError::ELevel level,
          const char* p1, const char* p2, const char* p3, const char* p4);
-
-bool oktolog();
 } // anonymous namespace
 
 // Our master list of all LLSingletons is itself an LLSingleton. We used to
@@ -279,8 +277,6 @@ void LLSingletonBase::reset_initializing(list_t::size_type size)
 
 void LLSingletonBase::MasterList::LockedInitializing::log(const char* verb, const char* name)
 {
-    if (oktolog())
-    {
         LL_DEBUGS("LLSingleton") << verb << ' ' << demangle(name) << ';';
         if (mList)
         {
@@ -292,7 +288,6 @@ void LLSingletonBase::MasterList::LockedInitializing::log(const char* verb, cons
             }
         }
         LL_ENDL;
-    }
 }
 
 void LLSingletonBase::capture_dependency()
@@ -455,33 +450,11 @@ void LLSingletonBase::deleteAll()
 
 /*---------------------------- Logging helpers -----------------------------*/
 namespace {
-bool oktolog()
-{
-    // See comments in log() below.
-    return LLError::is_available();
-}
 
 void log(LLError::ELevel level,
          const char* p1, const char* p2, const char* p3, const char* p4)
 {
-    // The is_available() test below ensures that we'll stop logging once
-    // LLError has been cleaned up. If we had a similar portable test for
-    // std::cerr, this would be a good place to use it.
-
-    // Check LLError::is_available() because some of LLError's infrastructure
-    // is itself an LLSingleton. If that LLSingleton has not yet been
-    // initialized, trying to log will engage LLSingleton machinery... and
-    // around and around we go.
-    if (LLError::is_available())
-    {
-        LL_VLOGS(level, "LLSingleton") << p1 << p2 << p3 << p4 << LL_ENDL;
-    }
-    else
-    {
-        // Caller may be a test program, or something else whose stderr is
-        // visible to the user.
-        std::cerr << p1 << p2 << p3 << p4 << std::endl;
-    }
+    LL_VLOGS(level, "LLSingleton") << p1 << p2 << p3 << p4 << LL_ENDL;
 }
 
 } // anonymous namespace        
diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp
index 70b41a0a5f..f58d49ec0a 100644
--- a/indra/newview/llappviewer.cpp
+++ b/indra/newview/llappviewer.cpp
@@ -2109,14 +2109,12 @@ bool LLAppViewer::cleanup()
 	// still see above are calls that MUST happen before the generic cleanup
 	// kicks in.
 
-	// The logging subsystem depends on an LLSingleton. Any logging after
-	// LLSingletonBase::deleteAll() won't be recorded.
-	LL_INFOS() << "Goodbye!" << LL_ENDL;
-
 	// This calls every remaining LLSingleton's cleanupSingleton() and
 	// deleteSingleton() methods.
 	LLSingletonBase::deleteAll();
 
+    LL_INFOS() << "Goodbye!" << LL_ENDL;
+
 	removeDumpDir();
 
 	// return 0;