From 374f20edf09ff8194c715d190c114eaacac00bfe Mon Sep 17 00:00:00 2001
From: Dave Parks <davep@lindenlab.com>
Date: Wed, 3 Oct 2012 14:30:21 -0500
Subject: [PATCH] Fix non-thread-safe refcounting of LLHTTPClient::Responder
and fix out-of-order deletion of LLTextureFetch on shutdown
---
indra/llcommon/llthread.cpp | 35 +++++++++++++----
indra/llcommon/llthread.h | 30 +++++++++-----
indra/llmessage/llcurl.cpp | 38 ++++++------------
indra/llmessage/llcurl.h | 15 ++-----
indra/newview/llappviewer.cpp | 6 ++-
indra/newview/llassetuploadresponders.cpp | 8 ++--
indra/newview/lleventpoll.cpp | 2 +-
indra/newview/llfloatertos.cpp | 6 +--
indra/newview/lltexturefetch.cpp | 2 +
indra/newview/lltranslate.h | 4 +-
indra/newview/llviewermessage.cpp | 4 +-
indra/newview/llviewerregion.cpp | 5 +--
.../updater/llupdatechecker.cpp | 39 +------------------
.../updater/llupdatechecker.h | 34 +++++++++++++++-
14 files changed, 117 insertions(+), 111 deletions(-)
diff --git a/indra/llcommon/llthread.cpp b/indra/llcommon/llthread.cpp
index a6ad6b125c1..c2fbb544a88 100644
--- a/indra/llcommon/llthread.cpp
+++ b/indra/llcommon/llthread.cpp
@@ -114,7 +114,7 @@ LLThread::LLThread(const std::string& name, apr_pool_t *poolp) :
apr_pool_create(&mAPRPoolp, NULL); // Create a subpool for this thread
}
mRunCondition = new LLCondition(mAPRPoolp);
-
+ mDataLock = new LLMutex(mAPRPoolp);
mLocalAPRFilePoolp = NULL ;
}
@@ -173,7 +173,10 @@ void LLThread::shutdown()
}
delete mRunCondition;
- mRunCondition = 0;
+ mRunCondition = NULL;
+
+ delete mDataLock;
+ mDataLock = NULL;
if (mIsLocalPool && mAPRPoolp)
{
@@ -242,28 +245,30 @@ bool LLThread::runCondition(void)
// 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();
}
@@ -285,12 +290,12 @@ void LLThread::yield()
void LLThread::wake()
{
- mRunCondition->lock();
+ mDataLock->lock();
if(!shouldSleep())
{
mRunCondition->signal();
}
- mRunCondition->unlock();
+ mDataLock->unlock();
}
void LLThread::wakeLocked()
@@ -481,6 +486,19 @@ LLThreadSafeRefCount::LLThreadSafeRefCount() :
{
}
+LLThreadSafeRefCount::LLThreadSafeRefCount(const LLThreadSafeRefCount& src)
+{
+ if (sMutex)
+ {
+ sMutex->lock();
+ }
+ mRef = 0;
+ if (sMutex)
+ {
+ sMutex->unlock();
+ }
+}
+
LLThreadSafeRefCount::~LLThreadSafeRefCount()
{
if (mRef != 0)
@@ -489,6 +507,7 @@ LLThreadSafeRefCount::~LLThreadSafeRefCount()
}
}
+
//============================================================================
LLResponder::~LLResponder()
diff --git a/indra/llcommon/llthread.h b/indra/llcommon/llthread.h
index b52e70ab2eb..892e144911b 100644
--- a/indra/llcommon/llthread.h
+++ b/indra/llcommon/llthread.h
@@ -97,6 +97,7 @@ class LL_COMMON_API LLThread
protected:
std::string mName;
LLCondition* mRunCondition;
+ LLMutex* mDataLock;
apr_thread_t *mAPRThreadp;
apr_pool_t *mAPRPoolp;
@@ -122,15 +123,15 @@ class LL_COMMON_API LLThread
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();
};
//============================================================================
@@ -205,12 +206,12 @@ class LLMutexLock
void LLThread::lockData()
{
- mRunCondition->lock();
+ mDataLock->lock();
}
void LLThread::unlockData()
{
- mRunCondition->unlock();
+ mDataLock->unlock();
}
@@ -227,15 +228,26 @@ class LL_COMMON_API LLThreadSafeRefCount
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()
{
diff --git a/indra/llmessage/llcurl.cpp b/indra/llmessage/llcurl.cpp
index 0de47e01812..158e4ce091b 100644
--- a/indra/llmessage/llcurl.cpp
+++ b/indra/llmessage/llcurl.cpp
@@ -133,12 +133,12 @@ std::string LLCurl::getVersionString()
//////////////////////////////////////////////////////////////////////////////
LLCurl::Responder::Responder()
- : mReferenceCount(0)
{
}
LLCurl::Responder::~Responder()
{
+ LL_CHECK_MEMORY
}
// virtual
@@ -202,23 +202,6 @@ void LLCurl::Responder::completedHeader(U32 status, const std::string& reason, c
}
-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;
@@ -267,15 +250,18 @@ void LLCurl::Easy::releaseEasyHandle(CURL* handle)
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
@@ -315,9 +301,7 @@ LLCurl::Easy* LLCurl::Easy::getEasy()
LLCurl::Easy::~Easy()
{
- LL_CHECK_MEMORY
releaseEasyHandle(mCurlEasyHandle);
- LL_CHECK_MEMORY
--gCurlEasyCount;
curl_slist_free_all(mHeaders);
LL_CHECK_MEMORY
@@ -331,7 +315,6 @@ LLCurl::Easy::~Easy()
LL_CHECK_MEMORY
}
mResponder = NULL;
- LL_CHECK_MEMORY
}
void LLCurl::Easy::resetState()
@@ -617,6 +600,7 @@ void LLCurl::Multi::cleanup(bool deleted)
if(deleted)
{
easy->mResponder = NULL ; //avoid triggering mResponder.
+ LL_CHECK_MEMORY
}
delete easy;
LL_CHECK_MEMORY
@@ -1623,7 +1607,9 @@ void LLCurl::deleteEasyHandle(CURL* handle)
if(handle)
{
LLMutexLock lock(sHandleMutexp) ;
+ LL_CHECK_MEMORY
curl_easy_cleanup(handle) ;
+ LL_CHECK_MEMORY
sTotalHandles-- ;
}
}
diff --git a/indra/llmessage/llcurl.h b/indra/llmessage/llcurl.h
index d6a7714d4c8..62947bdb9d9 100644
--- a/indra/llmessage/llcurl.h
+++ b/indra/llmessage/llcurl.h
@@ -44,6 +44,8 @@
#include "llthread.h"
#include "llqueuedthread.h"
#include "llframetimer.h"
+#include "llpointer.h"
+
class LLMutex;
class LLCurlThread;
@@ -67,7 +69,7 @@ class LLCurl
F64 mSpeedDownload;
};
- class Responder
+ class Responder : public LLThreadSafeRefCount
{
//LOG_CLASS(Responder);
public:
@@ -126,13 +128,10 @@ class LLCurl
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;
/**
@@ -378,12 +377,6 @@ class LLCurlThread : public LLQueuedThread
void cleanupMulti(LLCurl::Multi* multi) ;
} ;
-namespace boost
-{
- void intrusive_ptr_add_ref(LLCurl::Responder* p);
- void intrusive_ptr_release(LLCurl::Responder* p);
-};
-
class LLCurlRequest
{
diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp
index fa68b9322ee..2b6d6d15fa9 100644
--- a/indra/newview/llappviewer.cpp
+++ b/indra/newview/llappviewer.cpp
@@ -1888,8 +1888,6 @@ bool LLAppViewer::cleanup()
delete sTextureCache;
sTextureCache = NULL;
- delete sTextureFetch;
- sTextureFetch = NULL;
delete sImageDecodeThread;
sImageDecodeThread = NULL;
delete mFastTimerLogThread;
@@ -1962,6 +1960,10 @@ bool LLAppViewer::cleanup()
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())
diff --git a/indra/newview/llassetuploadresponders.cpp b/indra/newview/llassetuploadresponders.cpp
index 65bfc990d1e..7b2c536f5ad 100644
--- a/indra/newview/llassetuploadresponders.cpp
+++ b/indra/newview/llassetuploadresponders.cpp
@@ -919,7 +919,7 @@ class LLNewAgentInventoryVariablePriceResponder::Impl
bool uploadConfirmationCallback(
const LLSD& notification,
const LLSD& response,
- boost::intrusive_ptr<LLNewAgentInventoryVariablePriceResponder> responder)
+ LLPointer<LLNewAgentInventoryVariablePriceResponder> responder)
{
S32 option;
std::string confirmation_url;
@@ -949,7 +949,7 @@ class LLNewAgentInventoryVariablePriceResponder::Impl
void confirmUpload(
const std::string& confirmation_url,
- boost::intrusive_ptr<LLNewAgentInventoryVariablePriceResponder> responder)
+ LLPointer<LLNewAgentInventoryVariablePriceResponder> responder)
{
if ( getFilename().empty() )
{
@@ -1124,7 +1124,7 @@ void LLNewAgentInventoryVariablePriceResponder::showConfirmationDialog(
// and cause sadness.
mImpl->confirmUpload(
confirmation_url,
- boost::intrusive_ptr<LLNewAgentInventoryVariablePriceResponder>(this));
+ LLPointer<LLNewAgentInventoryVariablePriceResponder>(this));
}
else
{
@@ -1157,7 +1157,7 @@ void LLNewAgentInventoryVariablePriceResponder::showConfirmationDialog(
mImpl,
_1,
_2,
- boost::intrusive_ptr<LLNewAgentInventoryVariablePriceResponder>(this)));
+ LLPointer<LLNewAgentInventoryVariablePriceResponder>(this)));
}
}
diff --git a/indra/newview/lleventpoll.cpp b/indra/newview/lleventpoll.cpp
index 4f4d9a40b4a..2c786b7f8be 100644
--- a/indra/newview/lleventpoll.cpp
+++ b/indra/newview/lleventpoll.cpp
@@ -86,7 +86,7 @@ namespace
class LLEventPollEventTimer : public LLEventTimer
{
- typedef boost::intrusive_ptr<LLEventPollResponder> EventPollResponderPtr;
+ typedef LLPointer<LLEventPollResponder> EventPollResponderPtr;
public:
LLEventPollEventTimer(F32 period, EventPollResponderPtr responder)
diff --git a/indra/newview/llfloatertos.cpp b/indra/newview/llfloatertos.cpp
index c5df7e16e97..a242b224cd7 100644
--- a/indra/newview/llfloatertos.cpp
+++ b/indra/newview/llfloatertos.cpp
@@ -71,9 +71,9 @@ class LLIamHere : public LLHTTPClient::Responder
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 )
@@ -102,7 +102,7 @@ class LLIamHere : public LLHTTPClient::Responder
// 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()
diff --git a/indra/newview/lltexturefetch.cpp b/indra/newview/lltexturefetch.cpp
index 7e6dfbc9d9a..a5eebf6c777 100644
--- a/indra/newview/lltexturefetch.cpp
+++ b/indra/newview/lltexturefetch.cpp
@@ -2944,7 +2944,9 @@ TFReqSendMetrics::doWork(LLTextureFetch * fetcher)
~lcl_responder()
{
+ LL_CHECK_MEMORY
mFetcher->decrCurlPOSTCount();
+ LL_CHECK_MEMORY
}
// virtual
diff --git a/indra/newview/lltranslate.h b/indra/newview/lltranslate.h
index c58e1adb8ca..db5ad9479c1 100755
--- a/indra/newview/lltranslate.h
+++ b/indra/newview/lltranslate.h
@@ -263,8 +263,8 @@ public :
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.
diff --git a/indra/newview/llviewermessage.cpp b/indra/newview/llviewermessage.cpp
index 7d7d1f30474..04c7a873745 100755
--- a/indra/newview/llviewermessage.cpp
+++ b/indra/newview/llviewermessage.cpp
@@ -3339,9 +3339,9 @@ public :
{
}
- 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)
+ 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 boost::intrusive_ptr<ChatTranslationReceiver>(new ChatTranslationReceiver(from_lang, to_lang, mesg, chat, toast_args));
+ return new ChatTranslationReceiver(from_lang, to_lang, mesg, chat, toast_args);
}
protected:
diff --git a/indra/newview/llviewerregion.cpp b/indra/newview/llviewerregion.cpp
index 493195aaa32..1ef729e2ff4 100644
--- a/indra/newview/llviewerregion.cpp
+++ b/indra/newview/llviewerregion.cpp
@@ -255,10 +255,9 @@ class BaseCapabilitiesComplete : public LLHTTPClient::Responder
}
}
- 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:
diff --git a/indra/viewer_components/updater/llupdatechecker.cpp b/indra/viewer_components/updater/llupdatechecker.cpp
index 4da774a5f6b..5edbbf9914c 100644
--- a/indra/viewer_components/updater/llupdatechecker.cpp
+++ b/indra/viewer_components/updater/llupdatechecker.cpp
@@ -51,37 +51,6 @@ class LLUpdateChecker::CheckError:
};
-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
//-----------------------------------------------------------------------------
@@ -134,13 +103,7 @@ void LLUpdateChecker::Implementation::checkVersion(std::string const & protocolV
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,
diff --git a/indra/viewer_components/updater/llupdatechecker.h b/indra/viewer_components/updater/llupdatechecker.h
index d882169068f..23f62a7c5ee 100644
--- a/indra/viewer_components/updater/llupdatechecker.h
+++ b/indra/viewer_components/updater/llupdatechecker.h
@@ -29,6 +29,7 @@
#include <boost/shared_ptr.hpp>
+#include "llhttpclient.h"
//
// Implements asynchronous checking for updates.
@@ -36,7 +37,36 @@
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;
@@ -48,7 +78,7 @@ class LLUpdateChecker {
std::string const & servicePath, std::string channel, std::string version);
private:
- boost::shared_ptr<Implementation> mImplementation;
+ LLPointer<Implementation> mImplementation;
};
--
GitLab