SL-16024: Return shared_ptr from LLInstanceTracker::getInstance().

It feels wrong to return a dumb LLInstanceTracker subclass* from getInstance()
when we use std::shared_ptr and std::weak_ptr internally. But tweak consumers
to use 'auto' or LLInstanceTracker::ptr_t in case we later revisit this
decision.

We did add a couple get() calls where it's important to obtain a dumb pointer.

11 files changed, 71 insertions, 48 deletions

diff --git a/indra/llcommon/llinstancetracker.h b/indra/llcommon/llinstancetracker.h
index 402333cca7..02535a59e7 100644
--- a/indra/llcommon/llinstancetracker.h
+++ b/indra/llcommon/llinstancetracker.h
@@ -83,13 +83,34 @@ class LLInstanceTracker
     typedef llthread::LockStatic<StaticData> LockStatic;
 
 public:
+    using ptr_t  = std::shared_ptr<T>;
+    using weak_t = std::weak_ptr<T>;
+
+    /**
+     * Storing a dumb T* somewhere external is a bad idea, since
+     * LLInstanceTracker subclasses are explicitly destroyed rather than
+     * managed by smart pointers. It's legal to declare stack instances of an
+     * LLInstanceTracker subclass. But it's reasonable to store a
+     * std::weak_ptr<T>, which will become invalid when the T instance is
+     * destroyed.
+     */
+    weak_t getWeak()
+    {
+        return mSelf;
+    }
+
+    static S32 instanceCount() 
+    { 
+        return LockStatic()->mMap.size(); 
+    }
+    
     // snapshot of std::pair<const KEY, std::shared_ptr<T>> pairs
     class snapshot
     {
         // It's very important that what we store in this snapshot are
         // weak_ptrs, NOT shared_ptrs. That's how we discover whether any
         // instance has been deleted during the lifespan of a snapshot.
-        typedef std::vector<std::pair<const KEY, std::weak_ptr<T>>> VectorType;
+        typedef std::vector<std::pair<const KEY, weak_t>> VectorType;
         // Dereferencing our iterator produces a std::shared_ptr for each
         // instance that still exists. Since we store weak_ptrs, that involves
         // two chained transformations:
@@ -98,7 +119,7 @@ public:
         // It is very important that we filter lazily, that is, during
         // traversal. Any one of our stored weak_ptrs might expire during
         // traversal.
-        typedef std::pair<const KEY, std::shared_ptr<T>> strong_pair;
+        typedef std::pair<const KEY, ptr_t> strong_pair;
         // Note for future reference: nat has not yet had any luck (up to
         // Boost 1.67) trying to use boost::transform_iterator with a hand-
         // coded functor, only with actual functions. In my experience, an
@@ -202,17 +223,12 @@ public:
         iterator end()   { return iterator(snapshot::end(),   key_getter); }
     };
 
-    static T* getInstance(const KEY& k)
+    static ptr_t getInstance(const KEY& k)
     {
         LockStatic lock;
         const InstanceMap& map(lock->mMap);
         typename InstanceMap::const_iterator found = map.find(k);
-        return (found == map.end()) ? NULL : found->second.get();
-    }
-
-    static S32 instanceCount() 
-    { 
-        return LockStatic()->mMap.size(); 
+        return (found == map.end()) ? NULL : found->second;
     }
 
 protected:
@@ -222,7 +238,9 @@ protected:
         // shared_ptr, so give it a no-op deleter. We store shared_ptrs in our
         // InstanceMap specifically so snapshot can store weak_ptrs so we can
         // detect deletions during traversals.
-        std::shared_ptr<T> ptr(static_cast<T*>(this), [](T*){});
+        ptr_t ptr(static_cast<T*>(this), [](T*){});
+        // save corresponding weak_ptr for future reference
+        mSelf = ptr;
         LockStatic lock;
         add_(lock, key, ptr);
     }
@@ -257,7 +275,7 @@ private:
     static std::string report(const char* key) { return report(std::string(key)); }
 
     // caller must instantiate LockStatic
-    void add_(LockStatic& lock, const KEY& key, const std::shared_ptr<T>& ptr) 
+    void add_(LockStatic& lock, const KEY& key, const ptr_t& ptr) 
     { 
         mInstanceKey = key; 
         InstanceMap& map = lock->mMap;
@@ -281,7 +299,7 @@ private:
             break;
         }
     }
-    std::shared_ptr<T> remove_(LockStatic& lock)
+    ptr_t remove_(LockStatic& lock)
     {
         InstanceMap& map = lock->mMap;
         typename InstanceMap::iterator iter = map.find(mInstanceKey);
@@ -295,6 +313,9 @@ private:
     }
 
 private:
+    // Storing a weak_ptr to self is a bit like deriving from
+    // std::enable_shared_from_this(), except more explicit.
+    weak_t mSelf;
     KEY mInstanceKey;
 };
 
@@ -326,6 +347,9 @@ class LLInstanceTracker<T, void, KEY_COLLISION_BEHAVIOR>
     typedef llthread::LockStatic<StaticData> LockStatic;
 
 public:
+    using ptr_t  = std::shared_ptr<T>;
+    using weak_t = std::weak_ptr<T>;
+
     /**
      * Storing a dumb T* somewhere external is a bad idea, since
      * LLInstanceTracker subclasses are explicitly destroyed rather than
@@ -334,12 +358,15 @@ public:
      * std::weak_ptr<T>, which will become invalid when the T instance is
      * destroyed.
      */
-    std::weak_ptr<T> getWeak()
+    weak_t getWeak()
     {
         return mSelf;
     }
     
-    static S32 instanceCount() { return LockStatic()->mSet.size(); }
+    static S32 instanceCount()
+    {
+        return LockStatic()->mSet.size();
+    }
 
     // snapshot of std::shared_ptr<T> pointers
     class snapshot
@@ -347,7 +374,7 @@ public:
         // It's very important that what we store in this snapshot are
         // weak_ptrs, NOT shared_ptrs. That's how we discover whether any
         // instance has been deleted during the lifespan of a snapshot.
-        typedef std::vector<std::weak_ptr<T>> VectorType;
+        typedef std::vector<weak_t> VectorType;
         // Dereferencing our iterator produces a std::shared_ptr for each
         // instance that still exists. Since we store weak_ptrs, that involves
         // two chained transformations:
@@ -453,7 +480,7 @@ protected:
 private:
     // Storing a weak_ptr to self is a bit like deriving from
     // std::enable_shared_from_this(), except more explicit.
-    std::weak_ptr<T> mSelf;
+    weak_t mSelf;
 };
 
 #endif
diff --git a/indra/llcommon/llleaplistener.cpp b/indra/llcommon/llleaplistener.cpp
index 3e6ce9092c..11bfec1b31 100644
--- a/indra/llcommon/llleaplistener.cpp
+++ b/indra/llcommon/llleaplistener.cpp
@@ -220,7 +220,7 @@ void LLLeapListener::getAPI(const LLSD& request) const
 {
     Response reply(LLSD(), request);
 
-    LLEventAPI* found = LLEventAPI::getInstance(request["api"]);
+    auto found = LLEventAPI::getInstance(request["api"]);
     if (found)
     {
         reply["name"] = found->getName();
diff --git a/indra/llui/llnotifications.cpp b/indra/llui/llnotifications.cpp
index b791a19c2b..88eda1c172 100644
--- a/indra/llui/llnotifications.cpp
+++ b/indra/llui/llnotifications.cpp
@@ -1387,7 +1387,7 @@ bool LLNotifications::failedUniquenessTest(const LLSD& payload)
 
 LLNotificationChannelPtr LLNotifications::getChannel(const std::string& channelName)
 {
-	return LLNotificationChannelPtr(LLNotificationChannel::getInstance(channelName));
+	return LLNotificationChannelPtr(LLNotificationChannel::getInstance(channelName).get());
 }
 
 
diff --git a/indra/llui/llstatbar.cpp b/indra/llui/llstatbar.cpp
index 6c8e63442b..8adcd664df 100644
--- a/indra/llui/llstatbar.cpp
+++ b/indra/llui/llstatbar.cpp
@@ -554,29 +554,25 @@ void LLStatBar::draw()
 void LLStatBar::setStat(const std::string& stat_name)
 {
 	using namespace LLTrace;
-	const StatType<CountAccumulator>*	count_stat;
-	const StatType<EventAccumulator>*	event_stat;
-	const StatType<SampleAccumulator>*	sample_stat;
-	const StatType<MemAccumulator>*		mem_stat;
 
-	if ((count_stat = StatType<CountAccumulator>::getInstance(stat_name)))
+	if (auto count_stat = StatType<CountAccumulator>::getInstance(stat_name))
 	{
-		mStat.countStatp = count_stat;
+		mStat.countStatp = count_stat.get();
 		mStatType = STAT_COUNT;
 	}
-	else if ((event_stat = StatType<EventAccumulator>::getInstance(stat_name)))
+	else if (auto event_stat = StatType<EventAccumulator>::getInstance(stat_name))
 	{
-		mStat.eventStatp = event_stat;
+		mStat.eventStatp = event_stat.get();
 		mStatType = STAT_EVENT;
 	}
-	else if ((sample_stat = StatType<SampleAccumulator>::getInstance(stat_name)))
+	else if (auto sample_stat = StatType<SampleAccumulator>::getInstance(stat_name))
 	{
-		mStat.sampleStatp = sample_stat;
+		mStat.sampleStatp = sample_stat.get();
 		mStatType = STAT_SAMPLE;
 	}
-	else if ((mem_stat = StatType<MemAccumulator>::getInstance(stat_name)))
+	else if (auto mem_stat = StatType<MemAccumulator>::getInstance(stat_name))
 	{
-		mStat.memStatp = mem_stat;
+		mStat.memStatp = mem_stat.get();
 		mStatType = STAT_MEM;
 	}
 }
diff --git a/indra/llxml/llcontrol.h b/indra/llxml/llcontrol.h
index 19508becc3..5da13f5010 100644
--- a/indra/llxml/llcontrol.h
+++ b/indra/llxml/llcontrol.h
@@ -405,8 +405,8 @@ public:
 					const T& default_value, 
 					const std::string& comment = "Declared In Code")
 	{
-		mCachedControlPtr = LLControlCache<T>::getInstance(name);
-		if (mCachedControlPtr.isNull())
+		mCachedControlPtr = LLControlCache<T>::getInstance(name).get();
+		if (! mCachedControlPtr)
 		{
 			mCachedControlPtr = new LLControlCache<T>(group, name, default_value, comment);
 		}
@@ -415,8 +415,8 @@ public:
 	LLCachedControl(LLControlGroup& group,
 					const std::string& name)
 	{
-		mCachedControlPtr = LLControlCache<T>::getInstance(name);
-		if (mCachedControlPtr.isNull())
+		mCachedControlPtr = LLControlCache<T>::getInstance(name).get();
+		if (! mCachedControlPtr)
 		{
 			mCachedControlPtr = new LLControlCache<T>(group, name);
 		}
diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp
index 891722e1bd..4a9a1ad0d4 100644
--- a/indra/newview/llappviewer.cpp
+++ b/indra/newview/llappviewer.cpp
@@ -2336,7 +2336,7 @@ bool LLAppViewer::loadSettingsFromDirectory(const std::string& location_key,
 			LL_INFOS("Settings") << "Attempting to load settings for the group " << file.name()
 			    << " - from location " << location_key << LL_ENDL;
 
-			LLControlGroup* settings_group = LLControlGroup::getInstance(file.name);
+			auto settings_group = LLControlGroup::getInstance(file.name);
 			if(!settings_group)
 			{
 				LL_WARNS("Settings") << "No matching settings group for name " << file.name() << LL_ENDL;
@@ -2638,7 +2638,7 @@ bool LLAppViewer::initConfiguration()
 					group_part = name.substr(0, pos);
 					name_part = name.substr(pos+1);
 					LL_INFOS() << "Setting " << group_part << "." << name_part << " to " << value << LL_ENDL;
-					LLControlGroup* g = LLControlGroup::getInstance(group_part);
+					auto g = LLControlGroup::getInstance(group_part);
 					if (g) control = g->getControl(name_part);
 				}
 				else
diff --git a/indra/newview/llbrowsernotification.cpp b/indra/newview/llbrowsernotification.cpp
index 0460bff1b4..30ac35fff7 100644
--- a/indra/newview/llbrowsernotification.cpp
+++ b/indra/newview/llbrowsernotification.cpp
@@ -43,14 +43,14 @@ LLBrowserNotification::LLBrowserNotification()
 bool LLBrowserNotification::processNotification(const LLNotificationPtr& notification)
 {
 	LLUUID media_id = notification->getPayload()["media_id"].asUUID();
-	LLMediaCtrl* media_instance = LLMediaCtrl::getInstance(media_id);
+	auto media_instance = LLMediaCtrl::getInstance(media_id);
 	if (media_instance)
 	{
 		media_instance->showNotification(notification);
 	}
 	else if (LLViewerMediaFocus::instance().getControlsMediaID() == media_id)
 	{
-		LLViewerMediaImpl* impl = LLViewerMedia::getInstance()->getMediaImplFromTextureID(media_id);
+		auto impl = LLViewerMedia::getInstance()->getMediaImplFromTextureID(media_id);
 		if (impl)
 		{
 			impl->showNotification(notification);
diff --git a/indra/newview/llfloaterwebcontent.cpp b/indra/newview/llfloaterwebcontent.cpp
index 23fd6d9c8e..ceab472c55 100644
--- a/indra/newview/llfloaterwebcontent.cpp
+++ b/indra/newview/llfloaterwebcontent.cpp
@@ -159,7 +159,7 @@ LLFloater* LLFloaterWebContent::create( Params p)
 //static
 void LLFloaterWebContent::closeRequest(const std::string &uuid)
 {
-	LLFloaterWebContent* floaterp = instance_tracker_t::getInstance(uuid);
+	auto floaterp = instance_tracker_t::getInstance(uuid);
 	if (floaterp)
 	{
 		floaterp->closeFloater(false);
@@ -169,7 +169,7 @@ void LLFloaterWebContent::closeRequest(const std::string &uuid)
 //static
 void LLFloaterWebContent::geometryChanged(const std::string &uuid, S32 x, S32 y, S32 width, S32 height)
 {
-	LLFloaterWebContent* floaterp = instance_tracker_t::getInstance(uuid);
+	auto floaterp = instance_tracker_t::getInstance(uuid);
 	if (floaterp)
 	{
 		floaterp->geometryChanged(x, y, width, height);
diff --git a/indra/newview/llnotificationofferhandler.cpp b/indra/newview/llnotificationofferhandler.cpp
index a9678b1e93..d9359d20cf 100644
--- a/indra/newview/llnotificationofferhandler.cpp
+++ b/indra/newview/llnotificationofferhandler.cpp
@@ -166,14 +166,14 @@ bool LLOfferHandler::processNotification(const LLNotificationPtr& notification)
 
 /*virtual*/ void LLOfferHandler::onChange(LLNotificationPtr p)
 {
-	LLToastNotifyPanel* panelp = LLToastNotifyPanel::getInstance(p->getID());
+	auto panelp = LLToastNotifyPanel::getInstance(p->getID());
 	if (panelp)
 	{
 		//
 		// HACK: if we're dealing with a notification embedded in IM, update it
 		// otherwise remove its toast
 		//
-		if (dynamic_cast<LLIMToastNotifyPanel*>(panelp))
+		if (dynamic_cast<LLIMToastNotifyPanel*>(panelp.get()))
 		{
 			panelp->updateNotification();
 		}
diff --git a/indra/newview/llviewercontrollistener.cpp b/indra/newview/llviewercontrollistener.cpp
index 3443bb644a..8820f9ec56 100644
--- a/indra/newview/llviewercontrollistener.cpp
+++ b/indra/newview/llviewercontrollistener.cpp
@@ -127,7 +127,7 @@ struct Info
 
 	LLEventAPI::Response response;
 	std::string groupname;
-	LLControlGroup* group;
+	LLControlGroup::ptr_t group;
 	std::string key;
 	LLControlVariable* control;
 };
@@ -187,7 +187,7 @@ void LLViewerControlListener::groups(LLSD const & request)
 
 struct CollectVars: public LLControlGroup::ApplyFunctor
 {
-	CollectVars(LLControlGroup* g):
+	CollectVars(LLControlGroup::ptr_t g):
 		mGroup(g)
 	{}
 
@@ -200,7 +200,7 @@ struct CollectVars: public LLControlGroup::ApplyFunctor
 					("comment", control->getComment()));
 	}
 
-	LLControlGroup* mGroup;
+	LLControlGroup::ptr_t mGroup;
 	LLSD vars;
 };
 
@@ -210,7 +210,7 @@ void LLViewerControlListener::vars(LLSD const & request)
 	// control name.
 	Response response(LLSD(), request);
 	std::string groupname(request["group"]);
-	LLControlGroup* group(LLControlGroup::getInstance(groupname));
+	auto group(LLControlGroup::getInstance(groupname));
 	if (! group)
 	{
 		return response.error(STRINGIZE("Unrecognized group '" << groupname << "'"));
diff --git a/indra/newview/llviewermessage.cpp b/indra/newview/llviewermessage.cpp
index 39c891c9c1..94d2d216b9 100644
--- a/indra/newview/llviewermessage.cpp
+++ b/indra/newview/llviewermessage.cpp
@@ -3978,8 +3978,8 @@ void process_sim_stats(LLMessageSystem *msg, void **user_data)
 		F32 stat_value;
 		msg->getU32("Stat", "StatID", stat_id, i);
 		msg->getF32("Stat", "StatValue", stat_value, i);
-		LLStatViewer::SimMeasurementSampler* measurementp = LLStatViewer::SimMeasurementSampler::getInstance((ESimStatID)stat_id);
-		
+		auto measurementp = LLStatViewer::SimMeasurementSampler::getInstance((ESimStatID)stat_id);
+
 		if (measurementp )
 		{
 			measurementp->sample(stat_value);