From d415e019a61cfd20bb1e254fbc9279f96047da85 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 24 Apr 2017 16:36:54 -0400 Subject: DRTVWR-418: Remove final shutdown cleanup as a cause of crashes. The recent LLSingleton work added a hook that would run during the C++ runtime's final destruction of static objects. When the LAST LLSingleton in any module was destroyed, its destructor would call LLSingletonBase::deleteAll(). That mechanism was intended to permit an application consuming LLSingletons to skip making an explicit deleteAll() call, knowing that all instantiated LLSingleton instances would eventually be cleaned up anyway. However -- experience proves that kicking off deleteAll() processing during the C++ runtime's final cleanup is too late. Too much has already been destroyed. That call tends to cause more shutdown crashes than it resolves. This commit deletes that whole mechanism. Going forward, if you want to clean up LLSingleton instances, you must explicitly call LLSingletonBase::deleteAll() during the application lifetime. If you don't, LLSingleton instances will simply be leaked -- which might be okay, considering the application is terminating anyway. --- indra/llcommon/llsingleton.cpp | 46 ++---------------------------------------- indra/llcommon/llsingleton.h | 30 ++++++++------------------- 2 files changed, 10 insertions(+), 66 deletions(-) (limited to 'indra') diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp index 9025e53bb2..18e1b96a5f 100644 --- a/indra/llcommon/llsingleton.cpp +++ b/indra/llcommon/llsingleton.cpp @@ -369,62 +369,20 @@ void LLSingletonBase::deleteAll() } } -/*------------------------ Final cleanup management ------------------------*/ -class LLSingletonBase::MasterRefcount -{ -public: - // store a POD int so it will be statically initialized to 0 - int refcount; -}; -static LLSingletonBase::MasterRefcount sMasterRefcount; - -LLSingletonBase::ref_ptr_t LLSingletonBase::get_master_refcount() -{ - // Calling this method constructs a new ref_ptr_t, which implicitly calls - // intrusive_ptr_add_ref(MasterRefcount*). - return &sMasterRefcount; -} - -void intrusive_ptr_add_ref(LLSingletonBase::MasterRefcount* mrc) -{ - // Count outstanding SingletonLifetimeManager instances. - ++mrc->refcount; -} - -void intrusive_ptr_release(LLSingletonBase::MasterRefcount* mrc) -{ - // Notice when each SingletonLifetimeManager instance is destroyed. - if (! --mrc->refcount) - { - // The last instance was destroyed. Time to kill any remaining - // LLSingletons -- but in dependency order. - LLSingletonBase::deleteAll(); - } -} - /*---------------------------- Logging helpers -----------------------------*/ namespace { bool oktolog() { // See comments in log() below. - return sMasterRefcount.refcount && LLError::is_available(); + return LLError::is_available(); } void log(LLError::ELevel level, const char* p1, const char* p2, const char* p3, const char* p4) { - // Check whether we're in the implicit final LLSingletonBase::deleteAll() - // call. We've carefully arranged for deleteAll() to be called when the - // last SingletonLifetimeManager instance is destroyed -- in other words, - // when the last translation unit containing an LLSingleton instance - // cleans up static data. That could happen after std::cerr is destroyed! // 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. As we do not, just - // don't log anything during implicit final deleteAll(). Detect that by - // the master refcount having gone to zero. - if (sMasterRefcount.refcount == 0) - return; + // 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 diff --git a/indra/llcommon/llsingleton.h b/indra/llcommon/llsingleton.h index 0d4a1f34f8..859e271e26 100644 --- a/indra/llcommon/llsingleton.h +++ b/indra/llcommon/llsingleton.h @@ -27,7 +27,6 @@ #include #include -#include #include #include #include @@ -36,8 +35,6 @@ class LLSingletonBase: private boost::noncopyable { public: class MasterList; - class MasterRefcount; - typedef boost::intrusive_ptr ref_ptr_t; private: // All existing LLSingleton instances are tracked in this master list. @@ -119,9 +116,6 @@ protected: const char* p3="", const char* p4=""); static std::string demangle(const char* mangled); - // obtain canonical ref_ptr_t - static ref_ptr_t get_master_refcount(); - // Default methods in case subclass doesn't declare them. virtual void initSingleton() {} virtual void cleanupSingleton() {} @@ -175,10 +169,6 @@ public: static void deleteAll(); }; -// support ref_ptr_t -void intrusive_ptr_add_ref(LLSingletonBase::MasterRefcount*); -void intrusive_ptr_release(LLSingletonBase::MasterRefcount*); - // Most of the time, we want LLSingleton_manage_master() to forward its // methods to real LLSingletonBase methods. template @@ -298,8 +288,7 @@ private: // stores pointer to singleton instance struct SingletonLifetimeManager { - SingletonLifetimeManager(): - mMasterRefcount(LLSingletonBase::get_master_refcount()) + SingletonLifetimeManager() { construct(); } @@ -317,17 +306,14 @@ private: // of static-object destruction, mean that we DO NOT WANT this // destructor to delete this LLSingleton. This destructor will run // without regard to any other LLSingleton whose cleanup might - // depend on its existence. What we really want is to count the - // runtime's attempts to cleanup LLSingleton static data -- and on - // the very last one, call LLSingletonBase::deleteAll(). That - // method will properly honor cross-LLSingleton dependencies. This - // is why we store an intrusive_ptr to a MasterRefcount: our - // ref_ptr_t member counts SingletonLifetimeManager instances. - // Once the runtime destroys the last of these, THEN we can delete - // every remaining LLSingleton. + // depend on its existence. If you want to clean up LLSingletons, + // call LLSingletonBase::deleteAll() sometime before static-object + // destruction begins. That method will properly honor cross- + // LLSingleton dependencies. Otherwise we simply leak LLSingleton + // instances at shutdown. Since the whole process is terminating + // anyway, that's not necessarily a bad thing; it depends on what + // resources your LLSingleton instances are managing. } - - LLSingletonBase::ref_ptr_t mMasterRefcount; }; protected: -- cgit v1.2.3 From 0d0f2a8aebd8b992764c227daac75c6964052645 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 27 Apr 2017 08:32:19 -0400 Subject: DRTVWR-418: Remove misleading comment -- no more implicit deleteAll(). The comment indicates that calling LLSingletonBase::deleteAll() is optional because the LLSingleton machinery implicitly calls that during final static-object cleanup. That is no longer true. --- indra/newview/llappviewer.cpp | 7 ------- 1 file changed, 7 deletions(-) (limited to 'indra') diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp index ae4e3df70f..27edb29681 100644 --- a/indra/newview/llappviewer.cpp +++ b/indra/newview/llappviewer.cpp @@ -2114,13 +2114,6 @@ bool LLAppViewer::cleanup() // This calls every remaining LLSingleton's deleteSingleton() method. // No class destructor should perform any cleanup that might take // significant realtime, or throw an exception. - // LLSingleton machinery includes a last-gasp implicit deleteAll() call, - // so this explicit call shouldn't strictly be necessary. However, by the - // time the runtime engages that implicit call, it may already have - // destroyed things like std::cerr -- so the implicit deleteAll() refrains - // from logging anything. Since both cleanupAll() and deleteAll() call - // their respective cleanup methods in computed dependency order, it's - // probably useful to be able to log that order. LLSingletonBase::deleteAll(); removeDumpDir(); -- cgit v1.2.3 From c04073da377a222d2d244c94683816c412dfad36 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 27 Apr 2017 08:40:01 -0400 Subject: DRTVWR-418: Use conventional LLSingleton init/cleanup for LLWinDebug. LLWinDebug, though an LLSingleton, had (and required explicit calls to) special init() and cleanup() methods. Kitty Barnett points out that the cleanup() method was actually being called after LLSingletonBase::deleteAll(), requiring resurrection of the deleted LLWinDebug, which sometimes led to crashes. (Resurrecting deleted LLSingletons is always suspect.) Change LLWinDebug::init() and cleanup() to the conventional initSingleton() and cleanupSingleton() methods. This eliminates the need to make special method calls at all. In particular, cleanupSingleton() will be called by the existing LLSingletonBase::cleanupAll() call near viewer shutdown. We retain the early LLWinDebug::instance() call, which implicitly initializes the LLWinDebug instance, because evidently we want that initialized early. But we no longer require a separate init() call. --- indra/newview/llappviewerwin32.cpp | 7 ++----- indra/newview/llwindebug.cpp | 4 ++-- indra/newview/llwindebug.h | 4 ++-- 3 files changed, 6 insertions(+), 9 deletions(-) (limited to 'indra') diff --git a/indra/newview/llappviewerwin32.cpp b/indra/newview/llappviewerwin32.cpp index 5107030476..d6039f6d7f 100644 --- a/indra/newview/llappviewerwin32.cpp +++ b/indra/newview/llappviewerwin32.cpp @@ -502,7 +502,8 @@ bool LLAppViewerWin32::init() disableWinErrorReporting(); #ifndef LL_RELEASE_FOR_DOWNLOAD - LLWinDebug::instance().init(); + // Merely requesting the LLSingleton instance initializes it. + LLWinDebug::instance(); #endif #if LL_WINDOWS @@ -526,10 +527,6 @@ bool LLAppViewerWin32::cleanup() gDXHardware.cleanup(); -#ifndef LL_RELEASE_FOR_DOWNLOAD - LLWinDebug::instance().cleanup(); -#endif - if (mIsConsoleAllocated) { FreeConsole(); diff --git a/indra/newview/llwindebug.cpp b/indra/newview/llwindebug.cpp index eff70ca0b2..92c80ce534 100644 --- a/indra/newview/llwindebug.cpp +++ b/indra/newview/llwindebug.cpp @@ -90,7 +90,7 @@ LONG NTAPI vectoredHandler(PEXCEPTION_POINTERS exception_infop) } // static -void LLWinDebug::init() +void LLWinDebug::initSingleton() { static bool s_first_run = true; // Load the dbghelp dll now, instead of waiting for the crash. @@ -135,7 +135,7 @@ void LLWinDebug::init() } } -void LLWinDebug::cleanup () +void LLWinDebug::cleanupSingleton() { gEmergencyMemoryReserve.release(); } diff --git a/indra/newview/llwindebug.h b/indra/newview/llwindebug.h index 90882cf04a..5a0d63f027 100644 --- a/indra/newview/llwindebug.h +++ b/indra/newview/llwindebug.h @@ -36,9 +36,9 @@ class LLWinDebug: { LLSINGLETON_EMPTY_CTOR(LLWinDebug); public: - static void init(); + static void initSingleton(); static void generateMinidump(struct _EXCEPTION_POINTERS *pExceptionInfo = NULL); - static void cleanup(); + static void cleanupSingleton(); private: static void writeDumpToFile(MINIDUMP_TYPE type, MINIDUMP_EXCEPTION_INFORMATION *ExInfop, const std::string& filename); }; -- cgit v1.2.3 From b94e2d0103168f143ee7579f01f0d45fc63a4e84 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 27 Apr 2017 09:02:37 -0400 Subject: DRTVWR-418: initSingleton(), cleanupSingleton() must be non-static. --- indra/newview/llwindebug.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'indra') diff --git a/indra/newview/llwindebug.h b/indra/newview/llwindebug.h index 5a0d63f027..7e5818ba1c 100644 --- a/indra/newview/llwindebug.h +++ b/indra/newview/llwindebug.h @@ -36,9 +36,9 @@ class LLWinDebug: { LLSINGLETON_EMPTY_CTOR(LLWinDebug); public: - static void initSingleton(); + void initSingleton(); static void generateMinidump(struct _EXCEPTION_POINTERS *pExceptionInfo = NULL); - static void cleanupSingleton(); + void cleanupSingleton(); private: static void writeDumpToFile(MINIDUMP_TYPE type, MINIDUMP_EXCEPTION_INFORMATION *ExInfop, const std::string& filename); }; -- cgit v1.2.3