Commits

davep committed 158f434

Fix non-thread-safe refcounting of LLHTTPClient::Responder and fix out-of-order deletion of LLTextureFetch on shutdown

Comments (0)

Files changed (14)

indra/llcommon/llthread.cpp

 		apr_pool_create(&mAPRPoolp, NULL); // Create a subpool for this thread
 	}
 	mRunCondition = new LLCondition(mAPRPoolp);
-
+	mDataLock = new LLMutex(mAPRPoolp);
 	mLocalAPRFilePoolp = NULL ;
 }
 
 	}
 
 	delete mRunCondition;
-	mRunCondition = 0;
+	mRunCondition = NULL;
+
+	delete mDataLock;
+	mDataLock = NULL;
 	
 	if (mIsLocalPool && mAPRPoolp)
 	{
 // Stop thread execution if requested until unpaused.
 void LLThread::checkPause()
 {
-	mRunCondition->lock();
+	mDataLock->lock();
 
 	// This is in a while loop because the pthread API allows for spurious wakeups.
 	while(shouldSleep())
 	{
+		mDataLock->unlock();
 		mRunCondition->wait(); // unlocks mRunCondition
+		mDataLock->lock();
 		// mRunCondition is locked when the thread wakes up
 	}
 	
- 	mRunCondition->unlock();
+ 	mDataLock->unlock();
 }
 
 //============================================================================
 
 void LLThread::setQuitting()
 {
-	mRunCondition->lock();
+	mDataLock->lock();
 	if (mStatus == RUNNING)
 	{
 		mStatus = QUITTING;
 	}
-	mRunCondition->unlock();
+	mDataLock->unlock();
 	wake();
 }
 
 
 void LLThread::wake()
 {
-	mRunCondition->lock();
+	mDataLock->lock();
 	if(!shouldSleep())
 	{
 		mRunCondition->signal();
 	}
-	mRunCondition->unlock();
+	mDataLock->unlock();
 }
 
 void LLThread::wakeLocked()
 {
 }
 
+LLThreadSafeRefCount::LLThreadSafeRefCount(const LLThreadSafeRefCount& src)
+{
+	if (sMutex)
+	{
+		sMutex->lock();
+	}
+	mRef = 0;
+	if (sMutex)
+	{
+		sMutex->unlock();
+	}
+}
+
 LLThreadSafeRefCount::~LLThreadSafeRefCount()
 { 
 	if (mRef != 0)
 	}
 }
 
+
 //============================================================================
 
 LLResponder::~LLResponder()

indra/llcommon/llthread.h

 protected:
 	std::string			mName;
 	LLCondition*		mRunCondition;
+	LLMutex*			mDataLock;
 
 	apr_thread_t		*mAPRThreadp;
 	apr_pool_t			*mAPRPoolp;
 	inline void unlockData();
 	
 	// This is the predicate that decides whether the thread should sleep.  
-	// It should only be called with mRunCondition locked, since the virtual runCondition() function may need to access
+	// It should only be called with mDataLock locked, since the virtual runCondition() function may need to access
 	// data structures that are thread-unsafe.
 	bool shouldSleep(void) { return (mStatus == RUNNING) && (isPaused() || (!runCondition())); }
 
 	// To avoid spurious signals (and the associated context switches) when the condition may or may not have changed, you can do the following:
-	// mRunCondition->lock();
+	// mDataLock->lock();
 	// if(!shouldSleep())
 	//     mRunCondition->signal();
-	// mRunCondition->unlock();
+	// mDataLock->unlock();
 };
 
 //============================================================================
 
 void LLThread::lockData()
 {
-	mRunCondition->lock();
+	mDataLock->lock();
 }
 
 void LLThread::unlockData()
 {
-	mRunCondition->unlock();
+	mDataLock->unlock();
 }
 
 
 private:
 	static LLMutex* sMutex;
 
-private:
-	LLThreadSafeRefCount(const LLThreadSafeRefCount&); // not implemented
-	LLThreadSafeRefCount&operator=(const LLThreadSafeRefCount&); // not implemented
-
 protected:
 	virtual ~LLThreadSafeRefCount(); // use unref()
 	
 public:
 	LLThreadSafeRefCount();
+	LLThreadSafeRefCount(const LLThreadSafeRefCount&);
+	LLThreadSafeRefCount&operator=(const LLThreadSafeRefCount& ref) 
+	{
+		if (sMutex)
+		{
+			sMutex->lock();
+		}
+		mRef = 0;
+		if (sMutex)
+		{
+			sMutex->unlock();
+		}
+	}
+
+
 	
 	void ref()
 	{

indra/llmessage/llcurl.cpp

 //////////////////////////////////////////////////////////////////////////////
 
 LLCurl::Responder::Responder()
-	: mReferenceCount(0)
 {
 }
 
 LLCurl::Responder::~Responder()
 {
+	LL_CHECK_MEMORY
 }
 
 // virtual
 
 }
 
-namespace boost
-{
-	void intrusive_ptr_add_ref(LLCurl::Responder* p)
-	{
-		++p->mReferenceCount;
-	}
-	
-	void intrusive_ptr_release(LLCurl::Responder* p)
-	{
-		if (p && 0 == --p->mReferenceCount)
-		{
-			delete p;
-		}
-	}
-};
-
-
 //////////////////////////////////////////////////////////////////////////////
 
 std::set<CURL*> LLCurl::Easy::sFreeHandles;
 	LLMutexLock lock(sHandleMutexp) ;
 	if (sActiveHandles.find(handle) != sActiveHandles.end())
 	{
+		LL_CHECK_MEMORY
 		sActiveHandles.erase(handle);
-
+		LL_CHECK_MEMORY
 		if(sFreeHandles.size() < MAX_NUM_FREE_HANDLES)
 		{
-		sFreeHandles.insert(handle);
-	}
-	else
-	{
+			sFreeHandles.insert(handle);
+			LL_CHECK_MEMORY
+		}
+		else
+		{
 			LLCurl::deleteEasyHandle(handle) ;
+			LL_CHECK_MEMORY
 		}
 	}
 	else
 
 LLCurl::Easy::~Easy()
 {
-	LL_CHECK_MEMORY
 	releaseEasyHandle(mCurlEasyHandle);
-	LL_CHECK_MEMORY
 	--gCurlEasyCount;
 	curl_slist_free_all(mHeaders);
 	LL_CHECK_MEMORY
 		LL_CHECK_MEMORY
 	}
 	mResponder = NULL;
-	LL_CHECK_MEMORY
 }
 
 void LLCurl::Easy::resetState()
 		if(deleted)
 		{
 			easy->mResponder = NULL ; //avoid triggering mResponder.
+			LL_CHECK_MEMORY
 		}
 		delete easy;
 		LL_CHECK_MEMORY
 	if(handle)
 	{
 		LLMutexLock lock(sHandleMutexp) ;
+		LL_CHECK_MEMORY
 		curl_easy_cleanup(handle) ;
+		LL_CHECK_MEMORY
 		sTotalHandles-- ;
 	}
 }

indra/llmessage/llcurl.h

 #include "llthread.h"
 #include "llqueuedthread.h"
 #include "llframetimer.h"
+#include "llpointer.h"
+
 
 class LLMutex;
 class LLCurlThread;
 		F64 mSpeedDownload;
 	};
 	
-	class Responder
+	class Responder : public LLThreadSafeRefCount
 	{
 	//LOG_CLASS(Responder);
 	public:
 				return false;
 			}
 
-	public: /* but not really -- don't touch this */
-		U32 mReferenceCount;
-
 	private:
 		std::string mURL;
 	};
-	typedef boost::intrusive_ptr<Responder>	ResponderPtr;
+	typedef LLPointer<Responder>	ResponderPtr;
 
 
 	/**
 	void cleanupMulti(LLCurl::Multi* multi) ;
 } ;
 
-namespace boost
-{
-	void intrusive_ptr_add_ref(LLCurl::Responder* p);
-	void intrusive_ptr_release(LLCurl::Responder* p);
-};
-
 
 class LLCurlRequest
 {

indra/newview/llappviewer.cpp

 
 	delete sTextureCache;
     sTextureCache = NULL;
-	delete sTextureFetch;
-    sTextureFetch = NULL;
 	delete sImageDecodeThread;
     sImageDecodeThread = NULL;
 	delete mFastTimerLogThread;
 	LLCurl::cleanupClass();
 	LL_CHECK_MEMORY
 
+	//MUST happen AFTER LLCurl::cleanupClass
+	delete sTextureFetch;
+    sTextureFetch = NULL;
+	
 	// If we're exiting to launch an URL, do that here so the screen
 	// is at the right resolution before we launch IE.
 	if (!gLaunchFileOnQuit.empty())

indra/newview/llassetuploadresponders.cpp

 	bool uploadConfirmationCallback(
 		const LLSD& notification,
 		const LLSD& response,
-		boost::intrusive_ptr<LLNewAgentInventoryVariablePriceResponder> responder)
+		LLPointer<LLNewAgentInventoryVariablePriceResponder> responder)
 	{
 		S32 option;
 		std::string confirmation_url;
 
 	void confirmUpload(
 		const std::string& confirmation_url,
-		boost::intrusive_ptr<LLNewAgentInventoryVariablePriceResponder> responder)
+		LLPointer<LLNewAgentInventoryVariablePriceResponder> responder)
 	{
 		if ( getFilename().empty() )
 		{
 		// and cause sadness.
 		mImpl->confirmUpload(
 			confirmation_url,
-			boost::intrusive_ptr<LLNewAgentInventoryVariablePriceResponder>(this));
+			LLPointer<LLNewAgentInventoryVariablePriceResponder>(this));
 	}
 	else
 	{
 				mImpl,
 				_1,
 				_2,
-				boost::intrusive_ptr<LLNewAgentInventoryVariablePriceResponder>(this)));
+				LLPointer<LLNewAgentInventoryVariablePriceResponder>(this)));
 	}
 }
 

indra/newview/lleventpoll.cpp

 
 	class LLEventPollEventTimer : public LLEventTimer
 	{
-		typedef boost::intrusive_ptr<LLEventPollResponder> EventPollResponderPtr;
+		typedef LLPointer<LLEventPollResponder> EventPollResponderPtr;
 
 	public:
 		LLEventPollEventTimer(F32 period, EventPollResponderPtr responder)

indra/newview/llfloatertos.cpp

 
 	public:
 
-		static boost::intrusive_ptr< LLIamHere > build( LLFloaterTOS* parent )
+		static LLIamHere* build( LLFloaterTOS* parent )
 		{
-			return boost::intrusive_ptr< LLIamHere >( new LLIamHere( parent ) );
+			return new LLIamHere( parent );
 		};
 		
 		virtual void  setParent( LLFloaterTOS* parentIn )
 
 // this is global and not a class member to keep crud out of the header file
 namespace {
-	boost::intrusive_ptr< LLIamHere > gResponsePtr = 0;
+	LLPointer< LLIamHere > gResponsePtr = 0;
 };
 
 BOOL LLFloaterTOS::postBuild()

indra/newview/lltexturefetch.cpp

         
         ~lcl_responder()
             {
+				LL_CHECK_MEMORY
                 mFetcher->decrCurlPOSTCount();
+				LL_CHECK_MEMORY
             }
 
 		// virtual

indra/newview/lltranslate.h

 		EService mService;
 	};
 
-	typedef boost::intrusive_ptr<TranslationReceiver> TranslationReceiverPtr;
-	typedef boost::intrusive_ptr<KeyVerificationReceiver> KeyVerificationReceiverPtr;
+	typedef LLPointer<TranslationReceiver> TranslationReceiverPtr;
+	typedef LLPointer<KeyVerificationReceiver> KeyVerificationReceiverPtr;
 
 	/**
 	 * Translate given text.

indra/newview/llviewermessage.cpp

 	{
 	}
 
-	static boost::intrusive_ptr<ChatTranslationReceiver> build(const std::string &from_lang, const std::string &to_lang, const std::string &mesg, const LLChat &chat, const LLSD &toast_args)
-	{
-		return boost::intrusive_ptr<ChatTranslationReceiver>(new ChatTranslationReceiver(from_lang, to_lang, mesg, chat, toast_args));
+	static ChatTranslationReceiver* build(const std::string &from_lang, const std::string &to_lang, const std::string &mesg, const LLChat &chat, const LLSD &toast_args)
+	{
+		return new ChatTranslationReceiver(from_lang, to_lang, mesg, chat, toast_args);
 	}
 
 protected:

indra/newview/llviewerregion.cpp

 		}
 	}
 
-    static boost::intrusive_ptr<BaseCapabilitiesComplete> build( U64 region_handle, S32 id )
+    static BaseCapabilitiesComplete* build( U64 region_handle, S32 id )
     {
-		return boost::intrusive_ptr<BaseCapabilitiesComplete>( 
-				new BaseCapabilitiesComplete(region_handle, id) );
+		return new BaseCapabilitiesComplete(region_handle, id);
     }
 
 private:

indra/viewer_components/updater/llupdatechecker.cpp

 };
 
 
-class LLUpdateChecker::Implementation:
-	public LLHTTPClient::Responder
-{
-public:
-	Implementation(Client & client);
-	~Implementation();
-	void checkVersion(std::string const & protocolVersion, std::string const & hostUrl, 
-			   std::string const & servicePath, std::string channel, std::string version);
-	
-	// Responder:
-	virtual void completed(U32 status,
-						   const std::string & reason,
-						   const LLSD& content);
-	virtual void error(U32 status, const std::string & reason);
-	
-private:	
-	static const char * sProtocolVersion;
-	
-	Client & mClient;
-	LLHTTPClient mHttpClient;
-	bool mInProgress;
-	std::string mVersion;
-	
-	std::string buildUrl(std::string const & protocolVersion, std::string const & hostUrl, 
-						 std::string const & servicePath, std::string channel, std::string version);
-
-	LOG_CLASS(LLUpdateChecker::Implementation);
-};
-
-
-
 // LLUpdateChecker
 //-----------------------------------------------------------------------------
 
 	std::string checkUrl = buildUrl(protocolVersion, hostUrl, servicePath, channel, version);
 	LL_INFOS("UpdateCheck") << "checking for updates at " << checkUrl << llendl;
 	
-	// The HTTP client will wrap a raw pointer in a boost::intrusive_ptr causing the
-	// passed object to be silently and automatically deleted.  We pass a self-
-	// referential intrusive pointer to which we add a reference to keep the
-	// client from deleting the update checker implementation instance.
-	LLHTTPClient::ResponderPtr temporaryPtr(this);
-	boost::intrusive_ptr_add_ref(temporaryPtr.get());
-	mHttpClient.get(checkUrl, temporaryPtr);
+	mHttpClient.get(checkUrl, this);
 }
 
 void LLUpdateChecker::Implementation::completed(U32 status,

indra/viewer_components/updater/llupdatechecker.h

 
 #include <boost/shared_ptr.hpp>
 
+#include "llhttpclient.h"
 
 //
 // Implements asynchronous checking for updates.
 class LLUpdateChecker {
 public:
 	class Client;
-	class Implementation;
+	class Implementation:
+
+	public LLHTTPClient::Responder
+	{
+	public:
+		Implementation(Client & client);
+		~Implementation();
+		void checkVersion(std::string const & protocolVersion, std::string const & hostUrl, 
+				   std::string const & servicePath, std::string channel, std::string version);
+	
+		// Responder:
+		virtual void completed(U32 status,
+							   const std::string & reason,
+							   const LLSD& content);
+		virtual void error(U32 status, const std::string & reason);
+	
+	private:	
+		static const char * sProtocolVersion;
+	
+		Client & mClient;
+		LLHTTPClient mHttpClient;
+		bool mInProgress;
+		std::string mVersion;
+	
+		std::string buildUrl(std::string const & protocolVersion, std::string const & hostUrl, 
+							 std::string const & servicePath, std::string channel, std::string version);
+
+		LOG_CLASS(LLUpdateChecker::Implementation);
+	};
+
 	
 	// An exception that may be raised on check errors.
 	class CheckError;
 			   std::string const & servicePath, std::string channel, std::string version);
 	
 private:
-	boost::shared_ptr<Implementation> mImplementation;
+	LLPointer<Implementation> mImplementation;
 };