EXT-5055 LLInstanceTracker promotes some dangerous patterns - detect them

5 files changed, 144 insertions, 89 deletions

diff --git a/indra/llcommon/lleventtimer.cpp b/indra/llcommon/lleventtimer.cpp
index b2fdf11197..d44e7ec1e6 100644
--- a/indra/llcommon/lleventtimer.cpp
+++ b/indra/llcommon/lleventtimer.cpp
@@ -42,7 +42,6 @@
 //		LLEventTimer Implementation
 //
 //////////////////////////////////////////////////////////////////////////////
-bool LLEventTimer::sInTickLoop = false;
 
 LLEventTimer::LLEventTimer(F32 period)
 : mEventTimer()
@@ -59,27 +58,28 @@ LLEventTimer::LLEventTimer(const LLDate& time)
 
 LLEventTimer::~LLEventTimer()
 {
-	llassert(!LLEventTimer::sInTickLoop); // this LLEventTimer was destroyed from within its own tick() function - bad.  if you want tick() to cause destruction of its own timer, make it return true.
 }
 
 //static
 void LLEventTimer::updateClass() 
 {
 	std::list<LLEventTimer*> completed_timers;
-	LLEventTimer::sInTickLoop = true;
-	for (instance_iter iter = beginInstances(); iter != endInstances(); ) 
+
 	{
-		LLEventTimer& timer = *iter++;
-		F32 et = timer.mEventTimer.getElapsedTimeF32();
-		if (timer.mEventTimer.getStarted() && et > timer.mPeriod) {
-			timer.mEventTimer.reset();
-			if ( timer.tick() )
-			{
-				completed_timers.push_back( &timer );
+		LLInstanceTrackerScopedGuard guard;
+		for (instance_iter iter = guard.beginInstances(); iter != guard.endInstances(); ) 
+		{
+			LLEventTimer& timer = *iter++;
+			F32 et = timer.mEventTimer.getElapsedTimeF32();
+			if (timer.mEventTimer.getStarted() && et > timer.mPeriod) {
+				timer.mEventTimer.reset();
+				if ( timer.tick() )
+				{
+					completed_timers.push_back( &timer );
+				}
 			}
 		}
 	}
-	LLEventTimer::sInTickLoop = false;
 
 	if ( completed_timers.size() > 0 )
 	{
diff --git a/indra/llcommon/lleventtimer.h b/indra/llcommon/lleventtimer.h
index f792138feb..5181cce52d 100644
--- a/indra/llcommon/lleventtimer.h
+++ b/indra/llcommon/lleventtimer.h
@@ -55,7 +55,6 @@ public:
 protected:
 	LLTimer mEventTimer;
 	F32 mPeriod;
-	static bool sInTickLoop;
 };
 
 #endif //LL_EVENTTIMER_H
diff --git a/indra/llcommon/llfasttimer_class.cpp b/indra/llcommon/llfasttimer_class.cpp
index 6d8d81e114..2e5edb1f3b 100644
--- a/indra/llcommon/llfasttimer_class.cpp
+++ b/indra/llcommon/llfasttimer_class.cpp
@@ -218,9 +218,10 @@ LLFastTimer::DeclareTimer::DeclareTimer(const std::string& name)
 // static
 void LLFastTimer::DeclareTimer::updateCachedPointers()
 {
+	DeclareTimer::LLInstanceTrackerScopedGuard guard;
 	// propagate frame state pointers to timer declarations
-	for (DeclareTimer::instance_iter it = DeclareTimer::beginInstances();
-		it != DeclareTimer::endInstances();
+	for (DeclareTimer::instance_iter it = guard.beginInstances();
+		it != guard.endInstances();
 		++it)
 	{
 		// update cached pointer
@@ -371,20 +372,23 @@ void LLFastTimer::NamedTimer::buildHierarchy()
 	if (sCurFrameIndex < 0 ) return;
 
 	// set up initial tree
-    for (instance_iter it = NamedTimer::beginInstances();
-		it != endInstances();
-		++it)
 	{
-		NamedTimer& timer = *it;
-		if (&timer == NamedTimerFactory::instance().getRootTimer()) continue;
-
-		// bootstrap tree construction by attaching to last timer to be on stack
-		// when this timer was called
-		if (timer.getFrameState().mLastCaller && timer.mParent == NamedTimerFactory::instance().getRootTimer())
+		NamedTimer::LLInstanceTrackerScopedGuard guard;
+		for (instance_iter it = guard.beginInstances();
+		     it != guard.endInstances();
+		     ++it)
 		{
-			timer.setParent(timer.getFrameState().mLastCaller->mTimer);
-			// no need to push up tree on first use, flag can be set spuriously
-			timer.getFrameState().mMoveUpTree = false;
+			NamedTimer& timer = *it;
+			if (&timer == NamedTimerFactory::instance().getRootTimer()) continue;
+			
+			// bootstrap tree construction by attaching to last timer to be on stack
+			// when this timer was called
+			if (timer.getFrameState().mLastCaller && timer.mParent == NamedTimerFactory::instance().getRootTimer())
+			{
+				timer.setParent(timer.getFrameState().mLastCaller->mTimer);
+				// no need to push up tree on first use, flag can be set spuriously
+				timer.getFrameState().mMoveUpTree = false;
+			}
 		}
 	}
 
@@ -486,18 +490,21 @@ void LLFastTimer::NamedTimer::resetFrame()
 		F64 total_time = 0;
 		LLSD sd;
 
-		for (NamedTimer::instance_iter it = NamedTimer::beginInstances();
-					it != NamedTimer::endInstances();
-					++it)
 		{
-			NamedTimer& timer = *it;
-			FrameState& info = timer.getFrameState();
-			sd[timer.getName()]["Time"] = (LLSD::Real) (info.mSelfTimeCounter*iclock_freq);	
-			sd[timer.getName()]["Calls"] = (LLSD::Integer) info.mCalls;
-			
-			// computing total time here because getting the root timer's getCountHistory
-			// doesn't work correctly on the first frame
-			total_time = total_time + info.mSelfTimeCounter * iclock_freq;
+			NamedTimer::LLInstanceTrackerScopedGuard guard;
+			for (NamedTimer::instance_iter it = guard.beginInstances();
+			     it != guard.endInstances();
+			     ++it)
+			{
+				NamedTimer& timer = *it;
+				FrameState& info = timer.getFrameState();
+				sd[timer.getName()]["Time"] = (LLSD::Real) (info.mSelfTimeCounter*iclock_freq);	
+				sd[timer.getName()]["Calls"] = (LLSD::Integer) info.mCalls;
+				
+				// computing total time here because getting the root timer's getCountHistory
+				// doesn't work correctly on the first frame
+				total_time = total_time + info.mSelfTimeCounter * iclock_freq;
+			}
 		}
 
 		sd["Total"]["Time"] = (LLSD::Real) total_time;
@@ -531,21 +538,24 @@ void LLFastTimer::NamedTimer::resetFrame()
 	DeclareTimer::updateCachedPointers();
 
 	// reset for next frame
-	for (NamedTimer::instance_iter it = NamedTimer::beginInstances();
-		it != NamedTimer::endInstances();
-		++it)
 	{
-		NamedTimer& timer = *it;
-
-		FrameState& info = timer.getFrameState();
-		info.mSelfTimeCounter = 0;
-		info.mCalls = 0;
-		info.mLastCaller = NULL;
-		info.mMoveUpTree = false;
-		// update parent pointer in timer state struct
-		if (timer.mParent)
+		NamedTimer::LLInstanceTrackerScopedGuard guard;
+		for (NamedTimer::instance_iter it = guard.beginInstances();
+		     it != guard.endInstances();
+		     ++it)
 		{
-			info.mParent = &timer.mParent->getFrameState();
+			NamedTimer& timer = *it;
+			
+			FrameState& info = timer.getFrameState();
+			info.mSelfTimeCounter = 0;
+			info.mCalls = 0;
+			info.mLastCaller = NULL;
+			info.mMoveUpTree = false;
+			// update parent pointer in timer state struct
+			if (timer.mParent)
+			{
+				info.mParent = &timer.mParent->getFrameState();
+			}
 		}
 	}
 
@@ -575,20 +585,23 @@ void LLFastTimer::NamedTimer::reset()
 	}
 
 	// reset all history
-	for (NamedTimer::instance_iter it = NamedTimer::beginInstances();
-		it != NamedTimer::endInstances();
-		++it)
 	{
-		NamedTimer& timer = *it;
-		if (&timer != NamedTimerFactory::instance().getRootTimer()) 
+		NamedTimer::LLInstanceTrackerScopedGuard guard;
+		for (NamedTimer::instance_iter it = guard.beginInstances();
+		     it != guard.endInstances();
+		     ++it)
 		{
-			timer.setParent(NamedTimerFactory::instance().getRootTimer());
+			NamedTimer& timer = *it;
+			if (&timer != NamedTimerFactory::instance().getRootTimer()) 
+			{
+				timer.setParent(NamedTimerFactory::instance().getRootTimer());
+			}
+			
+			timer.mCountAverage = 0;
+			timer.mCallAverage = 0;
+			memset(timer.mCountHistory, 0, sizeof(U32) * HISTORY_NUM);
+			memset(timer.mCallHistory, 0, sizeof(U32) * HISTORY_NUM);
 		}
-
-		timer.mCountAverage = 0;
-		timer.mCallAverage = 0;
-		memset(timer.mCountHistory, 0, sizeof(U32) * HISTORY_NUM);
-		memset(timer.mCallHistory, 0, sizeof(U32) * HISTORY_NUM);
 	}
 
 	sLastFrameIndex = 0;
diff --git a/indra/llcommon/llinstancetracker.h b/indra/llcommon/llinstancetracker.h
index 11fe523651..9df7998273 100644
--- a/indra/llcommon/llinstancetracker.h
+++ b/indra/llcommon/llinstancetracker.h
@@ -98,7 +98,10 @@ private:
 		mKey = key; 
 		getMap_()[key] = static_cast<T*>(this); 
 	}
-	void remove_() { getMap_().erase(mKey); }
+	void remove_()
+	{
+		getMap_().erase(mKey);
+	}
 
     static InstanceMap& getMap_()
     {
@@ -129,31 +132,65 @@ public:
 
 	/// for completeness of analogy with the generic implementation
 	static T* getInstance(T* k) { return k; }
-	static key_iter beginKeys() { return getSet_().begin(); }
-	static key_iter endKeys()   { return getSet_().end(); }
-	static instance_iter beginInstances() { return instance_iter(getSet_().begin()); }
-	static instance_iter endInstances()   { return instance_iter(getSet_().end()); }
 	static S32 instanceCount() { return getSet_().size(); }
 
+	// Instantiate this to get access to iterators for this type.  It's a 'guard' in the sense
+	// that it treats deletes of this type as errors as long as there is an instance of
+	// this class alive in scope somewhere (i.e. deleting while iterating is bad).
+	class LLInstanceTrackerScopedGuard
+	{
+	public:
+		LLInstanceTrackerScopedGuard()
+		{
+			++sIterationNestDepth;
+		}
+
+		~LLInstanceTrackerScopedGuard()
+		{
+			--sIterationNestDepth;
+		}
+
+		static instance_iter beginInstances() {	return instance_iter(getSet_().begin()); }
+		static instance_iter endInstances() { return instance_iter(getSet_().end()); }
+		static key_iter beginKeys() { return getSet_().begin(); }
+		static key_iter endKeys()   { return getSet_().end(); }
+	};
+
 protected:
-	LLInstanceTracker() { getSet_().insert(static_cast<T*>(this)); }
-	virtual ~LLInstanceTracker() { getSet_().erase(static_cast<T*>(this)); }
+	LLInstanceTracker()
+	{
+		// it's safe but unpredictable to create instances of this type while all instances are being iterated over.  I hate unpredictable.  This assert will probably be turned on early in the next development cycle.
+		//llassert(sIterationNestDepth == 0);
+		getSet_().insert(static_cast<T*>(this));
+	}
+	virtual ~LLInstanceTracker()
+	{
+		// it's unsafe to delete instances of this type while all instances are being iterated over.
+		llassert(sIterationNestDepth == 0);
+		getSet_().erase(static_cast<T*>(this));
+	}
 
-	LLInstanceTracker(const LLInstanceTracker& other) { getSet_().insert(static_cast<T*>(this)); }
+	LLInstanceTracker(const LLInstanceTracker& other)
+	{
+		//llassert(sIterationNestDepth == 0);
+		getSet_().insert(static_cast<T*>(this));
+	}
 
-    static InstanceSet& getSet_()   // called after getReady() but before go()
-    {
-        if (! sInstances)
-        {
-            sInstances = new InstanceSet;
-        }
-        return *sInstances;
-    }
+	static InstanceSet& getSet_()
+	{
+		if (! sInstances)
+		{
+			sInstances = new InstanceSet;
+		}
+		return *sInstances;
+	}
 
 	static InstanceSet* sInstances;
+	static S32 sIterationNestDepth;
 };
 
 template <typename T, typename KEY> typename LLInstanceTracker<T, KEY>::InstanceMap* LLInstanceTracker<T, KEY>::sInstances = NULL;
 template <typename T> typename LLInstanceTracker<T, T*>::InstanceSet* LLInstanceTracker<T, T*>::sInstances = NULL;
+template <typename T> S32 LLInstanceTracker<T, T*>::sIterationNestDepth = 0;
 
 #endif
diff --git a/indra/llcommon/tests/llinstancetracker_test.cpp b/indra/llcommon/tests/llinstancetracker_test.cpp
index 7415f2d33b..4bb3ec2922 100644
--- a/indra/llcommon/tests/llinstancetracker_test.cpp
+++ b/indra/llcommon/tests/llinstancetracker_test.cpp
@@ -138,23 +138,29 @@ namespace tut
         keys.insert(&one);
         keys.insert(&two);
         keys.insert(&three);
-        for (Unkeyed::key_iter ki(Unkeyed::beginKeys()), kend(Unkeyed::endKeys());
-             ki != kend; ++ki)
-        {
-            ensure_equals("spurious key", keys.erase(*ki), 1);
-        }
+	{
+		Unkeyed::LLInstanceTrackerScopedGuard guard;
+		for (Unkeyed::key_iter ki(guard.beginKeys()), kend(guard.endKeys());
+		     ki != kend; ++ki)
+		{
+			ensure_equals("spurious key", keys.erase(*ki), 1);
+		}
+	}
         ensure_equals("unreported key", keys.size(), 0);
 
         KeySet instances;
         instances.insert(&one);
         instances.insert(&two);
         instances.insert(&three);
-        for (Unkeyed::instance_iter ii(Unkeyed::beginInstances()), iend(Unkeyed::endInstances());
-             ii != iend; ++ii)
-        {
-            Unkeyed& ref = *ii;
-            ensure_equals("spurious instance", instances.erase(&ref), 1);
-        }
+	{
+		Unkeyed::LLInstanceTrackerScopedGuard guard;
+		for (Unkeyed::instance_iter ii(guard.beginInstances()), iend(guard.endInstances());
+		     ii != iend; ++ii)
+		{
+			Unkeyed& ref = *ii;
+			ensure_equals("spurious instance", instances.erase(&ref), 1);
+		}
+	}
         ensure_equals("unreported instance", instances.size(), 0);
     }
 } // namespace tut