SL-10297: Modify LL_ERRS and other deliberate crashes to avoid a common stack frame

12 files changed, 70 insertions, 194 deletions

diff --git a/BuildParams b/BuildParams
index c5f96d5ee3..c5f96d5ee3 100755..100644
--- a/BuildParams
+++ b/BuildParams
diff --git a/indra/llcommon/llerror.cpp b/indra/llcommon/llerror.cpp
index 335a0995fe..0ddce353f1 100644
--- a/indra/llcommon/llerror.cpp
+++ b/indra/llcommon/llerror.cpp
@@ -683,7 +683,6 @@ namespace
 		LLError::setDefaultLevel(LLError::LEVEL_INFO);
         LLError::setAlwaysFlush(true);
         LLError::setEnabledLogTypesMask(0xFFFFFFFF);
-		LLError::setFatalFunction(LLError::crashAndLoop);
 		LLError::setTimeFunction(LLError::utcTime);
 
 		// log_to_stderr is only false in the unit and integration tests to keep builds quieter
@@ -719,16 +718,16 @@ namespace LLError
 		commonInit(user_dir, app_dir, log_to_stderr);
 	}
 
-	void setFatalFunction(const FatalFunction& f)
+	void overrideCrashOnError(const FatalFunction& fatal_function)
 	{
 		SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig();
-		s->mCrashFunction = f;
+		s->mCrashFunction = fatal_function;
 	}
 
-	FatalFunction getFatalFunction()
+	void restoreCrashOnError()
 	{
 		SettingsConfigPtr s = Settings::getInstance()->getSettingsConfig();
-		return s->mCrashFunction;
+		s->mCrashFunction = NULL;
 	}
 
 	std::string getFatalMessage()
@@ -1306,12 +1305,12 @@ namespace LLError
 		return ;
 	}
 
-	void Log::flush(std::ostringstream* out, const CallSite& site)
+	bool Log::flush(std::ostringstream* out, const CallSite& site)
 	{
 		LLMutexTrylock lock(&gLogMutex,5);
 		if (!lock.isLocked())
 		{
-			return;
+			return true;
 		}
 
 		// If we hit a logging request very late during shutdown processing,
@@ -1319,7 +1318,7 @@ namespace LLError
 		// DO NOT resurrect them.
 		if (Settings::wasDeleted() || Globals::wasDeleted())
 		{
-			return;
+			return true;
 		}
 
 		Globals* g = Globals::getInstance();
@@ -1353,7 +1352,7 @@ namespace LLError
 				} 
 				else
 				{
-					return;
+					return true;
 				}
 			}
 			else 
@@ -1370,11 +1369,14 @@ namespace LLError
 		if (site.mLevel == LEVEL_ERROR)
 		{
 			g->mFatalMessage = message;
-			if (s->mCrashFunction)
-			{
-				s->mCrashFunction(message);
-			}
+            if (s->mCrashFunction)
+            {
+                s->mCrashFunction(message);
+                return false;
+            }
 		}
+
+        return true;
 	}
 }
 
@@ -1437,29 +1439,6 @@ namespace LLError
 		return s->mShouldLogCallCounter;
 	}
 
-#if LL_WINDOWS
-		// VC80 was optimizing the error away.
-		#pragma optimize("", off)
-#endif
-	void crashAndLoop(const std::string& message)
-	{
-		// Now, we go kaboom!
-		int* make_me_crash = NULL;
-
-		*make_me_crash = 0;
-
-		while(true)
-		{
-			// Loop forever, in case the crash didn't work?
-		}
-		
-		// this is an attempt to let Coverity and other semantic scanners know that this function won't be returning ever.
-		exit(EXIT_FAILURE);
-	}
-#if LL_WINDOWS
-		#pragma optimize("", on)
-#endif
-
 	std::string utcTime()
 	{
 		time_t now = time(NULL);
diff --git a/indra/llcommon/llerror.h b/indra/llcommon/llerror.h
index 0a78229555..cbade88f61 100644
--- a/indra/llcommon/llerror.h
+++ b/indra/llcommon/llerror.h
@@ -199,8 +199,12 @@ namespace LLError
 	public:
 		static bool shouldLog(CallSite&);
 		static std::ostringstream* out();
+
 		static void flush(std::ostringstream* out, char* message);
-		static void flush(std::ostringstream*, const CallSite&);
+
+        // returns false iff there is a fatal crash override in effect
+		static bool flush(std::ostringstream*, const CallSite&);
+
 		static std::string demangle(const char* mangled);
 	};
 	
@@ -367,10 +371,20 @@ typedef LLError::NoClassInfo _LL_CLASS_TO_LOG;
 
 #define LL_NEWLINE '\n'
 
-#define LL_ENDL                               \
-			LLError::End();                   \
-			LLError::Log::flush(_out, _site); \
-		}                                     \
+// Use this only in LL_ERRS or in a place that LL_ERRS may not be used
+#define LLERROR_CRASH         \
+{                             \
+    int* make_me_crash = NULL;\
+    *make_me_crash = 0;       \
+    exit(*make_me_crash);     \
+}
+
+#define LL_ENDL                                          \
+			LLError::End();                              \
+            if (LLError::Log::flush(_out, _site)         \
+                && _site.mLevel == LLError::LEVEL_ERROR) \
+                LLERROR_CRASH                            \
+        }                                                \
 	} while(0)
 
 // NEW Macros for debugging, allow the passing of a string tag
diff --git a/indra/llcommon/llerrorcontrol.h b/indra/llcommon/llerrorcontrol.h
index 276d22fc36..7ca6ddb737 100644
--- a/indra/llcommon/llerrorcontrol.h
+++ b/indra/llcommon/llerrorcontrol.h
@@ -92,41 +92,13 @@ namespace LLError
 	/*
 		Control functions.
 	*/
-
 	typedef boost::function<void(const std::string&)> FatalFunction;
-	LL_COMMON_API void crashAndLoop(const std::string& message);
-		// Default fatal function: access null pointer and loops forever
-
-	LL_COMMON_API void setFatalFunction(const FatalFunction&);
-		// The fatal function will be called when an message of LEVEL_ERROR
-		// is logged.  Note: supressing a LEVEL_ERROR message from being logged
-		// (by, for example, setting a class level to LEVEL_NONE), will keep
-		// the that message from causing the fatal funciton to be invoked.
-
-	LL_COMMON_API FatalFunction getFatalFunction();
-		// Retrieve the previously-set FatalFunction
 
+    LL_COMMON_API void overrideCrashOnError(const FatalFunction&);
+    LL_COMMON_API void restoreCrashOnError();
+    
 	LL_COMMON_API std::string getFatalMessage();
-		// Retrieve the message last passed to FatalFunction, if any
-
-	/// temporarily override the FatalFunction for the duration of a
-	/// particular scope, e.g. for unit tests
-	class LL_COMMON_API OverrideFatalFunction
-	{
-	public:
-		OverrideFatalFunction(const FatalFunction& func):
-			mPrev(getFatalFunction())
-		{
-			setFatalFunction(func);
-		}
-		~OverrideFatalFunction()
-		{
-			setFatalFunction(mPrev);
-		}
-
-	private:
-		FatalFunction mPrev;
-	};
+		// Retrieve the message last passed to LL_ERRS, if any
 
 	typedef std::string (*TimeFunction)();
 	LL_COMMON_API std::string utcTime();
diff --git a/indra/llcommon/llleap.cpp b/indra/llcommon/llleap.cpp
index cf8f8cc6a5..f7bfa36bb5 100644
--- a/indra/llcommon/llleap.cpp
+++ b/indra/llcommon/llleap.cpp
@@ -59,7 +59,6 @@ public:
         // pump name -- so it should NOT need tweaking for uniqueness.
         mReplyPump(LLUUID::generateNewID().asString()),
         mExpect(0),
-        mPrevFatalFunction(LLError::getFatalFunction()),
         // Instantiate a distinct LLLeapListener for this plugin. (Every
         // plugin will want its own collection of managed listeners, etc.)
         // Pass it a callback to our connect() method, so it can send events
@@ -146,7 +145,7 @@ public:
             .listen("LLLeap", boost::bind(&LLLeapImpl::rstderr, this, _1));
 
         // For our lifespan, intercept any LL_ERRS so we can notify plugin
-        LLError::setFatalFunction(boost::bind(&LLLeapImpl::fatalFunction, this, _1));
+        LLError::overrideCrashOnError(boost::bind(&LLLeapImpl::fatalFunction, this, _1));
 
         // Send child a preliminary event reporting our own reply-pump name --
         // which would otherwise be pretty tricky to guess!
@@ -162,8 +161,8 @@ public:
     virtual ~LLLeapImpl()
     {
         LL_DEBUGS("LLLeap") << "destroying LLLeap(\"" << mDesc << "\")" << LL_ENDL;
-        // Restore original FatalFunction
-        LLError::setFatalFunction(mPrevFatalFunction);
+        // Restore original fatal crash behavior for LL_ERRS
+        LLError::restoreCrashOnError();
     }
 
     // Listener for failed launch attempt
@@ -397,8 +396,8 @@ public:
             mainloop.post(nop);
         }
 
-        // forward the call to the previous FatalFunction
-        mPrevFatalFunction(error);
+        // go ahead and do the crash that LLError would have done
+        LLERROR_CRASH
     }
 
 private:
@@ -421,7 +420,6 @@ private:
         mStdinConnection, mStdoutConnection, mStdoutDataConnection, mStderrConnection;
     boost::scoped_ptr<LLEventPump::Blocker> mBlocker;
     LLProcess::ReadPipe::size_type mExpect;
-    LLError::FatalFunction mPrevFatalFunction;
     boost::scoped_ptr<LLLeapListener> mListener;
 };
 
diff --git a/indra/llcommon/llsingleton.cpp b/indra/llcommon/llsingleton.cpp
index c45c144570..6f254ef670 100644
--- a/indra/llcommon/llsingleton.cpp
+++ b/indra/llcommon/llsingleton.cpp
@@ -461,15 +461,7 @@ void LLSingletonBase::logerrs(const char* p1, const char* p2, const char* p3, co
     // https://www.youtube.com/watch?v=OMG7paGJqhQ (emphasis on OMG)
     std::ostringstream out;
     out << p1 << p2 << p3 << p4;
-    auto crash = LLError::getFatalFunction();
-    if (crash)
-    {
-        crash(out.str());
-    }
-    else
-    {
-        LLError::crashAndLoop(out.str());
-    }
+    LLERROR_CRASH;
 }
 
 std::string LLSingletonBase::demangle(const char* mangled)
diff --git a/indra/llcommon/tests/llerror_test.cpp b/indra/llcommon/tests/llerror_test.cpp
index 8e1f4c14ac..2f8923d2de 100644
--- a/indra/llcommon/tests/llerror_test.cpp
+++ b/indra/llcommon/tests/llerror_test.cpp
@@ -120,7 +120,7 @@ namespace tut
 
 			mPriorErrorSettings = LLError::saveAndResetSettings();
 			LLError::setDefaultLevel(LLError::LEVEL_DEBUG);
-			LLError::setFatalFunction(fatalCall);
+			LLError::overrideCrashOnError(fatalCall);
 			LLError::addRecorder(mRecorder);
 		}
 
diff --git a/indra/llcommon/tests/wrapllerrs.h b/indra/llcommon/tests/wrapllerrs.h
index b07d5afbd8..b280271476 100644
--- a/indra/llcommon/tests/wrapllerrs.h
+++ b/indra/llcommon/tests/wrapllerrs.h
@@ -54,17 +54,15 @@ struct WrapLLErrs
         // Resetting Settings discards the default Recorder that writes to
         // stderr. Otherwise, expected llerrs (LL_ERRS) messages clutter the
         // console output of successful tests, potentially confusing things.
-        mPriorErrorSettings(LLError::saveAndResetSettings()),
-        // Save shutdown function called by LL_ERRS
-        mPriorFatal(LLError::getFatalFunction())
+        mPriorErrorSettings(LLError::saveAndResetSettings())
     {
         // Make LL_ERRS call our own operator() method
-        LLError::setFatalFunction(boost::bind(&WrapLLErrs::operator(), this, _1));
+        LLError::overrideCrashOnError(boost::bind(&WrapLLErrs::operator(), this, _1));
     }
 
     ~WrapLLErrs()
     {
-        LLError::setFatalFunction(mPriorFatal);
+        LLError::restoreCrashOnError();
         LLError::restoreSettings(mPriorErrorSettings);
     }
 
@@ -203,7 +201,7 @@ public:
         mOldSettings(LLError::saveAndResetSettings()),
 		mRecorder(new CaptureLogRecorder())
     {
-        LLError::setFatalFunction(wouldHaveCrashed);
+        LLError::overrideCrashOnError(wouldHaveCrashed);
         LLError::setDefaultLevel(level);
         LLError::addRecorder(mRecorder);
     }
diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp
index a33e978c0a..e6ba3e4f7c 100644
--- a/indra/newview/llappviewer.cpp
+++ b/indra/newview/llappviewer.cpp
@@ -751,17 +751,6 @@ public:
 	}
 };
 
-namespace {
-// With Xcode 6, _exit() is too magical to use with boost::bind(), so provide
-// this little helper function.
-void fast_exit(int rc)
-{
-	_exit(rc);
-}
-
-
-}
-
 
 bool LLAppViewer::init()
 {
@@ -801,19 +790,6 @@ bool LLAppViewer::init()
 	initMaxHeapSize() ;
 	LLCoros::instance().setStackSize(gSavedSettings.getS32("CoroutineStackSize"));
 
-
-	// Although initLoggingAndGetLastDuration() is the right place to mess with
-	// setFatalFunction(), we can't query gSavedSettings until after
-	// initConfiguration().
-	S32 rc(gSavedSettings.getS32("QAModeTermCode"));
-	if (rc >= 0)
-	{
-		// QAModeTermCode set, terminate with that rc on LL_ERRS. Use
-		// fast_exit() rather than exit() because normal cleanup depends too
-		// much on successful startup!
-		LLError::setFatalFunction(boost::bind(fast_exit, rc));
-	}
-
     mAlloc.setProfilingEnabled(gSavedSettings.getBOOL("MemProfiling"));
 
 	// Initialize the non-LLCurl libcurl library.  Should be called
@@ -2120,28 +2096,9 @@ bool LLAppViewer::cleanup()
 	return true;
 }
 
-// A callback for LL_ERRS() to call during the watchdog error.
-void watchdog_llerrs_callback(const std::string &error_string)
-{
-	gLLErrorActivated = true;
-
 	gDebugInfo["FatalMessage"] = error_string;
 	LLAppViewer::instance()->writeDebugInfo();
 
-#ifdef LL_WINDOWS
-	RaiseException(0,0,0,0);
-#else
-	raise(SIGQUIT);
-#endif
-}
-
-// A callback for the watchdog to call.
-void watchdog_killer_callback()
-{
-	LLError::setFatalFunction(watchdog_llerrs_callback);
-	LL_ERRS() << "Watchdog killer event" << LL_ENDL;
-}
-
 bool LLAppViewer::initThreads()
 {
 	static const bool enable_threads = true;
@@ -2176,24 +2133,6 @@ bool LLAppViewer::initThreads()
 	return true;
 }
 
-void errorCallback(const std::string &error_string)
-{
-#ifndef LL_RELEASE_FOR_DOWNLOAD
-	OSMessageBox(error_string, LLTrans::getString("MBFatalError"), OSMB_OK);
-#endif
-
-	//Set the ErrorActivated global so we know to create a marker file
-	gLLErrorActivated = true;
-
-	gDebugInfo["FatalMessage"] = error_string;
-	// We're not already crashing -- we simply *intend* to crash. Since we
-	// haven't actually trashed anything yet, we can afford to write the whole
-	// static info file.
-	LLAppViewer::instance()->writeDebugInfo();
-
-	LLError::crashAndLoop(error_string);
-}
-
 void LLAppViewer::initLoggingAndGetLastDuration()
 {
 	//
@@ -2202,8 +2141,6 @@ void LLAppViewer::initLoggingAndGetLastDuration()
 	LLError::initForApplication( gDirUtilp->getExpandedFilename(LL_PATH_USER_SETTINGS, "")
                                 ,gDirUtilp->getExpandedFilename(LL_PATH_APP_SETTINGS, "")
                                 );
-	LLError::setFatalFunction(errorCallback);
-	//LLError::setTimeFunction(getRuntime);
 
 	// Remove the last ".old" log file.
 	std::string old_log_file = gDirUtilp->getExpandedFilename(LL_PATH_LOGS,
@@ -2952,6 +2889,7 @@ bool LLAppViewer::initWindow()
 	// Need to load feature table before cheking to start watchdog.
 	bool use_watchdog = false;
 	int watchdog_enabled_setting = gSavedSettings.getS32("WatchdogEnabled");
+
 	if (watchdog_enabled_setting == -1)
 	{
 		use_watchdog = !LLFeatureManager::getInstance()->isFeatureAvailable("WatchdogDisabled");
@@ -2962,11 +2900,16 @@ bool LLAppViewer::initWindow()
 		use_watchdog = bool(watchdog_enabled_setting);
 	}
 
+    LL_INFOS("AppInit") << "watchdog"
+                        << (use_watchdog ? " " : " NOT ")
+                        << "enabled"
+                        << " (setting = " << watchdog_enabled_setting << ")"
+                        << LL_ENDL;
+
 	if (use_watchdog)
 	{
-		LLWatchdog::getInstance()->init(watchdog_killer_callback);
+		LLWatchdog::getInstance()->init();
 	}
-	LL_INFOS("AppInit") << "watchdog setting is done." << LL_ENDL;
 
 	LLNotificationsUI::LLNotificationManager::getInstance();
 
@@ -3409,12 +3352,10 @@ void LLAppViewer::writeSystemInfo()
 	gDebugInfo["MainloopThreadID"] = (S32)thread_id;
 #endif
 
-	// "CrashNotHandled" is set here, while things are running well,
-	// in case of a freeze. If there is a freeze, the crash logger will be launched
-	// and can read this value from the debug_info.log.
-	// If the crash is handled by LLAppViewer::handleViewerCrash, ie not a freeze,
-	// then the value of "CrashNotHandled" will be set to true.
-	gDebugInfo["CrashNotHandled"] = (LLSD::Boolean)true;
+	// "CrashNotHandled" is obsolete; it used (not very successsfully)
+    // to try to distinguish crashes from freezes
+	gDebugInfo["CrashNotHandled"] = (LLSD::Boolean)false;
+    gDebugInfo["FatalMessage"] = LLError::getFatalMessage();
 
 	// Insert crash host url (url to post crash log to) if configured. This insures
 	// that the crash report will go to the proper location in the case of a
@@ -3567,10 +3508,6 @@ void LLAppViewer::handleViewerCrash()
 		gDebugInfo["Dynamic"]["MainloopTimeoutState"] = LLAppViewer::instance()->mMainloopTimeout->getState();
 	}
 
-	// The crash is being handled here so set this value to false.
-	// Otherwise the crash logger will think this crash was a freeze.
-	gDebugInfo["Dynamic"]["CrashNotHandled"] = (LLSD::Boolean)false;
-
 	//Write out the crash status file
 	//Use marker file style setup, as that's the simplest, especially since
 	//we're already in a crash situation
@@ -3642,6 +3579,8 @@ void LLAppViewer::handleViewerCrash()
 
 	if (LLWorld::instanceExists()) LLWorld::getInstance()->getInfo(gDebugInfo["Dynamic"]);
 
+	gDebugInfo["Dynamic"]["FatalMessage"] = LLError::getFatalMessage();
+
 	// Close the debug file
 	pApp->writeDebugInfo(false);  //false answers the isStatic question with the least overhead.
 }
diff --git a/indra/newview/llwatchdog.cpp b/indra/newview/llwatchdog.cpp
index dd6c77ca7d..2f3e5db84f 100644
--- a/indra/newview/llwatchdog.cpp
+++ b/indra/newview/llwatchdog.cpp
@@ -31,15 +31,6 @@
 
 const U32 WATCHDOG_SLEEP_TIME_USEC = 1000000;
 
-void default_killer_callback()
-{
-#ifdef LL_WINDOWS
-	RaiseException(0,0,0,0);
-#else
-	raise(SIGQUIT);
-#endif
-}
-
 // This class runs the watchdog timing thread.
 class LLWatchdogTimerThread : public LLThread
 {
@@ -157,8 +148,7 @@ void LLWatchdogTimeout::ping(const std::string& state)
 LLWatchdog::LLWatchdog() :
 	mSuspectsAccessMutex(),
 	mTimer(NULL),
-	mLastClockCount(0),
-	mKillerCallback(&default_killer_callback)
+	mLastClockCount(0)
 {
 }
 
@@ -180,9 +170,8 @@ void LLWatchdog::remove(LLWatchdogEntry* e)
 	unlockThread();
 }
 
-void LLWatchdog::init(killer_event_callback func)
+void LLWatchdog::init()
 {
-	mKillerCallback = func;
 	if(!mSuspectsAccessMutex && !mTimer)
 	{
 		mSuspectsAccessMutex = new LLMutex();
@@ -249,8 +238,7 @@ void LLWatchdog::run()
 				mTimer->stop();
 			}
 
-			LL_INFOS() << "Watchdog detected error:" << LL_ENDL;
-			mKillerCallback();
+            LL_ERRS() << "Watchdog timer expired; assuming viewer is hung and crashing" << LL_ENDL;
 		}
 	}
 
diff --git a/indra/newview/llwatchdog.h b/indra/newview/llwatchdog.h
index 9a6624258e..ce5cf748f4 100644
--- a/indra/newview/llwatchdog.h
+++ b/indra/newview/llwatchdog.h
@@ -83,9 +83,7 @@ public:
 	void add(LLWatchdogEntry* e);
 	void remove(LLWatchdogEntry* e);
 
-	typedef boost::function<void (void)> killer_event_callback;
-
-	void init(killer_event_callback func = NULL);
+	void init();
 	void run();
 	void cleanup();
     
@@ -98,8 +96,6 @@ private:
 	LLMutex* mSuspectsAccessMutex;
 	LLWatchdogTimerThread* mTimer;
 	U64 mLastClockCount;
-
-	killer_event_callback mKillerCallback;
 };
 
 #endif // LL_LLTHREADWATCHDOG_H
diff --git a/indra/test/test.cpp b/indra/test/test.cpp
index b14c2eb255..125de72b79 100644
--- a/indra/test/test.cpp
+++ b/indra/test/test.cpp
@@ -146,7 +146,7 @@ public:
 		mOldSettings(LLError::saveAndResetSettings()),
 		mRecorder(new RecordToTempFile(pool))
 	{
-		LLError::setFatalFunction(wouldHaveCrashed);
+		LLError::overrideCrashOnError(wouldHaveCrashed);
 		LLError::setDefaultLevel(level);
 		LLError::addRecorder(mRecorder);
 	}
@@ -508,7 +508,7 @@ void stream_groups(std::ostream& s, const char* app)
 
 void wouldHaveCrashed(const std::string& message)
 {
-	tut::fail("llerrs message: " + message);
+	tut::fail("fatal error message: " + message);
 }
 
 static LLTrace::ThreadRecorder* sMasterThreadRecorder = NULL;
@@ -532,7 +532,7 @@ int main(int argc, char **argv)
 		LLError::initForApplication(".", ".", false /* do not log to stderr */);
 		LLError::setDefaultLevel(LLError::LEVEL_DEBUG);
 	}	
-	LLError::setFatalFunction(wouldHaveCrashed);
+	LLError::overrideCrashOnError(wouldHaveCrashed);
 	std::string test_app_name(argv[0]);
 	std::string test_log = test_app_name + ".log";
 	LLFile::remove(test_log);